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 <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.
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
