Hi Igor. Thanks for the review! > On Feb 15, 2017, at 4:19 AM, Igor Mammedov <imamm...@redhat.com> wrote: > > On Tue, 14 Feb 2017 22:15:46 -0800 > b...@skyportsystems.com <mailto:b...@skyportsystems.com> wrote: > >> From: Ben Warren <b...@skyportsystems.com> >> >> 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 <b...@skyportsystems.com> >> --- >> default-configs/i386-softmmu.mak | 1 + >> default-configs/x86_64-softmmu.mak | 1 + >> hw/acpi/Makefile.objs | 1 + >> hw/acpi/vmgenid.c | 237 >> +++++++++++++++++++++++++++++++++++ >> hw/i386/acpi-build.c | 16 +++ >> include/hw/acpi/acpi_dev_interface.h | 1 + >> include/hw/acpi/vmgenid.h | 35 ++++++ >> 7 files changed, 292 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 0000000..b1b7b32 >> --- /dev/null >> +++ b/hw/acpi/vmgenid.c >> @@ -0,0 +1,237 @@ >> +/* >> + * Virtual Machine Generation ID Device >> + * >> + * Copyright (C) 2017 Skyport Systems. >> + * >> + * Author: Ben Warren <b...@skyportsystems.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 "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); >> + memcpy(&guid_le.data, &vms->guid.data, sizeof(vms->guid.data)); >> + 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 method returns two 32-bit words representing the lower and >> + * upper halves * of the physical address of the fw_cfg blob >> + * (holding the GUID) >> + */ >> + method = aml_method("ADDR", 0, AML_NOTSERIALIZED); >> + >> + addr = aml_local(0); >> + aml_append(method, aml_store(aml_package(2), addr)); >> + >> + aml_append(method, aml_store(aml_add(aml_name("VGIA"), >> + aml_int(VMGENID_GUID_OFFSET), >> NULL), >> + aml_index(addr, aml_int(0)))); >> + aml_append(method, aml_store(aml_int(0), aml_index(addr, aml_int(1)))); > Just curious, > so suggested in v5 simple declaration style > > Package(2) { > ADD(VGIA, VMGENID_GUID_OFFSET), > 0 > } > > wasn't working for you? > I tried and tried and pulled my hair out and couldn’t get it to work. I understand why this would be better, and will give it another quick go. >> + aml_append(method, aml_return(addr)); >> + >> + aml_append(dev, method); >> + aml_append(scope, dev); >> + aml_append(ssdt, scope); >> + >> + /* attach an ACPI notify */ >> + method = aml_method("\\_GPE._E05", 0, AML_NOTSERIALIZED); >> + aml_append(method, aml_notify(aml_name("\\_SB.VGEN"), aml_int(0x80))); >> + aml_append(ssdt, method); >> + >> + g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len); >> + >> + /* Allocate guest memory for the Data fw_cfg blob */ >> + bios_linker_loader_alloc(linker, VMGENID_GUID_FW_CFG_FILE, guid, 4096, >> + false /* page boundary, high memory */); >> + >> + /* Patch address of GUID fw_cfg blob into the ADDR fw_cfg blob >> + * so QEMU can write the GUID there. The address is expected to be >> + * < 4GB, but write 64 bits anyway. >> + */ >> + bios_linker_loader_write_pointer(linker, >> + VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint64_t), >> + VMGENID_GUID_FW_CFG_FILE); >> + >> + /* Patch address of GUID fw_cfg blob into the AML so OSPM can retrieve >> + * and read it. Note that while we provide storage for 64 bits, only >> + * the least-signficant 32 get patched into AML. >> + */ >> + bios_linker_loader_add_pointer(linker, >> + ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t), >> + VMGENID_GUID_FW_CFG_FILE, 0); >> + >> + build_header(linker, table_data, >> + (void *)(table_data->data + table_data->len - ssdt->buf->len), >> + "SSDT", ssdt->buf->len, 1, NULL, "VMGENID"); >> + free_aml_allocator(); >> +} >> + >> +void vmgenid_add_fw_cfg(VmGenIdState *vms, FWCfgState *s, GArray *guid) >> +{ >> + /* Create a read-only fw_cfg file for GUID */ >> + fw_cfg_add_file(s, VMGENID_GUID_FW_CFG_FILE, guid->data, >> + VMGENID_FW_CFG_SIZE); >> + /* Create a read-write fw_cfg file for Address */ >> + fw_cfg_add_file_callback(s, VMGENID_ADDR_FW_CFG_FILE, NULL, NULL, >> + vms->vmgenid_addr_le, >> + ARRAY_SIZE(vms->vmgenid_addr_le), false); >> +} >> + >> +static void vmgenid_update_guest(VmGenIdState *vms) >> +{ >> + Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL); >> + uint32_t vmgenid_addr; >> + QemuUUID guid_le; >> + >> + if (obj) { >> + /* Write the GUID to guest memory */ >> + memcpy(&vmgenid_addr, vms->vmgenid_addr_le, sizeof(vmgenid_addr)); >> + vmgenid_addr = le32_to_cpu(vmgenid_addr); >> + /* A zero value in vmgenid_addr means that BIOS has not yet written >> + * the address >> + */ >> + if (vmgenid_addr) { >> + /* QemuUUID has the first three words as big-endian, and expect >> + * that any GUIDs passed in will always be BE. The guest, >> + * however, will expect the fields to be little-endian. >> + * Perform a byte swap immediately before writing. >> + */ >> + memcpy(&guid_le.data, &vms->guid.data, sizeof(vms->guid.data)); > potential stack corruption if guid_le and vms->guid types ever diverge > why not just do > guid_le.data = guid.data > >> + 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 >> + */ >> + cpu_physical_memory_write(vmgenid_addr + VMGENID_GUID_OFFSET, >> + guid_le.data, sizeof(guid_le.data)); >> + /* Send _GPE.E05 event */ >> + acpi_send_event(DEVICE(obj), ACPI_VMGENID_CHANGE_STATUS); >> + } >> + } >> +} >> + >> +static void vmgenid_set_guid(Object *obj, const char *value, Error **errp) >> +{ >> + VmGenIdState *vms = VMGENID(obj); >> + >> + if (!strcmp(value, "auto")) { >> + qemu_uuid_generate(&vms->guid); >> + } else if (qemu_uuid_parse(value, &vms->guid) < 0) { >> + error_setg(errp, "'%s. %s': Failed to parse GUID string: %s", >> + object_get_typename(OBJECT(vms)), VMGENID_GUID, value); >> + return; >> + } >> + >> + vmgenid_update_guest(vms); >> +} >> + >> +/* After restoring an image, we need to update the guest memory and notify >> + * it of a potential change to VM Generation ID >> + */ >> +static int vmgenid_post_load(void *opaque, int version_id) >> +{ >> + VmGenIdState *vms = opaque; >> + vmgenid_update_guest(vms); >> + return 0; >> +} >> + >> +static const VMStateDescription vmstate_vmgenid = { >> + .name = "vmgenid", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .post_load = vmgenid_post_load, >> + .fields = (VMStateField[]) { >> + VMSTATE_UINT8_ARRAY(vmgenid_addr_le, VmGenIdState, >> sizeof(uint64_t)), >> + VMSTATE_END_OF_LIST() >> + }, >> +}; >> + >> +static void vmgenid_initfn(Object *obj) >> +{ >> + object_property_add_str(obj, VMGENID_GUID, NULL, vmgenid_set_guid, >> NULL); > missing: > object_property_set_description() > or even better use class properties here: > > object_class_property_add_str()/object_class_property_set_description() > >> +} >> + >> +static void vmgenid_handle_reset(void *opaque) >> +{ >> + VmGenIdState *vms = VMGENID(opaque); >> + /* Clear the guest-allocated GUID address when the VM resets */ >> + memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le)); >> +} >> + >> +static void vmgenid_realize(DeviceState *dev, Error **errp) >> +{ >> + VmGenIdState *vms = VMGENID(dev); >> + qemu_register_reset(vmgenid_handle_reset, vms); >> +} >> + >> +static void vmgenid_device_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + dc->vmsd = &vmstate_vmgenid; >> + dc->realize = vmgenid_realize; > it needs: > > dc->hotpluggable = false; > OK >> +} >> + >> +static const TypeInfo vmgenid_device_info = { >> + .name = VMGENID_DEVICE, >> + .parent = TYPE_DEVICE, >> + .instance_size = sizeof(VmGenIdState), >> + .instance_init = vmgenid_initfn, >> + .class_init = vmgenid_device_class_init, >> +}; >> + >> +static void vmgenid_register_types(void) >> +{ >> + type_register_static(&vmgenid_device_info); >> +} >> + >> +type_init(vmgenid_register_types) >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index 1c928ab..db04cf5 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -42,6 +42,7 @@ >> #include "hw/acpi/memory_hotplug.h" >> #include "sysemu/tpm.h" >> #include "hw/acpi/tpm.h" >> +#include "hw/acpi/vmgenid.h" >> #include "sysemu/tpm_backend.h" >> #include "hw/timer/mc146818rtc_regs.h" >> #include "sysemu/numa.h" >> @@ -2610,6 +2611,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState >> *machine) >> size_t aml_len = 0; >> GArray *tables_blob = tables->table_data; >> AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL }; >> + Object *vmgenid_dev; >> >> acpi_get_pm_info(&pm); >> acpi_get_misc_info(&misc); >> @@ -2653,6 +2655,13 @@ void acpi_build(AcpiBuildTables *tables, MachineState >> *machine) >> acpi_add_table(table_offsets, tables_blob); >> build_madt(tables_blob, tables->linker, pcms); >> >> + vmgenid_dev = find_vmgenid_dev(); >> + if (vmgenid_dev) { >> + acpi_add_table(table_offsets, tables_blob); >> + vmgenid_build_acpi(VMGENID(vmgenid_dev), tables_blob, >> + tables->vmgenid, tables->linker); >> + } >> + >> if (misc.has_hpet) { >> acpi_add_table(table_offsets, tables_blob); >> build_hpet(tables_blob, tables->linker); >> @@ -2823,6 +2832,7 @@ void acpi_setup(void) >> PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); >> AcpiBuildTables tables; >> AcpiBuildState *build_state; >> + Object *vmgenid_dev; >> >> if (!pcms->fw_cfg) { >> ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n"); >> @@ -2859,6 +2869,12 @@ void acpi_setup(void) >> fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, >> tables.tcpalog->data, acpi_data_len(tables.tcpalog)); >> >> + vmgenid_dev = find_vmgenid_dev(); >> + if (vmgenid_dev) { >> + vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg, >> + tables.vmgenid); >> + } >> + >> if (!pcmc->rsdp_in_ram) { >> /* >> * Keep for compatibility with old machine types. >> diff --git a/include/hw/acpi/acpi_dev_interface.h >> b/include/hw/acpi/acpi_dev_interface.h >> index 71d3c48..3c2e4e9 100644 >> --- a/include/hw/acpi/acpi_dev_interface.h >> +++ b/include/hw/acpi/acpi_dev_interface.h >> @@ -11,6 +11,7 @@ typedef enum { >> ACPI_CPU_HOTPLUG_STATUS = 4, >> ACPI_MEMORY_HOTPLUG_STATUS = 8, >> ACPI_NVDIMM_HOTPLUG_STATUS = 16, >> + ACPI_VMGENID_CHANGE_STATUS = 32, >> } AcpiEventStatusBits; >> >> #define TYPE_ACPI_DEVICE_IF "acpi-device-interface" >> diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h >> new file mode 100644 >> index 0000000..db7fa0e >> --- /dev/null >> +++ b/include/hw/acpi/vmgenid.h >> @@ -0,0 +1,35 @@ >> +#ifndef ACPI_VMGENID_H >> +#define ACPI_VMGENID_H >> + >> +#include "hw/acpi/bios-linker-loader.h" >> +#include "hw/qdev.h" >> +#include "qemu/uuid.h" >> + >> +#define VMGENID_DEVICE "vmgenid" >> +#define VMGENID_GUID "guid" >> +#define VMGENID_GUID_FW_CFG_FILE "etc/vmgenid_guid" >> +#define VMGENID_ADDR_FW_CFG_FILE "etc/vmgenid_addr" >> + >> +#define VMGENID_FW_CFG_SIZE 4096 /* Occupy a page of memory */ >> +#define VMGENID_GUID_OFFSET 40 /* allow space for >> + * OVMF SDT Header Probe Supressor >> + */ >> + >> +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE) >> + >> +typedef struct VmGenIdState { >> + DeviceClass parent_obj; >> + QemuUUID guid; /* The 128-bit GUID seen by the guest */ >> + uint8_t vmgenid_addr_le[8]; /* Address of the GUID (little-endian) */ >> +} VmGenIdState; >> + >> +static inline Object *find_vmgenid_dev(void) >> +{ >> + return object_resolve_path_type("", VMGENID_DEVICE, NULL); > What will happen if CLI would be: > -device vmgenid -device vmgenid > As I understand, it should be exclusive single instance device, > and there is nothing to prevent second instance of it. > I don’t know, I’ll see what I can do here. > >> +} >> + >> +void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid, >> + BIOSLinker *linker); >> +void vmgenid_add_fw_cfg(VmGenIdState *vms, FWCfgState *s, GArray *guid); >> + >> +#endif
smime.p7s
Description: S/MIME cryptographic signature