Re: [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support
> On Feb 16, 2017, at 11:03 AM, Laszlo Ersek wrote:
>
> On 02/16/17 19:32, Ben Warren wrote:
>>
>>> On Feb 16, 2017, at 1:56 AM, Igor Mammedov wrote:
>>>
>>> On Wed, 15 Feb 2017 22:18:14 -0800
>>> b...@skyportsystems.com wrote:
>>>
From: Ben Warren
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
----)
Signed-off-by: Ben Warren
>>> Reviewed-by: Igor Mammedov
>>>
>>> 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
smime.p7s
Description: S/MIME cryptographic signature
Re: [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support
On 02/16/17 19:32, Ben Warren wrote:
>
>> On Feb 16, 2017, at 1:56 AM, Igor Mammedov wrote:
>>
>> On Wed, 15 Feb 2017 22:18:14 -0800
>> b...@skyportsystems.com wrote:
>>
>>> From: Ben Warren
>>>
>>> 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
>>> ----)
>>>
>>> Signed-off-by: Ben Warren
>> Reviewed-by: Igor Mammedov
>>
>> 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
Re: [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support
> On Feb 16, 2017, at 1:56 AM, Igor Mammedov wrote:
>
> On Wed, 15 Feb 2017 22:18:14 -0800
> b...@skyportsystems.com wrote:
>
>> From: Ben Warren
>>
>> 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
>> ----)
>>
>> Signed-off-by: Ben Warren
> Reviewed-by: Igor Mammedov
>
> 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.
—Ben
smime.p7s
Description: S/MIME cryptographic signature
Re: [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support
On 02/16/17 07:18, b...@skyportsystems.com wrote: > From: Ben Warren > > 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 >----) > > Signed-off-by: Ben Warren > --- > default-configs/i386-softmmu.mak | 1 + > default-configs/x86_64-softmmu.mak | 1 + > hw/acpi/Makefile.objs| 1 + > hw/acpi/vmgenid.c| 239 > +++ > hw/i386/acpi-build.c | 16 +++ > include/hw/acpi/acpi_dev_interface.h | 1 + > include/hw/acpi/vmgenid.h| 35 + > 7 files changed, 294 insertions(+) > create mode 100644 hw/acpi/vmgenid.c > create mode 100644 include/hw/acpi/vmgenid.h I compared this version against v6. It looks good. I also tested it (with the fixup I mentioned under v7 1/8, and without live migration). Reviewed-by: Laszlo Ersek Tested-by: Laszlo Ersek Thanks Laszlo
Re: [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support
On Wed, 15 Feb 2017 22:18:14 -0800
b...@skyportsystems.com wrote:
> From: Ben Warren
>
> 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
>----)
>
> Signed-off-by: Ben Warren
Reviewed-by: Igor Mammedov
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.
> ---
> default-configs/i386-softmmu.mak | 1 +
> default-configs/x86_64-softmmu.mak | 1 +
> hw/acpi/Makefile.objs| 1 +
> hw/acpi/vmgenid.c| 239
> +++
> hw/i386/acpi-build.c | 16 +++
> include/hw/acpi/acpi_dev_interface.h | 1 +
> include/hw/acpi/vmgenid.h| 35 +
> 7 files changed, 294 insertions(+)
> create mode 100644 hw/acpi/vmgenid.c
> create mode 100644 include/hw/acpi/vmgenid.h
>
> diff --git a/default-configs/i386-softmmu.mak
> b/default-configs/i386-softmmu.mak
> index 48b07a4..029e952 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -59,3 +59,4 @@ CONFIG_I82801B11=y
> CONFIG_SMBIOS=y
> CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
> CONFIG_PXB=y
> +CONFIG_ACPI_VMGENID=y
> diff --git a/default-configs/x86_64-softmmu.mak
> b/default-configs/x86_64-softmmu.mak
> index fd96345..d1d7432 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -59,3 +59,4 @@ CONFIG_I82801B11=y
> CONFIG_SMBIOS=y
> CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
> CONFIG_PXB=y
> +CONFIG_ACPI_VMGENID=y
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index 6acf798..11c35bc 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -5,6 +5,7 @@ common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
> common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
> common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
> common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
> +common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
> common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>
> common-obj-y += acpi_interface.o
> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> new file mode 100644
> index 000..8fba7e0
> --- /dev/null
> +++ b/hw/acpi/vmgenid.c
> @@ -0,0 +1,239 @@
> +/*
> + * Virtual Machine Generation ID Device
> + *
> + * Copyright (C) 2017 Skyport Systems.
> + *
> + * Author: Ben Warren
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qmp-commands.h"
> +#include "hw/acpi/acpi.h"
> +#include "hw/acpi/aml-build.h"
> +#include "hw/acpi/vmgenid.h"
> +#include "hw/nvram/fw_cfg.h"
> +#include "sysemu/sysemu.h"
> +
> +void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid,
> +BIOSLinker *linker)
> +{
> +Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx;
> +uint32_t vgia_offset;
> +QemuUUID guid_le;
> +
> +/* Fill in the GUID values. These need to be converted to little-endian
> + * first, since that's what the guest expects
> + */
> +g_array_set_size(guid, VMGENID_FW_CFG_SIZE - ARRAY_SIZE(guid_le.data));
> +guid_le = vms->guid;
> +qemu_uuid_bswap(&guid_le);
> +/* The GUID is written at a fixed offset into the fw_cfg file
> + * in order to implement the "OVMF SDT Header probe suppressor"
> + * see docs/specs/vmgenid.txt for more details
> + */
> +g_array_insert_vals(guid, VMGENID_GUID_OFFSET, guid_le.data,
> +ARRAY_SIZE(guid_le.data));
> +
> +/* Put this in a separate SSDT table */
> +ssdt = init_aml_allocator();
> +
> +/* Reserve space for header */
> +acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
> +
> +/* Storage for the GUID address */
> +vgia_offset = table_data->len +
> +build_append_named_dword(ssdt->buf, "VGIA");
> +scope = aml_scope("\\_SB");
> +dev = aml_device("VGEN");
> +aml_append(dev, aml_name_decl("_HID", aml_string("QEMUVGID")));
> +aml_append(dev, aml_name_decl("_CID", aml_string("VM_Gen_Counter")));
> +aml_append(dev, aml_name_decl("_DDN", aml_string("VM_Gen_Counter")));
> +
> +/* Simple status method to check that address is linked and non-zero */
> +method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> +addr = aml_local(0);
> +aml_append(method, aml_store(aml_int(0xf), addr));
> +if_ctx = aml_if(aml_equal(aml_name("VGIA"), aml_int(0)));
> +aml_append(if_ctx, aml_store(aml_int(0), addr));
> +aml_append(method, if_ctx);
> +aml_append(method, aml_return(addr));
> +aml_append(dev, method);
> +
> +/* the ADDR meth
