[v7,4/8] ACPI: Add Virtual Machine Generation ID support

Commit Message

[e-mail] Feb. 16, 2017, 6:32 p.m. UTC

> On Feb 16, 2017, at 1:56 AM, Igor Mammedov <[e-mail]> wrote:
> 
> On Wed, 15 Feb 2017 22:18:14 -0800
> [e-mail] wrote:
> 
>> From: Ben Warren <[e-mail]>
>> 
>> 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 <[e-mail]>
> Reviewed-by: Igor Mammedov <[e-mail]>
> 
> 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:
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.

—Ben

<snip>

Comments

Laszlo Ersek Feb. 16, 2017, 7:03 p.m. UTC | #1

On 02/16/17 19:32, Ben Warren wrote:
> 
>> On Feb 16, 2017, at 1:56 AM, Igor Mammedov <[e-mail]> wrote:
>>
>> On Wed, 15 Feb 2017 22:18:14 -0800
>> [e-mail] wrote:
>>
>>> From: Ben Warren <[e-mail]>
>>>
>>> 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 <[e-mail]>
>> Reviewed-by: Igor Mammedov <[e-mail]>
>>
>> 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

[e-mail] Feb. 16, 2017, 7:05 p.m. UTC | #2

> On Feb 16, 2017, at 11:03 AM, Laszlo Ersek <[e-mail]> wrote:
> 
> On 02/16/17 19:32, Ben Warren wrote:
>> 
>>> On Feb 16, 2017, at 1:56 AM, Igor Mammedov <[e-mail]> wrote:
>>> 
>>> On Wed, 15 Feb 2017 22:18:14 -0800
>>> [e-mail] wrote:
>>> 
>>>> From: Ben Warren <[e-mail]>
>>>> 
>>>> 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 <[e-mail]>
>>> Reviewed-by: Igor Mammedov <[e-mail]>
>>> 
>>> 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.)
> 
OK, makes sense.
> Thanks!
> Laszlo

—Ben

Patch

===

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

===