* [Qemu-devel] [PATCH 0/2] some remaining vmgenid tweaks for 2.9 @ 2017-03-20 11:59 Laszlo Ersek 2017-03-20 11:59 ` [Qemu-devel] [PATCH 1/2] hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types Laszlo Ersek 2017-03-20 11:59 ` [Qemu-devel] [PATCH 2/2] hw/acpi/vmgenid: prevent more than one vmgenid device Laszlo Ersek 0 siblings, 2 replies; 14+ messages in thread From: Laszlo Ersek @ 2017-03-20 11:59 UTC (permalink / raw) To: qemu devel list; +Cc: Michael S. Tsirkin, Ben Warren, Igor Mammedov We've discussed both of these earlier (multiple times), and I'd been hoping that Ben would submit them. I guess I can send out a reminder formatted as a patch series. (I don't intend to post several iterations of this.) Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Ben Warren <ben@skyportsystems.com> Cc: Igor Mammedov <imammedo@redhat.com> Thanks Laszlo Laszlo Ersek (2): hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types hw/acpi/vmgenid: prevent more than one vmgenid device include/hw/acpi/vmgenid.h | 1 + include/hw/compat.h | 4 ++++ hw/acpi/vmgenid.c | 24 ++++++++++++++++++++++++ 3 files changed, 29 insertions(+) -- 2.9.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 1/2] hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types 2017-03-20 11:59 [Qemu-devel] [PATCH 0/2] some remaining vmgenid tweaks for 2.9 Laszlo Ersek @ 2017-03-20 11:59 ` Laszlo Ersek 2017-03-20 14:19 ` Michael S. Tsirkin 2017-03-20 11:59 ` [Qemu-devel] [PATCH 2/2] hw/acpi/vmgenid: prevent more than one vmgenid device Laszlo Ersek 1 sibling, 1 reply; 14+ messages in thread From: Laszlo Ersek @ 2017-03-20 11:59 UTC (permalink / raw) To: qemu devel list; +Cc: Michael S. Tsirkin, Ben Warren, Igor Mammedov The WRITE_POINTER linker/loader command that underlies VMGENID depends on commit baf2d5bfbac0 ("fw-cfg: support writeable blobs", 2017-01-12). That commit is not available in 2.8. Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Ben Warren <ben@skyportsystems.com> Cc: Igor Mammedov <imammedo@redhat.com> Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- include/hw/acpi/vmgenid.h | 1 + include/hw/compat.h | 4 ++++ hw/acpi/vmgenid.c | 14 ++++++++++++++ 3 files changed, 19 insertions(+) diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h index db7fa0e63303..8578476baebe 100644 --- a/include/hw/acpi/vmgenid.h +++ b/include/hw/acpi/vmgenid.h @@ -21,6 +21,7 @@ typedef struct VmGenIdState { DeviceClass parent_obj; QemuUUID guid; /* The 128-bit GUID seen by the guest */ uint8_t vmgenid_addr_le[8]; /* Address of the GUID (little-endian) */ + bool write_pointer_available; } VmGenIdState; static inline Object *find_vmgenid_dev(void) diff --git a/include/hw/compat.h b/include/hw/compat.h index fc8c3e060007..c9caa8cd1bdd 100644 --- a/include/hw/compat.h +++ b/include/hw/compat.h @@ -42,6 +42,10 @@ .driver = "isa-cirrus-vga",\ .property = "vgamem_mb",\ .value = "8",\ + },{\ + .driver = "vmgenid",\ + .property = "x-write-pointer-available",\ + .value = "off",\ }, #define HW_COMPAT_2_7 \ diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c index 7a3ad17d66ef..c3ddcc8e7cb0 100644 --- a/hw/acpi/vmgenid.c +++ b/hw/acpi/vmgenid.c @@ -205,9 +205,22 @@ static void vmgenid_handle_reset(void *opaque) memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le)); } +static Property vmgenid_properties[] = { + DEFINE_PROP_BOOL("x-write-pointer-available", VmGenIdState, + write_pointer_available, true), + DEFINE_PROP_END_OF_LIST(), +}; + static void vmgenid_realize(DeviceState *dev, Error **errp) { VmGenIdState *vms = VMGENID(dev); + + if (!vms->write_pointer_available) { + error_setg(errp, "%s requires DMA write support in fw_cfg, " + "which this machine type does not provide", VMGENID_DEVICE); + return; + } + qemu_register_reset(vmgenid_handle_reset, vms); } @@ -218,6 +231,7 @@ static void vmgenid_device_class_init(ObjectClass *klass, void *data) dc->vmsd = &vmstate_vmgenid; dc->realize = vmgenid_realize; dc->hotpluggable = false; + dc->props = vmgenid_properties; object_class_property_add_str(klass, VMGENID_GUID, NULL, vmgenid_set_guid, NULL); -- 2.9.3 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types 2017-03-20 11:59 ` [Qemu-devel] [PATCH 1/2] hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types Laszlo Ersek @ 2017-03-20 14:19 ` Michael S. Tsirkin 2017-03-20 14:39 ` Paolo Bonzini 0 siblings, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2017-03-20 14:19 UTC (permalink / raw) To: Laszlo Ersek; +Cc: qemu devel list, Ben Warren, Igor Mammedov On Mon, Mar 20, 2017 at 12:59:50PM +0100, Laszlo Ersek wrote: > The WRITE_POINTER linker/loader command that underlies VMGENID depends on > commit baf2d5bfbac0 ("fw-cfg: support writeable blobs", 2017-01-12). That > commit is not available in 2.8. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Ben Warren <ben@skyportsystems.com> > Cc: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> I don't understand why do we want to add code to break this intentionally. What issue does this fix? > --- > include/hw/acpi/vmgenid.h | 1 + > include/hw/compat.h | 4 ++++ > hw/acpi/vmgenid.c | 14 ++++++++++++++ > 3 files changed, 19 insertions(+) > > diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h > index db7fa0e63303..8578476baebe 100644 > --- a/include/hw/acpi/vmgenid.h > +++ b/include/hw/acpi/vmgenid.h > @@ -21,6 +21,7 @@ typedef struct VmGenIdState { > DeviceClass parent_obj; > QemuUUID guid; /* The 128-bit GUID seen by the guest */ > uint8_t vmgenid_addr_le[8]; /* Address of the GUID (little-endian) */ > + bool write_pointer_available; > } VmGenIdState; > > static inline Object *find_vmgenid_dev(void) > diff --git a/include/hw/compat.h b/include/hw/compat.h > index fc8c3e060007..c9caa8cd1bdd 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -42,6 +42,10 @@ > .driver = "isa-cirrus-vga",\ > .property = "vgamem_mb",\ > .value = "8",\ > + },{\ > + .driver = "vmgenid",\ > + .property = "x-write-pointer-available",\ > + .value = "off",\ > }, > > #define HW_COMPAT_2_7 \ > diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c > index 7a3ad17d66ef..c3ddcc8e7cb0 100644 > --- a/hw/acpi/vmgenid.c > +++ b/hw/acpi/vmgenid.c > @@ -205,9 +205,22 @@ static void vmgenid_handle_reset(void *opaque) > memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le)); > } > > +static Property vmgenid_properties[] = { > + DEFINE_PROP_BOOL("x-write-pointer-available", VmGenIdState, > + write_pointer_available, true), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > static void vmgenid_realize(DeviceState *dev, Error **errp) > { > VmGenIdState *vms = VMGENID(dev); > + > + if (!vms->write_pointer_available) { > + error_setg(errp, "%s requires DMA write support in fw_cfg, " > + "which this machine type does not provide", VMGENID_DEVICE); > + return; > + } > + > qemu_register_reset(vmgenid_handle_reset, vms); > } > > @@ -218,6 +231,7 @@ static void vmgenid_device_class_init(ObjectClass *klass, void *data) > dc->vmsd = &vmstate_vmgenid; > dc->realize = vmgenid_realize; > dc->hotpluggable = false; > + dc->props = vmgenid_properties; > > object_class_property_add_str(klass, VMGENID_GUID, NULL, > vmgenid_set_guid, NULL); > -- > 2.9.3 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types 2017-03-20 14:19 ` Michael S. Tsirkin @ 2017-03-20 14:39 ` Paolo Bonzini 2017-03-20 15:08 ` Laszlo Ersek 0 siblings, 1 reply; 14+ messages in thread From: Paolo Bonzini @ 2017-03-20 14:39 UTC (permalink / raw) To: Michael S. Tsirkin, Laszlo Ersek Cc: Igor Mammedov, qemu devel list, Ben Warren On 20/03/2017 15:19, Michael S. Tsirkin wrote: > On Mon, Mar 20, 2017 at 12:59:50PM +0100, Laszlo Ersek wrote: >> The WRITE_POINTER linker/loader command that underlies VMGENID depends on >> commit baf2d5bfbac0 ("fw-cfg: support writeable blobs", 2017-01-12). That >> commit is not available in 2.8. >> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Ben Warren <ben@skyportsystems.com> >> Cc: Igor Mammedov <imammedo@redhat.com> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > I don't understand why do we want to add code to break this > intentionally. What issue does this fix? Agreed. If vmgenid can work with old machine types and new QEMU, it should. For example, when using libvirt machine types identify the machine type where the VM was _created_ (unless someone changes it), not necessarily the minimal QEMU version that can run it. Paolo >> --- >> include/hw/acpi/vmgenid.h | 1 + >> include/hw/compat.h | 4 ++++ >> hw/acpi/vmgenid.c | 14 ++++++++++++++ >> 3 files changed, 19 insertions(+) >> >> diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h >> index db7fa0e63303..8578476baebe 100644 >> --- a/include/hw/acpi/vmgenid.h >> +++ b/include/hw/acpi/vmgenid.h >> @@ -21,6 +21,7 @@ typedef struct VmGenIdState { >> DeviceClass parent_obj; >> QemuUUID guid; /* The 128-bit GUID seen by the guest */ >> uint8_t vmgenid_addr_le[8]; /* Address of the GUID (little-endian) */ >> + bool write_pointer_available; >> } VmGenIdState; >> >> static inline Object *find_vmgenid_dev(void) >> diff --git a/include/hw/compat.h b/include/hw/compat.h >> index fc8c3e060007..c9caa8cd1bdd 100644 >> --- a/include/hw/compat.h >> +++ b/include/hw/compat.h >> @@ -42,6 +42,10 @@ >> .driver = "isa-cirrus-vga",\ >> .property = "vgamem_mb",\ >> .value = "8",\ >> + },{\ >> + .driver = "vmgenid",\ >> + .property = "x-write-pointer-available",\ >> + .value = "off",\ >> }, >> >> #define HW_COMPAT_2_7 \ >> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c >> index 7a3ad17d66ef..c3ddcc8e7cb0 100644 >> --- a/hw/acpi/vmgenid.c >> +++ b/hw/acpi/vmgenid.c >> @@ -205,9 +205,22 @@ static void vmgenid_handle_reset(void *opaque) >> memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le)); >> } >> >> +static Property vmgenid_properties[] = { >> + DEFINE_PROP_BOOL("x-write-pointer-available", VmGenIdState, >> + write_pointer_available, true), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> static void vmgenid_realize(DeviceState *dev, Error **errp) >> { >> VmGenIdState *vms = VMGENID(dev); >> + >> + if (!vms->write_pointer_available) { >> + error_setg(errp, "%s requires DMA write support in fw_cfg, " >> + "which this machine type does not provide", VMGENID_DEVICE); >> + return; >> + } >> + >> qemu_register_reset(vmgenid_handle_reset, vms); >> } >> >> @@ -218,6 +231,7 @@ static void vmgenid_device_class_init(ObjectClass *klass, void *data) >> dc->vmsd = &vmstate_vmgenid; >> dc->realize = vmgenid_realize; >> dc->hotpluggable = false; >> + dc->props = vmgenid_properties; >> >> object_class_property_add_str(klass, VMGENID_GUID, NULL, >> vmgenid_set_guid, NULL); >> -- >> 2.9.3 >> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types 2017-03-20 14:39 ` Paolo Bonzini @ 2017-03-20 15:08 ` Laszlo Ersek 2017-03-20 15:09 ` Paolo Bonzini 0 siblings, 1 reply; 14+ messages in thread From: Laszlo Ersek @ 2017-03-20 15:08 UTC (permalink / raw) To: Paolo Bonzini, Michael S. Tsirkin Cc: Igor Mammedov, qemu devel list, Ben Warren On 03/20/17 15:39, Paolo Bonzini wrote: > > > On 20/03/2017 15:19, Michael S. Tsirkin wrote: >> On Mon, Mar 20, 2017 at 12:59:50PM +0100, Laszlo Ersek wrote: >>> The WRITE_POINTER linker/loader command that underlies VMGENID depends on >>> commit baf2d5bfbac0 ("fw-cfg: support writeable blobs", 2017-01-12). That >>> commit is not available in 2.8. >>> >>> Cc: "Michael S. Tsirkin" <mst@redhat.com> >>> Cc: Ben Warren <ben@skyportsystems.com> >>> Cc: Igor Mammedov <imammedo@redhat.com> >>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> >> I don't understand why do we want to add code to break this >> intentionally. What issue does this fix? > > Agreed. If vmgenid can work with old machine types and new QEMU, it > should. For example, when using libvirt machine types identify the > machine type where the VM was _created_ (unless someone changes it), not > necessarily the minimal QEMU version that can run it. Yes, I understand that. The problem is this: (1) machine types up to and including 2.4 do not enable DMA for fw_cfg. Without DMA, the WRITE_POINTER linker/loader commands have no chance of working in the firmware. (2) ... the other part of the problem was actually in my thinking, sorry about that. I forgot that that "-device vmgenid" itself is not available in QEMU releases prior to 2.9. So we shouldn't worry about the case where write support is missing from fw_cfg, although DMA is available. In this patch I tried to address (non-)problem (2), thereby covering (real) problem (1). I now see that (2) doesn't actually exist. (1) does have to be addressed however. If you agree, I'll respin this patch. Thanks Laszlo > > Paolo > >>> --- >>> include/hw/acpi/vmgenid.h | 1 + >>> include/hw/compat.h | 4 ++++ >>> hw/acpi/vmgenid.c | 14 ++++++++++++++ >>> 3 files changed, 19 insertions(+) >>> >>> diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h >>> index db7fa0e63303..8578476baebe 100644 >>> --- a/include/hw/acpi/vmgenid.h >>> +++ b/include/hw/acpi/vmgenid.h >>> @@ -21,6 +21,7 @@ typedef struct VmGenIdState { >>> DeviceClass parent_obj; >>> QemuUUID guid; /* The 128-bit GUID seen by the guest */ >>> uint8_t vmgenid_addr_le[8]; /* Address of the GUID (little-endian) */ >>> + bool write_pointer_available; >>> } VmGenIdState; >>> >>> static inline Object *find_vmgenid_dev(void) >>> diff --git a/include/hw/compat.h b/include/hw/compat.h >>> index fc8c3e060007..c9caa8cd1bdd 100644 >>> --- a/include/hw/compat.h >>> +++ b/include/hw/compat.h >>> @@ -42,6 +42,10 @@ >>> .driver = "isa-cirrus-vga",\ >>> .property = "vgamem_mb",\ >>> .value = "8",\ >>> + },{\ >>> + .driver = "vmgenid",\ >>> + .property = "x-write-pointer-available",\ >>> + .value = "off",\ >>> }, >>> >>> #define HW_COMPAT_2_7 \ >>> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c >>> index 7a3ad17d66ef..c3ddcc8e7cb0 100644 >>> --- a/hw/acpi/vmgenid.c >>> +++ b/hw/acpi/vmgenid.c >>> @@ -205,9 +205,22 @@ static void vmgenid_handle_reset(void *opaque) >>> memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le)); >>> } >>> >>> +static Property vmgenid_properties[] = { >>> + DEFINE_PROP_BOOL("x-write-pointer-available", VmGenIdState, >>> + write_pointer_available, true), >>> + DEFINE_PROP_END_OF_LIST(), >>> +}; >>> + >>> static void vmgenid_realize(DeviceState *dev, Error **errp) >>> { >>> VmGenIdState *vms = VMGENID(dev); >>> + >>> + if (!vms->write_pointer_available) { >>> + error_setg(errp, "%s requires DMA write support in fw_cfg, " >>> + "which this machine type does not provide", VMGENID_DEVICE); >>> + return; >>> + } >>> + >>> qemu_register_reset(vmgenid_handle_reset, vms); >>> } >>> >>> @@ -218,6 +231,7 @@ static void vmgenid_device_class_init(ObjectClass *klass, void *data) >>> dc->vmsd = &vmstate_vmgenid; >>> dc->realize = vmgenid_realize; >>> dc->hotpluggable = false; >>> + dc->props = vmgenid_properties; >>> >>> object_class_property_add_str(klass, VMGENID_GUID, NULL, >>> vmgenid_set_guid, NULL); >>> -- >>> 2.9.3 >>> >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types 2017-03-20 15:08 ` Laszlo Ersek @ 2017-03-20 15:09 ` Paolo Bonzini 0 siblings, 0 replies; 14+ messages in thread From: Paolo Bonzini @ 2017-03-20 15:09 UTC (permalink / raw) To: Laszlo Ersek, Michael S. Tsirkin Cc: Igor Mammedov, qemu devel list, Ben Warren On 20/03/2017 16:08, Laszlo Ersek wrote: > (1) machine types up to and including 2.4 do not enable DMA for fw_cfg. > Without DMA, the WRITE_POINTER linker/loader commands have no chance of > working in the firmware. > > (2) ... the other part of the problem was actually in my thinking, sorry > about that. I forgot that that "-device vmgenid" itself is not available > in QEMU releases prior to 2.9. So we shouldn't worry about the case > where write support is missing from fw_cfg, although DMA is available. > > In this patch I tried to address (non-)problem (2), thereby covering > (real) problem (1). I now see that (2) doesn't actually exist. (1) does > have to be addressed however. > > If you agree, I'll respin this patch. Yes, please do respin it with pre-2.4 machine types limited only. Paolo ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 2/2] hw/acpi/vmgenid: prevent more than one vmgenid device 2017-03-20 11:59 [Qemu-devel] [PATCH 0/2] some remaining vmgenid tweaks for 2.9 Laszlo Ersek 2017-03-20 11:59 ` [Qemu-devel] [PATCH 1/2] hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types Laszlo Ersek @ 2017-03-20 11:59 ` Laszlo Ersek 2017-03-20 14:16 ` Michael S. Tsirkin 1 sibling, 1 reply; 14+ messages in thread From: Laszlo Ersek @ 2017-03-20 11:59 UTC (permalink / raw) To: qemu devel list; +Cc: Michael S. Tsirkin, Ben Warren, Igor Mammedov Multiple instances make no sense. Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Ben Warren <ben@skyportsystems.com> Cc: Igor Mammedov <imammedo@redhat.com> Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- hw/acpi/vmgenid.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c index c3ddcc8e7cb0..b5c0dfcf19e1 100644 --- a/hw/acpi/vmgenid.c +++ b/hw/acpi/vmgenid.c @@ -214,6 +214,8 @@ static Property vmgenid_properties[] = { static void vmgenid_realize(DeviceState *dev, Error **errp) { VmGenIdState *vms = VMGENID(dev); + Object *one_vmgenid; + bool ambiguous; if (!vms->write_pointer_available) { error_setg(errp, "%s requires DMA write support in fw_cfg, " @@ -221,6 +223,14 @@ static void vmgenid_realize(DeviceState *dev, Error **errp) return; } + one_vmgenid = object_resolve_path_type("", VMGENID_DEVICE, &ambiguous); + if (one_vmgenid == NULL) { + assert(ambiguous); + error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE); + return; + } + assert(one_vmgenid == OBJECT(vms)); + qemu_register_reset(vmgenid_handle_reset, vms); } -- 2.9.3 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/acpi/vmgenid: prevent more than one vmgenid device 2017-03-20 11:59 ` [Qemu-devel] [PATCH 2/2] hw/acpi/vmgenid: prevent more than one vmgenid device Laszlo Ersek @ 2017-03-20 14:16 ` Michael S. Tsirkin 2017-03-20 15:13 ` Laszlo Ersek 0 siblings, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2017-03-20 14:16 UTC (permalink / raw) To: Laszlo Ersek; +Cc: qemu devel list, Ben Warren, Igor Mammedov On Mon, Mar 20, 2017 at 12:59:51PM +0100, Laszlo Ersek wrote: > Multiple instances make no sense. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Ben Warren <ben@skyportsystems.com> > Cc: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> find_vmgenid_dev would be a better place for this. This is where the single instance assumption comes from ATM. > --- > hw/acpi/vmgenid.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c > index c3ddcc8e7cb0..b5c0dfcf19e1 100644 > --- a/hw/acpi/vmgenid.c > +++ b/hw/acpi/vmgenid.c > @@ -214,6 +214,8 @@ static Property vmgenid_properties[] = { > static void vmgenid_realize(DeviceState *dev, Error **errp) > { > VmGenIdState *vms = VMGENID(dev); > + Object *one_vmgenid; > + bool ambiguous; > > if (!vms->write_pointer_available) { > error_setg(errp, "%s requires DMA write support in fw_cfg, " > @@ -221,6 +223,14 @@ static void vmgenid_realize(DeviceState *dev, Error **errp) > return; > } > > + one_vmgenid = object_resolve_path_type("", VMGENID_DEVICE, &ambiguous); > + if (one_vmgenid == NULL) { > + assert(ambiguous); > + error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE); > + return; > + } > + assert(one_vmgenid == OBJECT(vms)); > + > qemu_register_reset(vmgenid_handle_reset, vms); > } > > -- > 2.9.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/acpi/vmgenid: prevent more than one vmgenid device 2017-03-20 14:16 ` Michael S. Tsirkin @ 2017-03-20 15:13 ` Laszlo Ersek 2017-03-20 16:22 ` Laszlo Ersek 0 siblings, 1 reply; 14+ messages in thread From: Laszlo Ersek @ 2017-03-20 15:13 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu devel list, Ben Warren, Igor Mammedov On 03/20/17 15:16, Michael S. Tsirkin wrote: > On Mon, Mar 20, 2017 at 12:59:51PM +0100, Laszlo Ersek wrote: >> Multiple instances make no sense. >> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Ben Warren <ben@skyportsystems.com> >> Cc: Igor Mammedov <imammedo@redhat.com> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > find_vmgenid_dev would be a better place for this. > This is where the single instance assumption comes from ATM. object_resolve_path_type() -- used internally in find_vmgenid_dev() -- returns NULL in either of two cases: there is no such device, or there are multiple devices. You can tell them apart by looking at the last parameter (called "ambiguous"), but find_vmgenid_dev() doesn't use that parameter. By the time we are in the vmgenid_realize() function, at least one vmgenid device is guaranteed to exist (the one which we are realizing). Therefore, this patch could be simplified as: if (find_vmgenid_dev() == NULL) { error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE); return; } I found that confusing, and wanted to spell out "ambiguous" with the assert(). If you prefer the above simpler (but harder to understand) check, I can do that too. Thanks Laszlo > > >> --- >> hw/acpi/vmgenid.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c >> index c3ddcc8e7cb0..b5c0dfcf19e1 100644 >> --- a/hw/acpi/vmgenid.c >> +++ b/hw/acpi/vmgenid.c >> @@ -214,6 +214,8 @@ static Property vmgenid_properties[] = { >> static void vmgenid_realize(DeviceState *dev, Error **errp) >> { >> VmGenIdState *vms = VMGENID(dev); >> + Object *one_vmgenid; >> + bool ambiguous; >> >> if (!vms->write_pointer_available) { >> error_setg(errp, "%s requires DMA write support in fw_cfg, " >> @@ -221,6 +223,14 @@ static void vmgenid_realize(DeviceState *dev, Error **errp) >> return; >> } >> >> + one_vmgenid = object_resolve_path_type("", VMGENID_DEVICE, &ambiguous); >> + if (one_vmgenid == NULL) { >> + assert(ambiguous); >> + error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE); >> + return; >> + } >> + assert(one_vmgenid == OBJECT(vms)); >> + >> qemu_register_reset(vmgenid_handle_reset, vms); >> } >> >> -- >> 2.9.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/acpi/vmgenid: prevent more than one vmgenid device 2017-03-20 15:13 ` Laszlo Ersek @ 2017-03-20 16:22 ` Laszlo Ersek 2017-03-20 16:26 ` Michael S. Tsirkin 0 siblings, 1 reply; 14+ messages in thread From: Laszlo Ersek @ 2017-03-20 16:22 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu devel list, Ben Warren, Igor Mammedov On 03/20/17 16:13, Laszlo Ersek wrote: > On 03/20/17 15:16, Michael S. Tsirkin wrote: >> On Mon, Mar 20, 2017 at 12:59:51PM +0100, Laszlo Ersek wrote: >>> Multiple instances make no sense. >>> >>> Cc: "Michael S. Tsirkin" <mst@redhat.com> >>> Cc: Ben Warren <ben@skyportsystems.com> >>> Cc: Igor Mammedov <imammedo@redhat.com> >>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> >> find_vmgenid_dev would be a better place for this. >> This is where the single instance assumption comes from ATM. > > object_resolve_path_type() -- used internally in find_vmgenid_dev() -- > returns NULL in either of two cases: there is no such device, or there > are multiple devices. You can tell them apart by looking at the last > parameter (called "ambiguous"), but find_vmgenid_dev() doesn't use that > parameter. > > By the time we are in the vmgenid_realize() function, at least one > vmgenid device is guaranteed to exist (the one which we are realizing). > Therefore, this patch could be simplified as: > > if (find_vmgenid_dev() == NULL) { > error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE); > return; > } > > I found that confusing, and wanted to spell out "ambiguous" with the > assert(). If you prefer the above simpler (but harder to understand) > check, I can do that too. Also, find_vmgenid_dev() only captures the single instance assumption, it does not dictate the assumption. The assumption comes from the spec. With the above in mind, what do you say about this patch? Do you want me to call find_vmgenid_dev() in the realize function (which will require a comment about object_resolve_path_type's behavior), or are you okay with the patch as-is? (The asserts make it clear IMO.) Thanks Laszlo > >> >> >>> --- >>> hw/acpi/vmgenid.c | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >>> >>> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c >>> index c3ddcc8e7cb0..b5c0dfcf19e1 100644 >>> --- a/hw/acpi/vmgenid.c >>> +++ b/hw/acpi/vmgenid.c >>> @@ -214,6 +214,8 @@ static Property vmgenid_properties[] = { >>> static void vmgenid_realize(DeviceState *dev, Error **errp) >>> { >>> VmGenIdState *vms = VMGENID(dev); >>> + Object *one_vmgenid; >>> + bool ambiguous; >>> >>> if (!vms->write_pointer_available) { >>> error_setg(errp, "%s requires DMA write support in fw_cfg, " >>> @@ -221,6 +223,14 @@ static void vmgenid_realize(DeviceState *dev, Error **errp) >>> return; >>> } >>> >>> + one_vmgenid = object_resolve_path_type("", VMGENID_DEVICE, &ambiguous); >>> + if (one_vmgenid == NULL) { >>> + assert(ambiguous); >>> + error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE); >>> + return; >>> + } >>> + assert(one_vmgenid == OBJECT(vms)); >>> + >>> qemu_register_reset(vmgenid_handle_reset, vms); >>> } >>> >>> -- >>> 2.9.3 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/acpi/vmgenid: prevent more than one vmgenid device 2017-03-20 16:22 ` Laszlo Ersek @ 2017-03-20 16:26 ` Michael S. Tsirkin 2017-03-20 16:39 ` Laszlo Ersek 0 siblings, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2017-03-20 16:26 UTC (permalink / raw) To: Laszlo Ersek; +Cc: qemu devel list, Ben Warren, Igor Mammedov On Mon, Mar 20, 2017 at 05:22:16PM +0100, Laszlo Ersek wrote: > On 03/20/17 16:13, Laszlo Ersek wrote: > > On 03/20/17 15:16, Michael S. Tsirkin wrote: > >> On Mon, Mar 20, 2017 at 12:59:51PM +0100, Laszlo Ersek wrote: > >>> Multiple instances make no sense. > >>> > >>> Cc: "Michael S. Tsirkin" <mst@redhat.com> > >>> Cc: Ben Warren <ben@skyportsystems.com> > >>> Cc: Igor Mammedov <imammedo@redhat.com> > >>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > >> > >> find_vmgenid_dev would be a better place for this. > >> This is where the single instance assumption comes from ATM. > > > > object_resolve_path_type() -- used internally in find_vmgenid_dev() -- > > returns NULL in either of two cases: there is no such device, or there > > are multiple devices. You can tell them apart by looking at the last > > parameter (called "ambiguous"), but find_vmgenid_dev() doesn't use that > > parameter. > > > > By the time we are in the vmgenid_realize() function, at least one > > vmgenid device is guaranteed to exist (the one which we are realizing). > > Therefore, this patch could be simplified as: > > > > if (find_vmgenid_dev() == NULL) { > > error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE); > > return; > > } > > > > I found that confusing, and wanted to spell out "ambiguous" with the > > assert(). If you prefer the above simpler (but harder to understand) > > check, I can do that too. > > Also, find_vmgenid_dev() only captures the single instance assumption, > it does not dictate the assumption. The assumption comes from the spec. I don't see this assumption anywhere in spec. What do you have in mind? > With the above in mind, what do you say about this patch? Do you want me > to call find_vmgenid_dev() in the realize function (which will require a > comment about object_resolve_path_type's behavior), or are you okay with > the patch as-is? (The asserts make it clear IMO.) > > Thanks > Laszlo I prefer calling find_vmgenid_dev, and adding a comment near find_vmgenid_dev. > > > >> > >> > >>> --- > >>> hw/acpi/vmgenid.c | 10 ++++++++++ > >>> 1 file changed, 10 insertions(+) > >>> > >>> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c > >>> index c3ddcc8e7cb0..b5c0dfcf19e1 100644 > >>> --- a/hw/acpi/vmgenid.c > >>> +++ b/hw/acpi/vmgenid.c > >>> @@ -214,6 +214,8 @@ static Property vmgenid_properties[] = { > >>> static void vmgenid_realize(DeviceState *dev, Error **errp) > >>> { > >>> VmGenIdState *vms = VMGENID(dev); > >>> + Object *one_vmgenid; > >>> + bool ambiguous; > >>> > >>> if (!vms->write_pointer_available) { > >>> error_setg(errp, "%s requires DMA write support in fw_cfg, " > >>> @@ -221,6 +223,14 @@ static void vmgenid_realize(DeviceState *dev, Error **errp) > >>> return; > >>> } > >>> > >>> + one_vmgenid = object_resolve_path_type("", VMGENID_DEVICE, &ambiguous); > >>> + if (one_vmgenid == NULL) { > >>> + assert(ambiguous); > >>> + error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE); > >>> + return; > >>> + } > >>> + assert(one_vmgenid == OBJECT(vms)); > >>> + > >>> qemu_register_reset(vmgenid_handle_reset, vms); > >>> } > >>> > >>> -- > >>> 2.9.3 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/acpi/vmgenid: prevent more than one vmgenid device 2017-03-20 16:26 ` Michael S. Tsirkin @ 2017-03-20 16:39 ` Laszlo Ersek 2017-03-20 16:57 ` Michael S. Tsirkin 0 siblings, 1 reply; 14+ messages in thread From: Laszlo Ersek @ 2017-03-20 16:39 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu devel list, Ben Warren, Igor Mammedov On 03/20/17 17:26, Michael S. Tsirkin wrote: > On Mon, Mar 20, 2017 at 05:22:16PM +0100, Laszlo Ersek wrote: >> On 03/20/17 16:13, Laszlo Ersek wrote: >>> On 03/20/17 15:16, Michael S. Tsirkin wrote: >>>> On Mon, Mar 20, 2017 at 12:59:51PM +0100, Laszlo Ersek wrote: >>>>> Multiple instances make no sense. >>>>> >>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com> >>>>> Cc: Ben Warren <ben@skyportsystems.com> >>>>> Cc: Igor Mammedov <imammedo@redhat.com> >>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>>> >>>> find_vmgenid_dev would be a better place for this. >>>> This is where the single instance assumption comes from ATM. >>> >>> object_resolve_path_type() -- used internally in find_vmgenid_dev() -- >>> returns NULL in either of two cases: there is no such device, or there >>> are multiple devices. You can tell them apart by looking at the last >>> parameter (called "ambiguous"), but find_vmgenid_dev() doesn't use that >>> parameter. >>> >>> By the time we are in the vmgenid_realize() function, at least one >>> vmgenid device is guaranteed to exist (the one which we are realizing). >>> Therefore, this patch could be simplified as: >>> >>> if (find_vmgenid_dev() == NULL) { >>> error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE); >>> return; >>> } >>> >>> I found that confusing, and wanted to spell out "ambiguous" with the >>> assert(). If you prefer the above simpler (but harder to understand) >>> check, I can do that too. >> >> Also, find_vmgenid_dev() only captures the single instance assumption, >> it does not dictate the assumption. The assumption comes from the spec. > > I don't see this assumption anywhere in spec. What do you have in mind? It has language like "1. Put the generation ID in an 8-byte aligned buffer in guest RAM [...]" "2. Expose a device somewhere in the ACPI namespace [...]" "5. When the generation ID changes, execute an ACPI Notify operation on the generation ID device [...]" "After the identifier has been made persistent in the configuration [...]" The spec defines a system-wide feature, and in all contexts it implies there is only one of those things. The multiple device case is undefined by omission, if you will. >> With the above in mind, what do you say about this patch? Do you want me >> to call find_vmgenid_dev() in the realize function (which will require a >> comment about object_resolve_path_type's behavior), or are you okay with >> the patch as-is? (The asserts make it clear IMO.) >> >> Thanks >> Laszlo > > I prefer calling find_vmgenid_dev, and adding a comment > near find_vmgenid_dev. Near the function definition in "include/hw/acpi/vmgenid.h", or the call site in the realize function? Thanks Laszlo > >>> >>>> >>>> >>>>> --- >>>>> hw/acpi/vmgenid.c | 10 ++++++++++ >>>>> 1 file changed, 10 insertions(+) >>>>> >>>>> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c >>>>> index c3ddcc8e7cb0..b5c0dfcf19e1 100644 >>>>> --- a/hw/acpi/vmgenid.c >>>>> +++ b/hw/acpi/vmgenid.c >>>>> @@ -214,6 +214,8 @@ static Property vmgenid_properties[] = { >>>>> static void vmgenid_realize(DeviceState *dev, Error **errp) >>>>> { >>>>> VmGenIdState *vms = VMGENID(dev); >>>>> + Object *one_vmgenid; >>>>> + bool ambiguous; >>>>> >>>>> if (!vms->write_pointer_available) { >>>>> error_setg(errp, "%s requires DMA write support in fw_cfg, " >>>>> @@ -221,6 +223,14 @@ static void vmgenid_realize(DeviceState *dev, Error **errp) >>>>> return; >>>>> } >>>>> >>>>> + one_vmgenid = object_resolve_path_type("", VMGENID_DEVICE, &ambiguous); >>>>> + if (one_vmgenid == NULL) { >>>>> + assert(ambiguous); >>>>> + error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE); >>>>> + return; >>>>> + } >>>>> + assert(one_vmgenid == OBJECT(vms)); >>>>> + >>>>> qemu_register_reset(vmgenid_handle_reset, vms); >>>>> } >>>>> >>>>> -- >>>>> 2.9.3 >>> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/acpi/vmgenid: prevent more than one vmgenid device 2017-03-20 16:39 ` Laszlo Ersek @ 2017-03-20 16:57 ` Michael S. Tsirkin 2017-03-20 17:05 ` Laszlo Ersek 0 siblings, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2017-03-20 16:57 UTC (permalink / raw) To: Laszlo Ersek; +Cc: qemu devel list, Ben Warren, Igor Mammedov On Mon, Mar 20, 2017 at 05:39:18PM +0100, Laszlo Ersek wrote: > On 03/20/17 17:26, Michael S. Tsirkin wrote: > > On Mon, Mar 20, 2017 at 05:22:16PM +0100, Laszlo Ersek wrote: > >> On 03/20/17 16:13, Laszlo Ersek wrote: > >>> On 03/20/17 15:16, Michael S. Tsirkin wrote: > >>>> On Mon, Mar 20, 2017 at 12:59:51PM +0100, Laszlo Ersek wrote: > >>>>> Multiple instances make no sense. > >>>>> > >>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com> > >>>>> Cc: Ben Warren <ben@skyportsystems.com> > >>>>> Cc: Igor Mammedov <imammedo@redhat.com> > >>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > >>>> > >>>> find_vmgenid_dev would be a better place for this. > >>>> This is where the single instance assumption comes from ATM. > >>> > >>> object_resolve_path_type() -- used internally in find_vmgenid_dev() -- > >>> returns NULL in either of two cases: there is no such device, or there > >>> are multiple devices. You can tell them apart by looking at the last > >>> parameter (called "ambiguous"), but find_vmgenid_dev() doesn't use that > >>> parameter. > >>> > >>> By the time we are in the vmgenid_realize() function, at least one > >>> vmgenid device is guaranteed to exist (the one which we are realizing). > >>> Therefore, this patch could be simplified as: > >>> > >>> if (find_vmgenid_dev() == NULL) { > >>> error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE); > >>> return; > >>> } > >>> > >>> I found that confusing, and wanted to spell out "ambiguous" with the > >>> assert(). If you prefer the above simpler (but harder to understand) > >>> check, I can do that too. > >> > >> Also, find_vmgenid_dev() only captures the single instance assumption, > >> it does not dictate the assumption. The assumption comes from the spec. > > > > I don't see this assumption anywhere in spec. What do you have in mind? > > It has language like > > "1. Put the generation ID in an 8-byte aligned buffer in guest RAM [...]" > > "2. Expose a device somewhere in the ACPI namespace [...]" > > "5. When the generation ID changes, execute an ACPI Notify operation on > the generation ID device [...]" > > "After the identifier has been made persistent in the configuration [...]" > > The spec defines a system-wide feature, and in all contexts it implies > there is only one of those things. The multiple device case is undefined > by omission, if you will. I see. > >> With the above in mind, what do you say about this patch? Do you want me > >> to call find_vmgenid_dev() in the realize function (which will require a > >> comment about object_resolve_path_type's behavior), or are you okay with > >> the patch as-is? (The asserts make it clear IMO.) > >> > >> Thanks > >> Laszlo > > > > I prefer calling find_vmgenid_dev, and adding a comment > > near find_vmgenid_dev. > > Near the function definition in "include/hw/acpi/vmgenid.h", or the call > site in the realize function? > > Thanks > Laszlo I'd put it near the function itself. > > > >>> > >>>> > >>>> > >>>>> --- > >>>>> hw/acpi/vmgenid.c | 10 ++++++++++ > >>>>> 1 file changed, 10 insertions(+) > >>>>> > >>>>> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c > >>>>> index c3ddcc8e7cb0..b5c0dfcf19e1 100644 > >>>>> --- a/hw/acpi/vmgenid.c > >>>>> +++ b/hw/acpi/vmgenid.c > >>>>> @@ -214,6 +214,8 @@ static Property vmgenid_properties[] = { > >>>>> static void vmgenid_realize(DeviceState *dev, Error **errp) > >>>>> { > >>>>> VmGenIdState *vms = VMGENID(dev); > >>>>> + Object *one_vmgenid; > >>>>> + bool ambiguous; > >>>>> > >>>>> if (!vms->write_pointer_available) { > >>>>> error_setg(errp, "%s requires DMA write support in fw_cfg, " > >>>>> @@ -221,6 +223,14 @@ static void vmgenid_realize(DeviceState *dev, Error **errp) > >>>>> return; > >>>>> } > >>>>> > >>>>> + one_vmgenid = object_resolve_path_type("", VMGENID_DEVICE, &ambiguous); > >>>>> + if (one_vmgenid == NULL) { > >>>>> + assert(ambiguous); > >>>>> + error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE); > >>>>> + return; > >>>>> + } > >>>>> + assert(one_vmgenid == OBJECT(vms)); > >>>>> + > >>>>> qemu_register_reset(vmgenid_handle_reset, vms); > >>>>> } > >>>>> > >>>>> -- > >>>>> 2.9.3 > >>> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/acpi/vmgenid: prevent more than one vmgenid device 2017-03-20 16:57 ` Michael S. Tsirkin @ 2017-03-20 17:05 ` Laszlo Ersek 0 siblings, 0 replies; 14+ messages in thread From: Laszlo Ersek @ 2017-03-20 17:05 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu devel list, Ben Warren, Igor Mammedov On 03/20/17 17:57, Michael S. Tsirkin wrote: > On Mon, Mar 20, 2017 at 05:39:18PM +0100, Laszlo Ersek wrote: >> On 03/20/17 17:26, Michael S. Tsirkin wrote: >>> On Mon, Mar 20, 2017 at 05:22:16PM +0100, Laszlo Ersek wrote: >>>> On 03/20/17 16:13, Laszlo Ersek wrote: >>>>> On 03/20/17 15:16, Michael S. Tsirkin wrote: >>>>>> On Mon, Mar 20, 2017 at 12:59:51PM +0100, Laszlo Ersek wrote: >>>>>>> Multiple instances make no sense. >>>>>>> >>>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com> >>>>>>> Cc: Ben Warren <ben@skyportsystems.com> >>>>>>> Cc: Igor Mammedov <imammedo@redhat.com> >>>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>>>>> >>>>>> find_vmgenid_dev would be a better place for this. >>>>>> This is where the single instance assumption comes from ATM. >>>>> >>>>> object_resolve_path_type() -- used internally in find_vmgenid_dev() -- >>>>> returns NULL in either of two cases: there is no such device, or there >>>>> are multiple devices. You can tell them apart by looking at the last >>>>> parameter (called "ambiguous"), but find_vmgenid_dev() doesn't use that >>>>> parameter. >>>>> >>>>> By the time we are in the vmgenid_realize() function, at least one >>>>> vmgenid device is guaranteed to exist (the one which we are realizing). >>>>> Therefore, this patch could be simplified as: >>>>> >>>>> if (find_vmgenid_dev() == NULL) { >>>>> error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE); >>>>> return; >>>>> } >>>>> >>>>> I found that confusing, and wanted to spell out "ambiguous" with the >>>>> assert(). If you prefer the above simpler (but harder to understand) >>>>> check, I can do that too. >>>> >>>> Also, find_vmgenid_dev() only captures the single instance assumption, >>>> it does not dictate the assumption. The assumption comes from the spec. >>> >>> I don't see this assumption anywhere in spec. What do you have in mind? >> >> It has language like >> >> "1. Put the generation ID in an 8-byte aligned buffer in guest RAM [...]" >> >> "2. Expose a device somewhere in the ACPI namespace [...]" >> >> "5. When the generation ID changes, execute an ACPI Notify operation on >> the generation ID device [...]" >> >> "After the identifier has been made persistent in the configuration [...]" >> >> The spec defines a system-wide feature, and in all contexts it implies >> there is only one of those things. The multiple device case is undefined >> by omission, if you will. > > I see. > >>>> With the above in mind, what do you say about this patch? Do you want me >>>> to call find_vmgenid_dev() in the realize function (which will require a >>>> comment about object_resolve_path_type's behavior), or are you okay with >>>> the patch as-is? (The asserts make it clear IMO.) >>>> >>>> Thanks >>>> Laszlo >>> >>> I prefer calling find_vmgenid_dev, and adding a comment >>> near find_vmgenid_dev. >> >> Near the function definition in "include/hw/acpi/vmgenid.h", or the call >> site in the realize function? >> >> Thanks >> Laszlo > > I'd put it near the function itself. Thanks -- meanwhile I figured I'd put a comment at both locations, just to be sure. I'm about to post v2. Thanks! Laszlo ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-03-20 17:05 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-20 11:59 [Qemu-devel] [PATCH 0/2] some remaining vmgenid tweaks for 2.9 Laszlo Ersek 2017-03-20 11:59 ` [Qemu-devel] [PATCH 1/2] hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types Laszlo Ersek 2017-03-20 14:19 ` Michael S. Tsirkin 2017-03-20 14:39 ` Paolo Bonzini 2017-03-20 15:08 ` Laszlo Ersek 2017-03-20 15:09 ` Paolo Bonzini 2017-03-20 11:59 ` [Qemu-devel] [PATCH 2/2] hw/acpi/vmgenid: prevent more than one vmgenid device Laszlo Ersek 2017-03-20 14:16 ` Michael S. Tsirkin 2017-03-20 15:13 ` Laszlo Ersek 2017-03-20 16:22 ` Laszlo Ersek 2017-03-20 16:26 ` Michael S. Tsirkin 2017-03-20 16:39 ` Laszlo Ersek 2017-03-20 16:57 ` Michael S. Tsirkin 2017-03-20 17:05 ` Laszlo Ersek
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.