[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2] i386: Add a Virtual Machine Generation ID d
|
From: |
Michael S. Tsirkin |
|
Subject: |
Re: [Qemu-devel] [PATCH 2/2] i386: Add a Virtual Machine Generation ID device. |
|
Date: |
Thu, 2 Oct 2014 16:30:38 +0300 |
On Thu, Oct 02, 2014 at 04:14:05PM +0300, Gal Hammer wrote:
> 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 <address@hidden>
> >>Reviewed-By: Igor Mammedov <address@hidden>
> >>---
> >> 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.
I guess you can reserve some memory in E820, but that's pretty
involved.
It might be a better idea to allocate buffer e.g. like TPM does, but
fill the data in from hypervisor, by passing the physical address back
to host. _INI might work for this.
> >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.
What will update the value? ACPI tables in qemu are ignored.
> >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 <address@hidden>
> >>+ *
> >>+ * 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
> >>