When reporting the id of virtio-based memory devices, we always have to
take the one of the proxy device (parent).

Expose the function, so especially virtio-based memory devices can
reuse the function when filling out the id in MemoryDeviceInfo.

Details:

When the user creates a virtio device (e.g.  virtio-balloon-pci), two
devices are actually created.

1. Virtio proxy device (e.g. TYPE_VIRTIO_BALLOON_PCI)
2. The "real" virtio device (e.g. TYPE_VIRTIO_BALLOON).

1. aliases all properties of 2, so 2. can be properly configured using 1.
1. gets the device ID set specified by the user. 2. gets no ID set.

If we want to make 2. a MemoryDevice but report errors/information to the
user, we always have to report the id of 1. (because that's the device the
user instantiated and configured).

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c         | 21 +++++++++++++++++++--
 include/hw/mem/memory-device.h |  1 +
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index a31ba73ea7..89a0c584be 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -19,6 +19,22 @@
 #include "sysemu/kvm.h"
 #include "trace.h"
 
+const char *memory_device_id(const MemoryDeviceState *md)
+{
+    Object *obj = OBJECT(md);
+
+    /* always use the ID of the proxy device for virtio devices */
+    if (object_dynamic_cast(obj, TYPE_VIRTIO_DEVICE)) {
+        if (obj->parent && object_dynamic_cast(obj->parent, TYPE_DEVICE)) {
+            const DeviceState *parent_dev = DEVICE(obj->parent);
+
+            return parent_dev->id;
+        }
+        return NULL;
+    }
+    return DEVICE(md)->id;
+}
+
 static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
 {
     const MemoryDeviceState *md_a = MEMORY_DEVICE(a);
@@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     for (item = list; item; item = g_slist_next(item)) {
         MemoryDeviceState *md = item->data;
         const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
+        const char *id = memory_device_id(md);
         uint64_t md_size, md_addr;
 
         md_addr = mdc->get_addr(md);
@@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
 
         if (ranges_overlap(md_addr, md_size, new_addr, size)) {
             if (hint) {
-                const DeviceState *d = DEVICE(md);
-                error_setg(errp, "address range conflicts with '%s'", d->id);
+                error_setg(errp, "address range conflicts with '%s'",
+                           id ? id : 0);
                 goto out;
             }
             new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index 05cb9437b7..324cc45b6f 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -54,6 +54,7 @@ typedef struct MemoryDeviceClass {
                              MemoryDeviceInfo *info);
 } MemoryDeviceClass;
 
+const char *memory_device_id(const MemoryDeviceState *md);
 MemoryDeviceInfoList *qmp_memory_device_list(void);
 uint64_t get_plugged_memory_size(void);
 void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
-- 
2.17.1

* David Hildenbrand (david@redhat.com) wrote:
> When reporting the id of virtio-based memory devices, we always have to
> take the one of the proxy device (parent).
> 
> Expose the function, so especially virtio-based memory devices can
> reuse the function when filling out the id in MemoryDeviceInfo.
> 
> Details:
> 
> When the user creates a virtio device (e.g.  virtio-balloon-pci), two
> devices are actually created.
> 
> 1. Virtio proxy device (e.g. TYPE_VIRTIO_BALLOON_PCI)
> 2. The "real" virtio device (e.g. TYPE_VIRTIO_BALLOON).
> 
> 1. aliases all properties of 2, so 2. can be properly configured using 1.
> 1. gets the device ID set specified by the user. 2. gets no ID set.
> 
> If we want to make 2. a MemoryDevice but report errors/information to the
> user, we always have to report the id of 1. (because that's the device the
> user instantiated and configured).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hw/mem/memory-device.c         | 21 +++++++++++++++++++--
>  include/hw/mem/memory-device.h |  1 +
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index a31ba73ea7..89a0c584be 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -19,6 +19,22 @@
>  #include "sysemu/kvm.h"
>  #include "trace.h"
>  
> +const char *memory_device_id(const MemoryDeviceState *md)
> +{
> +    Object *obj = OBJECT(md);
> +
> +    /* always use the ID of the proxy device for virtio devices */
> +    if (object_dynamic_cast(obj, TYPE_VIRTIO_DEVICE)) {
> +        if (obj->parent && object_dynamic_cast(obj->parent, TYPE_DEVICE)) {
> +            const DeviceState *parent_dev = DEVICE(obj->parent);
> +
> +            return parent_dev->id;
> +        }
> +        return NULL;
> +    }
> +    return DEVICE(md)->id;
> +}
> +
>  static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
>  {
>      const MemoryDeviceState *md_a = MEMORY_DEVICE(a);
> @@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>      for (item = list; item; item = g_slist_next(item)) {
>          MemoryDeviceState *md = item->data;
>          const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
> +        const char *id = memory_device_id(md);
>          uint64_t md_size, md_addr;
>  
>          md_addr = mdc->get_addr(md);
> @@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>  
>          if (ranges_overlap(md_addr, md_size, new_addr, size)) {
>              if (hint) {
> -                const DeviceState *d = DEVICE(md);
> -                error_setg(errp, "address range conflicts with '%s'", d->id);
> +                error_setg(errp, "address range conflicts with '%s'",
> +                           id ? id : 0);
>                  goto out;
>              }
>              new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
> index 05cb9437b7..324cc45b6f 100644
> --- a/include/hw/mem/memory-device.h
> +++ b/include/hw/mem/memory-device.h
> @@ -54,6 +54,7 @@ typedef struct MemoryDeviceClass {
>                               MemoryDeviceInfo *info);
>  } MemoryDeviceClass;
>  
> +const char *memory_device_id(const MemoryDeviceState *md);
>  MemoryDeviceInfoList *qmp_memory_device_list(void);
>  uint64_t get_plugged_memory_size(void);
>  void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Actually on second thoughts, a question:

* David Hildenbrand (david@redhat.com) wrote:
> When reporting the id of virtio-based memory devices, we always have to
> take the one of the proxy device (parent).
> 
> Expose the function, so especially virtio-based memory devices can
> reuse the function when filling out the id in MemoryDeviceInfo.
> 
> Details:
> 
> When the user creates a virtio device (e.g.  virtio-balloon-pci), two
> devices are actually created.
> 
> 1. Virtio proxy device (e.g. TYPE_VIRTIO_BALLOON_PCI)
> 2. The "real" virtio device (e.g. TYPE_VIRTIO_BALLOON).
> 
> 1. aliases all properties of 2, so 2. can be properly configured using 1.
> 1. gets the device ID set specified by the user. 2. gets no ID set.
> 
> If we want to make 2. a MemoryDevice but report errors/information to the
> user, we always have to report the id of 1. (because that's the device the
> user instantiated and configured).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/memory-device.c         | 21 +++++++++++++++++++--
>  include/hw/mem/memory-device.h |  1 +
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index a31ba73ea7..89a0c584be 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -19,6 +19,22 @@
>  #include "sysemu/kvm.h"
>  #include "trace.h"
>  
> +const char *memory_device_id(const MemoryDeviceState *md)
> +{
> +    Object *obj = OBJECT(md);
> +
> +    /* always use the ID of the proxy device for virtio devices */
> +    if (object_dynamic_cast(obj, TYPE_VIRTIO_DEVICE)) {
> +        if (obj->parent && object_dynamic_cast(obj->parent, TYPE_DEVICE)) {
> +            const DeviceState *parent_dev = DEVICE(obj->parent);
> +
> +            return parent_dev->id;
> +        }
> +        return NULL;
> +    }
> +    return DEVICE(md)->id;
> +}
> +
>  static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
>  {
>      const MemoryDeviceState *md_a = MEMORY_DEVICE(a);
> @@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>      for (item = list; item; item = g_slist_next(item)) {
>          MemoryDeviceState *md = item->data;
>          const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
> +        const char *id = memory_device_id(md);
>          uint64_t md_size, md_addr;
>  
>          md_addr = mdc->get_addr(md);
> @@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>  
>          if (ranges_overlap(md_addr, md_size, new_addr, size)) {
>              if (hint) {
> -                const DeviceState *d = DEVICE(md);
> -                error_setg(errp, "address range conflicts with '%s'", d->id);
> +                error_setg(errp, "address range conflicts with '%s'",
> +                           id ? id : 0);

What's that 'id ? id : 0' trick for?

Dave

>                  goto out;
>              }
>              new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
> index 05cb9437b7..324cc45b6f 100644
> --- a/include/hw/mem/memory-device.h
> +++ b/include/hw/mem/memory-device.h
> @@ -54,6 +54,7 @@ typedef struct MemoryDeviceClass {
>                               MemoryDeviceInfo *info);
>  } MemoryDeviceClass;
>  
> +const char *memory_device_id(const MemoryDeviceState *md);
>  MemoryDeviceInfoList *qmp_memory_device_list(void);
>  uint64_t get_plugged_memory_size(void);
>  void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

>>  static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
>>  {
>>      const MemoryDeviceState *md_a = MEMORY_DEVICE(a);
>> @@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>>      for (item = list; item; item = g_slist_next(item)) {
>>          MemoryDeviceState *md = item->data;
>>          const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
>> +        const char *id = memory_device_id(md);
>>          uint64_t md_size, md_addr;
>>  
>>          md_addr = mdc->get_addr(md);
>> @@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>>  
>>          if (ranges_overlap(md_addr, md_size, new_addr, size)) {
>>              if (hint) {
>> -                const DeviceState *d = DEVICE(md);
>> -                error_setg(errp, "address range conflicts with '%s'", d->id);
>> +                error_setg(errp, "address range conflicts with '%s'",
>> +                           id ? id : 0);
> 
> What's that 'id ? id : 0' trick for?

0 -> "", then it actually makes sense :)

I'll fix this up, thanks!


-- 

Thanks,

David / dhildenb

* David Hildenbrand (david@redhat.com) wrote:
> 
> >>  static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
> >>  {
> >>      const MemoryDeviceState *md_a = MEMORY_DEVICE(a);
> >> @@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
> >>      for (item = list; item; item = g_slist_next(item)) {
> >>          MemoryDeviceState *md = item->data;
> >>          const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
> >> +        const char *id = memory_device_id(md);
> >>          uint64_t md_size, md_addr;
> >>  
> >>          md_addr = mdc->get_addr(md);
> >> @@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
> >>  
> >>          if (ranges_overlap(md_addr, md_size, new_addr, size)) {
> >>              if (hint) {
> >> -                const DeviceState *d = DEVICE(md);
> >> -                error_setg(errp, "address range conflicts with '%s'", d->id);
> >> +                error_setg(errp, "address range conflicts with '%s'",
> >> +                           id ? id : 0);
> > 
> > What's that 'id ? id : 0' trick for?
> 
> 0 -> "", then it actually makes sense :)
> 
> I'll fix this up, thanks!

Except that:

   address range conflicts with ''
isn't very helpful.
Why would you get a NULL id ?

Dave

> 
> -- 
> 
> Thanks,
> 
> David / dhildenb
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 31.08.2018 12:43, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>>
>>>>  static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
>>>>  {
>>>>      const MemoryDeviceState *md_a = MEMORY_DEVICE(a);
>>>> @@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>>>>      for (item = list; item; item = g_slist_next(item)) {
>>>>          MemoryDeviceState *md = item->data;
>>>>          const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
>>>> +        const char *id = memory_device_id(md);
>>>>          uint64_t md_size, md_addr;
>>>>  
>>>>          md_addr = mdc->get_addr(md);
>>>> @@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>>>>  
>>>>          if (ranges_overlap(md_addr, md_size, new_addr, size)) {
>>>>              if (hint) {
>>>> -                const DeviceState *d = DEVICE(md);
>>>> -                error_setg(errp, "address range conflicts with '%s'", d->id);
>>>> +                error_setg(errp, "address range conflicts with '%s'",
>>>> +                           id ? id : 0);
>>>
>>> What's that 'id ? id : 0' trick for?
>>
>> 0 -> "", then it actually makes sense :)
>>
>> I'll fix this up, thanks!
> 
> Except that:
> 
>    address range conflicts with ''
> isn't very helpful.
> Why would you get a NULL id ?

This is easy: don't specify an id for a memory device:

Unfortunately, if the user does not give ids to devices, there is no way
of telling him what we are talking about.

qemu-system-x86_64 -machine pc -m 4G,maxmem=20G,slots=4 \
	-object memory-backend-ram,id=mem0,size=4G \
	-object memory-backend-ram,id=mem1,size=4G \
	-device pc-dimm,memdev=mem0,addr=0x140000000 \
	-device pc-dimm,memdev=mem1,addr=0x140000000

qemu-system-x86_64: -device pc-dimm,memdev=mem1,addr=0x140000000:
address range conflicts with '(null)'

(I thought providing NULL would lead to a crash, but it is actually
handled properly)

So while not being able to indicate an id is not nice, I can simply
forward the id directly here.

Thanks!

> 
> Dave
> 
>>
>> -- 
>>
>> Thanks,
>>
>> David / dhildenb
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 


-- 

Thanks,

David / dhildenb

* David Hildenbrand (david@redhat.com) wrote:
> On 31.08.2018 12:43, Dr. David Alan Gilbert wrote:
> > * David Hildenbrand (david@redhat.com) wrote:
> >>
> >>>>  static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
> >>>>  {
> >>>>      const MemoryDeviceState *md_a = MEMORY_DEVICE(a);
> >>>> @@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
> >>>>      for (item = list; item; item = g_slist_next(item)) {
> >>>>          MemoryDeviceState *md = item->data;
> >>>>          const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
> >>>> +        const char *id = memory_device_id(md);
> >>>>          uint64_t md_size, md_addr;
> >>>>  
> >>>>          md_addr = mdc->get_addr(md);
> >>>> @@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
> >>>>  
> >>>>          if (ranges_overlap(md_addr, md_size, new_addr, size)) {
> >>>>              if (hint) {
> >>>> -                const DeviceState *d = DEVICE(md);
> >>>> -                error_setg(errp, "address range conflicts with '%s'", d->id);
> >>>> +                error_setg(errp, "address range conflicts with '%s'",
> >>>> +                           id ? id : 0);
> >>>
> >>> What's that 'id ? id : 0' trick for?
> >>
> >> 0 -> "", then it actually makes sense :)
> >>
> >> I'll fix this up, thanks!
> > 
> > Except that:
> > 
> >    address range conflicts with ''
> > isn't very helpful.
> > Why would you get a NULL id ?
> 
> This is easy: don't specify an id for a memory device:
> 
> Unfortunately, if the user does not give ids to devices, there is no way
> of telling him what we are talking about.
> 
> qemu-system-x86_64 -machine pc -m 4G,maxmem=20G,slots=4 \
> 	-object memory-backend-ram,id=mem0,size=4G \
> 	-object memory-backend-ram,id=mem1,size=4G \
> 	-device pc-dimm,memdev=mem0,addr=0x140000000 \
> 	-device pc-dimm,memdev=mem1,addr=0x140000000
> 
> qemu-system-x86_64: -device pc-dimm,memdev=mem1,addr=0x140000000:
> address range conflicts with '(null)'
> 
> (I thought providing NULL would lead to a crash, but it is actually
> handled properly)
> 
> So while not being able to indicate an id is not nice, I can simply
> forward the id directly here.

OK, or use something like  id ? id : "(unnamed)"

Dave

> Thanks!
> 
> > 
> > Dave
> > 
> >>
> >> -- 
> >>
> >> Thanks,
> >>
> >> David / dhildenb
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
> 
> -- 
> 
> Thanks,
> 
> David / dhildenb
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 08/31/2018 06:18 AM, David Hildenbrand wrote:

>>>>> -                error_setg(errp, "address range conflicts with '%s'", d->id);
>>>>> +                error_setg(errp, "address range conflicts with '%s'",
>>>>> +                           id ? id : 0);
>>>>
>>>> What's that 'id ? id : 0' trick for?
>>>
>>> 0 -> "", then it actually makes sense :)
>>>

> 
> qemu-system-x86_64: -device pc-dimm,memdev=mem1,addr=0x140000000:
> address range conflicts with '(null)'
> 
> (I thought providing NULL would lead to a crash, but it is actually
> handled properly)

Well, glibc handles it. But POSIX says it is undefined, and there are 
other libc where it indeed crashes.  It's better to pass an explicit 
non-null placeholder than to rely on glibc turning NULL into "(null)".

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Wed, Aug 29, 2018 at 05:36:18PM +0200, David Hildenbrand wrote:
> When reporting the id of virtio-based memory devices, we always have to
> take the one of the proxy device (parent).
> 
> Expose the function, so especially virtio-based memory devices can
> reuse the function when filling out the id in MemoryDeviceInfo.
> 
> Details:
> 
> When the user creates a virtio device (e.g.  virtio-balloon-pci), two
> devices are actually created.
> 
> 1. Virtio proxy device (e.g. TYPE_VIRTIO_BALLOON_PCI)
> 2. The "real" virtio device (e.g. TYPE_VIRTIO_BALLOON).
> 
> 1. aliases all properties of 2, so 2. can be properly configured using 1.
> 1. gets the device ID set specified by the user. 2. gets no ID set.
> 
> If we want to make 2. a MemoryDevice but report errors/information to the
> user, we always have to report the id of 1. (because that's the device the
> user instantiated and configured).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/memory-device.c         | 21 +++++++++++++++++++--
>  include/hw/mem/memory-device.h |  1 +
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index a31ba73ea7..89a0c584be 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -19,6 +19,22 @@
>  #include "sysemu/kvm.h"
>  #include "trace.h"
>  
> +const char *memory_device_id(const MemoryDeviceState *md)
> +{
> +    Object *obj = OBJECT(md);
> +
> +    /* always use the ID of the proxy device for virtio devices */
> +    if (object_dynamic_cast(obj, TYPE_VIRTIO_DEVICE)) {
> +        if (obj->parent && object_dynamic_cast(obj->parent, TYPE_DEVICE)) {
> +            const DeviceState *parent_dev = DEVICE(obj->parent);
> +
> +            return parent_dev->id;
> +        }
> +        return NULL;

I don't like having virtio-specific code on memory-device.c.
What about making it generic?  Let device 2 register a read-only
property for the user-visible ID, and make memory_device_id() use
that property if it's present.


> +    }
> +    return DEVICE(md)->id;
> +}
> +
>  static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
>  {
>      const MemoryDeviceState *md_a = MEMORY_DEVICE(a);
> @@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>      for (item = list; item; item = g_slist_next(item)) {
>          MemoryDeviceState *md = item->data;
>          const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
> +        const char *id = memory_device_id(md);
>          uint64_t md_size, md_addr;
>  
>          md_addr = mdc->get_addr(md);
> @@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>  
>          if (ranges_overlap(md_addr, md_size, new_addr, size)) {
>              if (hint) {
> -                const DeviceState *d = DEVICE(md);
> -                error_setg(errp, "address range conflicts with '%s'", d->id);
> +                error_setg(errp, "address range conflicts with '%s'",
> +                           id ? id : 0);
>                  goto out;
>              }
>              new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
> index 05cb9437b7..324cc45b6f 100644
> --- a/include/hw/mem/memory-device.h
> +++ b/include/hw/mem/memory-device.h
> @@ -54,6 +54,7 @@ typedef struct MemoryDeviceClass {
>                               MemoryDeviceInfo *info);
>  } MemoryDeviceClass;
>  
> +const char *memory_device_id(const MemoryDeviceState *md);
>  MemoryDeviceInfoList *qmp_memory_device_list(void);
>  uint64_t get_plugged_memory_size(void);
>  void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
> -- 
> 2.17.1
> 
> 

-- 
Eduardo

On 31.08.2018 13:23, Eduardo Habkost wrote:
> On Wed, Aug 29, 2018 at 05:36:18PM +0200, David Hildenbrand wrote:
>> When reporting the id of virtio-based memory devices, we always have to
>> take the one of the proxy device (parent).
>>
>> Expose the function, so especially virtio-based memory devices can
>> reuse the function when filling out the id in MemoryDeviceInfo.
>>
>> Details:
>>
>> When the user creates a virtio device (e.g.  virtio-balloon-pci), two
>> devices are actually created.
>>
>> 1. Virtio proxy device (e.g. TYPE_VIRTIO_BALLOON_PCI)
>> 2. The "real" virtio device (e.g. TYPE_VIRTIO_BALLOON).
>>
>> 1. aliases all properties of 2, so 2. can be properly configured using 1.
>> 1. gets the device ID set specified by the user. 2. gets no ID set.
>>
>> If we want to make 2. a MemoryDevice but report errors/information to the
>> user, we always have to report the id of 1. (because that's the device the
>> user instantiated and configured).
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/mem/memory-device.c         | 21 +++++++++++++++++++--
>>  include/hw/mem/memory-device.h |  1 +
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> index a31ba73ea7..89a0c584be 100644
>> --- a/hw/mem/memory-device.c
>> +++ b/hw/mem/memory-device.c
>> @@ -19,6 +19,22 @@
>>  #include "sysemu/kvm.h"
>>  #include "trace.h"
>>  
>> +const char *memory_device_id(const MemoryDeviceState *md)
>> +{
>> +    Object *obj = OBJECT(md);
>> +
>> +    /* always use the ID of the proxy device for virtio devices */
>> +    if (object_dynamic_cast(obj, TYPE_VIRTIO_DEVICE)) {
>> +        if (obj->parent && object_dynamic_cast(obj->parent, TYPE_DEVICE)) {
>> +            const DeviceState *parent_dev = DEVICE(obj->parent);
>> +
>> +            return parent_dev->id;
>> +        }
>> +        return NULL;
> 
> I don't like having virtio-specific code on memory-device.c.
> What about making it generic?  Let device 2 register a read-only
> property for the user-visible ID, and make memory_device_id() use
> that property if it's present.

Valid point. Or avoid properties and add a function to the memory-device
class?


-- 

Thanks,

David / dhildenb

On Fri, Aug 31, 2018 at 01:26:03PM +0200, David Hildenbrand wrote:
> On 31.08.2018 13:23, Eduardo Habkost wrote:
> > On Wed, Aug 29, 2018 at 05:36:18PM +0200, David Hildenbrand wrote:
> >> When reporting the id of virtio-based memory devices, we always have to
> >> take the one of the proxy device (parent).
> >>
> >> Expose the function, so especially virtio-based memory devices can
> >> reuse the function when filling out the id in MemoryDeviceInfo.
> >>
> >> Details:
> >>
> >> When the user creates a virtio device (e.g.  virtio-balloon-pci), two
> >> devices are actually created.
> >>
> >> 1. Virtio proxy device (e.g. TYPE_VIRTIO_BALLOON_PCI)
> >> 2. The "real" virtio device (e.g. TYPE_VIRTIO_BALLOON).
> >>
> >> 1. aliases all properties of 2, so 2. can be properly configured using 1.
> >> 1. gets the device ID set specified by the user. 2. gets no ID set.
> >>
> >> If we want to make 2. a MemoryDevice but report errors/information to the
> >> user, we always have to report the id of 1. (because that's the device the
> >> user instantiated and configured).
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/mem/memory-device.c         | 21 +++++++++++++++++++--
> >>  include/hw/mem/memory-device.h |  1 +
> >>  2 files changed, 20 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> >> index a31ba73ea7..89a0c584be 100644
> >> --- a/hw/mem/memory-device.c
> >> +++ b/hw/mem/memory-device.c
> >> @@ -19,6 +19,22 @@
> >>  #include "sysemu/kvm.h"
> >>  #include "trace.h"
> >>  
> >> +const char *memory_device_id(const MemoryDeviceState *md)
> >> +{
> >> +    Object *obj = OBJECT(md);
> >> +
> >> +    /* always use the ID of the proxy device for virtio devices */
> >> +    if (object_dynamic_cast(obj, TYPE_VIRTIO_DEVICE)) {
> >> +        if (obj->parent && object_dynamic_cast(obj->parent, TYPE_DEVICE)) {
> >> +            const DeviceState *parent_dev = DEVICE(obj->parent);
> >> +
> >> +            return parent_dev->id;
> >> +        }
> >> +        return NULL;
> > 
> > I don't like having virtio-specific code on memory-device.c.
> > What about making it generic?  Let device 2 register a read-only
> > property for the user-visible ID, and make memory_device_id() use
> > that property if it's present.
> 
> Valid point. Or avoid properties and add a function to the memory-device
> class?

That works too, and it was my first thought.  But if you want a
method whose only purpose is to return a single value without
affecting object state, a QOM property seems like a perfect fit.

Either of those options would be good enough for me, though.

-- 
Eduardo

>>> I don't like having virtio-specific code on memory-device.c.
>>> What about making it generic?  Let device 2 register a read-only
>>> property for the user-visible ID, and make memory_device_id() use
>>> that property if it's present.
>>
>> Valid point. Or avoid properties and add a function to the memory-device
>> class?
> 
> That works too, and it was my first thought.  But if you want a
> method whose only purpose is to return a single value without
> affecting object state, a QOM property seems like a perfect fit.
> 
> Either of those options would be good enough for me, though.
> 

The function would only have to be defined for those overwriting it.
Will have a look. Thanks!

-- 

Thanks,

David / dhildenb