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

[Peter Crosthwaite]

On Mon, Jun 9, 2014 at 12:44 PM, Hu Tao <address@hidden> wrote:
> On Fri, Jun 06, 2014 at 02:48:15PM +0200, Igor Mammedov wrote:
>> On Fri, 6 Jun 2014 17:29:38 +0800
>> Hu Tao <address@hidden> wrote:
>>
>> > 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.
>> I've think Peter's suggested not to rise a error form dummy function.
>> So I've dropped it from abstract HostMemoryBackend class in v4 so later
>> concrete class should set it's own implementation, which
>> TYPE_MEMORY_BACKEND_RAM does.
>
> I'd suggest adding a new function HostMemoryBackend::alloc() then in
> HostmemoryBackend::complete() we can do things like:
>
>   alloc();
>   mbind();
>   preallocate();
>
>
> If letting subclasses do the allocation in its' own complete() function,
> we will have trouble setting memory policies and like in
> HostmemoryBackend::complete().
>

If both the abstract and concrete class want to do something for a
particular hook that's ok. The concrete class overrides, but should
just call the superclass implementation from it's own hook. No need
for new abstract APIs.

Regards,
Peter


> Hu
>