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

[Laszlo Ersek]

On 07/03/17 20:27, Eduardo Habkost wrote:
> 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.

Thank you for the explanation -- I agree we shouldn't up-end the code
now, but I think two sets of documentation should be added:

- the commit message should capture your explanation

- the bios_linker_loader_can_write_pointer() function should have a
reminder that board code must realize fw_cfg first, as a fixed (sysbus?)
device, before another device realize function reaches
bios_linker_loader_can_write_pointer().

(CC'ing Mark Cave-Ayland for any necessary co-ordination.)

> We have at least one other device that also assumes fw_cfg_find() can be
> safely used on realize: pvpanic.

Good point.

Thanks
Laszlo