[Qemu-devel] [PATCH memory v2 0/3] Memory Region Naming - take 2

[Qemu-devel] [PATCH memory v2 0/3] Memory Region Naming - take 2

[Peter Crosthwaite]

Hi,

Here is an attempt at a proper fix to the memory region naming bugs as
reported by Peter. The Xen compile bug and the memory leak.

Regards,
Peter

Changed since v1:
Constify xen physmap name string (Stefan W review).
Change memory_region_name to caching based approach.



Peter Crosthwaite (3):
  xen-hvm: Constify string
  xen: hvm: Abstract away memory region name ref
  memory: Lazy init name from QOM name as needed

 memory.c  |  5 ++++-
 xen-hvm.c | 11 +++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

-- 
2.1.0.1.g27b9230

[Qemu-devel] [PATCH memory v2 1/3] xen-hvm: Constify string

[Peter Crosthwaite]

It's constant, and sourced from existing const strings. Avoid dodgy
casts by converting to const.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 xen-hvm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen-hvm.c b/xen-hvm.c
index 91de2e2..d763e86 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -71,7 +71,7 @@ static inline ioreq_t *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
 typedef struct XenPhysmap {
     hwaddr start_addr;
     ram_addr_t size;
-    char *name;
+    const char *name;
     hwaddr phys_offset;
 
     QLIST_ENTRY(XenPhysmap) list;
@@ -330,7 +330,7 @@ go_physmap:
 
     physmap->start_addr = start_addr;
     physmap->size = size;
-    physmap->name = (char *)mr->name;
+    physmap->name = mr->name;
     physmap->phys_offset = phys_offset;
 
     QLIST_INSERT_HEAD(&state->physmap, physmap, list);
-- 
2.1.0.1.g27b9230

Re: [Qemu-devel] [PATCH memory v2 1/3] xen-hvm: Constify string

[Stefan Weil]

Am 26.08.2014 um 05:09 schrieb Peter Crosthwaite:
> It's constant, and sourced from existing const strings. Avoid dodgy
> casts by converting to const.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  xen-hvm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen-hvm.c b/xen-hvm.c
> index 91de2e2..d763e86 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -71,7 +71,7 @@ static inline ioreq_t *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
>  typedef struct XenPhysmap {
>      hwaddr start_addr;
>      ram_addr_t size;
> -    char *name;
> +    const char *name;
>      hwaddr phys_offset;
>  
>      QLIST_ENTRY(XenPhysmap) list;
> @@ -330,7 +330,7 @@ go_physmap:
>  
>      physmap->start_addr = start_addr;
>      physmap->size = size;
> -    physmap->name = (char *)mr->name;
> +    physmap->name = mr->name;
>      physmap->phys_offset = phys_offset;
>  
>      QLIST_INSERT_HEAD(&state->physmap, physmap, list);
> 


Thanks.

Reviewed-by: Stefan Weil <sw@weilnetz.de>

[Qemu-devel] [PATCH memory v2 2/3] xen: hvm: Abstract away memory region name ref

[Peter Crosthwaite]

The mr->name field is removed. This slipped through compile testing.
Fix.

Reviewed-by: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed since v1:
Removed dodgy (char *) cast

 xen-hvm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen-hvm.c b/xen-hvm.c
index d763e86..0d09940 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -291,6 +291,7 @@ static int xen_add_to_physmap(XenIOState *state,
     hwaddr pfn, start_gpfn;
     hwaddr phys_offset = memory_region_get_ram_addr(mr);
     char path[80], value[17];
+    const char *mr_name;
 
     if (get_physmapping(state, start_addr, size)) {
         return 0;
@@ -326,11 +327,13 @@ go_physmap:
         }
     }
 
+    mr_name = memory_region_name(mr);
+
     physmap = g_malloc(sizeof (XenPhysmap));
 
     physmap->start_addr = start_addr;
     physmap->size = size;
-    physmap->name = mr->name;
+    physmap->name = mr_name;
     physmap->phys_offset = phys_offset;
 
     QLIST_INSERT_HEAD(&state->physmap, physmap, list);
@@ -354,11 +357,11 @@ go_physmap:
     if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
         return -1;
     }
-    if (mr->name) {
+    if (mr_name) {
         snprintf(path, sizeof(path),
                 "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
                 xen_domid, (uint64_t)phys_offset);
-        if (!xs_write(state->xenstore, 0, path, mr->name, strlen(mr->name))) {
+        if (!xs_write(state->xenstore, 0, path, mr_name, strlen(mr_name))) {
             return -1;
         }
     }
-- 
2.1.0.1.g27b9230

[Qemu-devel] [PATCH memory v2 3/3] memory: Lazy init name from QOM name as needed

[Peter Crosthwaite]

To support name retrieval of MemoryRegions that were created
dynamically (that is, not via memory_region_init and friends). We
cache the name in MemoryRegion's state as
object_get_canonical_path_component mallocs the returned value
so it's not suitable for direct return to callers. Memory already
frees the name field, so this will be garbage collected along with
the MR object.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 memory.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/memory.c b/memory.c
index 42317a2..fc16e5f 100644
--- a/memory.c
+++ b/memory.c
@@ -914,7 +914,6 @@ void memory_region_init(MemoryRegion *mr,
     if (size == UINT64_MAX) {
         mr->size = int128_2_64();
     }
-    mr->name = g_strdup(name);
 
     if (name) {
         object_property_add_child_array(owner, name, OBJECT(mr));
@@ -1309,6 +1308,10 @@ uint64_t memory_region_size(MemoryRegion *mr)
 
 const char *memory_region_name(const MemoryRegion *mr)
 {
+    if (!mr->name) {
+        ((MemoryRegion *)mr)->name =
+            object_get_canonical_path_component(OBJECT(mr));
+    }
     return mr->name;
 }
 
-- 
2.1.0.1.g27b9230

Re: [Qemu-devel] [PATCH memory v2 3/3] memory: Lazy init name from QOM name as needed

[Paolo Bonzini]

Il 26/08/2014 05:10, Peter Crosthwaite ha scritto:
> To support name retrieval of MemoryRegions that were created
> dynamically (that is, not via memory_region_init and friends). We
> cache the name in MemoryRegion's state as
> object_get_canonical_path_component mallocs the returned value
> so it's not suitable for direct return to callers. Memory already
> frees the name field, so this will be garbage collected along with
> the MR object.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  memory.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/memory.c b/memory.c
> index 42317a2..fc16e5f 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -914,7 +914,6 @@ void memory_region_init(MemoryRegion *mr,
>      if (size == UINT64_MAX) {
>          mr->size = int128_2_64();
>      }
> -    mr->name = g_strdup(name);
>  
>      if (name) {
>          object_property_add_child_array(owner, name, OBJECT(mr));
> @@ -1309,6 +1308,10 @@ uint64_t memory_region_size(MemoryRegion *mr)
>  
>  const char *memory_region_name(const MemoryRegion *mr)
>  {
> +    if (!mr->name) {
> +        ((MemoryRegion *)mr)->name =
> +            object_get_canonical_path_component(OBJECT(mr));
> +    }
>      return mr->name;
>  }
>  
> 

Oh if we only used C++... :)

Thanks, looks good!

Paolo

Re: [Qemu-devel] [PATCH memory v2 3/3] memory: Lazy init name from QOM name as needed

[Paolo Bonzini]

Il 26/08/2014 05:10, Peter Crosthwaite ha scritto:
> To support name retrieval of MemoryRegions that were created
> dynamically (that is, not via memory_region_init and friends). We
> cache the name in MemoryRegion's state as
> object_get_canonical_path_component mallocs the returned value
> so it's not suitable for direct return to callers. Memory already
> frees the name field, so this will be garbage collected along with
> the MR object.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  memory.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/memory.c b/memory.c
> index 42317a2..fc16e5f 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -914,7 +914,6 @@ void memory_region_init(MemoryRegion *mr,
>      if (size == UINT64_MAX) {
>          mr->size = int128_2_64();
>      }
> -    mr->name = g_strdup(name);

This causes a bunch of [0] to appear in the "info mtree" output.
They're a bit ugly, so I'm removing this line for now.

Paolo

>      if (name) {
>          object_property_add_child_array(owner, name, OBJECT(mr));
> @@ -1309,6 +1308,10 @@ uint64_t memory_region_size(MemoryRegion *mr)
>  
>  const char *memory_region_name(const MemoryRegion *mr)
>  {
> +    if (!mr->name) {
> +        ((MemoryRegion *)mr)->name =
> +            object_get_canonical_path_component(OBJECT(mr));
> +    }
>      return mr->name;
>  }
>  
> 

Re: [Qemu-devel] [PATCH memory v2 3/3] memory: Lazy init name from QOM name as needed

[Peter Crosthwaite]

On Fri, Aug 29, 2014 at 12:06 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 26/08/2014 05:10, Peter Crosthwaite ha scritto:
>> To support name retrieval of MemoryRegions that were created
>> dynamically (that is, not via memory_region_init and friends). We
>> cache the name in MemoryRegion's state as
>> object_get_canonical_path_component mallocs the returned value
>> so it's not suitable for direct return to callers. Memory already
>> frees the name field, so this will be garbage collected along with
>> the MR object.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  memory.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/memory.c b/memory.c
>> index 42317a2..fc16e5f 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -914,7 +914,6 @@ void memory_region_init(MemoryRegion *mr,
>>      if (size == UINT64_MAX) {
>>          mr->size = int128_2_64();
>>      }
>> -    mr->name = g_strdup(name);
>
> This causes a bunch of [0] to appear in the "info mtree" output.
> They're a bit ugly, so I'm removing this line for now.
>

Yeh, so a solution to that was to patch qom to drop the [*] for
singletons added via arrayified property adders. One possible semantic
that I can think of is the 0th element always has a property alias so
that foo and foo[0] mean the same thing. This tricky part is, when
"[1]" comes along the preferred name (the canonical path) for the 0th
element should switch from "foo" to "foo[0]". Is it sane to switch
child and alias strings for "[0]" when "[1]" comes along?

Regards,
Peter

> Paolo
>
>>      if (name) {
>>          object_property_add_child_array(owner, name, OBJECT(mr));
>> @@ -1309,6 +1308,10 @@ uint64_t memory_region_size(MemoryRegion *mr)
>>
>>  const char *memory_region_name(const MemoryRegion *mr)
>>  {
>> +    if (!mr->name) {
>> +        ((MemoryRegion *)mr)->name =
>> +            object_get_canonical_path_component(OBJECT(mr));
>> +    }
>>      return mr->name;
>>  }
>>
>>
>
>

Re: [Qemu-devel] [PATCH memory v2 3/3] memory: Lazy init name from QOM name as needed

[Paolo Bonzini]

Il 28/08/2014 16:27, Peter Crosthwaite ha scritto:
> Yeh, so a solution to that was to patch qom to drop the [*] for
> singletons added via arrayified property adders. One possible semantic
> that I can think of is the 0th element always has a property alias so
> that foo and foo[0] mean the same thing. This tricky part is, when
> "[1]" comes along the preferred name (the canonical path) for the 0th
> element should switch from "foo" to "foo[0]". Is it sane to switch
> child and alias strings for "[0]" when "[1]" comes along?

You and I both know that the right solution is to move the [*] from
memory.c to the callers... :)

Paolo

Re: [Qemu-devel] [PATCH memory v2 3/3] memory: Lazy init name from QOM name as needed

[Peter Crosthwaite]

On Fri, Aug 29, 2014 at 12:57 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 28/08/2014 16:27, Peter Crosthwaite ha scritto:
>> Yeh, so a solution to that was to patch qom to drop the [*] for
>> singletons added via arrayified property adders. One possible semantic
>> that I can think of is the 0th element always has a property alias so
>> that foo and foo[0] mean the same thing. This tricky part is, when
>> "[1]" comes along the preferred name (the canonical path) for the 0th
>> element should switch from "foo" to "foo[0]". Is it sane to switch
>> child and alias strings for "[0]" when "[1]" comes along?
>
> You and I both know that the right solution is to move the [*] from
> memory.c to the callers... :)
>

Hmm that's a lot of changes :(

Regards,
Peter

> Paolo
>

Re: [Qemu-devel] [PATCH memory v2 0/3] Memory Region Naming - take 2

[Stefano Stabellini]

Sorry for the late reply.

I just wanted to confirm that it's all working now.

Thanks,

Stefano

On Mon, 25 Aug 2014, Peter Crosthwaite wrote:
> Hi,
> 
> Here is an attempt at a proper fix to the memory region naming bugs as
> reported by Peter. The Xen compile bug and the memory leak.
> 
> Regards,
> Peter
> 
> Changed since v1:
> Constify xen physmap name string (Stefan W review).
> Change memory_region_name to caching based approach.
> 
> 
> 
> Peter Crosthwaite (3):
>   xen-hvm: Constify string
>   xen: hvm: Abstract away memory region name ref
>   memory: Lazy init name from QOM name as needed
> 
>  memory.c  |  5 ++++-
>  xen-hvm.c | 11 +++++++----
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> -- 
> 2.1.0.1.g27b9230
>