| Message ID | 20180926094219.20322-18-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | memory-device: complete refactoring + virtio-pmem | expand |
On Wed, 26 Sep 2018 11:42:12 +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. Provide a default function that can be overridden. > > 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). > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > Signed-off-by: David Hildenbrand <david@redhat.com> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/mem/memory-device.c | 19 +++++++++++++++++-- > include/hw/mem/memory-device.h | 5 +++++ > 2 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c > index cf85199a72..6dc40e8764 100644 > --- a/hw/mem/memory-device.c > +++ b/hw/mem/memory-device.c > @@ -174,8 +174,10 @@ 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(md); > + > + error_setg(errp, "address range conflicts with '%s'", > + id ? id : "(unnamed)"); > goto out; > } > new_addr = QEMU_ALIGN_UP(md_addr + md_size, align); > @@ -328,10 +330,23 @@ uint64_t memory_device_get_region_size(const MemoryDeviceState *md, > return memory_region_size(mr); > } > > +static const char *memory_device_get_device_id(const MemoryDeviceState *md) > +{ > + return DEVICE(md)->id; > +} > + > +static void memory_device_class_init(ObjectClass *oc, void *data) > +{ > + MemoryDeviceClass *mdc = MEMORY_DEVICE_CLASS(oc); > + > + mdc->get_device_id = memory_device_get_device_id; > +} > + > static const TypeInfo memory_device_info = { > .name = TYPE_MEMORY_DEVICE, > .parent = TYPE_INTERFACE, > .class_size = sizeof(MemoryDeviceClass), > + .class_init = memory_device_class_init, > }; > > static void memory_device_register_types(void) > diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h > index 97de693bca..52fc117ce2 100644 > --- a/include/hw/mem/memory-device.h > +++ b/include/hw/mem/memory-device.h > @@ -45,6 +45,10 @@ typedef struct MemoryDeviceState { > * 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: 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. The default > + * implementation (unless overridden) will return the ordinary device id. > */ > typedef struct MemoryDeviceClass { > /* private */ > @@ -57,6 +61,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);
Hi David, On 9/26/18 11:42 AM, 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. Provide a default function that can be overridden. > > 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. s/set// ? > > If we want to make 2. a MemoryDevice but report errors/information to the to be reworded? > user, we always have to report the id of 1. (because that's the device the > user instantiated and configured). > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/mem/memory-device.c | 19 +++++++++++++++++-- > include/hw/mem/memory-device.h | 5 +++++ > 2 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c > index cf85199a72..6dc40e8764 100644 > --- a/hw/mem/memory-device.c > +++ b/hw/mem/memory-device.c > @@ -174,8 +174,10 @@ 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(md); > + > + error_setg(errp, "address range conflicts with '%s'", address range conflicts with memory device id='%s"? Thanks Eric > + id ? id : "(unnamed)")> goto out; > } > new_addr = QEMU_ALIGN_UP(md_addr + md_size, align); > @@ -328,10 +330,23 @@ uint64_t memory_device_get_region_size(const MemoryDeviceState *md, > return memory_region_size(mr); > } > > +static const char *memory_device_get_device_id(const MemoryDeviceState *md) > +{ > + return DEVICE(md)->id; > +} > + > +static void memory_device_class_init(ObjectClass *oc, void *data) > +{ > + MemoryDeviceClass *mdc = MEMORY_DEVICE_CLASS(oc); > + > + mdc->get_device_id = memory_device_get_device_id; > +} > + > static const TypeInfo memory_device_info = { > .name = TYPE_MEMORY_DEVICE, > .parent = TYPE_INTERFACE, > .class_size = sizeof(MemoryDeviceClass), > + .class_init = memory_device_class_init, > }; > > static void memory_device_register_types(void) > diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h > index 97de693bca..52fc117ce2 100644 > --- a/include/hw/mem/memory-device.h > +++ b/include/hw/mem/memory-device.h > @@ -45,6 +45,10 @@ typedef struct MemoryDeviceState { > * 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: 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. The default > + * implementation (unless overridden) will return the ordinary device id. > */ > typedef struct MemoryDeviceClass { > /* private */ > @@ -57,6 +61,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); >
On 30/09/2018 18:19, Auger Eric wrote: > Hi David, > > On 9/26/18 11:42 AM, 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. Provide a default function that can be overridden. >> >> 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. > s/set// ? >> >> If we want to make 2. a MemoryDevice but report errors/information to the > to be reworded? >> user, we always have to report the id of 1. (because that's the device the >> user instantiated and configured). Both parts slightly reworded. >> >> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/mem/memory-device.c | 19 +++++++++++++++++-- >> include/hw/mem/memory-device.h | 5 +++++ >> 2 files changed, 22 insertions(+), 2 deletions(-) >> >> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c >> index cf85199a72..6dc40e8764 100644 >> --- a/hw/mem/memory-device.c >> +++ b/hw/mem/memory-device.c >> @@ -174,8 +174,10 @@ 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(md); >> + >> + error_setg(errp, "address range conflicts with '%s'", > address range conflicts with memory device id='%s"? Yes, I can do that. Thanks!
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c index cf85199a72..6dc40e8764 100644 --- a/hw/mem/memory-device.c +++ b/hw/mem/memory-device.c @@ -174,8 +174,10 @@ 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(md); + + error_setg(errp, "address range conflicts with '%s'", + id ? id : "(unnamed)"); goto out; } new_addr = QEMU_ALIGN_UP(md_addr + md_size, align); @@ -328,10 +330,23 @@ uint64_t memory_device_get_region_size(const MemoryDeviceState *md, return memory_region_size(mr); } +static const char *memory_device_get_device_id(const MemoryDeviceState *md) +{ + return DEVICE(md)->id; +} + +static void memory_device_class_init(ObjectClass *oc, void *data) +{ + MemoryDeviceClass *mdc = MEMORY_DEVICE_CLASS(oc); + + mdc->get_device_id = memory_device_get_device_id; +} + static const TypeInfo memory_device_info = { .name = TYPE_MEMORY_DEVICE, .parent = TYPE_INTERFACE, .class_size = sizeof(MemoryDeviceClass), + .class_init = memory_device_class_init, }; static void memory_device_register_types(void) diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h index 97de693bca..52fc117ce2 100644 --- a/include/hw/mem/memory-device.h +++ b/include/hw/mem/memory-device.h @@ -45,6 +45,10 @@ typedef struct MemoryDeviceState { * 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: 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. The default + * implementation (unless overridden) will return the ordinary device id. */ typedef struct MemoryDeviceClass { /* private */ @@ -57,6 +61,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);