From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52905) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZghc-0001XI-Ux for qemu-devel@nongnu.org; Thu, 02 Oct 2014 09:45:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XZghY-0001c9-AZ for qemu-devel@nongnu.org; Thu, 02 Oct 2014 09:45:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45064) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZghX-0001by-Hv for qemu-devel@nongnu.org; Thu, 02 Oct 2014 09:45:47 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s92DjkPc027942 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 2 Oct 2014 09:45:46 -0400 Message-ID: <542D4F9D.3020104@redhat.com> Date: Thu, 02 Oct 2014 16:14:05 +0300 From: Gal Hammer MIME-Version: 1.0 References: <1410953992-14542-1-git-send-email-ghammer@redhat.com> <1410953992-14542-3-git-send-email-ghammer@redhat.com> <20141002124955.GB25552@redhat.com> In-Reply-To: <20141002124955.GB25552@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] i386: Add a Virtual Machine Generation ID device. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Paolo Bonzini , qemu-devel@nongnu.org, Igor Mammedov 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 >> Reviewed-By: Igor Mammedov >> --- >> 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 . >> + */ >> + >> +/**************************************************************** >> + * 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 >> + * >> + * 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 >>