Re: [Qemu-devel] [PATCH memory v2 6/9] memory: MemoryRegion: QOMify

Re: [Qemu-devel] [PATCH memory v2 6/9] memory: MemoryRegion: QOMify

[Peter Crosthwaite]

On Wed, May 28, 2014 at 2:24 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 27/05/2014 11:02, Peter Crosthwaite ha scritto:
>
>>
>> +void memory_region_destroy(MemoryRegion *mr)
>> +{
>> +    /*FIXME: whatever the opposite of object initialize is, do it here */
>> +    memory_region_finalize(OBJECT(mr));
>> +}
>> +
>
>
> That's object_unref.
>

Fixed. Thanks.

Regards,
Peter

> Paolo
>

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
<peter.crosthwaite@xilinx.com> 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 <peter.crosthwaite@xilinx.com>
> ---
> 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
>

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

[Paolo Bonzini]

Il 02/06/2014 04:40, Peter Crosthwaite ha scritto:
>> >  {
>> >      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.

Note that object_resolve_path_component expects link<FOO> properties to 
have a LinkProperty stored in prop->opaque.  Does this hold in your case?

Perhaps we can instead add a new ->resolve function pointer to properties.

Paolo

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

[Peter Crosthwaite]

On Tue, Jun 3, 2014 at 5:51 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 02/06/2014 04:40, Peter Crosthwaite ha scritto:
>
>>> >  {
>>> >      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.
>
>
> Note that object_resolve_path_component expects link<FOO> properties to have
> a LinkProperty stored in prop->opaque.  Does this hold in your case?
>

Probably not. Using canonical paths through links would not work in
this case. Nice catch.

> Perhaps we can instead add a new ->resolve function pointer to properties.
>

The other option is we open up the check fn from
object_property_add_link as being usable for setter side-effects. No
change of code, just I guess "check" would then be a wrong name. Then
there's no need for a low-level link.

Probably easier than yet another fn hook.

Regards,
Peter

> Paolo
>

Re: [Qemu-devel] [PATCH memory v2 8/9] memory: MemoryRegion: Add may-overlap and priority props

[Peter Crosthwaite]

On Tue, May 27, 2014 at 7:04 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> QOM propertyify the .may-overlap and .priority fields. The setters
> will re-add the memory as a subregion if needed (i.e. the values change
> when the memory region is already contained).
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> changed since v1:
> Converted priority to signed type
>
>  include/exec/memory.h |  2 +-
>  memory.c              | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 371c066..117c0d3 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -157,7 +157,7 @@ struct MemoryRegion {
>      bool flush_coalesced_mmio;
>      MemoryRegion *alias;
>      hwaddr alias_offset;
> -    int priority;
> +    int32_t priority;
>      bool may_overlap;
>      QTAILQ_HEAD(subregions, MemoryRegion) subregions;
>      QTAILQ_ENTRY(MemoryRegion) subregions_link;
> diff --git a/memory.c b/memory.c
> index a95bb1e..ee761a2 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -955,6 +955,55 @@ static void memory_region_release_container(Object *obj, const char *name,
>      }
>  }
>
> +static void memory_region_get_priority(Object *obj, Visitor *v, void *opaque,
> +                                       const char *name, Error **errp)
> +{
> +    MemoryRegion *mr = MEMORY_REGION(obj);
> +    Error *local_err = NULL;
> +    uint32_t value = mr->addr;

That's a copy paste error. Fixed. Also change type to int32_t to
support signed priorities via QOM setters/getters.

Regards,
Peter

> +
> +    visit_type_uint32(v, &value, name, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +    }
> +}
> +
> +static void memory_region_set_priority(Object *obj, Visitor *v, void *opaque,
> +                                       const char *name, Error **errp)
> +{
> +    MemoryRegion *mr = MEMORY_REGION(obj);
> +    Error *local_err = NULL;
> +    uint32_t value;
> +
> +    visit_type_uint32(v, &value, name, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    if (mr->priority != value) {
> +        mr->priority = value;
> +        memory_region_readd_subregion(mr);
> +    }
> +}
> +
> +static bool memory_region_get_may_overlap(Object *obj, Error **errp)
> +{
> +    MemoryRegion *mr = MEMORY_REGION(obj);
> +
> +    return mr->may_overlap;
> +}
> +
> +static void memory_region_set_may_overlap(Object *obj, bool value, Error **errp)
> +{
> +    MemoryRegion *mr = MEMORY_REGION(obj);
> +
> +    if (mr->may_overlap != value) {
> +        mr->may_overlap = value;
> +        memory_region_readd_subregion(mr);
> +    }
> +}
> +
>  static void memory_region_initfn(Object *obj)
>  {
>      MemoryRegion *mr = MEMORY_REGION(obj);
> @@ -980,6 +1029,14 @@ static void memory_region_initfn(Object *obj)
>                          memory_region_get_addr,
>                          memory_region_set_addr,
>                          NULL, NULL, &error_abort);
> +    object_property_add(OBJECT(mr), "priority", "uint32",
> +                        memory_region_get_priority,
> +                        memory_region_set_priority,
> +                        NULL, NULL, &error_abort);
> +    object_property_add_bool(OBJECT(mr), "may-overlap",
> +                             memory_region_get_may_overlap,
> +                             memory_region_set_may_overlap,
> +                             &error_abort);
>  }
>
>  static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
> --
> 1.9.3.1.ga73a6ad
>