On 02/16/17 19:32, Ben Warren wrote:
>
>> On Feb 16, 2017, at 1:56 AM, Igor Mammedov <imamm...@redhat.com> wrote:
>>
>> On Wed, 15 Feb 2017 22:18:14 -0800
>> b...@skyportsystems.com wrote:
>>
>>> From: Ben Warren <b...@skyportsystems.com>
>>>
>>> This implements the VM Generation ID feature by passing a 128-bit
>>> GUID to the guest via a fw_cfg blob.
>>> Any time the GUID changes, an ACPI notify event is sent to the guest
>>>
>>> The user interface is a simple device with one parameter:
>>> - guid (string, must be "auto" or in UUID format
>>> xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx)
>>>
>>> Signed-off-by: Ben Warren <b...@skyportsystems.com>
>> Reviewed-by: Igor Mammedov <imamm...@redhat.com>
>>
>> Thing to get fixed on top is to check
>> that there isn't vmgenid device present already
>> into realizefn() and fail gracefully is there is.
>>
> I made the change as follows:
> ===
>
> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> index c8465df..fa3de6d 100644
> --- a/hw/acpi/vmgenid.c
> +++ b/hw/acpi/vmgenid.c
> @@ -207,8 +207,15 @@ static void vmgenid_handle_reset(void *opaque)
>
> static void vmgenid_realize(DeviceState *dev, Error **errp)
> {
> + static bool is_realized = false;
> VmGenIdState *vms = VMGENID(dev);
> +
> + if (is_realized) {
> + error_setg(errp, "Device %s may only be specified once",
> + VMGENID_DEVICE);
> + }
> qemu_register_reset(vmgenid_handle_reset, vms);
> + is_realized = true;
> }
>
> ===
> Sample output:
> $ ../workspace/qemu/x86_64-softmmu/qemu-system-x86_64 -nographic -bios
> ../workspace/seabios/out/bios.bin -machine type=q35 -hda
> beiwe-with-vmgenid-driver.qcow2 -qmp unix:./qmp-sock,server,nowait -device
> vmgenid,guid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb66" -device
> vmgenid,guid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb67"
> qemu-system-x86_64: -device
> vmgenid,guid=324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb67: Device vmgenid may only be
> specified once
>
>
> Good enough? If so, I’ll roll it in. Otherwise, it’ll be a supplemental
> patch.
Ehm, I'd say let's postpone that patch. Function (or even file) scope
static variables that plaster over QOM shortcomings (or, well,
"non-trivial QOM patterns") are strongly frowned upon. I've done it
myself before (totally as a last resort, really -- not kidding!), and it
was real hard to get into the tree.
This deserves more discussion, so please delay it.
(While at it, please remember my other recommendation for realize():
enforcing that the running machine type / version provides the guest
with the DMA interface for fw_cfg. For that, based on prior consensus,
you'll need another device property, to be set in parallel to
fw_cfg_mem's and fw_cfg_io's "dma-enabled" property. So I think it makes
sense to address all of these things in a separate, followup series.)
Thanks!
Laszlo
