On Thu, 20 Sep 2018 12:32:37 +0200
David Hildenbrand <da...@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 <da...@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);
