Re: [Qemu-devel] [PATCH v3 05/34] add memdev backend infrastructure

[Hu Tao]

On Fri, May 30, 2014 at 09:59:55AM +0200, Igor Mammedov wrote:
> On Fri, 30 May 2014 00:05:51 +1000
> Peter Crosthwaite <address@hidden> wrote:
> 
> > On Tue, May 27, 2014 at 11:01 PM, Igor Mammedov <address@hidden> wrote:
> > > Provides framework for splitting host RAM allocation/
> > > policies into a separate backend that could be used
> > > by devices.
> > >
> > > Initially only legacy RAM backend is provided, which
> > > uses memory_region_init_ram() allocator and compatible
> > > with every CLI option that affects memory_region_init_ram().
> > >
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > Signed-off-by: Paolo Bonzini <address@hidden>
> > > ---
> > > v4:
> > >  - don't use nonexisting anymore error_is_set()
> > > v3:
> > >  - fix path leak & use object_get_canonical_path_component()
> > >    for getting object name
> > > v2:
> > >  - reuse UserCreatable interface instead of custom callbacks
> > > ---
> > >  backends/Makefile.objs   |    2 +
> > >  backends/hostmem-ram.c   |   54 ++++++++++++++++++++++
> > >  backends/hostmem.c       |  113 
> > > ++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/sysemu/hostmem.h |   60 ++++++++++++++++++++++++
> > >  4 files changed, 229 insertions(+), 0 deletions(-)
> > >  create mode 100644 backends/hostmem-ram.c
> > >  create mode 100644 backends/hostmem.c
> > >  create mode 100644 include/sysemu/hostmem.h
> > >
> > > diff --git a/backends/Makefile.objs b/backends/Makefile.objs
> > > index 591ddcf..7fb7acd 100644
> > > --- a/backends/Makefile.objs
> > > +++ b/backends/Makefile.objs
> > > @@ -6,3 +6,5 @@ common-obj-$(CONFIG_BRLAPI) += baum.o
> > >  baum.o-cflags := $(SDL_CFLAGS)
> > >
> > >  common-obj-$(CONFIG_TPM) += tpm.o
> > > +
> > > +common-obj-y += hostmem.o hostmem-ram.o
> > > diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
> > > new file mode 100644
> > > index 0000000..cbf7e5a
> > > --- /dev/null
> > > +++ b/backends/hostmem-ram.c
> > > @@ -0,0 +1,54 @@
> > > +/*
> > > + * QEMU Host Memory Backend
> > > + *
> > > + * Copyright (C) 2013 Red Hat Inc
> > > + *
> > > + * Authors:
> > > + *   Igor Mammedov <address@hidden>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > > later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> > > +#include "sysemu/hostmem.h"
> > > +#include "qom/object_interfaces.h"
> > > +
> > > +#define TYPE_MEMORY_BACKEND_RAM "memory-ram"
> > > +
> > > +
> > > +static void
> > > +ram_backend_memory_init(UserCreatable *uc, Error **errp)
> > > +{
> > > +    HostMemoryBackend *backend = MEMORY_BACKEND(uc);
> > > +    char *path;
> > > +
> > > +    if (!backend->size) {
> > > +        error_setg(errp, "can't create backend with size 0");
> > > +        return;
> > > +    }
> > > +
> > > +    path = object_get_canonical_path_component(OBJECT(backend));
> > > +    memory_region_init_ram(&backend->mr, OBJECT(backend), path,
> > 
> > Passing the full canonical path as the name of memory region is
> > redundant as that information is already passed via the owner
> > argument. It should just be a shorthand.
> > 
> > > +                           backend->size);
> > > +    g_free(path);
> > > +}
> > > +
> > > +static void
> > > +ram_backend_class_init(ObjectClass *oc, void *data)
> > > +{
> > > +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> > > +
> > > +    ucc->complete = ram_backend_memory_init;
> > > +}
> > > +
> > > +static const TypeInfo ram_backend_info = {
> > > +    .name = TYPE_MEMORY_BACKEND_RAM,
> > > +    .parent = TYPE_MEMORY_BACKEND,
> > > +    .class_init = ram_backend_class_init,
> > > +};
> > > +
> > > +static void register_types(void)
> > > +{
> > > +    type_register_static(&ram_backend_info);
> > > +}
> > > +
> > > +type_init(register_types);
> > > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > > new file mode 100644
> > > index 0000000..8a34b0f
> > > --- /dev/null
> > > +++ b/backends/hostmem.c
> > > @@ -0,0 +1,113 @@
> > > +/*
> > > + * QEMU Host Memory Backend
> > > + *
> > > + * Copyright (C) 2013 Red Hat Inc
> > > + *
> > > + * Authors:
> > > + *   Igor Mammedov <address@hidden>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > > later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> > > +#include "sysemu/hostmem.h"
> > > +#include "sysemu/sysemu.h"
> > > +#include "qapi/visitor.h"
> > > +#include "qapi/qmp/qerror.h"
> > > +#include "qemu/config-file.h"
> > > +#include "qom/object_interfaces.h"
> > > +
> > > +static void
> > > +hostmemory_backend_get_size(Object *obj, Visitor *v, void *opaque,
> > > +                            const char *name, Error **errp)
> > > +{
> > > +    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> > > +    uint64_t value = backend->size;
> > > +
> > > +    visit_type_size(v, &value, name, errp);
> > > +}
> > > +
> > > +static void
> > > +hostmemory_backend_set_size(Object *obj, Visitor *v, void *opaque,
> > > +                            const char *name, Error **errp)
> > > +{
> > > +    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> > > +    Error *local_err = NULL;
> > > +    uint64_t value;
> > > +
> > > +    if (memory_region_size(&backend->mr)) {
> > > +        error_setg(&local_err, "cannot change property value\n");
> > > +        goto out;
> > > +    }
> > > +
> > > +    visit_type_size(v, &value, name, errp);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +    if (!value) {
> > > +        error_setg(&local_err, "Property '%s.%s' doesn't take value '%"
> > > +                   PRIu64 "'", object_get_typename(obj), name , value);
> > > +        goto out;
> > > +    }
> > > +    backend->size = value;
> > > +out:
> > > +    error_propagate(errp, local_err);
> > > +}
> > > +
> > > +static void hostmemory_backend_initfn(Object *obj)
> > 
> > can you just call this _init and ..
> > 
> > > +{
> > > +    object_property_add(obj, "size", "int",
> > > +                        hostmemory_backend_get_size,
> > > +                        hostmemory_backend_set_size, NULL, NULL, NULL);
> > > +}
> > > +
> > > +static void hostmemory_backend_finalize(Object *obj)
> > > +{
> > > +    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> > > +
> > > +    if (memory_region_size(&backend->mr)) {
> > > +        memory_region_destroy(&backend->mr);
> > > +    }
> > > +}
> > > +
> > > +static void
> > > +hostmemory_backend_memory_init(UserCreatable *uc, Error **errp)
> > 
> > And this becomes "_complete" for naming consistency?
> > 
> > > +{
> > > +    error_setg(errp, "memory_init is not implemented for type [%s]",
> > > +               object_get_typename(OBJECT(uc)));
> > 
> > What if complete for the concrete class is a genuine NOP? I think
> > that's for the child class decide. All this check is doing is
> > mandating that the child does "something" without any form of validity
> > checking. I would just drop it completely.

NUMA binding patch needs it. HostMemoryBackend can do some common task
such as setting memory policies, prealloc memory, etc. after subclasses
allocate memory regions. see https://github.com/taohu/qemu/commits/numa
for codes doing this.

For now the complete function can be just left empty and fill later.

Regards,
Hu Tao