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

[Paolo Bonzini]

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.

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