Re: [Qemu-devel] [PATCH 1/7] vmgenid: replace x-write-pointer-available hack

[Eduardo Habkost]

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 <address@hidden>
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  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