Re: [Qemu-devel] [PATCH 1/2] hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types

[Laszlo Ersek]

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" <address@hidden>
>>> Cc: Ben Warren <address@hidden>
>>> Cc: Igor Mammedov <address@hidden>
>>> Signed-off-by: Laszlo Ersek <address@hidden>
>>
>> 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
>>>
>>