| Message ID | 20170320115951.25345-2-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 >
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 >> >
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 >>> >>
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
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);
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(+)