On Mon, Jul 03, 2017 at 08:06:33PM +0200, Laszlo Ersek wrote: > On 06/29/17 15:23, Marc-André Lureau wrote: > > This compat property sole function is to prevent the device from being > > instantiated. Instead of requiring an extra compat property, check if > > fw_cfg has DMA enabled. > > > > This has the additional benefit of handling other cases properly, like: > > > > $ qemu-system-x86_64 -device vmgenid -machine none > > qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write support > > in fw_cfg, which this machine type does not provide > > $ qemu-system-x86_64 -device vmgenid -machine pc-i440fx-2.9 -global > > fw_cfg.dma_enabled=off > > qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write support > > in fw_cfg, which this machine type does not provide > > $ qemu-system-x86_64 -device vmgenid -machine pc-i440fx-2.6 -global > > fw_cfg.dma_enabled=on > > [boots normally] > > > > Suggested-by: Eduardo Habkost <ehabk...@redhat.com> > > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > > --- > > include/hw/acpi/bios-linker-loader.h | 2 ++ > > include/hw/compat.h | 4 ---- > > hw/acpi/bios-linker-loader.c | 6 ++++++ > > hw/acpi/vmgenid.c | 9 +-------- > > 4 files changed, 9 insertions(+), 12 deletions(-) > > > > diff --git a/include/hw/acpi/bios-linker-loader.h > > b/include/hw/acpi/bios-linker-loader.h > > index efe17b0b9c..a711dbced8 100644 > > --- a/include/hw/acpi/bios-linker-loader.h > > +++ b/include/hw/acpi/bios-linker-loader.h > > @@ -7,6 +7,8 @@ typedef struct BIOSLinker { > > GArray *file_list; > > } BIOSLinker; > > > > +bool bios_linker_loader_can_write_pointer(void); > > + > > BIOSLinker *bios_linker_loader_init(void); > > > > void bios_linker_loader_alloc(BIOSLinker *linker, > > diff --git a/include/hw/compat.h b/include/hw/compat.h > > index 26cd5851a5..36f02179ac 100644 > > --- a/include/hw/compat.h > > +++ b/include/hw/compat.h > > @@ -150,10 +150,6 @@ > > .driver = "fw_cfg_io",\ > > .property = "dma_enabled",\ > > .value = "off",\ > > - },{\ > > - .driver = "vmgenid",\ > > - .property = "x-write-pointer-available",\ > > - .value = "off",\ > > }, > > > > #define HW_COMPAT_2_3 \ > > diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c > > index 046183a0f1..587d62cb93 100644 > > --- a/hw/acpi/bios-linker-loader.c > > +++ b/hw/acpi/bios-linker-loader.c > > @@ -168,6 +168,12 @@ bios_linker_find_file(const BIOSLinker *linker, const > > char *name) > > return NULL; > > } > > > > +bool bios_linker_loader_can_write_pointer(void) > > +{ > > + FWCfgState *fw_cfg = fw_cfg_find(); > > + return fw_cfg && fw_cfg_dma_enabled(fw_cfg); > > +} > > + > > /* > > * bios_linker_loader_alloc: ask guest to load file into guest memory. > > * > > diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c > > index a32b847fe0..ab5da293fd 100644 > > --- a/hw/acpi/vmgenid.c > > +++ b/hw/acpi/vmgenid.c > > @@ -205,17 +205,11 @@ 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) { > > + if (!bios_linker_loader_can_write_pointer()) { > > error_setg(errp, "%s requires DMA write support in fw_cfg, " > > "which this machine type does not provide", > > VMGENID_DEVICE); > > return; > > @@ -239,7 +233,6 @@ 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); > > > > I believe we discussed this approach back then (but I can't find the > relevant messages, of course). > > What guarantees that, by the time you call fw_cfg_find() from > vmgenid_realize() -- that is, from the realize function of an > independent device --, the fw_cfg device will have been realized (with > its properties having taken their final values)? I don't see how the > ordering is guaranteed here; please explain (preferably in the commit > message).
Good point. What makes this work is the fact that fw_cfg is a built-in device that is initialized very early by the machine init code. We could remove that requirement, but it would require reporting errors from the machine_done notifier (in acpi_setup(), probably). I'm not sure it would be worth the extra complexity. We have at least one other device that also assumes fw_cfg_find() can be safely used on realize: pvpanic. -- Eduardo