Re: [Qemu-devel] [PATCH 2/2] i386: Add a Virtual Machine Generation ID device.

[Gal Hammer <ghammer@redhat.com>]

On 02/10/2014 15:49, Michael S. Tsirkin wrote:
> On Wed, Sep 17, 2014 at 02:39:52PM +0300, Gal Hammer wrote:
>> Based on Microsoft's sepecifications (paper can be dowloaded from
>> http://go.microsoft.com/fwlink/?LinkId=260709), add a device
>> description to the SSDT ACPI table.
>>
>> The GUID is set using a new "vmgenid" device.
>>
>> Signed-off-by: Gal Hammer <ghammer@redhat.com>
>> Reviewed-By: Igor Mammedov <imammedo@redhat.com>
>> ---
>>   default-configs/i386-softmmu.mak   |   1 +
>>   default-configs/x86_64-softmmu.mak |   1 +
>>   hw/i386/Makefile.objs              |   2 +-
>>   hw/i386/acpi-build.c               |  39 +++++++
>>   hw/i386/ssdt-vmgenid.dsl           |  63 ++++++++++++
>>   hw/i386/ssdt-vmgenid.hex.generated | 206 +++++++++++++++++++++++++++++++++++++
>>   hw/misc/Makefile.objs              |   1 +
>>   hw/misc/vmgenid.c                  |  84 +++++++++++++++
>>   include/hw/i386/pc.h               |   3 +
>
> Patch scripts/update-acpi.sh as well please.

I understand what should be changed there. The script just copy all the 
*.hex files to *.hex.generated.

>>   9 files changed, 399 insertions(+), 1 deletion(-)
>>   create mode 100644 hw/i386/ssdt-vmgenid.dsl
>>   create mode 100644 hw/i386/ssdt-vmgenid.hex.generated
>>   create mode 100644 hw/misc/vmgenid.c
>>
>> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
>> index 8e08841..bd33c75 100644
>> --- a/default-configs/i386-softmmu.mak
>> +++ b/default-configs/i386-softmmu.mak
>> @@ -45,3 +45,4 @@ CONFIG_IOAPIC=y
>>   CONFIG_ICC_BUS=y
>>   CONFIG_PVPANIC=y
>>   CONFIG_MEM_HOTPLUG=y
>> +CONFIG_VMGENID=y
>> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
>> index 66557ac..006fc7c 100644
>> --- a/default-configs/x86_64-softmmu.mak
>> +++ b/default-configs/x86_64-softmmu.mak
>> @@ -45,3 +45,4 @@ CONFIG_IOAPIC=y
>>   CONFIG_ICC_BUS=y
>>   CONFIG_PVPANIC=y
>>   CONFIG_MEM_HOTPLUG=y
>> +CONFIG_VMGENID=y
>> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
>> index 9d419ad..cd1beb3 100644
>> --- a/hw/i386/Makefile.objs
>> +++ b/hw/i386/Makefile.objs
>> @@ -12,7 +12,7 @@ hw/i386/acpi-build.o: hw/i386/acpi-build.c hw/i386/acpi-dsdt.hex \
>>   	hw/i386/ssdt-proc.hex hw/i386/ssdt-pcihp.hex hw/i386/ssdt-misc.hex \
>>   	hw/i386/acpi-dsdt.hex hw/i386/q35-acpi-dsdt.hex \
>>   	hw/i386/q35-acpi-dsdt.hex hw/i386/ssdt-mem.hex \
>> -	hw/i386/ssdt-tpm.hex
>> +	hw/i386/ssdt-tpm.hex hw/i386/ssdt-vmgenid.hex
>>
>>   iasl-option=$(shell if test -z "`$(1) $(2) 2>&1 > /dev/null`" \
>>       ; then echo "$(2)"; else echo "$(3)"; fi ;)
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index a313321..72d5a88 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -96,6 +96,8 @@ typedef struct AcpiMiscInfo {
>>       const unsigned char *dsdt_code;
>>       unsigned dsdt_size;
>>       uint16_t pvpanic_port;
>> +    bool vm_generation_id_set;
>> +    uint8_t vm_generation_id[16];
>>   } AcpiMiscInfo;
>>
>>   typedef struct AcpiBuildPciBusHotplugState {
>> @@ -216,6 +218,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
>>       info->has_hpet = hpet_find();
>>       info->has_tpm = tpm_find();
>>       info->pvpanic_port = pvpanic_port();
>> +    info->vm_generation_id_set = vm_generation_id(info->vm_generation_id);
>>   }
>>
>>   static void acpi_get_pci_info(PcPciInfo *info)
>> @@ -710,6 +713,7 @@ static inline char acpi_get_hex(uint32_t val)
>>   #include "hw/i386/ssdt-misc.hex"
>>   #include "hw/i386/ssdt-pcihp.hex"
>>   #include "hw/i386/ssdt-tpm.hex"
>> +#include "hw/i386/ssdt-vmgenid.hex"
>>
>>   static void
>>   build_append_notify_method(GArray *device, const char *name,
>> @@ -1246,6 +1250,37 @@ build_tpm_ssdt(GArray *table_data, GArray *linker)
>>       memcpy(tpm_ptr, ssdt_tpm_aml, sizeof(ssdt_tpm_aml));
>>   }
>>
>> +static void
>> +build_vmgenid_ssdt(GArray *table_data, GArray *linker, AcpiMiscInfo *info)
>> +{
>> +    int vgid_start = table_data->len;
>> +    void *vgid_ptr;
>> +    uint8_t *vm_gid_ptr;
>> +    uint32_t vm_gid_physical_address;
>> +
>> +    vgid_ptr = acpi_data_push(table_data, sizeof(ssdt_vmgenid_aml));
>> +    memcpy(vgid_ptr, ssdt_vmgenid_aml, sizeof(ssdt_vmgenid_aml));
>> +
>> +    vm_gid_ptr = acpi_data_get_ptr(vgid_ptr, sizeof(ssdt_vmgenid_aml),
>> +                                   *ssdt_acpi_vm_gid,
>> +                                   sizeof(info->vm_generation_id));
>> +    memcpy(vm_gid_ptr, info->vm_generation_id,
>> +           sizeof(info->vm_generation_id));
>> +
>> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>> +                                   ACPI_BUILD_TABLE_FILE,
>> +                                   table_data,
>> +                                   vgid_ptr + *ssdt_acpi_vm_gid_addr,
>> +                                   sizeof(uint32_t));
>> +
>> +    vm_gid_physical_address = vgid_start + *ssdt_acpi_vm_gid;
>> +    ACPI_BUILD_SET_LE(vgid_ptr, sizeof(ssdt_vmgenid_aml),
>> +                      *ssdt_acpi_vm_gid_addr, 32, vm_gid_physical_address);
>> +
>> +    build_header(linker, table_data, vgid_ptr, "SSDT",
>> +                 sizeof(ssdt_vmgenid_aml), 1);
>> +}
>> +
>>   typedef enum {
>>       MEM_AFFINITY_NOFLAGS      = 0,
>>       MEM_AFFINITY_ENABLED      = (1 << 0),
>> @@ -1617,6 +1652,10 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>>           acpi_add_table(table_offsets, tables->table_data);
>>           build_tpm_ssdt(tables->table_data, tables->linker);
>>       }
>> +    if (misc.vm_generation_id_set) {
>> +        acpi_add_table(table_offsets, tables->table_data);
>> +        build_vmgenid_ssdt(tables->table_data, tables->linker, &misc);
>> +    }
>>       if (guest_info->numa_nodes) {
>>           acpi_add_table(table_offsets, tables->table_data);
>>           build_srat(tables->table_data, tables->linker, &cpu, guest_info);
>> diff --git a/hw/i386/ssdt-vmgenid.dsl b/hw/i386/ssdt-vmgenid.dsl
>> new file mode 100644
>> index 0000000..fee376f
>> --- /dev/null
>> +++ b/hw/i386/ssdt-vmgenid.dsl
>> @@ -0,0 +1,63 @@
>> +/*
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> +
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> +
>> + * You should have received a copy of the GNU General Public License along
>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +/****************************************************************
>> + * Virtual Machine Generation ID Device
>> + ****************************************************************/
>> +
>> +ACPI_EXTRACT_ALL_CODE ssdt_vmgenid_aml
>> +
>> +DefinitionBlock (
>> +    "ssdt-vmgenid.aml", // Output Filename
>> +    "SSDT",             // Signature
>> +    0x01,               // SSDT Compliance Revision
>> +    "BXPC",             // OEMID
>> +    "BXSSDTSUSP",       // TABLE ID
>> +    0x1                 // OEM Revision
>> +    )
>> +{
>> +    Scope(\_SB) {
>> +
>> +        Device(VMGI) {
>> +            Name(_HID, "QEMU0002")
>> +            Name(_CID, "VM_Gen_Counter")
>> +            Name(_DDN, "VM_Gen_Counter")
>> +
>> +            ACPI_EXTRACT_NAME_DWORD_CONST ssdt_acpi_vm_gid_addr
>> +            Name(VGIA, 0x12345678)
>> +
>> +            ACPI_EXTRACT_NAME_BUFFER16 ssdt_acpi_vm_gid
>> +            Name(VGID, Buffer(16) {
>> +                0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +                0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 })
>
> IMHO, statically allocating the ID in ACPI like this is a mistake.

Do you have an idea where else it can be allocated? Since it is just an 
ACPI declared device without resources. A PCI device with a memory 
region looks over-kill just to have a known physical address.

> For example, your guest might be in hybernation, in this
> case it will not re-read ACPI tables on boot.

In this case the VGID physical address should stay the same and only the 
value is updated. So I don't think should be a problem.

> Also - what guarantees that this buffer is 8 byte aligned?

The spec say nothing about the memory-alignment of the VGID's physical 
address. If the driver can't read an unaligned address I guess it can do 
a simple pointers calculation and read from the closest aligned address 
and beyond the VGID buffer, no?

>> +
>> +            Method(_STA, 0, NotSerialized) {
>> +                Store(VGIA, Local0)
>> +                If (LEqual(Local0, Zero)) {
>> +                    Return (0x00)
>> +                } Else {
>> +                    Return (0x0F)
>> +                }
>> +            }
>> +
>> +            Method(ADDR, 0, Serialized) {
>> +                Store(Package(2) { }, Local0)
>> +                Store(VGIA, Index(Local0, 0))
>> +                Store(0x0000, Index(Local0, 1))
>> +                return (Local0)
>> +            }
>> +        }
>> +    }
>> +}
>> diff --git a/hw/i386/ssdt-vmgenid.hex.generated b/hw/i386/ssdt-vmgenid.hex.generated
>> new file mode 100644
>> index 0000000..e96cb36
>> --- /dev/null
>> +++ b/hw/i386/ssdt-vmgenid.hex.generated
>> @@ -0,0 +1,206 @@
>> +static unsigned char ssdt_vmgenid_aml[] = {
>> +0x53,
>> +0x53,
>> +0x44,
>> +0x54,
>> +0xc6,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x1,
>> +0xeb,
>> +0x42,
>> +0x58,
>> +0x50,
>> +0x43,
>> +0x0,
>> +0x0,
>> +0x42,
>> +0x58,
>> +0x53,
>> +0x53,
>> +0x44,
>> +0x54,
>> +0x53,
>> +0x55,
>> +0x1,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x49,
>> +0x4e,
>> +0x54,
>> +0x4c,
>> +0x24,
>> +0x4,
>> +0x14,
>> +0x20,
>> +0x10,
>> +0x41,
>> +0xa,
>> +0x5c,
>> +0x5f,
>> +0x53,
>> +0x42,
>> +0x5f,
>> +0x5b,
>> +0x82,
>> +0x48,
>> +0x9,
>> +0x56,
>> +0x4d,
>> +0x47,
>> +0x49,
>> +0x8,
>> +0x5f,
>> +0x48,
>> +0x49,
>> +0x44,
>> +0xd,
>> +0x51,
>> +0x45,
>> +0x4d,
>> +0x55,
>> +0x30,
>> +0x30,
>> +0x30,
>> +0x32,
>> +0x0,
>> +0x8,
>> +0x5f,
>> +0x43,
>> +0x49,
>> +0x44,
>> +0xd,
>> +0x56,
>> +0x4d,
>> +0x5f,
>> +0x47,
>> +0x65,
>> +0x6e,
>> +0x5f,
>> +0x43,
>> +0x6f,
>> +0x75,
>> +0x6e,
>> +0x74,
>> +0x65,
>> +0x72,
>> +0x0,
>> +0x8,
>> +0x5f,
>> +0x44,
>> +0x44,
>> +0x4e,
>> +0xd,
>> +0x56,
>> +0x4d,
>> +0x5f,
>> +0x47,
>> +0x65,
>> +0x6e,
>> +0x5f,
>> +0x43,
>> +0x6f,
>> +0x75,
>> +0x6e,
>> +0x74,
>> +0x65,
>> +0x72,
>> +0x0,
>> +0x8,
>> +0x56,
>> +0x47,
>> +0x49,
>> +0x41,
>> +0xc,
>> +0x78,
>> +0x56,
>> +0x34,
>> +0x12,
>> +0x8,
>> +0x56,
>> +0x47,
>> +0x49,
>> +0x44,
>> +0x11,
>> +0x13,
>> +0xa,
>> +0x10,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x0,
>> +0x14,
>> +0x18,
>> +0x5f,
>> +0x53,
>> +0x54,
>> +0x41,
>> +0x0,
>> +0x70,
>> +0x56,
>> +0x47,
>> +0x49,
>> +0x41,
>> +0x60,
>> +0xa0,
>> +0x6,
>> +0x93,
>> +0x60,
>> +0x0,
>> +0xa4,
>> +0x0,
>> +0xa1,
>> +0x4,
>> +0xa4,
>> +0xa,
>> +0xf,
>> +0x14,
>> +0x1c,
>> +0x41,
>> +0x44,
>> +0x44,
>> +0x52,
>> +0x8,
>> +0x70,
>> +0x12,
>> +0x2,
>> +0x2,
>> +0x60,
>> +0x70,
>> +0x56,
>> +0x47,
>> +0x49,
>> +0x41,
>> +0x88,
>> +0x60,
>> +0x0,
>> +0x0,
>> +0x70,
>> +0x0,
>> +0x88,
>> +0x60,
>> +0x1,
>> +0x0,
>> +0xa4,
>> +0x60
>> +};
>> +static unsigned char ssdt_acpi_vm_gid_addr[] = {
>> +0x73
>> +};
>> +static unsigned char ssdt_acpi_vm_gid[] = {
>> +0x80
>> +};
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index 979e532..c18b800 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -41,3 +41,4 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o
>>   obj-$(CONFIG_ZYNQ) += zynq_slcr.o
>>
>>   obj-$(CONFIG_PVPANIC) += pvpanic.o
>> +obj-$(CONFIG_VMGENID) += vmgenid.o
>> diff --git a/hw/misc/vmgenid.c b/hw/misc/vmgenid.c
>> new file mode 100644
>> index 0000000..81cc49e
>> --- /dev/null
>> +++ b/hw/misc/vmgenid.c
>> @@ -0,0 +1,84 @@
>> +/*
>> + *  Virtual Machine Generation ID Device
>> + *
>> + *  Copyright (C) 2014 Red Hat Inc.
>> + *
>> + *  Authors: Gal Hammer <ghammer@redhat.com>
>> + *
>> + * 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 "hw/i386/pc.h"
>> +#include "hw/sysbus.h"
>> +
>> +#define VMGENID_DEVICE "vmgenid"
>> +
>> +#define PROPERTY_UUID "uuid"
>> +
>> +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE)
>> +
>> +typedef struct VmGenIdState {
>> +    SysBusDevice parent_obj;
>> +    char *guid_arg;
>> +} VmGenIdState;
>> +
>> +bool vm_generation_id(uint8_t id[16])
>> +{
>> +    Object *o = object_resolve_path_type("", VMGENID_DEVICE, NULL);
>> +    char *guid;
>> +
>> +    if (!o) {
>> +        return false;
>> +    }
>> +    guid = object_property_get_str(o, PROPERTY_UUID, NULL);
>> +    /* actual uuid validation was checked during realize. */
>> +    (void)qemu_uuid_parse(guid, id);
>> +    return true;
>> +}
>> +
>> +static void vmgenid_realize(DeviceState *dev, Error **errp)
>> +{
>> +    VmGenIdState *s = VMGENID(dev);
>> +    uint8_t id[16];
>> +
>> +    if (!s->guid_arg) {
>> +        error_setg(errp, "missing uuid.");
>> +        return;
>> +    }
>> +
>> +    if (qemu_uuid_parse(s->guid_arg, id) < 0) {
>> +        error_setg(errp, "Fail to parse UUID string.");
>> +        return;
>> +    }
>> +}
>> +
>> +static Property vmgenid_device_properties[] = {
>> +    DEFINE_PROP_STRING(PROPERTY_UUID, VmGenIdState, guid_arg),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void vmgenid_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->realize = vmgenid_realize;
>> +    dc->props = vmgenid_device_properties;
>> +    dc->cannot_instantiate_with_device_add_yet = false;
>> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> +}
>> +
>> +static const TypeInfo vmgenid_device_info = {
>> +    .name          = VMGENID_DEVICE,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(VmGenIdState),
>> +    .class_init    = vmgenid_class_init,
>> +};
>> +
>> +static void vmgenid_register_types(void)
>> +{
>> +    type_register_static(&vmgenid_device_info);
>> +}
>> +
>> +type_init(vmgenid_register_types)
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 77316d5..40ecccb 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -290,6 +290,9 @@ void pc_system_firmware_init(MemoryRegion *rom_memory,
>>   /* pvpanic.c */
>>   uint16_t pvpanic_port(void);
>>
>> +/* vmgenid.c */
>> +bool vm_generation_id(uint8_t id[16]);
>> +
>>   /* e820 types */
>>   #define E820_RAM        1
>>   #define E820_RESERVED   2
>> --
>> 1.9.3
>>