diff mbox series

[v3,16/22] memory-device: add optional function get_device_id()

Message ID 20180920103243.28474-17-david@redhat.com (mailing list archive)
State New, archived
Headers show
Series memory-device: complete refactoring + virtio-pmem | expand

Commit Message

David Hildenbrand Sept. 20, 2018, 10:32 a.m. UTC
When reporting the id of virtio-based memory devices, we always have to
take the one of the proxy device (parent), not the one of the memory
device directly.

Let's generalize this by allowing memory devices to specify an optional
"get_device_id" function. This id can then be used to report errors to the
user from memory-device.c code, without having to special case e.g.
virtio devices.

While at it, properly treat id == NULL and report "(unnamed)" instead.

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         | 7 +++++--
 include/hw/mem/memory-device.h | 4 ++++
 2 files changed, 9 insertions(+), 2 deletions(-)

Comments

David Gibson Sept. 24, 2018, 2:13 a.m. UTC | #1
On Thu, Sep 20, 2018 at 12:32:37PM +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), not the one of the memory
> device directly.
> 
> Let's generalize this by allowing memory devices to specify an optional
> "get_device_id" function. This id can then be used to report errors to the
> user from memory-device.c code, without having to special case e.g.
> virtio devices.
> 
> While at it, properly treat id == NULL and report "(unnamed)" instead.
> 
> 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: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/mem/memory-device.c         | 7 +++++--
>  include/hw/mem/memory-device.h | 4 ++++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index 534bd38313..92878fc327 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -174,8 +174,11 @@ 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);
> +                const char *id = mdc->get_device_id ? mdc->get_device_id(md) :
> +                                 DEVICE(md)->id;
> +
> +                error_setg(errp, "address range conflicts with '%s'",
> +                           id ? id : "(unnamed)");
>                  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 fd0b43c224..66143cffc6 100644
> --- a/include/hw/mem/memory-device.h
> +++ b/include/hw/mem/memory-device.h
> @@ -42,6 +42,9 @@ typedef struct MemoryDeviceState {
>   * of multiple successive memory regions, a covering memory region is to
>   * be used. Scattered memory regions are not supported for single devices.
>   * @fill_device_info: Translate current @md state into #MemoryDeviceInfo.
> + * @get_device_id: Optional. Allows memory devices behind proxy devices
> + * (e.g. virtio based) to report the id of the proxy device to the user
> + * instead of the (empty) id of the memory device.
>   */
>  typedef struct MemoryDeviceClass {
>      /* private */
> @@ -54,6 +57,7 @@ typedef struct MemoryDeviceClass {
>      MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);
>      void (*fill_device_info)(const MemoryDeviceState *md,
>                               MemoryDeviceInfo *info);
> +    const char *(*get_device_id)(const MemoryDeviceState *md);
>  } MemoryDeviceClass;
>  
>  MemoryDeviceInfoList *qmp_memory_device_list(void);
Igor Mammedov Sept. 24, 2018, 2:40 p.m. UTC | #2
On Thu, 20 Sep 2018 12:32:37 +0200
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), not the one of the memory
> device directly.
> 
> Let's generalize this by allowing memory devices to specify an optional
> "get_device_id" function. This id can then be used to report errors to the
> user from memory-device.c code, without having to special case e.g.
> virtio devices.
> 
> While at it, properly treat id == NULL and report "(unnamed)" instead.
> 
> 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         | 7 +++++--
>  include/hw/mem/memory-device.h | 4 ++++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index 534bd38313..92878fc327 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -174,8 +174,11 @@ 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);
> +                const char *id = mdc->get_device_id ? mdc->get_device_id(md) :
> +                                 DEVICE(md)->id;
it could be better if default MemoryDeviceClass::get_device_id() would return
DEVICE(md)->id and we override it for virtio based devices later.
Then get_device_id() could be reused direcly elsewhere without a conditional assign.

> +
> +                error_setg(errp, "address range conflicts with '%s'",
> +                           id ? id : "(unnamed)");
>                  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 fd0b43c224..66143cffc6 100644
> --- a/include/hw/mem/memory-device.h
> +++ b/include/hw/mem/memory-device.h
> @@ -42,6 +42,9 @@ typedef struct MemoryDeviceState {
>   * of multiple successive memory regions, a covering memory region is to
>   * be used. Scattered memory regions are not supported for single devices.
>   * @fill_device_info: Translate current @md state into #MemoryDeviceInfo.
> + * @get_device_id: Optional. Allows memory devices behind proxy devices
> + * (e.g. virtio based) to report the id of the proxy device to the user
> + * instead of the (empty) id of the memory device.
>   */
>  typedef struct MemoryDeviceClass {
>      /* private */
> @@ -54,6 +57,7 @@ typedef struct MemoryDeviceClass {
>      MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);
>      void (*fill_device_info)(const MemoryDeviceState *md,
>                               MemoryDeviceInfo *info);
> +    const char *(*get_device_id)(const MemoryDeviceState *md);
>  } MemoryDeviceClass;
>  
>  MemoryDeviceInfoList *qmp_memory_device_list(void);
David Hildenbrand Sept. 25, 2018, 8:09 p.m. UTC | #3
On 24/09/2018 16:40, Igor Mammedov wrote:
> On Thu, 20 Sep 2018 12:32:37 +0200
> 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), not the one of the memory
>> device directly.
>>
>> Let's generalize this by allowing memory devices to specify an optional
>> "get_device_id" function. This id can then be used to report errors to the
>> user from memory-device.c code, without having to special case e.g.
>> virtio devices.
>>
>> While at it, properly treat id == NULL and report "(unnamed)" instead.
>>
>> 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         | 7 +++++--
>>  include/hw/mem/memory-device.h | 4 ++++
>>  2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> index 534bd38313..92878fc327 100644
>> --- a/hw/mem/memory-device.c
>> +++ b/hw/mem/memory-device.c
>> @@ -174,8 +174,11 @@ 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);
>> +                const char *id = mdc->get_device_id ? mdc->get_device_id(md) :
>> +                                 DEVICE(md)->id;
> it could be better if default MemoryDeviceClass::get_device_id() would return
> DEVICE(md)->id and we override it for virtio based devices later.
> Then get_device_id() could be reused direcly elsewhere without a conditional assign.
> 

I wonder if default implementations are possible for interfaces, I will
give it a try.
diff mbox series

Patch

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 534bd38313..92878fc327 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -174,8 +174,11 @@  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);
+                const char *id = mdc->get_device_id ? mdc->get_device_id(md) :
+                                 DEVICE(md)->id;
+
+                error_setg(errp, "address range conflicts with '%s'",
+                           id ? id : "(unnamed)");
                 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 fd0b43c224..66143cffc6 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -42,6 +42,9 @@  typedef struct MemoryDeviceState {
  * of multiple successive memory regions, a covering memory region is to
  * be used. Scattered memory regions are not supported for single devices.
  * @fill_device_info: Translate current @md state into #MemoryDeviceInfo.
+ * @get_device_id: Optional. Allows memory devices behind proxy devices
+ * (e.g. virtio based) to report the id of the proxy device to the user
+ * instead of the (empty) id of the memory device.
  */
 typedef struct MemoryDeviceClass {
     /* private */
@@ -54,6 +57,7 @@  typedef struct MemoryDeviceClass {
     MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);
     void (*fill_device_info)(const MemoryDeviceState *md,
                              MemoryDeviceInfo *info);
+    const char *(*get_device_id)(const MemoryDeviceState *md);
 } MemoryDeviceClass;
 
 MemoryDeviceInfoList *qmp_memory_device_list(void);