qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Igor Mammedov
Subject: Re: [Qemu-devel] [PATCH v3 05/34] add memdev backend infrastructure
Date: Fri, 30 May 2014 09:59:55 +0200

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.
> 
> Regards,
> Peter
> 
> > +}
> > +
> > +MemoryRegion *
> > +host_memory_backend_get_memory(HostMemoryBackend *backend, Error **errp)
> > +{
> > +    return memory_region_size(&backend->mr) ? &backend->mr : NULL;
> > +}
> > +
> > +static void
> > +hostmemory_backend_class_init(ObjectClass *oc, void *data)
> > +{
> > +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> > +
> > +    ucc->complete = hostmemory_backend_memory_init;
> > +}
> > +
> > +static const TypeInfo hostmemory_backend_info = {
> > +    .name = TYPE_MEMORY_BACKEND,
> > +    .parent = TYPE_OBJECT,
> > +    .abstract = true,
> > +    .class_size = sizeof(HostMemoryBackendClass),
> > +    .class_init = hostmemory_backend_class_init,
> > +    .instance_size = sizeof(HostMemoryBackend),
> > +    .instance_init = hostmemory_backend_initfn,
> > +    .instance_finalize = hostmemory_backend_finalize,
> > +    .interfaces = (InterfaceInfo[]) {
> > +        { TYPE_USER_CREATABLE },
> > +        { }
> > +    }
> > +};
> > +
> > +static void register_types(void)
> > +{
> > +    type_register_static(&hostmemory_backend_info);
> > +}
> > +
> > +type_init(register_types);
> > diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
> > new file mode 100644
> > index 0000000..bc3ffb3
> > --- /dev/null
> > +++ b/include/sysemu/hostmem.h
> > @@ -0,0 +1,60 @@
> > +/*
> > + * 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.
> > + */
> > +#ifndef QEMU_RAM_H
> > +#define QEMU_RAM_H
> > +
> > +#include "qom/object.h"
> > +#include "qapi/error.h"
> > +#include "exec/memory.h"
> > +#include "qemu/option.h"
> > +
> > +#define TYPE_MEMORY_BACKEND "memory"
> > +#define MEMORY_BACKEND(obj) \
> > +    OBJECT_CHECK(HostMemoryBackend, (obj), TYPE_MEMORY_BACKEND)
> > +#define MEMORY_BACKEND_GET_CLASS(obj) \
> > +    OBJECT_GET_CLASS(HostMemoryBackendClass, (obj), TYPE_MEMORY_BACKEND)
> > +#define MEMORY_BACKEND_CLASS(klass) \
> > +    OBJECT_CLASS_CHECK(HostMemoryBackendClass, (klass), 
> > TYPE_MEMORY_BACKEND)
> > +
> > +typedef struct HostMemoryBackend HostMemoryBackend;
> > +typedef struct HostMemoryBackendClass HostMemoryBackendClass;
> > +
> > +/**
> > + * HostMemoryBackendClass:
> > + * @parent_class: opaque parent class container
> > + */
> > +struct HostMemoryBackendClass {
> > +    ObjectClass parent_class;
> > +};
> > +
> > +/**
> > + * @HostMemoryBackend
> > + *
> > + * @parent: opaque parent object container
> > + * @size: amount of memory backend provides
> > + * @id: unique identification string in memdev namespace
> > + * @mr: MemoryRegion representing host memory belonging to backend
> > + */
> > +struct HostMemoryBackend {
> > +    /* private */
> > +    Object parent;
> > +
> > +    /* protected */
> > +    uint64_t size;
> > +
> > +    MemoryRegion mr;
> > +};
> > +
> > +MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend,
> > +                                             Error **errp);
> > +
> > +#endif
> > --
> > 1.7.1
> >
> >
> 
I'll amend patch except of MemoryRegion that we agreed to leave as is in this
mail thread. 

-- 
Regards,
  Igor



reply via email to

[Prev in Thread] Current Thread [Next in Thread]