Re: [Qemu-devel] [PATCH memory v2 7/9] memory: MemoryRegion: Add container and addr props

[Peter Crosthwaite]

On Tue, May 27, 2014 at 7:03 PM, Peter Crosthwaite
<address@hidden> wrote:
> Expose the already existing .parent and .addr fields as QOM properties.
> .parent (i.e. the field describing the memory region that contains this
> one in Memory hierachy) is renamed "container". This is to avoid
> confusion with the owner field, which is much more akin to an actual QOM
> parent.
>
> Setting the .parent ("container") will cause the memory subregion adding
> to happen.  Nullifying or changing the .parent will delete or relocate
> (to a different container) the subregion resp.
>
> Setting or changing the address will relocate the memory region.
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
> changed since v1:
> Converted container to low level link property and added subregion setup.
>
>  memory.c | 108 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 108 insertions(+)
>
> diff --git a/memory.c b/memory.c
> index d9d3c07..a95bb1e 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -16,6 +16,7 @@
>  #include "exec/memory.h"
>  #include "exec/address-spaces.h"
>  #include "exec/ioport.h"
> +#include "qapi/visitor.h"
>  #include "qemu/bitops.h"
>  #include "qom/object.h"
>  #include "trace.h"
> @@ -861,9 +862,104 @@ void memory_region_init(MemoryRegion *mr,
>      mr->name = g_strdup(name);
>  }
>
> +static void memory_region_get_addr(Object *obj, Visitor *v, void *opaque,
> +                                   const char *name, Error **errp)
> +{
> +    MemoryRegion *mr = MEMORY_REGION(obj);
> +    Error *local_err = NULL;
> +    uint64_t value = mr->addr;
> +
> +    visit_type_uint64(v, &value, name, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +    }
> +}
> +
> +static void memory_region_set_addr(Object *obj, Visitor *v, void *opaque,
> +                                   const char *name, Error **errp)
> +{
> +    MemoryRegion *mr = MEMORY_REGION(obj);
> +    Error *local_err = NULL;
> +    uint64_t value;
> +
> +    visit_type_uint64(v, &value, name, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    memory_region_set_address(mr, value);
> +}
> +
> +static void memory_region_set_container(Object *obj, Visitor *v, void 
> *opaque,
> +                                        const char *name, Error **errp)
> +{
> +    MemoryRegion *mr = MEMORY_REGION(obj);
> +    Error *local_err = NULL;
> +    MemoryRegion *old_parent = mr->parent;
> +    MemoryRegion *new_parent = NULL;
> +    char *path = NULL;
> +
> +    visit_type_str(v, &path, name, &local_err);
> +
> +    if (!local_err && strcmp(path, "") != 0) {
> +        new_parent = MEMORY_REGION(object_resolve_link(obj, name, path,
> +                                   &local_err));
> +    }
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    object_ref(OBJECT(new_parent));
> +
> +    memory_region_transaction_begin();
> +    memory_region_ref(mr);
> +    if (old_parent) {
> +        memory_region_del_subregion(old_parent, mr);
> +    }
> +    mr->parent = new_parent;
> +    if (new_parent) {
> +        do_memory_region_add_subregion_common(mr);
> +    }
> +    memory_region_unref(mr);
> +    memory_region_transaction_commit();
> +
> +    object_unref(OBJECT(old_parent));
> +}
> +
> +static void memory_region_get_container(Object *obj, Visitor *v, void 
> *opaque,
> +                                        const char *name, Error **errp)
> +{
> +    MemoryRegion *mr = MEMORY_REGION(obj);
> +    gchar *path = (gchar *)"";
> +
> +    if (mr->parent) {
> +        path = object_get_canonical_path(OBJECT(mr->parent));
> +    }
> +    visit_type_str(v, &path, name, errp);
> +    if (mr->parent) {
> +        g_free(path);
> +    }
> +}
> +
> +static void memory_region_release_container(Object *obj, const char *name,
> +                                            void *opaque)
> +{
> +    MemoryRegion *mr = MEMORY_REGION(obj);
> +
> +    if (mr->parent) {
> +        memory_region_del_subregion(mr->parent, mr);
> +        object_unref(OBJECT(mr->parent));
> +    }
> +}
> +
>  static void memory_region_initfn(Object *obj)
>  {
>      MemoryRegion *mr = MEMORY_REGION(obj);
> +    gchar *container_link_type = g_strdup_printf("link<%s>",
> +                                                 TYPE_MEMORY_REGION);

Since TYPE_MEMORY_REGION is a literal string constant, this can be
done with regular "" "" style string concatenation. Dropped the strdup
in V3.

Regards,
Peter

>
>      mr->ops = &unassigned_mem_ops;
>      mr->enabled = true;
> @@ -872,6 +968,18 @@ static void memory_region_initfn(Object *obj)
>      QTAILQ_INIT(&mr->subregions);
>      memset(&mr->subregions_link, 0, sizeof mr->subregions_link);
>      QTAILQ_INIT(&mr->coalesced);
> +
> +    object_property_add(OBJECT(mr), "container", container_link_type,
> +                        memory_region_get_container,
> +                        memory_region_set_container,
> +                        memory_region_release_container,
> +                        NULL, &error_abort);
> +    g_free(container_link_type);
> +
> +    object_property_add(OBJECT(mr), "addr", "uint64",
> +                        memory_region_get_addr,
> +                        memory_region_set_addr,
> +                        NULL, NULL, &error_abort);
>  }
>
>  static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
> --
> 1.9.3.1.ga73a6ad
>