| Message ID | 28ff6ff023dd041fc7557955dea916adce72efea.1486285434.git.ben@skyportsystems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Feb 05, 2017 at 01:12:00AM -0800, ben@skyportsystems.com wrote: > From: Ben Warren <ben@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 <ben@skyportsystems.com> Thanks! I think it's mostly in a good shape. Comments inside. > --- > default-configs/i386-softmmu.mak | 1 + > default-configs/x86_64-softmmu.mak | 1 + > hw/acpi/Makefile.objs | 1 + > hw/acpi/vmgenid.c | 206 +++++++++++++++++++++++++++++++++++ > hw/i386/acpi-build.c | 10 ++ > include/hw/acpi/acpi_dev_interface.h | 1 + > include/hw/acpi/vmgenid.h | 37 +++++++ > 7 files changed, 257 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 384cefb..1a43542 100644 > --- a/default-configs/i386-softmmu.mak > +++ b/default-configs/i386-softmmu.mak > @@ -58,3 +58,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 491a191..aee8b08 100644 > --- a/default-configs/x86_64-softmmu.mak > +++ b/default-configs/x86_64-softmmu.mak > @@ -58,3 +58,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..6c9ecfd > --- /dev/null > +++ b/hw/acpi/vmgenid.c > @@ -0,0 +1,206 @@ > +/* > + * Virtual Machine Generation ID Device > + * > + * Copyright (C) 2017 Skyport Systems. > + * > + * Author: Ben Warren <ben@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(GArray *table_data, GArray *guid, BIOSLinker *linker) > +{ > + Object *obj; > + VmGenIdState *s; > + Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx; > + uint32_t vgia_offset; > + > + obj = find_vmgenid_dev(NULL); > + assert(obj); > + s = VMGENID(obj); > + > + /* Fill in the GUID values */ > + if (guid->len != VMGENID_FW_CFG_SIZE) { > + g_array_set_size(guid, VMGENID_FW_CFG_SIZE); > + } > + g_array_insert_vals(guid, VMGENID_GUID_OFFSET, s->guid.data, 16); ARRAY_SIZE(s->guid.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) */ /* * multiline comments * look like this */ > + 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)))); > + 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 */ add ... so QEMU can write GUID there > + bios_linker_loader_add_pointer(linker, > + VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint32_t), > + VMGENID_GUID_FW_CFG_FILE, 0, true); > + > + /* Patch address of GUID fw_cfg blob into the AML */ add ... so OSPM can retrieve and read it > + bios_linker_loader_add_pointer(linker, > + ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t), > + VMGENID_GUID_FW_CFG_FILE, 0, false); > + > + 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(FWCfgState *s, GArray *guid) > +{ > + Object *obj = find_vmgenid_dev(NULL); > + assert(obj); > + VmGenIdState *vms = VMGENID(obj); > + > + /* 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, Seems wrong. What if guest updates the address after command line set it? You want a callback to copy guid there. > + vms->vgia_le, sizeof(uint32_t), false); Should be ARRAY_SIZE(vms->vgia_le). Also, I thought GUID is at an offset from the beginning in order to trick OVMF, isn't it? > +} > + > +static void vmgenid_update_guest(VmGenIdState *s) > +{ > + Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL); > + uint32_t vgia; > + > + if (obj) { > + /* Write the GUID to guest memory */ > + memcpy(&vgia, s->vgia_le, sizeof(vgia)); > + vgia = le32_to_cpu(vgia); > + if (vgia) { add a comment saying 0 means bios did not run yet. > + cpu_physical_memory_write(vgia + VMGENID_GUID_OFFSET, > + s->guid.data, sizeof(s->guid.data)); Avoid this. You want dma.h APIs. > + /* 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 *s = VMGENID(obj); > + > + if (!strncmp(value, "auto", 4)) { I'd use strcmp here. > + qemu_uuid_generate(&s->guid); > + } else if (qemu_uuid_parse(value, &s->guid) < 0) { > + error_setg(errp, "'%s. %s': Failed to parse GUID string: %s", > + object_get_typename(OBJECT(s)), VMGENID_GUID, value); > + return; > + } > + /* 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, so store that way internally. This makes my head spin. It's the actual guid, right? Only place we need it is when we copy it into guest memory. So swap there - and do it to a copy so you won't need these comments. > Make > + * sure to swap back whenever reporting via monitor */ And what code does this swap back? > + qemu_uuid_bswap(&s->guid); > + > + /* Send the ACPI notify */ > + vmgenid_update_guest(s); > +} > + > +/* 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 *s = opaque; > + vmgenid_update_guest(s); > + 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(vgia_le, VmGenIdState, sizeof(uint32_t)), > + VMSTATE_END_OF_LIST() > + }, > +}; > + > +static void vmgenid_initfn(Object *obj) > +{ > + object_property_add_str(obj, VMGENID_GUID, NULL, vmgenid_set_guid, NULL); > +} > + > +static void vmgenid_device_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->vmsd = &vmstate_vmgenid; > +} > + > +static const TypeInfo vmgenid_device_info = { > + .name = VMGENID_DEVICE, > + .parent = TYPE_SYS_BUS_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 78a1d84..4c40f76 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" > @@ -2653,6 +2654,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) > acpi_add_table(table_offsets, tables_blob); > build_madt(tables_blob, tables->linker, pcms); > > + if (find_vmgenid_dev(NULL)) { > + acpi_add_table(table_offsets, tables_blob); > + vmgenid_build_acpi(tables_blob, tables->vmgenid, tables->linker); > + } > + > if (misc.has_hpet) { > acpi_add_table(table_offsets, tables_blob); > build_hpet(tables_blob, tables->linker); > @@ -2859,6 +2865,10 @@ void acpi_setup(void) > fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, > tables.tcpalog->data, acpi_data_len(tables.tcpalog)); > > + if (find_vmgenid_dev(NULL)) { > + vmgenid_add_fw_cfg(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..b60437a > --- /dev/null > +++ b/include/hw/acpi/vmgenid.h > @@ -0,0 +1,37 @@ > +#ifndef ACPI_VMGENID_H > +#define ACPI_VMGENID_H > + > +#include "hw/acpi/bios-linker-loader.h" > +#include "hw/sysbus.h" > +#include "qemu/uuid.h" > + > +#define VMGENID_DEVICE "vmgenid" > +#define VMGENID_GUID "guid" > +#define VMGENID_GUID_FW_CFG_FILE "etc/vmgenid" Maybe 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 */ ACPI header size is 36 bytes. I'd expect just to use that. Where does 40 come from? I think it's an 8 bytes alignment requirement. So do QEMU_ALIGN_UP(sizeof(AcpiTableHeader), 8) Add a comment explaining that 8. > + > +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid); > +void vmgenid_build_acpi(GArray *table_data, GArray *guid, BIOSLinker *linker); > + > +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE) > + > +typedef struct VmGenIdState { > + SysBusDevice parent_obj; > + QemuUUID guid; > + uint8_t vgia_le[4]; Please give this fields a better name. Is this the address? Pls add comments, too. How about we make it 8 byte so it's future proof? alternatively put it in a ram block instead. > +} VmGenIdState; > + > +static Object *find_vmgenid_dev(Error **errp) > +{ > + Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL); > + if (!obj && errp) { > + error_setg(errp, "%s is not found", VMGENID_DEVICE); > + } > + return obj; > +} > + > +#endif > -- > 2.7.4
> On Feb 6, 2017, at 8:15 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Sun, Feb 05, 2017 at 01:12:00AM -0800, ben@skyportsystems.com <mailto:ben@skyportsystems.com> wrote: >> From: Ben Warren <ben@skyportsystems.com <mailto:ben@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 <ben@skyportsystems.com <mailto:ben@skyportsystems.com>> > > > Thanks! > I think it's mostly in a good shape. > Comments inside. > Thanks! Hopefully the next rev has everything you want. > >> --- >> default-configs/i386-softmmu.mak | 1 + >> default-configs/x86_64-softmmu.mak | 1 + >> hw/acpi/Makefile.objs | 1 + >> hw/acpi/vmgenid.c | 206 +++++++++++++++++++++++++++++++++++ >> hw/i386/acpi-build.c | 10 ++ >> include/hw/acpi/acpi_dev_interface.h | 1 + >> include/hw/acpi/vmgenid.h | 37 +++++++ >> 7 files changed, 257 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 384cefb..1a43542 100644 >> --- a/default-configs/i386-softmmu.mak >> +++ b/default-configs/i386-softmmu.mak >> @@ -58,3 +58,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 491a191..aee8b08 100644 >> --- a/default-configs/x86_64-softmmu.mak >> +++ b/default-configs/x86_64-softmmu.mak >> @@ -58,3 +58,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..6c9ecfd >> --- /dev/null >> +++ b/hw/acpi/vmgenid.c >> @@ -0,0 +1,206 @@ >> +/* >> + * Virtual Machine Generation ID Device >> + * >> + * Copyright (C) 2017 Skyport Systems. >> + * >> + * Author: Ben Warren <ben@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(GArray *table_data, GArray *guid, BIOSLinker *linker) >> +{ >> + Object *obj; >> + VmGenIdState *s; >> + Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx; >> + uint32_t vgia_offset; >> + >> + obj = find_vmgenid_dev(NULL); >> + assert(obj); >> + s = VMGENID(obj); >> + >> + /* Fill in the GUID values */ >> + if (guid->len != VMGENID_FW_CFG_SIZE) { >> + g_array_set_size(guid, VMGENID_FW_CFG_SIZE); >> + } >> + g_array_insert_vals(guid, VMGENID_GUID_OFFSET, s->guid.data, 16); > > ARRAY_SIZE(s->guid.data); > OK >> + >> + /* 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) */ > > /* > * multiline comments > * look like this > */ > OK >> + 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)))); >> + 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 */ > > add ... so QEMU can write GUID there > OK >> + bios_linker_loader_add_pointer(linker, >> + VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint32_t), >> + VMGENID_GUID_FW_CFG_FILE, 0, true); >> + >> + /* Patch address of GUID fw_cfg blob into the AML */ > > add ... so OSPM can retrieve and read it > OK >> + bios_linker_loader_add_pointer(linker, >> + ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t), >> + VMGENID_GUID_FW_CFG_FILE, 0, false); >> + >> + 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(FWCfgState *s, GArray *guid) >> +{ >> + Object *obj = find_vmgenid_dev(NULL); >> + assert(obj); >> + VmGenIdState *vms = VMGENID(obj); >> + >> + /* 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, > > Seems wrong. What if guest updates the address after command line > set it? You want a callback to copy guid there. > Sure, I can do that. My understanding was that this is a read callback, but if it also is called upon a write, we should do what you suggest. > >> + vms->vgia_le, sizeof(uint32_t), false); > > Should be ARRAY_SIZE(vms->vgia_le). > > Also, I thought GUID is at an offset from the beginning > in order to trick OVMF, isn't it? > The offset is taken into account in the GUID fw_cfg file. The writing to guest memory, we add the offset to VGIA. > >> +} >> + >> +static void vmgenid_update_guest(VmGenIdState *s) >> +{ >> + Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL); >> + uint32_t vgia; >> + >> + if (obj) { >> + /* Write the GUID to guest memory */ >> + memcpy(&vgia, s->vgia_le, sizeof(vgia)); >> + vgia = le32_to_cpu(vgia); >> + if (vgia) { > > add a comment saying 0 means bios did not run yet. OK > >> + cpu_physical_memory_write(vgia + VMGENID_GUID_OFFSET, >> + s->guid.data, sizeof(s->guid.data)); > > Avoid this. You want dma.h APIs. I’ll have a look at those. This function was recommended to me in an earlier patch version. > >> + /* 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 *s = VMGENID(obj); >> + >> + if (!strncmp(value, "auto", 4)) { > > I'd use strcmp here. OK > >> + qemu_uuid_generate(&s->guid); >> + } else if (qemu_uuid_parse(value, &s->guid) < 0) { >> + error_setg(errp, "'%s. %s': Failed to parse GUID string: %s", >> + object_get_typename(OBJECT(s)), VMGENID_GUID, value); >> + return; >> + } >> + /* 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, so store that way internally. > > This makes my head spin. It's the actual guid, right? Only place we need it is when > we copy it into guest memory. So swap there - > and do it to a copy so you won't need these comments. I understand. I wanted to only do the swap at one place and decided to do it closest to the user interface, but can do it at the other end instead. > >> Make >> + * sure to swap back whenever reporting via monitor */ > > And what code does this swap back? A later patch where the QMP code is added. But I’ll remove that necessity. > >> + qemu_uuid_bswap(&s->guid); >> + >> + /* Send the ACPI notify */ >> + vmgenid_update_guest(s); >> +} >> + >> +/* 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 *s = opaque; >> + vmgenid_update_guest(s); >> + 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(vgia_le, VmGenIdState, sizeof(uint32_t)), >> + VMSTATE_END_OF_LIST() >> + }, >> +}; >> + >> +static void vmgenid_initfn(Object *obj) >> +{ >> + object_property_add_str(obj, VMGENID_GUID, NULL, vmgenid_set_guid, NULL); >> +} >> + >> +static void vmgenid_device_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + dc->vmsd = &vmstate_vmgenid; >> +} >> + >> +static const TypeInfo vmgenid_device_info = { >> + .name = VMGENID_DEVICE, >> + .parent = TYPE_SYS_BUS_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 78a1d84..4c40f76 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" >> @@ -2653,6 +2654,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) >> acpi_add_table(table_offsets, tables_blob); >> build_madt(tables_blob, tables->linker, pcms); >> >> + if (find_vmgenid_dev(NULL)) { >> + acpi_add_table(table_offsets, tables_blob); >> + vmgenid_build_acpi(tables_blob, tables->vmgenid, tables->linker); >> + } >> + >> if (misc.has_hpet) { >> acpi_add_table(table_offsets, tables_blob); >> build_hpet(tables_blob, tables->linker); >> @@ -2859,6 +2865,10 @@ void acpi_setup(void) >> fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, >> tables.tcpalog->data, acpi_data_len(tables.tcpalog)); >> >> + if (find_vmgenid_dev(NULL)) { >> + vmgenid_add_fw_cfg(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..b60437a >> --- /dev/null >> +++ b/include/hw/acpi/vmgenid.h >> @@ -0,0 +1,37 @@ >> +#ifndef ACPI_VMGENID_H >> +#define ACPI_VMGENID_H >> + >> +#include "hw/acpi/bios-linker-loader.h" >> +#include "hw/sysbus.h" >> +#include "qemu/uuid.h" >> + >> +#define VMGENID_DEVICE "vmgenid" >> +#define VMGENID_GUID "guid" >> +#define VMGENID_GUID_FW_CFG_FILE "etc/vmgenid" > > Maybe vmgenid_guid ? Sure. > >> +#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 */ > > ACPI header size is 36 bytes. I'd expect just to use that. > Where does 40 come from? I think it's an 8 bytes alignment requirement. > The number 40 was asked for by Laszlo for the OVMF SDT Header Probe Suppressor, which I don’t know anything about. This GUID isn’t going in an ACPI table, so I’m not sure what you mean here. We allocate on a page boundary and 40 bytes in should be 8-byte aligned. > So do QEMU_ALIGN_UP(sizeof(AcpiTableHeader), 8) > > Add a comment explaining that 8. > >> + >> +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid); >> +void vmgenid_build_acpi(GArray *table_data, GArray *guid, BIOSLinker *linker); >> + >> +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE) >> + >> +typedef struct VmGenIdState { >> + SysBusDevice parent_obj; >> + QemuUUID guid; >> + uint8_t vgia_le[4]; > > Please give this fields a better name. Is this the address? > Pls add comments, too. > This variable name was suggested by Laszlo. the ‘a’ in ‘vgia’ refers to address, but I’ll add some comments. > How about we make it 8 byte so it's future proof? I can do that, but a previous conversation we had made it clear that guests would never allocate above 4GB so 64 bits wasn’t necessary. > alternatively put it in a ram block instead. > >> +} VmGenIdState; >> + >> +static Object *find_vmgenid_dev(Error **errp) >> +{ >> + Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL); >> + if (!obj && errp) { >> + error_setg(errp, "%s is not found", VMGENID_DEVICE); >> + } >> + return obj; >> +} >> + >> +#endif >> -- >> 2.7.4
On Mon, Feb 06, 2017 at 09:29:30AM -0800, Ben Warren wrote: > > On Feb 6, 2017, at 8:15 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Sun, Feb 05, 2017 at 01:12:00AM -0800, ben@skyportsystems.com wrote: > > From: Ben Warren <ben@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 <ben@skyportsystems.com> > > > > Thanks! > I think it's mostly in a good shape. > Comments inside. > > > Thanks! Hopefully the next rev has everything you want. > > > > --- > default-configs/i386-softmmu.mak | 1 + > default-configs/x86_64-softmmu.mak | 1 + > hw/acpi/Makefile.objs | 1 + > hw/acpi/vmgenid.c | 206 > +++++++++++++++++++++++++++++++++++ > hw/i386/acpi-build.c | 10 ++ > include/hw/acpi/acpi_dev_interface.h | 1 + > include/hw/acpi/vmgenid.h | 37 +++++++ > 7 files changed, 257 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 384cefb..1a43542 100644 > --- a/default-configs/i386-softmmu.mak > +++ b/default-configs/i386-softmmu.mak > @@ -58,3 +58,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 491a191..aee8b08 100644 > --- a/default-configs/x86_64-softmmu.mak > +++ b/default-configs/x86_64-softmmu.mak > @@ -58,3 +58,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..6c9ecfd > --- /dev/null > +++ b/hw/acpi/vmgenid.c > @@ -0,0 +1,206 @@ > +/* > + * Virtual Machine Generation ID Device > + * > + * Copyright (C) 2017 Skyport Systems. > + * > + * Author: Ben Warren <ben@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(GArray *table_data, GArray *guid, BIOSLinker > *linker) > +{ > + Object *obj; > + VmGenIdState *s; > + Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx; > + uint32_t vgia_offset; > + > + obj = find_vmgenid_dev(NULL); > + assert(obj); > + s = VMGENID(obj); > + > + /* Fill in the GUID values */ > + if (guid->len != VMGENID_FW_CFG_SIZE) { > + g_array_set_size(guid, VMGENID_FW_CFG_SIZE); > + } > + g_array_insert_vals(guid, VMGENID_GUID_OFFSET, s->guid.data, 16); > > > ARRAY_SIZE(s->guid.data); > > > OK > > + > + /* 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) */ > > > /* > * multiline comments > * look like this > */ > > > OK > > + 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)))); > + 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 */ > > > add ... so QEMU can write GUID there > > > OK > > + bios_linker_loader_add_pointer(linker, > + VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint32_t), > + VMGENID_GUID_FW_CFG_FILE, 0, true); > + > + /* Patch address of GUID fw_cfg blob into the AML */ > > > add ... so OSPM can retrieve and read it > > > OK > > + bios_linker_loader_add_pointer(linker, > + ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t), > + VMGENID_GUID_FW_CFG_FILE, 0, false); > + > + 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(FWCfgState *s, GArray *guid) > +{ > + Object *obj = find_vmgenid_dev(NULL); > + assert(obj); > + VmGenIdState *vms = VMGENID(obj); > + > + /* 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, > > > Seems wrong. What if guest updates the address after command line > set it? You want a callback to copy guid there. > > > Sure, I can do that. My understanding was that this is a read callback, but if > it also is called upon a write, we should do what you suggest. Hmm you are right. But we really need to call something on write though - unlike read, it must be called after write. Otherwise I don't see how it can work if you set gen id before guest boots. I guess this means we need yet another callback per file. FWCfgWriteCallback ? Can you implement this in hw/nvram/fw_cfg.c? It's rather straight-forward to do. > > > + vms->vgia_le, sizeof(uint32_t), false); > > > Should be ARRAY_SIZE(vms->vgia_le). > > Also, I thought GUID is at an offset from the beginning > in order to trick OVMF, isn't it? > > > The offset is taken into account in the GUID fw_cfg file. The writing to guest > memory, we add the offset to VGIA. I keep forgetting it's the address. Really please rename it using full words. > > +} > + > +static void vmgenid_update_guest(VmGenIdState *s) > +{ > + Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, > NULL); > + uint32_t vgia; > + > + if (obj) { > + /* Write the GUID to guest memory */ > + memcpy(&vgia, s->vgia_le, sizeof(vgia)); > + vgia = le32_to_cpu(vgia); > + if (vgia) { > > > add a comment saying 0 means bios did not run yet. > > OK > > > > + cpu_physical_memory_write(vgia + VMGENID_GUID_OFFSET, > + s->guid.data, sizeof(s-> > guid.data)); > > > Avoid this. You want dma.h APIs. > > I’ll have a look at those. This function was recommended to me in an earlier > patch version. > > > > + /* 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 *s = VMGENID(obj); > + > + if (!strncmp(value, "auto", 4)) { > > > I'd use strcmp here. > > OK > > > > + qemu_uuid_generate(&s->guid); > + } else if (qemu_uuid_parse(value, &s->guid) < 0) { > + error_setg(errp, "'%s. %s': Failed to parse GUID string: %s", > + object_get_typename(OBJECT(s)), VMGENID_GUID, > value); > + return; > + } > + /* 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, so store that way internally. > > > This makes my head spin. It's the actual guid, right? Only place we need it > is when > we copy it into guest memory. So swap there - > and do it to a copy so you won't need these comments. > > I understand. I wanted to only do the swap at one place and decided to do it > closest to the user interface, but can do it at the other end instead. > > > > Make > + * sure to swap back whenever reporting via monitor */ > > > And what code does this swap back? > > A later patch where the QMP code is added. But I’ll remove that necessity. > > > > + qemu_uuid_bswap(&s->guid); > + > + /* Send the ACPI notify */ > + vmgenid_update_guest(s); > +} > + > +/* 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 *s = opaque; > + vmgenid_update_guest(s); > + 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(vgia_le, VmGenIdState, sizeof(uint32_t)), > + VMSTATE_END_OF_LIST() > + }, > +}; > + > +static void vmgenid_initfn(Object *obj) > +{ > + object_property_add_str(obj, VMGENID_GUID, NULL, vmgenid_set_guid, > NULL); > +} > + > +static void vmgenid_device_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->vmsd = &vmstate_vmgenid; > +} > + > +static const TypeInfo vmgenid_device_info = { > + .name = VMGENID_DEVICE, > + .parent = TYPE_SYS_BUS_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 78a1d84..4c40f76 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" > @@ -2653,6 +2654,11 @@ void acpi_build(AcpiBuildTables *tables, > MachineState *machine) > acpi_add_table(table_offsets, tables_blob); > build_madt(tables_blob, tables->linker, pcms); > > + if (find_vmgenid_dev(NULL)) { > + acpi_add_table(table_offsets, tables_blob); > + vmgenid_build_acpi(tables_blob, tables->vmgenid, tables-> > linker); > + } > + > if (misc.has_hpet) { > acpi_add_table(table_offsets, tables_blob); > build_hpet(tables_blob, tables->linker); > @@ -2859,6 +2865,10 @@ void acpi_setup(void) > fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, > tables.tcpalog->data, acpi_data_len > (tables.tcpalog)); > > + if (find_vmgenid_dev(NULL)) { > + vmgenid_add_fw_cfg(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..b60437a > --- /dev/null > +++ b/include/hw/acpi/vmgenid.h > @@ -0,0 +1,37 @@ > +#ifndef ACPI_VMGENID_H > +#define ACPI_VMGENID_H > + > +#include "hw/acpi/bios-linker-loader.h" > +#include "hw/sysbus.h" > +#include "qemu/uuid.h" > + > +#define VMGENID_DEVICE "vmgenid" > +#define VMGENID_GUID "guid" > +#define VMGENID_GUID_FW_CFG_FILE "etc/vmgenid" > > > Maybe vmgenid_guid ? > > Sure. > > > > +#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 */ > > > ACPI header size is 36 bytes. I'd expect just to use that. > Where does 40 come from? I think it's an 8 bytes alignment requirement. > > > The number 40 was asked for by Laszlo for the OVMF SDT Header Probe Suppressor, > which I don’t know anything about. I think I do though :) > This GUID isn’t going in an ACPI table, so > I’m not sure what you mean here. We allocate on a page boundary and 40 bytes > in should be 8-byte aligned. > > So do QEMU_ALIGN_UP(sizeof(AcpiTableHeader), 8) > > Add a comment explaining that 8. Laszlo could you suggest a comment explaining a bit about what OVMF does here? > > + > +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid); > +void vmgenid_build_acpi(GArray *table_data, GArray *guid, BIOSLinker > *linker); > + > +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE) > + > +typedef struct VmGenIdState { > + SysBusDevice parent_obj; > + QemuUUID guid; > + uint8_t vgia_le[4]; > > > Please give this fields a better name. Is this the address? > Pls add comments, too. > > > This variable name was suggested by Laszlo. the ‘a’ in ‘vgia’ refers to > address, but I’ll add some comments. vmgenid_addr_le? > How about we make it 8 byte so it's future proof? > > I can do that, but a previous conversation we had made it clear that guests > would never allocate above 4GB so 64 bits wasn’t necessary. Right, it's just very painful to change once we make it 32 bit. > alternatively put it in a ram block instead. > > > +} VmGenIdState; > + > +static Object *find_vmgenid_dev(Error **errp) > +{ > + Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL); > + if (!obj && errp) { > + error_setg(errp, "%s is not found", VMGENID_DEVICE); > + } > + return obj; > +} > + > +#endif > -- > 2.7.4 > >
> On Feb 6, 2017, at 9:41 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Feb 06, 2017 at 09:29:30AM -0800, Ben Warren wrote: >> >> On Feb 6, 2017, at 8:15 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> On Sun, Feb 05, 2017 at 01:12:00AM -0800, ben@skyportsystems.com wrote: >> >> From: Ben Warren <ben@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 <ben@skyportsystems.com> >> >> >> >> Thanks! >> I think it's mostly in a good shape. >> Comments inside. >> >> >> Thanks! Hopefully the next rev has everything you want. >> >> >> >> --- >> default-configs/i386-softmmu.mak | 1 + >> default-configs/x86_64-softmmu.mak | 1 + >> hw/acpi/Makefile.objs | 1 + >> hw/acpi/vmgenid.c | 206 >> +++++++++++++++++++++++++++++++++++ >> hw/i386/acpi-build.c | 10 ++ >> include/hw/acpi/acpi_dev_interface.h | 1 + >> include/hw/acpi/vmgenid.h | 37 +++++++ >> 7 files changed, 257 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 384cefb..1a43542 100644 >> --- a/default-configs/i386-softmmu.mak >> +++ b/default-configs/i386-softmmu.mak >> @@ -58,3 +58,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 491a191..aee8b08 100644 >> --- a/default-configs/x86_64-softmmu.mak >> +++ b/default-configs/x86_64-softmmu.mak >> @@ -58,3 +58,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..6c9ecfd >> --- /dev/null >> +++ b/hw/acpi/vmgenid.c >> @@ -0,0 +1,206 @@ >> +/* >> + * Virtual Machine Generation ID Device >> + * >> + * Copyright (C) 2017 Skyport Systems. >> + * >> + * Author: Ben Warren <ben@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(GArray *table_data, GArray *guid, BIOSLinker >> *linker) >> +{ >> + Object *obj; >> + VmGenIdState *s; >> + Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx; >> + uint32_t vgia_offset; >> + >> + obj = find_vmgenid_dev(NULL); >> + assert(obj); >> + s = VMGENID(obj); >> + >> + /* Fill in the GUID values */ >> + if (guid->len != VMGENID_FW_CFG_SIZE) { >> + g_array_set_size(guid, VMGENID_FW_CFG_SIZE); >> + } >> + g_array_insert_vals(guid, VMGENID_GUID_OFFSET, s->guid.data, 16); >> >> >> ARRAY_SIZE(s->guid.data); >> >> >> OK >> >> + >> + /* 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) */ >> >> >> /* >> * multiline comments >> * look like this >> */ >> >> >> OK >> >> + 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)))); >> + 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 */ >> >> >> add ... so QEMU can write GUID there >> >> >> OK >> >> + bios_linker_loader_add_pointer(linker, >> + VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint32_t), >> + VMGENID_GUID_FW_CFG_FILE, 0, true); >> + >> + /* Patch address of GUID fw_cfg blob into the AML */ >> >> >> add ... so OSPM can retrieve and read it >> >> >> OK >> >> + bios_linker_loader_add_pointer(linker, >> + ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t), >> + VMGENID_GUID_FW_CFG_FILE, 0, false); >> + >> + 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(FWCfgState *s, GArray *guid) >> +{ >> + Object *obj = find_vmgenid_dev(NULL); >> + assert(obj); >> + VmGenIdState *vms = VMGENID(obj); >> + >> + /* 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, >> >> >> Seems wrong. What if guest updates the address after command line >> set it? You want a callback to copy guid there. >> >> >> Sure, I can do that. My understanding was that this is a read callback, but if >> it also is called upon a write, we should do what you suggest. > > Hmm you are right. But we really need to call something > on write though - unlike read, it must be called after write. > Otherwise I don't see how it can work if you set gen id before > guest boots. > > I guess this means we need yet another callback per file. > FWCfgWriteCallback ? > > Can you implement this in hw/nvram/fw_cfg.c? > It's rather straight-forward to do. > The reason it works is that we put the initial contents of the GUID (as supplied by command-line) into the GUID fw_cfg in the ‘vmgenid_build_acpi()’ function, which is guaranteed to happen before the guest boots. The only time QEMU needs to know VGIA is on later updates to the GUID (via monitor) or when restoring. If you really think this extra complexity is needed, I can do so, but it seems to work very well as-is. > >> >> >> + vms->vgia_le, sizeof(uint32_t), false); >> >> >> Should be ARRAY_SIZE(vms->vgia_le). >> >> Also, I thought GUID is at an offset from the beginning >> in order to trick OVMF, isn't it? >> >> >> The offset is taken into account in the GUID fw_cfg file. The writing to guest >> memory, we add the offset to VGIA. > > I keep forgetting it's the address. Really please rename it > using full words. > Will do. > >> >> +} >> + >> +static void vmgenid_update_guest(VmGenIdState *s) >> +{ >> + Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, >> NULL); >> + uint32_t vgia; >> + >> + if (obj) { >> + /* Write the GUID to guest memory */ >> + memcpy(&vgia, s->vgia_le, sizeof(vgia)); >> + vgia = le32_to_cpu(vgia); >> + if (vgia) { >> >> >> add a comment saying 0 means bios did not run yet. >> >> OK >> >> >> >> + cpu_physical_memory_write(vgia + VMGENID_GUID_OFFSET, >> + s->guid.data, sizeof(s-> >> guid.data)); >> >> >> Avoid this. You want dma.h APIs. >> >> I’ll have a look at those. This function was recommended to me in an earlier >> patch version. >> >> >> >> + /* 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 *s = VMGENID(obj); >> + >> + if (!strncmp(value, "auto", 4)) { >> >> >> I'd use strcmp here. >> >> OK >> >> >> >> + qemu_uuid_generate(&s->guid); >> + } else if (qemu_uuid_parse(value, &s->guid) < 0) { >> + error_setg(errp, "'%s. %s': Failed to parse GUID string: %s", >> + object_get_typename(OBJECT(s)), VMGENID_GUID, >> value); >> + return; >> + } >> + /* 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, so store that way internally. >> >> >> This makes my head spin. It's the actual guid, right? Only place we need it >> is when >> we copy it into guest memory. So swap there - >> and do it to a copy so you won't need these comments. >> >> I understand. I wanted to only do the swap at one place and decided to do it >> closest to the user interface, but can do it at the other end instead. >> >> >> >> Make >> + * sure to swap back whenever reporting via monitor */ >> >> >> And what code does this swap back? >> >> A later patch where the QMP code is added. But I’ll remove that necessity. >> >> >> >> + qemu_uuid_bswap(&s->guid); >> + >> + /* Send the ACPI notify */ >> + vmgenid_update_guest(s); >> +} >> + >> +/* 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 *s = opaque; >> + vmgenid_update_guest(s); >> + 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(vgia_le, VmGenIdState, sizeof(uint32_t)), >> + VMSTATE_END_OF_LIST() >> + }, >> +}; >> + >> +static void vmgenid_initfn(Object *obj) >> +{ >> + object_property_add_str(obj, VMGENID_GUID, NULL, vmgenid_set_guid, >> NULL); >> +} >> + >> +static void vmgenid_device_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + dc->vmsd = &vmstate_vmgenid; >> +} >> + >> +static const TypeInfo vmgenid_device_info = { >> + .name = VMGENID_DEVICE, >> + .parent = TYPE_SYS_BUS_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 78a1d84..4c40f76 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" >> @@ -2653,6 +2654,11 @@ void acpi_build(AcpiBuildTables *tables, >> MachineState *machine) >> acpi_add_table(table_offsets, tables_blob); >> build_madt(tables_blob, tables->linker, pcms); >> >> + if (find_vmgenid_dev(NULL)) { >> + acpi_add_table(table_offsets, tables_blob); >> + vmgenid_build_acpi(tables_blob, tables->vmgenid, tables-> >> linker); >> + } >> + >> if (misc.has_hpet) { >> acpi_add_table(table_offsets, tables_blob); >> build_hpet(tables_blob, tables->linker); >> @@ -2859,6 +2865,10 @@ void acpi_setup(void) >> fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, >> tables.tcpalog->data, acpi_data_len >> (tables.tcpalog)); >> >> + if (find_vmgenid_dev(NULL)) { >> + vmgenid_add_fw_cfg(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..b60437a >> --- /dev/null >> +++ b/include/hw/acpi/vmgenid.h >> @@ -0,0 +1,37 @@ >> +#ifndef ACPI_VMGENID_H >> +#define ACPI_VMGENID_H >> + >> +#include "hw/acpi/bios-linker-loader.h" >> +#include "hw/sysbus.h" >> +#include "qemu/uuid.h" >> + >> +#define VMGENID_DEVICE "vmgenid" >> +#define VMGENID_GUID "guid" >> +#define VMGENID_GUID_FW_CFG_FILE "etc/vmgenid" >> >> >> Maybe vmgenid_guid ? >> >> Sure. >> >> >> >> +#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 */ >> >> >> ACPI header size is 36 bytes. I'd expect just to use that. >> Where does 40 come from? I think it's an 8 bytes alignment requirement. >> >> >> The number 40 was asked for by Laszlo for the OVMF SDT Header Probe Suppressor, >> which I don’t know anything about. > > I think I do though :) > >> This GUID isn’t going in an ACPI table, so >> I’m not sure what you mean here. We allocate on a page boundary and 40 bytes >> in should be 8-byte aligned. >> >> So do QEMU_ALIGN_UP(sizeof(AcpiTableHeader), 8) >> >> Add a comment explaining that 8. > > Laszlo could you suggest a comment explaining a bit about what > OVMF does here? > >> >> + >> +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid); >> +void vmgenid_build_acpi(GArray *table_data, GArray *guid, BIOSLinker >> *linker); >> + >> +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE) >> + >> +typedef struct VmGenIdState { >> + SysBusDevice parent_obj; >> + QemuUUID guid; >> + uint8_t vgia_le[4]; >> >> >> Please give this fields a better name. Is this the address? >> Pls add comments, too. >> >> >> This variable name was suggested by Laszlo. the ‘a’ in ‘vgia’ refers to >> address, but I’ll add some comments. > > vmgenid_addr_le? > > >> How about we make it 8 byte so it's future proof? >> >> I can do that, but a previous conversation we had made it clear that guests >> would never allocate above 4GB so 64 bits wasn’t necessary. > > Right, it's just very painful to change once we make it 32 bit. > OK, make sense. I’ll wait to hear from Laszlo before continuing. >> alternatively put it in a ram block instead. >> >> >> +} VmGenIdState; >> + >> +static Object *find_vmgenid_dev(Error **errp) >> +{ >> + Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL); >> + if (!obj && errp) { >> + error_setg(errp, "%s is not found", VMGENID_DEVICE); >> + } >> + return obj; >> +} >> + >> +#endif >> -- >> 2.7.4
On Mon, Feb 06, 2017 at 09:59:55AM -0800, Ben Warren wrote: > +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid) > +{ > + Object *obj = find_vmgenid_dev(NULL); > + assert(obj); > + VmGenIdState *vms = VMGENID(obj); > + > + /* 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, > > > Seems wrong. What if guest updates the address after command line > set it? You want a callback to copy guid there. > > > Sure, I can do that. My understanding was that this is a read > callback, but if > it also is called upon a write, we should do what you suggest. > > > Hmm you are right. But we really need to call something > on write though - unlike read, it must be called after write. > Otherwise I don't see how it can work if you set gen id before > guest boots. > > I guess this means we need yet another callback per file. > FWCfgWriteCallback ? > > Can you implement this in hw/nvram/fw_cfg.c? > It's rather straight-forward to do. > > > The reason it works is that we put the initial contents of the GUID (as > supplied by command-line) into the GUID fw_cfg in the ‘vmgenid_build_acpi()’ > function, which is guaranteed to happen before the guest boots. The only time > QEMU needs to know VGIA is on later updates to the GUID (via monitor) or when > restoring. If you really think this extra complexity is needed, I can do so, > but it seems to work very well as-is. I see. So it's a race condition I think. It works unless you change the gen id after bios read the guid from the guid file but before it wrote out the address. Or do I miss something?
> On Feb 6, 2017, at 10:17 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Feb 06, 2017 at 09:59:55AM -0800, Ben Warren wrote: >> +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid) >> +{ >> + Object *obj = find_vmgenid_dev(NULL); >> + assert(obj); >> + VmGenIdState *vms = VMGENID(obj); >> + >> + /* 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, >> >> >> Seems wrong. What if guest updates the address after command line >> set it? You want a callback to copy guid there. >> >> >> Sure, I can do that. My understanding was that this is a read >> callback, but if >> it also is called upon a write, we should do what you suggest. >> >> >> Hmm you are right. But we really need to call something >> on write though - unlike read, it must be called after write. >> Otherwise I don't see how it can work if you set gen id before >> guest boots. >> >> I guess this means we need yet another callback per file. >> FWCfgWriteCallback ? >> >> Can you implement this in hw/nvram/fw_cfg.c? >> It's rather straight-forward to do. >> >> >> The reason it works is that we put the initial contents of the GUID (as >> supplied by command-line) into the GUID fw_cfg in the ‘vmgenid_build_acpi()’ >> function, which is guaranteed to happen before the guest boots. The only time >> QEMU needs to know VGIA is on later updates to the GUID (via monitor) or when >> restoring. If you really think this extra complexity is needed, I can do so, >> but it seems to work very well as-is. > > I see. So it's a race condition I think. It works unless you change the > gen id after bios read the guid from the guid file but before it wrote > out the address. > > Or do I miss something? > I don’t think it’s an issue because BIOS is not a consumer of the GUID. I suppose there’s a window where another GUID could be written via monitor prior to bios writing back, but the change wouldn’t be applied to memory in vmgenid_update_guest() because vgia=0. That would be solved by a write callback and we could remove the GUID copying from vmgenid_build_acpi(). It’s a very unlikely scenario, though. > -- > MST
On Mon, Feb 06, 2017 at 10:48:05AM -0800, Ben Warren wrote: > > > On Feb 6, 2017, at 10:17 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Feb 06, 2017 at 09:59:55AM -0800, Ben Warren wrote: > >> +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid) > >> +{ > >> + Object *obj = find_vmgenid_dev(NULL); > >> + assert(obj); > >> + VmGenIdState *vms = VMGENID(obj); > >> + > >> + /* 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, > >> > >> > >> Seems wrong. What if guest updates the address after command line > >> set it? You want a callback to copy guid there. > >> > >> > >> Sure, I can do that. My understanding was that this is a read > >> callback, but if > >> it also is called upon a write, we should do what you suggest. > >> > >> > >> Hmm you are right. But we really need to call something > >> on write though - unlike read, it must be called after write. > >> Otherwise I don't see how it can work if you set gen id before > >> guest boots. > >> > >> I guess this means we need yet another callback per file. > >> FWCfgWriteCallback ? > >> > >> Can you implement this in hw/nvram/fw_cfg.c? > >> It's rather straight-forward to do. > >> > >> > >> The reason it works is that we put the initial contents of the GUID (as > >> supplied by command-line) into the GUID fw_cfg in the ‘vmgenid_build_acpi()’ > >> function, which is guaranteed to happen before the guest boots. The only time > >> QEMU needs to know VGIA is on later updates to the GUID (via monitor) or when > >> restoring. If you really think this extra complexity is needed, I can do so, > >> but it seems to work very well as-is. > > > > I see. So it's a race condition I think. It works unless you change the > > gen id after bios read the guid from the guid file but before it wrote > > out the address. > > > > Or do I miss something? > > > I don’t think it’s an issue because BIOS is not a consumer of the > GUID. I suppose there’s a window where another GUID could be written > via monitor prior to bios writing back, but the change wouldn’t be > applied to memory in vmgenid_update_guest() because vgia=0. That > would be solved by a write callback and we could remove the GUID > copying from vmgenid_build_acpi(). It’s a very unlikely scenario, > though. When people run 100000 VMs and up, every unlikely scenario tends to trigger. > > -- > > MST >
> On Feb 6, 2017, at 11:04 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Feb 06, 2017 at 10:48:05AM -0800, Ben Warren wrote: >> >>> On Feb 6, 2017, at 10:17 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >>> >>> On Mon, Feb 06, 2017 at 09:59:55AM -0800, Ben Warren wrote: >>>> +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid) >>>> +{ >>>> + Object *obj = find_vmgenid_dev(NULL); >>>> + assert(obj); >>>> + VmGenIdState *vms = VMGENID(obj); >>>> + >>>> + /* 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, >>>> >>>> >>>> Seems wrong. What if guest updates the address after command line >>>> set it? You want a callback to copy guid there. >>>> >>>> >>>> Sure, I can do that. My understanding was that this is a read >>>> callback, but if >>>> it also is called upon a write, we should do what you suggest. >>>> >>>> >>>> Hmm you are right. But we really need to call something >>>> on write though - unlike read, it must be called after write. >>>> Otherwise I don't see how it can work if you set gen id before >>>> guest boots. >>>> >>>> I guess this means we need yet another callback per file. >>>> FWCfgWriteCallback ? >>>> >>>> Can you implement this in hw/nvram/fw_cfg.c? >>>> It's rather straight-forward to do. >>>> >>>> >>>> The reason it works is that we put the initial contents of the GUID (as >>>> supplied by command-line) into the GUID fw_cfg in the ‘vmgenid_build_acpi()’ >>>> function, which is guaranteed to happen before the guest boots. The only time >>>> QEMU needs to know VGIA is on later updates to the GUID (via monitor) or when >>>> restoring. If you really think this extra complexity is needed, I can do so, >>>> but it seems to work very well as-is. >>> >>> I see. So it's a race condition I think. It works unless you change the >>> gen id after bios read the guid from the guid file but before it wrote >>> out the address. >>> >>> Or do I miss something? >>> >> I don’t think it’s an issue because BIOS is not a consumer of the >> GUID. I suppose there’s a window where another GUID could be written >> via monitor prior to bios writing back, but the change wouldn’t be >> applied to memory in vmgenid_update_guest() because vgia=0. That >> would be solved by a write callback and we could remove the GUID >> copying from vmgenid_build_acpi(). It’s a very unlikely scenario, >> though. > > When people run 100000 VMs and up, every unlikely scenario tends to > trigger. > Right, your point is valid and I don’t want to ever have to debug that type of race. It turns out that the callback in fw_cfg is tied to select(), not read(), so it is called upon write(). There doesn’t seem to be a need to add a new callback after all. I’ll go down this path to remove this potential race. >>> -- >>> MST
On Sun, 5 Feb 2017 01:12:00 -0800 ben@skyportsystems.com wrote: > From: Ben Warren <ben@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 <ben@skyportsystems.com> > --- > default-configs/i386-softmmu.mak | 1 + > default-configs/x86_64-softmmu.mak | 1 + > hw/acpi/Makefile.objs | 1 + > hw/acpi/vmgenid.c | 206 +++++++++++++++++++++++++++++++++++ > hw/i386/acpi-build.c | 10 ++ > include/hw/acpi/acpi_dev_interface.h | 1 + > include/hw/acpi/vmgenid.h | 37 +++++++ > 7 files changed, 257 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 384cefb..1a43542 100644 > --- a/default-configs/i386-softmmu.mak > +++ b/default-configs/i386-softmmu.mak > @@ -58,3 +58,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 491a191..aee8b08 100644 > --- a/default-configs/x86_64-softmmu.mak > +++ b/default-configs/x86_64-softmmu.mak > @@ -58,3 +58,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..6c9ecfd > --- /dev/null > +++ b/hw/acpi/vmgenid.c > @@ -0,0 +1,206 @@ > +/* > + * Virtual Machine Generation ID Device > + * > + * Copyright (C) 2017 Skyport Systems. > + * > + * Author: Ben Warren <ben@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(GArray *table_data, GArray *guid, BIOSLinker *linker) > +{ > + Object *obj; > + VmGenIdState *s; > + Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx; > + uint32_t vgia_offset; > + > + obj = find_vmgenid_dev(NULL); get obj from caller > + assert(obj); > + s = VMGENID(obj); > + > + /* Fill in the GUID values */ > + if (guid->len != VMGENID_FW_CFG_SIZE) { > + g_array_set_size(guid, VMGENID_FW_CFG_SIZE); does it have to be conditional? > + } > + g_array_insert_vals(guid, VMGENID_GUID_OFFSET, s->guid.data, 16); > + > + /* 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)))); I'd rather have "VGIA" patched wit address to GUID and not the blob start, see next comment about how. Also usage aml_index() looks a bit confusing, how about: pkg = aml_local(0) aml_store(aml_package(2), pkg) aml_append(pkg, aml_name("VGIA")) aml_append(pkg, aml_int(0)) > + 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 */); does it guaranties that blob will be allocated in cacheable page? (SeaBIOS/OVMF) > + > + /* Patch address of GUID fw_cfg blob into the ADDR fw_cfg blob */ > + bios_linker_loader_add_pointer(linker, > + VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint32_t), > + VMGENID_GUID_FW_CFG_FILE, 0, true); > + > + /* Patch address of GUID fw_cfg blob into the AML */ > + bios_linker_loader_add_pointer(linker, > + ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t), > + VMGENID_GUID_FW_CFG_FILE, 0, false); see @src_offset argument description, putting VMGENID_GUID_OFFSET would make patched place point directly to GUID > + > + 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(FWCfgState *s, GArray *guid) > +{ > + Object *obj = find_vmgenid_dev(NULL); > + assert(obj); > + VmGenIdState *vms = VMGENID(obj); > + > + /* 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->vgia_le, sizeof(uint32_t), false); > +} > + > +static void vmgenid_update_guest(VmGenIdState *s) > +{ > + Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL); > + uint32_t vgia; > + > + if (obj) { > + /* Write the GUID to guest memory */ > + memcpy(&vgia, s->vgia_le, sizeof(vgia)); > + vgia = le32_to_cpu(vgia); > + if (vgia) { > + cpu_physical_memory_write(vgia + VMGENID_GUID_OFFSET, > + s->guid.data, sizeof(s->guid.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 *s = VMGENID(obj); > + > + if (!strncmp(value, "auto", 4)) { > + qemu_uuid_generate(&s->guid); > + } else if (qemu_uuid_parse(value, &s->guid) < 0) { > + error_setg(errp, "'%s. %s': Failed to parse GUID string: %s", > + object_get_typename(OBJECT(s)), VMGENID_GUID, value); > + return; > + } > + /* 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, so store that way internally. Make > + * sure to swap back whenever reporting via monitor */ > + qemu_uuid_bswap(&s->guid); > + > + /* Send the ACPI notify */ > + vmgenid_update_guest(s); > +} > + > +/* 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 *s = opaque; > + vmgenid_update_guest(s); > + 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(vgia_le, VmGenIdState, sizeof(uint32_t)), file with address could be memory region and migrated automatically, ex: rsdp_mr + acpi_add_rom_blob + acpi_ram_update > + VMSTATE_END_OF_LIST() > + }, > +}; > + > +static void vmgenid_initfn(Object *obj) > +{ > + object_property_add_str(obj, VMGENID_GUID, NULL, vmgenid_set_guid, NULL); > +} > + > +static void vmgenid_device_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->vmsd = &vmstate_vmgenid; > +} > + > +static const TypeInfo vmgenid_device_info = { > + .name = VMGENID_DEVICE, > + .parent = TYPE_SYS_BUS_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 78a1d84..4c40f76 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" > @@ -2653,6 +2654,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) > acpi_add_table(table_offsets, tables_blob); > build_madt(tables_blob, tables->linker, pcms); > > + if (find_vmgenid_dev(NULL)) { > + acpi_add_table(table_offsets, tables_blob); > + vmgenid_build_acpi(tables_blob, tables->vmgenid, tables->linker); pass find_vmgenid_dev(NULL) result as an argument to vmgenid_build_acpi() so it won't have to do lookup again. > + } > + > if (misc.has_hpet) { > acpi_add_table(table_offsets, tables_blob); > build_hpet(tables_blob, tables->linker); > @@ -2859,6 +2865,10 @@ void acpi_setup(void) > fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, > tables.tcpalog->data, acpi_data_len(tables.tcpalog)); > > + if (find_vmgenid_dev(NULL)) { it's second lookup, cache result of the 1st call and reuse it here > + vmgenid_add_fw_cfg(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..b60437a > --- /dev/null > +++ b/include/hw/acpi/vmgenid.h > @@ -0,0 +1,37 @@ > +#ifndef ACPI_VMGENID_H > +#define ACPI_VMGENID_H > + > +#include "hw/acpi/bios-linker-loader.h" > +#include "hw/sysbus.h" > +#include "qemu/uuid.h" > + > +#define VMGENID_DEVICE "vmgenid" > +#define VMGENID_GUID "guid" > +#define VMGENID_GUID_FW_CFG_FILE "etc/vmgenid" > +#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 */ > + > +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid); > +void vmgenid_build_acpi(GArray *table_data, GArray *guid, BIOSLinker *linker); > + > +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE) > + > +typedef struct VmGenIdState { > + SysBusDevice parent_obj; Could it work with just plain Device parent? If it works then you can drop patch: [PATCH v5 08/10] PC: Support dynamic sysbus on pc_i440fx > + QemuUUID guid; > + uint8_t vgia_le[4]; > +} VmGenIdState; > + > +static Object *find_vmgenid_dev(Error **errp) > +{ > + Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL); > + if (!obj && errp) { all callers pass NULL as errp, I'd just drop errp altogether. > + error_setg(errp, "%s is not found", VMGENID_DEVICE); > + } > + return obj; > +} > + > +#endif
On Mon, 6 Feb 2017 19:41:36 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, Feb 06, 2017 at 09:29:30AM -0800, Ben Warren wrote: > > > > On Feb 6, 2017, at 8:15 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Sun, Feb 05, 2017 at 01:12:00AM -0800, ben@skyportsystems.com wrote: > > > > From: Ben Warren <ben@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 <ben@skyportsystems.com> > > [...] > > > > This variable name was suggested by Laszlo. the ‘a’ in ‘vgia’ refers to > > address, but I’ll add some comments. > > vmgenid_addr_le? > > > > How about we make it 8 byte so it's future proof? > > > > I can do that, but a previous conversation we had made it clear that guests > > would never allocate above 4GB so 64 bits wasn’t necessary. > > Right, it's just very painful to change once we make it 32 bit. I'd keep it 32 bit since it's in variable in global AML context and our table revision is 1, so per spec OSPM should handle it as 32 number. Chances of guest survival on boot would depend on AML interpreter tolerance to AML errors, which for windows usually is 0 and leads to non obvious BSOD. If we leave DWORD, even XP will be able to boot fine and only report unknown device.
On Tue, Feb 07, 2017 at 03:00:32PM +0100, Igor Mammedov wrote: > On Mon, 6 Feb 2017 19:41:36 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Mon, Feb 06, 2017 at 09:29:30AM -0800, Ben Warren wrote: > > > > > > On Feb 6, 2017, at 8:15 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Sun, Feb 05, 2017 at 01:12:00AM -0800, ben@skyportsystems.com wrote: > > > > > > From: Ben Warren <ben@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 <ben@skyportsystems.com> > > > > [...] > > > > > > > This variable name was suggested by Laszlo. the ‘a’ in ‘vgia’ refers to > > > address, but I’ll add some comments. > > > > vmgenid_addr_le? > > > > > > > How about we make it 8 byte so it's future proof? > > > > > > I can do that, but a previous conversation we had made it clear that guests > > > would never allocate above 4GB so 64 bits wasn’t necessary. > > > > Right, it's just very painful to change once we make it 32 bit. > I'd keep it 32 bit since it's in variable in global AML context and our > table revision is 1, so per spec OSPM should handle it as 32 number. > Chances of guest survival on boot would depend on AML interpreter tolerance > to AML errors, which for windows usually is 0 and leads to non obvious BSOD. > If we leave DWORD, even XP will be able to boot fine and only report > unknown device. AML reports 2 DWORDS per spec, no issue there. I guess it's exactly for XP compatibility.
On Tue, Feb 07, 2017 at 02:48:22PM +0100, Igor Mammedov wrote: > On Sun, 5 Feb 2017 01:12:00 -0800 > ben@skyportsystems.com wrote: > > > From: Ben Warren <ben@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 <ben@skyportsystems.com> > > --- > > default-configs/i386-softmmu.mak | 1 + > > default-configs/x86_64-softmmu.mak | 1 + > > hw/acpi/Makefile.objs | 1 + > > hw/acpi/vmgenid.c | 206 +++++++++++++++++++++++++++++++++++ > > hw/i386/acpi-build.c | 10 ++ > > include/hw/acpi/acpi_dev_interface.h | 1 + > > include/hw/acpi/vmgenid.h | 37 +++++++ > > 7 files changed, 257 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 384cefb..1a43542 100644 > > --- a/default-configs/i386-softmmu.mak > > +++ b/default-configs/i386-softmmu.mak > > @@ -58,3 +58,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 491a191..aee8b08 100644 > > --- a/default-configs/x86_64-softmmu.mak > > +++ b/default-configs/x86_64-softmmu.mak > > @@ -58,3 +58,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..6c9ecfd > > --- /dev/null > > +++ b/hw/acpi/vmgenid.c > > @@ -0,0 +1,206 @@ > > +/* > > + * Virtual Machine Generation ID Device > > + * > > + * Copyright (C) 2017 Skyport Systems. > > + * > > + * Author: Ben Warren <ben@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(GArray *table_data, GArray *guid, BIOSLinker *linker) > > +{ > > + Object *obj; > > + VmGenIdState *s; > > + Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx; > > + uint32_t vgia_offset; > > + > > + obj = find_vmgenid_dev(NULL); > get obj from caller > > > + assert(obj); > > + s = VMGENID(obj); > > + > > + /* Fill in the GUID values */ > > + if (guid->len != VMGENID_FW_CFG_SIZE) { > > + g_array_set_size(guid, VMGENID_FW_CFG_SIZE); > does it have to be conditional? > > > + } > > + g_array_insert_vals(guid, VMGENID_GUID_OFFSET, s->guid.data, 16); > > + > > + /* 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)))); > I'd rather have "VGIA" patched wit address to GUID and not the blob start, > see next comment about how. > > Also usage aml_index() looks a bit confusing, how about: > > pkg = aml_local(0) > aml_store(aml_package(2), pkg) > aml_append(pkg, aml_name("VGIA")) > aml_append(pkg, aml_int(0)) > > > + 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 */); > does it guaranties that blob will be allocated in cacheable page? > (SeaBIOS/OVMF) Reserving it from guest. Pretty much, yes. > > + > > + /* Patch address of GUID fw_cfg blob into the ADDR fw_cfg blob */ > > + bios_linker_loader_add_pointer(linker, > > + VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint32_t), > > + VMGENID_GUID_FW_CFG_FILE, 0, true); > > + > > + /* Patch address of GUID fw_cfg blob into the AML */ > > + bios_linker_loader_add_pointer(linker, > > + ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t), > > + VMGENID_GUID_FW_CFG_FILE, 0, false); > see @src_offset argument description, > putting VMGENID_GUID_OFFSET would make patched place point directly to GUID > > > + > > + 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(FWCfgState *s, GArray *guid) > > +{ > > + Object *obj = find_vmgenid_dev(NULL); > > + assert(obj); > > + VmGenIdState *vms = VMGENID(obj); > > + > > + /* 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->vgia_le, sizeof(uint32_t), false); > > +} > > + > > +static void vmgenid_update_guest(VmGenIdState *s) > > +{ > > + Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL); > > + uint32_t vgia; > > + > > + if (obj) { > > + /* Write the GUID to guest memory */ > > + memcpy(&vgia, s->vgia_le, sizeof(vgia)); > > + vgia = le32_to_cpu(vgia); > > + if (vgia) { > > + cpu_physical_memory_write(vgia + VMGENID_GUID_OFFSET, > > + s->guid.data, sizeof(s->guid.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 *s = VMGENID(obj); > > + > > + if (!strncmp(value, "auto", 4)) { > > + qemu_uuid_generate(&s->guid); > > + } else if (qemu_uuid_parse(value, &s->guid) < 0) { > > + error_setg(errp, "'%s. %s': Failed to parse GUID string: %s", > > + object_get_typename(OBJECT(s)), VMGENID_GUID, value); > > + return; > > + } > > + /* 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, so store that way internally. Make > > + * sure to swap back whenever reporting via monitor */ > > + qemu_uuid_bswap(&s->guid); > > + > > + /* Send the ACPI notify */ > > + vmgenid_update_guest(s); > > +} > > + > > +/* 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 *s = opaque; > > + vmgenid_update_guest(s); > > + 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(vgia_le, VmGenIdState, sizeof(uint32_t)), > file with address could be memory region and migrated automatically, > ex: rsdp_mr + acpi_add_rom_blob + acpi_ram_update > > > + VMSTATE_END_OF_LIST() > > + }, > > +}; > > + > > +static void vmgenid_initfn(Object *obj) > > +{ > > + object_property_add_str(obj, VMGENID_GUID, NULL, vmgenid_set_guid, NULL); > > +} > > + > > +static void vmgenid_device_class_init(ObjectClass *klass, void *data) > > +{ > > + DeviceClass *dc = DEVICE_CLASS(klass); > > + > > + dc->vmsd = &vmstate_vmgenid; > > +} > > + > > +static const TypeInfo vmgenid_device_info = { > > + .name = VMGENID_DEVICE, > > + .parent = TYPE_SYS_BUS_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 78a1d84..4c40f76 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" > > @@ -2653,6 +2654,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) > > acpi_add_table(table_offsets, tables_blob); > > build_madt(tables_blob, tables->linker, pcms); > > > > + if (find_vmgenid_dev(NULL)) { > > + acpi_add_table(table_offsets, tables_blob); > > + vmgenid_build_acpi(tables_blob, tables->vmgenid, tables->linker); > pass find_vmgenid_dev(NULL) result as an argument to vmgenid_build_acpi() > so it won't have to do lookup again. > > > + } > > + > > if (misc.has_hpet) { > > acpi_add_table(table_offsets, tables_blob); > > build_hpet(tables_blob, tables->linker); > > @@ -2859,6 +2865,10 @@ void acpi_setup(void) > > fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, > > tables.tcpalog->data, acpi_data_len(tables.tcpalog)); > > > > + if (find_vmgenid_dev(NULL)) { > it's second lookup, cache result of the 1st call and reuse it here > > > + vmgenid_add_fw_cfg(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..b60437a > > --- /dev/null > > +++ b/include/hw/acpi/vmgenid.h > > @@ -0,0 +1,37 @@ > > +#ifndef ACPI_VMGENID_H > > +#define ACPI_VMGENID_H > > + > > +#include "hw/acpi/bios-linker-loader.h" > > +#include "hw/sysbus.h" > > +#include "qemu/uuid.h" > > + > > +#define VMGENID_DEVICE "vmgenid" > > +#define VMGENID_GUID "guid" > > +#define VMGENID_GUID_FW_CFG_FILE "etc/vmgenid" > > +#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 */ > > + > > +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid); > > +void vmgenid_build_acpi(GArray *table_data, GArray *guid, BIOSLinker *linker); > > + > > +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE) > > + > > +typedef struct VmGenIdState { > > + SysBusDevice parent_obj; > Could it work with just plain Device parent? > If it works then you can drop patch: > [PATCH v5 08/10] PC: Support dynamic sysbus on pc_i440fx > > > + QemuUUID guid; > > + uint8_t vgia_le[4]; > > +} VmGenIdState; > > + > > +static Object *find_vmgenid_dev(Error **errp) > > +{ > > + Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL); > > + if (!obj && errp) { > all callers pass NULL as errp, I'd just drop errp altogether. > > > + error_setg(errp, "%s is not found", VMGENID_DEVICE); > > + } > > + return obj; > > +} > > + > > +#endif
On Tue, 7 Feb 2017 17:35:19 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Feb 07, 2017 at 03:00:32PM +0100, Igor Mammedov wrote: > > On Mon, 6 Feb 2017 19:41:36 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Mon, Feb 06, 2017 at 09:29:30AM -0800, Ben Warren wrote: > > > > > > > > On Feb 6, 2017, at 8:15 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Sun, Feb 05, 2017 at 01:12:00AM -0800, ben@skyportsystems.com wrote: > > > > > > > > From: Ben Warren <ben@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 <ben@skyportsystems.com> > > > > > > [...] > > > > > > > > > > This variable name was suggested by Laszlo. the ‘a’ in ‘vgia’ refers to > > > > address, but I’ll add some comments. > > > > > > vmgenid_addr_le? > > > > > > > > > > How about we make it 8 byte so it's future proof? > > > > > > > > I can do that, but a previous conversation we had made it clear that guests > > > > would never allocate above 4GB so 64 bits wasn’t necessary. > > > > > > Right, it's just very painful to change once we make it 32 bit. > > I'd keep it 32 bit since it's in variable in global AML context and our > > table revision is 1, so per spec OSPM should handle it as 32 number. > > Chances of guest survival on boot would depend on AML interpreter tolerance > > to AML errors, which for windows usually is 0 and leads to non obvious BSOD. > > If we leave DWORD, even XP will be able to boot fine and only report > > unknown device. > > AML reports 2 DWORDS per spec, no issue there. I guess it's exactly for > XP compatibility. it's only ADDR method thought but we are talking about Named DWORD vs QWORD VGIA variable which is patched by linker command
On Tue, Feb 07, 2017 at 05:04:56PM +0100, Igor Mammedov wrote: > On Tue, 7 Feb 2017 17:35:19 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Tue, Feb 07, 2017 at 03:00:32PM +0100, Igor Mammedov wrote: > > > On Mon, 6 Feb 2017 19:41:36 +0200 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Mon, Feb 06, 2017 at 09:29:30AM -0800, Ben Warren wrote: > > > > > > > > > > On Feb 6, 2017, at 8:15 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > On Sun, Feb 05, 2017 at 01:12:00AM -0800, ben@skyportsystems.com wrote: > > > > > > > > > > From: Ben Warren <ben@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 <ben@skyportsystems.com> > > > > > > > > [...] > > > > > > > > > > > > > This variable name was suggested by Laszlo. the ‘a’ in ‘vgia’ refers to > > > > > address, but I’ll add some comments. > > > > > > > > vmgenid_addr_le? > > > > > > > > > > > > > How about we make it 8 byte so it's future proof? > > > > > > > > > > I can do that, but a previous conversation we had made it clear that guests > > > > > would never allocate above 4GB so 64 bits wasn’t necessary. > > > > > > > > Right, it's just very painful to change once we make it 32 bit. > > > I'd keep it 32 bit since it's in variable in global AML context and our > > > table revision is 1, so per spec OSPM should handle it as 32 number. > > > Chances of guest survival on boot would depend on AML interpreter tolerance > > > to AML errors, which for windows usually is 0 and leads to non obvious BSOD. > > > If we leave DWORD, even XP will be able to boot fine and only report > > > unknown device. > > > > AML reports 2 DWORDS per spec, no issue there. I guess it's exactly for > > XP compatibility. > > it's only ADDR method thought but we are talking about > Named DWORD vs QWORD VGIA variable which is patched by linker command Oh sorry. Yes, you want to keep that one a DWORD. Does not affect the interface. So write 64 bits into fw cfg, but patch 32 bits into AML.
On 02/05/17 10:12, ben@skyportsystems.com wrote: > From: Ben Warren <ben@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 <ben@skyportsystems.com> > --- > default-configs/i386-softmmu.mak | 1 + > default-configs/x86_64-softmmu.mak | 1 + > hw/acpi/Makefile.objs | 1 + > hw/acpi/vmgenid.c | 206 +++++++++++++++++++++++++++++++++++ > hw/i386/acpi-build.c | 10 ++ > include/hw/acpi/acpi_dev_interface.h | 1 + > include/hw/acpi/vmgenid.h | 37 +++++++ > 7 files changed, 257 insertions(+) > create mode 100644 hw/acpi/vmgenid.c > create mode 100644 include/hw/acpi/vmgenid.h [snip code] So, I'm late to the game. I can't possibly comment on all the concerns that have been raised scattered around the thread, exactly *where* they have been raised. However, I will try to collect the concerns here. (1) Byte swapping for the UUID. The idea of QemuUUID is that wherever it is stored in a non-ephemeral fashion, it is in BE representation. The guest needs it in LE, so we should convert it temporarily -- using a local variable -- just before writing it to guest memory, and then forget about the LE representation promptly. As far as I understand, we all agree on this (Michael, Ben, myself -- I think Igor hasn't commented on this). (2) Structure of the vmgenid fw_cfg blob, AKA "why that darn offset 40 decimal". I explained it under points (6) and (7) in the following message: Message-Id: <c16a03d5-820a-f719-81ea-43858f903395@redhat.com> URL: https://www.mail-archive.com/qemu-devel@nongnu.org/msg425226.html The story is that *wherever* an ADD_POINTER command points to -- that is, at the *exact* target address --, OVMF will look for an ACPI table header, somewhat heuristically. If it finds a byte pattern that (a) fits into the remaining blob and (b) passes some superficial ACPI table header checks, such as length and checksum, then OVMF assumes that the blob contains an ACPI table there, and passes the address (again, the exact, relocated, absolute target address of ADD_POINTER) to EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(). We want to disable this heuristic for the vmgenid blob. *If* the blob contained only 16 bytes (for the GUID), then the heuristic would automatically disable itself, because the ACPI table header (36 bytes) is larger than 16 bytes, so OVMF wouldn't even look. However, for the caching and other VMGENID requirements, we need to allocate a full page with the blob (4KB), hence OVMF *will* look. If we placed the GUID right at the start of the page, then OVMF would sanity-check it as an ACPI table header. The check would *most likely* not pass, so things would be fine in practice, but we can do better than that: just put 40 zero bytes at the front of the blob. And this is why the ADD_POINTER command has to point to the beginning of the blob (i.e., 36 zero bytes), to "suppress the OVMF ACPI SDT header detection". (The other 4 bytes are for arriving at an address divisible by 8, which is a VMGENID requirement for the GUID.) The consequence is that both the ADDR method and QEMU's guest memory access code have to add 40 manually. (3) Whether the VGIA named object should be a DWORD or a QWORD in ACPI. Personally, I'm fine with either, but I see that DWORD is more compatible with old OSes. What I really care about though is the new WRITE_POINTER command structure. That should support 8 bytes as well. So this is exactly how Michael summarized it ultimately: - use a DWORD for VGIA in ACPI, - with a matching 4-byte wide ADD_POINTER command; - write the address with a 4-byte wide WRITE_POINTER command into fw_cfg, - *but* the WRITE_POINTER command struct should support 8-byte as well, - and the fw_cfg file that is written (vmgenid_addr) should have size 8 from the start. (The guest will simply write offsets 0..3 inclusive, in LE byte order.) (4) IIRC Igor asked if sizing the blob to 4KB would guarantee that the GUID ends up in a cacheable page -- yes (see also Michael's followup). This size guarantees that the GUID will have a full page to itself, and its allocated from normal guest RAM, as Reserved memory (SeaBIOS, IIRC) or EfiACPIMemoryNVS memory (OVMF). Note that the VMGENID spec says, in ACPI terms, "It must not be in ranges reported as AddressRangeMemory or AddressRangeACPI", but that's fine: ACPI speak UEFI speak suitable for VMGENID? ------------------ --------------------- --------------------- AddressRangeMemory EfiConventionalMemory no AddressRangeACPI EfiACPIReclaimMemory no AddressRangeNVS EfiACPIMemoryNVS yes, used by OVMF AddressRangeReserved EfiReservedMemoryType yes, used by SeaBIOS (5) The race and fw_cfg callbacks. (a) Michael and Ben identified a problem where - QEMU places the initial GUID in the "vmgenid" fw_cfg blob, - the guest downloads it, - the host admin changes the GUID via the monitor, - and the guest writes the address to "vmgenid_addr" *only* after that. In other words, the monitor action occurs between the guest's processing of the ALLOCATE and WRITE_POINTER linker/loader commands. (b) A similar problem is rebooting the guest. - The guest starts up fine, - the OS runs, - the host admin changes the GUID via the monitor, - and the guest sees the update fine. - At this point the guest is rebooted (within the same QEMU instance). Since the fw_cfg blob is not rebuilt -- more precisely, the blob *is* rebuilt, but it is then thrown away in acpi_build_update() --, the guest will see a different GUID from the last value set on the monitor. (And querying the monitor will actually return that last-set value, which is no longer known by the guest.) The suggestion for these problems was to add a callback to "one" of the fw_cfg files in question. Let's investigate the writeable "vmgenid_addr" file first. Here the idea is to rewrite the GUID in guest RAM as soon as the address is known from the guest, regardless of what GUID the guest placed there originally, through downloading the "vmgenid" blob. (i) We have no write callbacks at the moment. Before we removed the data port based write support in QEMU 2.4, the write callback used to be invoked when the end of the fw_cfg blob was reached. Since we intend to pack several addresses in the same writeable fw_cfg blob down the road, at different offsets, this method wouldn't work; the final offset in the blob might not be reached at all. (Another thing that wouldn't work is an 8-byte writeable fw_cfg blob, and a 4-byte WRITE_POINTER command that only rewrites offsets 0..3 inclusve -- see (3) above.) So I don't think that a write callback is viable, at least with the "many addresses at different offsets in the same blob" feature. Unless we want to register different callbacks (or different callback arguments) for different offsets. And then a large write could trigger a series of callbacks, even, if it covers several special offsets. This looks too complex to me. (ii) What we do have now is a select callback. Unfortunately, the guest is not required to select the item and atomically rewrite the blob, in the same fw_cfg DMA operation. The guest is allowed to do the selection with the IO port method, and then write the blob separately with DMA. (This is what OVMF does.) IOW, when we'd get the callback (for the select), the address would not have been written yet. So I don't think that a select callback for the writeable "vmgenid_addr" file is viable either. (iii) Let's see a select callback for the "vmgenid" blob itself! The idea is, whenever the guest selects this blob, immediately refresh the blob from the last set GUID. Then let the guest download the just-refreshed blob. The main observation here is that we can *fail* QMP commands. That's good enough if we can tell *when* to fail them. Under this idea, we would fail the "set-vm-generation-id" QMP command with "EAGAIN" if the quest had selected the "vmgenid" blob since machine reset -- this can be tracked with a latch -- *but* the particular offset range in the "vmgenid_addr" blob were still zero. This is exactly the window between the ALLOCATE and WRITE_POINTER commands where updates from the monitor would be lost. So let's just tell the management layer to try again later (like 1 second later). By that time, any sane guest will have written the address into "vmgenid_addr". Thanks Laszlo
On Wed, 8 Feb 2017 01:48:42 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > On 02/05/17 10:12, ben@skyportsystems.com wrote: > > From: Ben Warren <ben@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 <ben@skyportsystems.com> > > --- > > default-configs/i386-softmmu.mak | 1 + > > default-configs/x86_64-softmmu.mak | 1 + > > hw/acpi/Makefile.objs | 1 + > > hw/acpi/vmgenid.c | 206 +++++++++++++++++++++++++++++++++++ > > hw/i386/acpi-build.c | 10 ++ > > include/hw/acpi/acpi_dev_interface.h | 1 + > > include/hw/acpi/vmgenid.h | 37 +++++++ > > 7 files changed, 257 insertions(+) > > create mode 100644 hw/acpi/vmgenid.c > > create mode 100644 include/hw/acpi/vmgenid.h > > [snip code] > > So, I'm late to the game. I can't possibly comment on all the concerns > that have been raised scattered around the thread, exactly *where* they > have been raised. However, I will try to collect the concerns here. > > > (1) Byte swapping for the UUID. > > The idea of QemuUUID is that wherever it is stored in a non-ephemeral > fashion, it is in BE representation. The guest needs it in LE, so we > should convert it temporarily -- using a local variable -- just before > writing it to guest memory, and then forget about the LE representation > promptly. > > As far as I understand, we all agree on this (Michael, Ben, myself -- I > think Igor hasn't commented on this). > > > (2) Structure of the vmgenid fw_cfg blob, AKA "why that darn offset 40 > decimal". > > I explained it under points (6) and (7) in the following message: > > Message-Id: <c16a03d5-820a-f719-81ea-43858f903395@redhat.com> > URL: https://www.mail-archive.com/qemu-devel@nongnu.org/msg425226.html > > The story is that *wherever* an ADD_POINTER command points to -- that > is, at the *exact* target address --, OVMF will look for an ACPI table > header, somewhat heuristically. If it finds a byte pattern that (a) fits > into the remaining blob and (b) passes some superficial ACPI table > header checks, such as length and checksum, then OVMF assumes that the > blob contains an ACPI table there, and passes the address (again, the > exact, relocated, absolute target address of ADD_POINTER) to > EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(). > > We want to disable this heuristic for the vmgenid blob. *If* the blob > contained only 16 bytes (for the GUID), then the heuristic would > automatically disable itself, because the ACPI table header (36 bytes) > is larger than 16 bytes, so OVMF wouldn't even look. However, for the > caching and other VMGENID requirements, we need to allocate a full page > with the blob (4KB), hence OVMF *will* look. If we placed the GUID right > at the start of the page, then OVMF would sanity-check it as an ACPI > table header. The check would *most likely* not pass, so things would be > fine in practice, but we can do better than that: just put 40 zero bytes > at the front of the blob. > > And this is why the ADD_POINTER command has to point to the beginning of > the blob (i.e., 36 zero bytes), to "suppress the OVMF ACPI SDT header > detection". (The other 4 bytes are for arriving at an address divisible > by 8, which is a VMGENID requirement for the GUID.) that's deserves a comment in code/doc as it's non obvious to someone who is not familiar with OVMF. > The consequence is that both the ADDR method and QEMU's guest memory > access code have to add 40 manually. > > > (3) Whether the VGIA named object should be a DWORD or a QWORD in ACPI. > Personally, I'm fine with either, but I see that DWORD is more > compatible with old OSes. It's not only old OSes, but newer ones as well (32 bit and to some degree 64-bit). We should not put QWORD objects in global scope until we switch DSDT rev to 2.0 or higher AND ready to drop support for ACPI 1.0 guests. DSDT rev is important for 64-bit guests as well as OSPM chooses supported integer length based on it. > > What I really care about though is the new WRITE_POINTER command > structure. That should support 8 bytes as well. > > So this is exactly how Michael summarized it ultimately: > - use a DWORD for VGIA in ACPI, > - with a matching 4-byte wide ADD_POINTER command; > - write the address with a 4-byte wide WRITE_POINTER command into > fw_cfg, > - *but* the WRITE_POINTER command struct should support 8-byte as well, > - and the fw_cfg file that is written (vmgenid_addr) should have size 8 > from the start. (The guest will simply write offsets 0..3 inclusive, > in LE byte order.) ACK above summary. > (4) IIRC Igor asked if sizing the blob to 4KB would guarantee that the > GUID ends up in a cacheable page -- yes (see also Michael's followup). > This size guarantees that the GUID will have a full page to itself, and > its allocated from normal guest RAM, as Reserved memory (SeaBIOS, IIRC) > or EfiACPIMemoryNVS memory (OVMF). > > Note that the VMGENID spec says, in ACPI terms, "It must not be in > ranges reported as AddressRangeMemory or AddressRangeACPI", but that's fine: > > ACPI speak UEFI speak suitable for VMGENID? > ------------------ --------------------- --------------------- > AddressRangeMemory EfiConventionalMemory no > AddressRangeACPI EfiACPIReclaimMemory no > AddressRangeNVS EfiACPIMemoryNVS yes, used by OVMF > AddressRangeReserved EfiReservedMemoryType yes, used by SeaBIOS > > > (5) The race and fw_cfg callbacks. > > (a) Michael and Ben identified a problem where > - QEMU places the initial GUID in the "vmgenid" fw_cfg blob, > - the guest downloads it, > - the host admin changes the GUID via the monitor, > - and the guest writes the address to "vmgenid_addr" *only* after that. > > In other words, the monitor action occurs between the guest's processing > of the ALLOCATE and WRITE_POINTER linker/loader commands. > > (b) A similar problem is rebooting the guest. > - The guest starts up fine, > - the OS runs, > - the host admin changes the GUID via the monitor, > - and the guest sees the update fine. > - At this point the guest is rebooted (within the same QEMU instance). > > Since the fw_cfg blob is not rebuilt -- more precisely, the blob *is* > rebuilt, but it is then thrown away in acpi_build_update() --, the guest > will see a different GUID from the last value set on the monitor. (And > querying the monitor will actually return that last-set value, which is > no longer known by the guest.) > > > The suggestion for these problems was to add a callback to "one" of the > fw_cfg files in question. > > Let's investigate the writeable "vmgenid_addr" file first. Here the idea > is to rewrite the GUID in guest RAM as soon as the address is known from > the guest, regardless of what GUID the guest placed there originally, > through downloading the "vmgenid" blob. > > (i) We have no write callbacks at the moment. Before we removed the data > port based write support in QEMU 2.4, the write callback used to be > invoked when the end of the fw_cfg blob was reached. Since we intend to > pack several addresses in the same writeable fw_cfg blob down the road, > at different offsets, this method wouldn't work; the final offset in the > blob might not be reached at all. (Another thing that wouldn't work is > an 8-byte writeable fw_cfg blob, and a 4-byte WRITE_POINTER command that > only rewrites offsets 0..3 inclusve -- see (3) above.) > > So I don't think that a write callback is viable, at least with the > "many addresses at different offsets in the same blob" feature. Unless > we want to register different callbacks (or different callback > arguments) for different offsets. And then a large write could trigger a > series of callbacks, even, if it covers several special offsets. This > looks too complex to me. > > (ii) What we do have now is a select callback. Unfortunately, the guest > is not required to select the item and atomically rewrite the blob, in > the same fw_cfg DMA operation. The guest is allowed to do the selection > with the IO port method, and then write the blob separately with DMA. > (This is what OVMF does.) IOW, when we'd get the callback (for the > select), the address would not have been written yet. > > So I don't think that a select callback for the writeable "vmgenid_addr" > file is viable either. > > (iii) Let's see a select callback for the "vmgenid" blob itself! The > idea is, whenever the guest selects this blob, immediately refresh the > blob from the last set GUID. Then let the guest download the > just-refreshed blob. > > The main observation here is that we can *fail* QMP commands. That's > good enough if we can tell *when* to fail them. > > Under this idea, we would fail the "set-vm-generation-id" QMP command > with "EAGAIN" if the quest had selected the "vmgenid" blob since machine > reset -- this can be tracked with a latch -- *but* the particular offset > range in the "vmgenid_addr" blob were still zero. > > This is exactly the window between the ALLOCATE and WRITE_POINTER > commands where updates from the monitor would be lost. So let's just > tell the management layer to try again later (like 1 second later). By > that time, any sane guest will have written the address into "vmgenid_addr". > > Thanks > Laszlo
On 02/08/17 12:04, Igor Mammedov wrote: > On Wed, 8 Feb 2017 01:48:42 +0100 > Laszlo Ersek <lersek@redhat.com> wrote: [snip] >> (2) Structure of the vmgenid fw_cfg blob, AKA "why that darn offset 40 >> decimal". >> >> I explained it under points (6) and (7) in the following message: >> >> Message-Id: <c16a03d5-820a-f719-81ea-43858f903395@redhat.com> >> URL: https://www.mail-archive.com/qemu-devel@nongnu.org/msg425226.html >> >> The story is that *wherever* an ADD_POINTER command points to -- that >> is, at the *exact* target address --, OVMF will look for an ACPI table >> header, somewhat heuristically. If it finds a byte pattern that (a) fits >> into the remaining blob and (b) passes some superficial ACPI table >> header checks, such as length and checksum, then OVMF assumes that the >> blob contains an ACPI table there, and passes the address (again, the >> exact, relocated, absolute target address of ADD_POINTER) to >> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(). >> >> We want to disable this heuristic for the vmgenid blob. *If* the blob >> contained only 16 bytes (for the GUID), then the heuristic would >> automatically disable itself, because the ACPI table header (36 bytes) >> is larger than 16 bytes, so OVMF wouldn't even look. However, for the >> caching and other VMGENID requirements, we need to allocate a full page >> with the blob (4KB), hence OVMF *will* look. If we placed the GUID right >> at the start of the page, then OVMF would sanity-check it as an ACPI >> table header. The check would *most likely* not pass, so things would be >> fine in practice, but we can do better than that: just put 40 zero bytes >> at the front of the blob. >> >> And this is why the ADD_POINTER command has to point to the beginning of >> the blob (i.e., 36 zero bytes), to "suppress the OVMF ACPI SDT header >> detection". (The other 4 bytes are for arriving at an address divisible >> by 8, which is a VMGENID requirement for the GUID.) > that's deserves a comment in code/doc as it's non obvious to someone > who is not familiar with OVMF. It is mentioned in the document "docs/specs/vmgenid.txt", added in patch 3. I didn't want to push for more details there, but if you think it's helpful or even required, then Ben should please simply add a Rationale for padding at the front (OVMF SDT Header probe suppressor) --------------------------------------------------------------------- subsection under the GUID Storage Format ------------------- section, and just dump the above into it. Maybe tone down the over-use of *bold*. :) > > >> The consequence is that both the ADDR method and QEMU's guest memory >> access code have to add 40 manually. >> >> >> (3) Whether the VGIA named object should be a DWORD or a QWORD in ACPI. >> Personally, I'm fine with either, but I see that DWORD is more >> compatible with old OSes. > It's not only old OSes, but newer ones as well (32 bit and to some degree 64-bit). > We should not put QWORD objects in global scope until we switch > DSDT rev to 2.0 or higher AND ready to drop support for ACPI 1.0 guests. > DSDT rev is important for 64-bit guests as well as OSPM chooses > supported integer length based on it. Good point. DWORD is fine with me. [snipping the rest, nothing for me to add there, it seems] Thanks! Laszlo
On Wed, 8 Feb 2017 12:17:54 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > On 02/08/17 12:04, Igor Mammedov wrote: > > On Wed, 8 Feb 2017 01:48:42 +0100 > > Laszlo Ersek <lersek@redhat.com> wrote: > > [snip] > > >> (2) Structure of the vmgenid fw_cfg blob, AKA "why that darn offset 40 > >> decimal". > >> > >> I explained it under points (6) and (7) in the following message: > >> > >> Message-Id: <c16a03d5-820a-f719-81ea-43858f903395@redhat.com> > >> URL: https://www.mail-archive.com/qemu-devel@nongnu.org/msg425226.html > >> > >> The story is that *wherever* an ADD_POINTER command points to -- that > >> is, at the *exact* target address --, OVMF will look for an ACPI table > >> header, somewhat heuristically. If it finds a byte pattern that (a) fits > >> into the remaining blob and (b) passes some superficial ACPI table > >> header checks, such as length and checksum, then OVMF assumes that the > >> blob contains an ACPI table there, and passes the address (again, the > >> exact, relocated, absolute target address of ADD_POINTER) to > >> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(). > >> > >> We want to disable this heuristic for the vmgenid blob. *If* the blob > >> contained only 16 bytes (for the GUID), then the heuristic would > >> automatically disable itself, because the ACPI table header (36 bytes) > >> is larger than 16 bytes, so OVMF wouldn't even look. However, for the > >> caching and other VMGENID requirements, we need to allocate a full page > >> with the blob (4KB), hence OVMF *will* look. If we placed the GUID right > >> at the start of the page, then OVMF would sanity-check it as an ACPI > >> table header. The check would *most likely* not pass, so things would be > >> fine in practice, but we can do better than that: just put 40 zero bytes > >> at the front of the blob. > >> > >> And this is why the ADD_POINTER command has to point to the beginning of > >> the blob (i.e., 36 zero bytes), to "suppress the OVMF ACPI SDT header > >> detection". (The other 4 bytes are for arriving at an address divisible > >> by 8, which is a VMGENID requirement for the GUID.) > > that's deserves a comment in code/doc as it's non obvious to someone > > who is not familiar with OVMF. > > It is mentioned in the document "docs/specs/vmgenid.txt", added in patch 3. > > I didn't want to push for more details there, but if you think it's > helpful or even required, then Ben should please simply add a > > Rationale for padding at the front (OVMF SDT Header probe suppressor) > --------------------------------------------------------------------- > > subsection under the > > GUID Storage Format > ------------------- > > section, and just dump the above into it. > > Maybe tone down the over-use of *bold*. :) + comment/reference to doc in the code where this 'suppressor' is used, otherwise it would be easy to forget why pointer points to the blob start instead of directly to GUID. > > Thanks! > Laszlo > >
Thanks for reviewing Igor. > On Feb 7, 2017, at 5:48 AM, Igor Mammedov <imammedo@redhat.com> wrote: > > On Sun, 5 Feb 2017 01:12:00 -0800 > ben@skyportsystems.com <mailto:ben@skyportsystems.com> wrote: > >> From: Ben Warren <ben@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 <ben@skyportsystems.com> >> --- >> default-configs/i386-softmmu.mak | 1 + >> default-configs/x86_64-softmmu.mak | 1 + >> hw/acpi/Makefile.objs | 1 + >> hw/acpi/vmgenid.c | 206 +++++++++++++++++++++++++++++++++++ >> hw/i386/acpi-build.c | 10 ++ >> include/hw/acpi/acpi_dev_interface.h | 1 + >> include/hw/acpi/vmgenid.h | 37 +++++++ >> 7 files changed, 257 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 384cefb..1a43542 100644 >> --- a/default-configs/i386-softmmu.mak >> +++ b/default-configs/i386-softmmu.mak >> @@ -58,3 +58,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 491a191..aee8b08 100644 >> --- a/default-configs/x86_64-softmmu.mak >> +++ b/default-configs/x86_64-softmmu.mak >> @@ -58,3 +58,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..6c9ecfd >> --- /dev/null >> +++ b/hw/acpi/vmgenid.c >> @@ -0,0 +1,206 @@ >> +/* >> + * Virtual Machine Generation ID Device >> + * >> + * Copyright (C) 2017 Skyport Systems. >> + * >> + * Author: Ben Warren <ben@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(GArray *table_data, GArray *guid, BIOSLinker *linker) >> +{ >> + Object *obj; >> + VmGenIdState *s; >> + Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx; >> + uint32_t vgia_offset; >> + >> + obj = find_vmgenid_dev(NULL); > get obj from caller > >> + assert(obj); >> + s = VMGENID(obj); >> + >> + /* Fill in the GUID values */ >> + if (guid->len != VMGENID_FW_CFG_SIZE) { >> + g_array_set_size(guid, VMGENID_FW_CFG_SIZE); > does it have to be conditional? > No, I guess not. >> + } >> + g_array_insert_vals(guid, VMGENID_GUID_OFFSET, s->guid.data, 16); >> + >> + /* 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)))); > I'd rather have "VGIA" patched wit address to GUID and not the blob start, > see next comment about how. > > Also usage aml_index() looks a bit confusing, how about: > > pkg = aml_local(0) > aml_store(aml_package(2), pkg) > aml_append(pkg, aml_name("VGIA")) > aml_append(pkg, aml_int(0)) > I’ll try that, it does look simpler. I thought I tried this and couldn’t get it to work, but will try again. >> + 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 */); > does it guaranties that blob will be allocated in cacheable page? > (SeaBIOS/OVMF) > >> + >> + /* Patch address of GUID fw_cfg blob into the ADDR fw_cfg blob */ >> + bios_linker_loader_add_pointer(linker, >> + VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint32_t), >> + VMGENID_GUID_FW_CFG_FILE, 0, true); >> + >> + /* Patch address of GUID fw_cfg blob into the AML */ >> + bios_linker_loader_add_pointer(linker, >> + ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t), >> + VMGENID_GUID_FW_CFG_FILE, 0, false); > see @src_offset argument description, > putting VMGENID_GUID_OFFSET would make patched place point directly to GUID > >> + >> + 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(FWCfgState *s, GArray *guid) >> +{ >> + Object *obj = find_vmgenid_dev(NULL); >> + assert(obj); >> + VmGenIdState *vms = VMGENID(obj); >> + >> + /* 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->vgia_le, sizeof(uint32_t), false); >> +} >> + >> +static void vmgenid_update_guest(VmGenIdState *s) >> +{ >> + Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL); >> + uint32_t vgia; >> + >> + if (obj) { >> + /* Write the GUID to guest memory */ >> + memcpy(&vgia, s->vgia_le, sizeof(vgia)); >> + vgia = le32_to_cpu(vgia); >> + if (vgia) { >> + cpu_physical_memory_write(vgia + VMGENID_GUID_OFFSET, >> + s->guid.data, sizeof(s->guid.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 *s = VMGENID(obj); >> + >> + if (!strncmp(value, "auto", 4)) { >> + qemu_uuid_generate(&s->guid); >> + } else if (qemu_uuid_parse(value, &s->guid) < 0) { >> + error_setg(errp, "'%s. %s': Failed to parse GUID string: %s", >> + object_get_typename(OBJECT(s)), VMGENID_GUID, value); >> + return; >> + } >> + /* 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, so store that way internally. Make >> + * sure to swap back whenever reporting via monitor */ >> + qemu_uuid_bswap(&s->guid); >> + >> + /* Send the ACPI notify */ >> + vmgenid_update_guest(s); >> +} >> + >> +/* 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 *s = opaque; >> + vmgenid_update_guest(s); >> + 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(vgia_le, VmGenIdState, sizeof(uint32_t)), > file with address could be memory region and migrated automatically, > ex: rsdp_mr + acpi_add_rom_blob + acpi_ram_update > I don’t follow. Can you please elaborate? >> + VMSTATE_END_OF_LIST() >> + }, >> +}; >> + >> +static void vmgenid_initfn(Object *obj) >> +{ >> + object_property_add_str(obj, VMGENID_GUID, NULL, vmgenid_set_guid, NULL); >> +} >> + >> +static void vmgenid_device_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + dc->vmsd = &vmstate_vmgenid; >> +} >> + >> +static const TypeInfo vmgenid_device_info = { >> + .name = VMGENID_DEVICE, >> + .parent = TYPE_SYS_BUS_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 78a1d84..4c40f76 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" >> @@ -2653,6 +2654,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) >> acpi_add_table(table_offsets, tables_blob); >> build_madt(tables_blob, tables->linker, pcms); >> >> + if (find_vmgenid_dev(NULL)) { >> + acpi_add_table(table_offsets, tables_blob); >> + vmgenid_build_acpi(tables_blob, tables->vmgenid, tables->linker); > pass find_vmgenid_dev(NULL) result as an argument to vmgenid_build_acpi() > so it won't have to do lookup again. > OK >> + } >> + >> if (misc.has_hpet) { >> acpi_add_table(table_offsets, tables_blob); >> build_hpet(tables_blob, tables->linker); >> @@ -2859,6 +2865,10 @@ void acpi_setup(void) >> fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, >> tables.tcpalog->data, acpi_data_len(tables.tcpalog)); >> >> + if (find_vmgenid_dev(NULL)) { > it's second lookup, cache result of the 1st call and reuse it here > OK >> + vmgenid_add_fw_cfg(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..b60437a >> --- /dev/null >> +++ b/include/hw/acpi/vmgenid.h >> @@ -0,0 +1,37 @@ >> +#ifndef ACPI_VMGENID_H >> +#define ACPI_VMGENID_H >> + >> +#include "hw/acpi/bios-linker-loader.h" >> +#include "hw/sysbus.h" >> +#include "qemu/uuid.h" >> + >> +#define VMGENID_DEVICE "vmgenid" >> +#define VMGENID_GUID "guid" >> +#define VMGENID_GUID_FW_CFG_FILE "etc/vmgenid" >> +#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 */ >> + >> +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid); >> +void vmgenid_build_acpi(GArray *table_data, GArray *guid, BIOSLinker *linker); >> + >> +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE) >> + >> +typedef struct VmGenIdState { >> + SysBusDevice parent_obj; > Could it work with just plain Device parent? > If it works then you can drop patch: > [PATCH v5 08/10] PC: Support dynamic sysbus on pc_i440fx > Huh, I was sure I tried that originally and couldn’t make it work, but it turns out it does. Definitely better. >> + QemuUUID guid; >> + uint8_t vgia_le[4]; >> +} VmGenIdState; >> + >> +static Object *find_vmgenid_dev(Error **errp) >> +{ >> + Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL); >> + if (!obj && errp) { > all callers pass NULL as errp, I'd just drop errp altogether. > OK >> + error_setg(errp, "%s is not found", VMGENID_DEVICE); >> + } >> + return obj; >> +} >> + >> +#endif
> On Feb 7, 2017, at 4:48 PM, Laszlo Ersek <lersek@redhat.com> wrote: > > On 02/05/17 10:12, ben@skyportsystems.com <mailto:ben@skyportsystems.com> wrote: >> From: Ben Warren <ben@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 <ben@skyportsystems.com> >> --- >> default-configs/i386-softmmu.mak | 1 + >> default-configs/x86_64-softmmu.mak | 1 + >> hw/acpi/Makefile.objs | 1 + >> hw/acpi/vmgenid.c | 206 +++++++++++++++++++++++++++++++++++ >> hw/i386/acpi-build.c | 10 ++ >> include/hw/acpi/acpi_dev_interface.h | 1 + >> include/hw/acpi/vmgenid.h | 37 +++++++ >> 7 files changed, 257 insertions(+) >> create mode 100644 hw/acpi/vmgenid.c >> create mode 100644 include/hw/acpi/vmgenid.h > > [snip code] > > So, I'm late to the game. I can't possibly comment on all the concerns > that have been raised scattered around the thread, exactly *where* they > have been raised. However, I will try to collect the concerns here. > > > (1) Byte swapping for the UUID. > > The idea of QemuUUID is that wherever it is stored in a non-ephemeral > fashion, it is in BE representation. The guest needs it in LE, so we > should convert it temporarily -- using a local variable -- just before > writing it to guest memory, and then forget about the LE representation > promptly. > > As far as I understand, we all agree on this (Michael, Ben, myself -- I > think Igor hasn't commented on this). > > Sure, will rework. > (2) Structure of the vmgenid fw_cfg blob, AKA "why that darn offset 40 > decimal". > > I explained it under points (6) and (7) in the following message: > > Message-Id: <c16a03d5-820a-f719-81ea-43858f903395@redhat.com <mailto:c16a03d5-820a-f719-81ea-43858f903395@redhat.com>> > URL: https://www.mail-archive.com/qemu-devel@nongnu.org/msg425226.html <https://www.mail-archive.com/qemu-devel@nongnu.org/msg425226.html> > > The story is that *wherever* an ADD_POINTER command points to -- that > is, at the *exact* target address --, OVMF will look for an ACPI table > header, somewhat heuristically. If it finds a byte pattern that (a) fits > into the remaining blob and (b) passes some superficial ACPI table > header checks, such as length and checksum, then OVMF assumes that the > blob contains an ACPI table there, and passes the address (again, the > exact, relocated, absolute target address of ADD_POINTER) to > EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(). > > We want to disable this heuristic for the vmgenid blob. *If* the blob > contained only 16 bytes (for the GUID), then the heuristic would > automatically disable itself, because the ACPI table header (36 bytes) > is larger than 16 bytes, so OVMF wouldn't even look. However, for the > caching and other VMGENID requirements, we need to allocate a full page > with the blob (4KB), hence OVMF *will* look. If we placed the GUID right > at the start of the page, then OVMF would sanity-check it as an ACPI > table header. The check would *most likely* not pass, so things would be > fine in practice, but we can do better than that: just put 40 zero bytes > at the front of the blob. > > And this is why the ADD_POINTER command has to point to the beginning of > the blob (i.e., 36 zero bytes), to "suppress the OVMF ACPI SDT header > detection". (The other 4 bytes are for arriving at an address divisible > by 8, which is a VMGENID requirement for the GUID.) > > The consequence is that both the ADDR method and QEMU's guest memory > access code have to add 40 manually. > Igor has recommended that I have the add_pointer() call that patches AML have an offset of 40 from the start of the source file, which will result in the VGIA AML variable pointing to the GUID, not offset by 40. I assume this isn’t a problem for you, but please confirm. > > (3) Whether the VGIA named object should be a DWORD or a QWORD in ACPI. > Personally, I'm fine with either, but I see that DWORD is more > compatible with old OSes. > > What I really care about though is the new WRITE_POINTER command > structure. That should support 8 bytes as well. > > So this is exactly how Michael summarized it ultimately: > - use a DWORD for VGIA in ACPI, > - with a matching 4-byte wide ADD_POINTER command; > - write the address with a 4-byte wide WRITE_POINTER command into > fw_cfg, > - *but* the WRITE_POINTER command struct should support 8-byte as well, > - and the fw_cfg file that is written (vmgenid_addr) should have size 8 > from the start. (The guest will simply write offsets 0..3 inclusive, > in LE byte order.) > This is an easy change, WRITE_POINTER already supports 8-byte writes. > > (4) IIRC Igor asked if sizing the blob to 4KB would guarantee that the > GUID ends up in a cacheable page -- yes (see also Michael's followup). > This size guarantees that the GUID will have a full page to itself, and > its allocated from normal guest RAM, as Reserved memory (SeaBIOS, IIRC) > or EfiACPIMemoryNVS memory (OVMF). > > Note that the VMGENID spec says, in ACPI terms, "It must not be in > ranges reported as AddressRangeMemory or AddressRangeACPI", but that's fine: > > ACPI speak UEFI speak suitable for VMGENID? > ------------------ --------------------- --------------------- > AddressRangeMemory EfiConventionalMemory no > AddressRangeACPI EfiACPIReclaimMemory no > AddressRangeNVS EfiACPIMemoryNVS yes, used by OVMF > AddressRangeReserved EfiReservedMemoryType yes, used by SeaBIOS > > > (5) The race and fw_cfg callbacks. > > (a) Michael and Ben identified a problem where > - QEMU places the initial GUID in the "vmgenid" fw_cfg blob, > - the guest downloads it, > - the host admin changes the GUID via the monitor, > - and the guest writes the address to "vmgenid_addr" *only* after that. > > In other words, the monitor action occurs between the guest's processing > of the ALLOCATE and WRITE_POINTER linker/loader commands. > > (b) A similar problem is rebooting the guest. > - The guest starts up fine, > - the OS runs, > - the host admin changes the GUID via the monitor, > - and the guest sees the update fine. > - At this point the guest is rebooted (within the same QEMU instance). > > Since the fw_cfg blob is not rebuilt -- more precisely, the blob *is* > rebuilt, but it is then thrown away in acpi_build_update() --, the guest > will see a different GUID from the last value set on the monitor. (And > querying the monitor will actually return that last-set value, which is > no longer known by the guest.) > > > The suggestion for these problems was to add a callback to "one" of the > fw_cfg files in question. > > Let's investigate the writeable "vmgenid_addr" file first. Here the idea > is to rewrite the GUID in guest RAM as soon as the address is known from > the guest, regardless of what GUID the guest placed there originally, > through downloading the "vmgenid" blob. > > (i) We have no write callbacks at the moment. Before we removed the data > port based write support in QEMU 2.4, the write callback used to be > invoked when the end of the fw_cfg blob was reached. Since we intend to > pack several addresses in the same writeable fw_cfg blob down the road, > at different offsets, this method wouldn't work; the final offset in the > blob might not be reached at all. (Another thing that wouldn't work is > an 8-byte writeable fw_cfg blob, and a 4-byte WRITE_POINTER command that > only rewrites offsets 0..3 inclusve -- see (3) above.) > > So I don't think that a write callback is viable, at least with the > "many addresses at different offsets in the same blob" feature. Unless > we want to register different callbacks (or different callback > arguments) for different offsets. And then a large write could trigger a > series of callbacks, even, if it covers several special offsets. This > looks too complex to me. > > (ii) What we do have now is a select callback. Unfortunately, the guest > is not required to select the item and atomically rewrite the blob, in > the same fw_cfg DMA operation. The guest is allowed to do the selection > with the IO port method, and then write the blob separately with DMA. > (This is what OVMF does.) IOW, when we'd get the callback (for the > select), the address would not have been written yet. > > So I don't think that a select callback for the writeable "vmgenid_addr" > file is viable either. > > (iii) Let's see a select callback for the "vmgenid" blob itself! The > idea is, whenever the guest selects this blob, immediately refresh the > blob from the last set GUID. Then let the guest download the > just-refreshed blob. > I like this approach, and have it working. I assume by “refresh the blob” you mean making a call to fw_cfg_modify_file(). If that is the case, I need to modify the function fw_cfg_modify_bytes_read() because of the following commented-out problem lines (656:657 in fw_cfg.c): /* return the old data to the function caller, avoid memory leak */ ptr = s->entries[arch][key].data; s->entries[arch][key].data = data; s->entries[arch][key].len = len; // s->entries[arch][key].callback_opaque = NULL; // s->entries[arch][key].allow_write = false; As you can see, this function modifies not only a fw_cfg object’s data, but also its metadata, wiping out the callback parameter and forcing it to read-only. I don’t know the history of this function, so want to tread lightly. The second parameter doesn’t impact me, since this fw_cfg object is read-only, but the callback parameter one does. > The main observation here is that we can *fail* QMP commands. That's > good enough if we can tell *when* to fail them. > > Under this idea, we would fail the "set-vm-generation-id" QMP command > with "EAGAIN" if the quest had selected the "vmgenid" blob since machine > reset -- this can be tracked with a latch -- *but* the particular offset > range in the "vmgenid_addr" blob were still zero. > > This is exactly the window between the ALLOCATE and WRITE_POINTER > commands where updates from the monitor would be lost. So let's just > tell the management layer to try again later (like 1 second later). By > that time, any sane guest will have written the address into "vmgenid_addr”. > This seems sane. I’ll just return EAGAIN if vmgenid_addr is zero, meaning the guest is not fully synchronized yet so don’t accept input. One other more radical idea I had was to just remove the monitor GUID modification capability completely. The VM Generation ID needs to change only in the following situations: - new VM (command line processed) - start from snapshot (command line processed, VGIA restored from VmState. - VM recovered from backup (like a new VM, command line processed). - VM imported, copied or cloned (like a new VM, command line processed). - failed over from a disaster recovery environment. Here I’m not sure. It’s possible I suppose that one would have a DR VM in hot standby, but would want to change its VM Generation ID in order to force Windows to re-synchronize Active Directory, crypto seeds etc. Here the monitor might be necessary. > Thanks > Laszlo
On 02/05/17 10:12, ben@skyportsystems.com wrote: > From: Ben Warren <ben@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 <ben@skyportsystems.com> > --- > default-configs/i386-softmmu.mak | 1 + > default-configs/x86_64-softmmu.mak | 1 + > hw/acpi/Makefile.objs | 1 + > hw/acpi/vmgenid.c | 206 +++++++++++++++++++++++++++++++++++ > hw/i386/acpi-build.c | 10 ++ > include/hw/acpi/acpi_dev_interface.h | 1 + > include/hw/acpi/vmgenid.h | 37 +++++++ > 7 files changed, 257 insertions(+) > create mode 100644 hw/acpi/vmgenid.c > create mode 100644 include/hw/acpi/vmgenid.h [snip] > +static void vmgenid_device_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->vmsd = &vmstate_vmgenid; > +} I think in this function, you should set up a dc->realize member as well. And, in that dc->realize member, you should call qemu_register_reset(). Because, as it stands now, the "vgia_le" field is not cleared on reset. It should be; when the guest is rebooted, we should forget any address returned by its firmware. In this reset callback, you should also clear the (new) latch that tracks whether the "vmgenid" blob was selected since reset. (The latch is for approach (iii) against the race.) Thanks, Laszlo
On Wed, 8 Feb 2017 12:19:24 -0800 Ben Warren <ben@skyportsystems.com> wrote: > Thanks for reviewing Igor. > > > On Feb 7, 2017, at 5:48 AM, Igor Mammedov <imammedo@redhat.com> wrote: > > > > On Sun, 5 Feb 2017 01:12:00 -0800 > > ben@skyportsystems.com <mailto:ben@skyportsystems.com> wrote: > > > >> From: Ben Warren <ben@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 <ben@skyportsystems.com> > >> --- > >> default-configs/i386-softmmu.mak | 1 + > >> default-configs/x86_64-softmmu.mak | 1 + > >> hw/acpi/Makefile.objs | 1 + > >> hw/acpi/vmgenid.c | 206 +++++++++++++++++++++++++++++++++++ > >> hw/i386/acpi-build.c | 10 ++ > >> include/hw/acpi/acpi_dev_interface.h | 1 + > >> include/hw/acpi/vmgenid.h | 37 +++++++ > >> 7 files changed, 257 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 384cefb..1a43542 100644 > >> --- a/default-configs/i386-softmmu.mak > >> +++ b/default-configs/i386-softmmu.mak > >> @@ -58,3 +58,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 491a191..aee8b08 100644 > >> --- a/default-configs/x86_64-softmmu.mak > >> +++ b/default-configs/x86_64-softmmu.mak > >> @@ -58,3 +58,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..6c9ecfd > >> --- /dev/null > >> +++ b/hw/acpi/vmgenid.c > >> @@ -0,0 +1,206 @@ > >> +/* > >> + * Virtual Machine Generation ID Device > >> + * > >> + * Copyright (C) 2017 Skyport Systems. > >> + * > >> + * Author: Ben Warren <ben@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(GArray *table_data, GArray *guid, BIOSLinker *linker) > >> +{ > >> + Object *obj; > >> + VmGenIdState *s; > >> + Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx; > >> + uint32_t vgia_offset; > >> + > >> + obj = find_vmgenid_dev(NULL); > > get obj from caller > > > >> + assert(obj); > >> + s = VMGENID(obj); > >> + > >> + /* Fill in the GUID values */ > >> + if (guid->len != VMGENID_FW_CFG_SIZE) { > >> + g_array_set_size(guid, VMGENID_FW_CFG_SIZE); > > does it have to be conditional? > > > No, I guess not. > >> + } > >> + g_array_insert_vals(guid, VMGENID_GUID_OFFSET, s->guid.data, 16); > >> + > >> + /* 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)))); > > I'd rather have "VGIA" patched wit address to GUID and not the blob start, > > see next comment about how. > > > > Also usage aml_index() looks a bit confusing, how about: > > > > pkg = aml_local(0) > > aml_store(aml_package(2), pkg) > > aml_append(pkg, aml_name("VGIA")) > > aml_append(pkg, aml_int(0)) > > > I’ll try that, it does look simpler. I thought I tried this and couldn’t get it to work, but will try again. > >> + 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 */); > > does it guaranties that blob will be allocated in cacheable page? > > (SeaBIOS/OVMF) > > > >> + > >> + /* Patch address of GUID fw_cfg blob into the ADDR fw_cfg blob */ > >> + bios_linker_loader_add_pointer(linker, > >> + VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint32_t), > >> + VMGENID_GUID_FW_CFG_FILE, 0, true); > >> + > >> + /* Patch address of GUID fw_cfg blob into the AML */ > >> + bios_linker_loader_add_pointer(linker, > >> + ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t), > >> + VMGENID_GUID_FW_CFG_FILE, 0, false); > > see @src_offset argument description, > > putting VMGENID_GUID_OFFSET would make patched place point directly to GUID > > > >> + > >> + 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(FWCfgState *s, GArray *guid) > >> +{ > >> + Object *obj = find_vmgenid_dev(NULL); > >> + assert(obj); > >> + VmGenIdState *vms = VMGENID(obj); > >> + > >> + /* 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->vgia_le, sizeof(uint32_t), false); > >> +} > >> + > >> +static void vmgenid_update_guest(VmGenIdState *s) > >> +{ > >> + Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL); > >> + uint32_t vgia; > >> + > >> + if (obj) { > >> + /* Write the GUID to guest memory */ > >> + memcpy(&vgia, s->vgia_le, sizeof(vgia)); > >> + vgia = le32_to_cpu(vgia); > >> + if (vgia) { > >> + cpu_physical_memory_write(vgia + VMGENID_GUID_OFFSET, > >> + s->guid.data, sizeof(s->guid.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 *s = VMGENID(obj); > >> + > >> + if (!strncmp(value, "auto", 4)) { > >> + qemu_uuid_generate(&s->guid); > >> + } else if (qemu_uuid_parse(value, &s->guid) < 0) { > >> + error_setg(errp, "'%s. %s': Failed to parse GUID string: %s", > >> + object_get_typename(OBJECT(s)), VMGENID_GUID, value); > >> + return; > >> + } > >> + /* 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, so store that way internally. Make > >> + * sure to swap back whenever reporting via monitor */ > >> + qemu_uuid_bswap(&s->guid); > >> + > >> + /* Send the ACPI notify */ > >> + vmgenid_update_guest(s); > >> +} > >> + > >> +/* 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 *s = opaque; > >> + vmgenid_update_guest(s); > >> + 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(vgia_le, VmGenIdState, sizeof(uint32_t)), > > file with address could be memory region and migrated automatically, > > ex: rsdp_mr + acpi_add_rom_blob + acpi_ram_update > > > I don’t follow. Can you please elaborate? acpi_add_rom_blob() allocates memory that is automatically migrated without need for explicit VMSTATE_FOO magic. rsdp_mr is an example that uses this approach. > >> + VMSTATE_END_OF_LIST() > >> + }, > >> +}; > >> + > >> +static void vmgenid_initfn(Object *obj) > >> +{ > >> + object_property_add_str(obj, VMGENID_GUID, NULL, vmgenid_set_guid, NULL); > >> +} > >> + > >> +static void vmgenid_device_class_init(ObjectClass *klass, void *data) > >> +{ > >> + DeviceClass *dc = DEVICE_CLASS(klass); > >> + > >> + dc->vmsd = &vmstate_vmgenid; > >> +} > >> + > >> +static const TypeInfo vmgenid_device_info = { > >> + .name = VMGENID_DEVICE, > >> + .parent = TYPE_SYS_BUS_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 78a1d84..4c40f76 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" > >> @@ -2653,6 +2654,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) > >> acpi_add_table(table_offsets, tables_blob); > >> build_madt(tables_blob, tables->linker, pcms); > >> > >> + if (find_vmgenid_dev(NULL)) { > >> + acpi_add_table(table_offsets, tables_blob); > >> + vmgenid_build_acpi(tables_blob, tables->vmgenid, tables->linker); > > pass find_vmgenid_dev(NULL) result as an argument to vmgenid_build_acpi() > > so it won't have to do lookup again. > > > OK > >> + } > >> + > >> if (misc.has_hpet) { > >> acpi_add_table(table_offsets, tables_blob); > >> build_hpet(tables_blob, tables->linker); > >> @@ -2859,6 +2865,10 @@ void acpi_setup(void) > >> fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, > >> tables.tcpalog->data, acpi_data_len(tables.tcpalog)); > >> > >> + if (find_vmgenid_dev(NULL)) { > > it's second lookup, cache result of the 1st call and reuse it here > > > OK > >> + vmgenid_add_fw_cfg(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..b60437a > >> --- /dev/null > >> +++ b/include/hw/acpi/vmgenid.h > >> @@ -0,0 +1,37 @@ > >> +#ifndef ACPI_VMGENID_H > >> +#define ACPI_VMGENID_H > >> + > >> +#include "hw/acpi/bios-linker-loader.h" > >> +#include "hw/sysbus.h" > >> +#include "qemu/uuid.h" > >> + > >> +#define VMGENID_DEVICE "vmgenid" > >> +#define VMGENID_GUID "guid" > >> +#define VMGENID_GUID_FW_CFG_FILE "etc/vmgenid" > >> +#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 */ > >> + > >> +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid); > >> +void vmgenid_build_acpi(GArray *table_data, GArray *guid, BIOSLinker *linker); > >> + > >> +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE) > >> + > >> +typedef struct VmGenIdState { > >> + SysBusDevice parent_obj; > > Could it work with just plain Device parent? > > If it works then you can drop patch: > > [PATCH v5 08/10] PC: Support dynamic sysbus on pc_i440fx > > > Huh, I was sure I tried that originally and couldn’t make it work, but it turns out it does. Definitely better. > >> + QemuUUID guid; > >> + uint8_t vgia_le[4]; > >> +} VmGenIdState; > >> + > >> +static Object *find_vmgenid_dev(Error **errp) > >> +{ > >> + Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL); > >> + if (!obj && errp) { > > all callers pass NULL as errp, I'd just drop errp altogether. > > > OK > >> + error_setg(errp, "%s is not found", VMGENID_DEVICE); > >> + } > >> + return obj; > >> +} > >> + > >> +#endif >
On Wed, 8 Feb 2017 01:48:42 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > On 02/05/17 10:12, ben@skyportsystems.com wrote: > > From: Ben Warren <ben@skyportsystems.com> [...] > (2) Structure of the vmgenid fw_cfg blob, AKA "why that darn offset 40 > decimal". > > I explained it under points (6) and (7) in the following message: > > Message-Id: <c16a03d5-820a-f719-81ea-43858f903395@redhat.com> > URL: https://www.mail-archive.com/qemu-devel@nongnu.org/msg425226.html > > The story is that *wherever* an ADD_POINTER command points to -- that > is, at the *exact* target address --, OVMF will look for an ACPI table > header, somewhat heuristically. If it finds a byte pattern that (a) fits > into the remaining blob and (b) passes some superficial ACPI table > header checks, such as length and checksum, then OVMF assumes that the > blob contains an ACPI table there, and passes the address (again, the > exact, relocated, absolute target address of ADD_POINTER) to > EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(). > > We want to disable this heuristic for the vmgenid blob. *If* the blob > contained only 16 bytes (for the GUID), then the heuristic would > automatically disable itself, because the ACPI table header (36 bytes) > is larger than 16 bytes, so OVMF wouldn't even look. However, for the > caching and other VMGENID requirements, we need to allocate a full page > with the blob (4KB), hence OVMF *will* look. If we placed the GUID right > at the start of the page, then OVMF would sanity-check it as an ACPI > table header. The check would *most likely* not pass, so things would be > fine in practice, but we can do better than that: just put 40 zero bytes > at the front of the blob. > > And this is why the ADD_POINTER command has to point to the beginning of > the blob (i.e., 36 zero bytes), to "suppress the OVMF ACPI SDT header > detection". (The other 4 bytes are for arriving at an address divisible > by 8, which is a VMGENID requirement for the GUID.) > > The consequence is that both the ADDR method and QEMU's guest memory > access code have to add 40 manually. The longer I look "suppress the OVMF ACPI SDT header detection", the less I like approach. It looks somewhat backwards where a firmware forces QEMU to use non obvious offsets to workaround OVMF ACPI table detection heuristics. Can we add and use explicit flag to mark blobs as ACPI tables, so that OVMF won't have to guess whether to hand off table as ACPI to UEFI stack or just keep it to yourself? [...] > Thanks > Laszlo >
On Thu, Feb 09, 2017 at 06:23:16PM +0100, Igor Mammedov wrote: > On Wed, 8 Feb 2017 01:48:42 +0100 > Laszlo Ersek <lersek@redhat.com> wrote: > > > On 02/05/17 10:12, ben@skyportsystems.com wrote: > > > From: Ben Warren <ben@skyportsystems.com> > [...] > > > (2) Structure of the vmgenid fw_cfg blob, AKA "why that darn offset 40 > > decimal". > > > > I explained it under points (6) and (7) in the following message: > > > > Message-Id: <c16a03d5-820a-f719-81ea-43858f903395@redhat.com> > > URL: https://www.mail-archive.com/qemu-devel@nongnu.org/msg425226.html > > > > The story is that *wherever* an ADD_POINTER command points to -- that > > is, at the *exact* target address --, OVMF will look for an ACPI table > > header, somewhat heuristically. If it finds a byte pattern that (a) fits > > into the remaining blob and (b) passes some superficial ACPI table > > header checks, such as length and checksum, then OVMF assumes that the > > blob contains an ACPI table there, and passes the address (again, the > > exact, relocated, absolute target address of ADD_POINTER) to > > EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(). > > > > We want to disable this heuristic for the vmgenid blob. *If* the blob > > contained only 16 bytes (for the GUID), then the heuristic would > > automatically disable itself, because the ACPI table header (36 bytes) > > is larger than 16 bytes, so OVMF wouldn't even look. However, for the > > caching and other VMGENID requirements, we need to allocate a full page > > with the blob (4KB), hence OVMF *will* look. If we placed the GUID right > > at the start of the page, then OVMF would sanity-check it as an ACPI > > table header. The check would *most likely* not pass, so things would be > > fine in practice, but we can do better than that: just put 40 zero bytes > > at the front of the blob. > > > > And this is why the ADD_POINTER command has to point to the beginning of > > the blob (i.e., 36 zero bytes), to "suppress the OVMF ACPI SDT header > > detection". (The other 4 bytes are for arriving at an address divisible > > by 8, which is a VMGENID requirement for the GUID.) > > > > The consequence is that both the ADDR method and QEMU's guest memory > > access code have to add 40 manually. > The longer I look "suppress the OVMF ACPI SDT header detection", > the less I like approach. > > It looks somewhat backwards where a firmware forces QEMU > to use non obvious offsets to workaround OVMF ACPI table detection > heuristics. > Can we add and use explicit flag to mark blobs as ACPI tables, > so that OVMF won't have to guess whether to hand off table > as ACPI to UEFI stack or just keep it to yourself? Seems like the minor enough hack. At this stage I'm inclined to say let's merge with the current approach, and we can do a patch on top if we have the time. > > [...] > > Thanks > > Laszlo > >
On 02/09/17 18:23, Igor Mammedov wrote: > On Wed, 8 Feb 2017 01:48:42 +0100 > Laszlo Ersek <lersek@redhat.com> wrote: > >> On 02/05/17 10:12, ben@skyportsystems.com wrote: >>> From: Ben Warren <ben@skyportsystems.com> > [...] > >> (2) Structure of the vmgenid fw_cfg blob, AKA "why that darn offset 40 >> decimal". >> >> I explained it under points (6) and (7) in the following message: >> >> Message-Id: <c16a03d5-820a-f719-81ea-43858f903395@redhat.com> >> URL: https://www.mail-archive.com/qemu-devel@nongnu.org/msg425226.html >> >> The story is that *wherever* an ADD_POINTER command points to -- that >> is, at the *exact* target address --, OVMF will look for an ACPI table >> header, somewhat heuristically. If it finds a byte pattern that (a) fits >> into the remaining blob and (b) passes some superficial ACPI table >> header checks, such as length and checksum, then OVMF assumes that the >> blob contains an ACPI table there, and passes the address (again, the >> exact, relocated, absolute target address of ADD_POINTER) to >> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(). >> >> We want to disable this heuristic for the vmgenid blob. *If* the blob >> contained only 16 bytes (for the GUID), then the heuristic would >> automatically disable itself, because the ACPI table header (36 bytes) >> is larger than 16 bytes, so OVMF wouldn't even look. However, for the >> caching and other VMGENID requirements, we need to allocate a full page >> with the blob (4KB), hence OVMF *will* look. If we placed the GUID right >> at the start of the page, then OVMF would sanity-check it as an ACPI >> table header. The check would *most likely* not pass, so things would be >> fine in practice, but we can do better than that: just put 40 zero bytes >> at the front of the blob. >> >> And this is why the ADD_POINTER command has to point to the beginning of >> the blob (i.e., 36 zero bytes), to "suppress the OVMF ACPI SDT header >> detection". (The other 4 bytes are for arriving at an address divisible >> by 8, which is a VMGENID requirement for the GUID.) >> >> The consequence is that both the ADDR method and QEMU's guest memory >> access code have to add 40 manually. > The longer I look "suppress the OVMF ACPI SDT header detection", > the less I like approach. > > It looks somewhat backwards where a firmware forces QEMU > to use non obvious offsets to workaround OVMF ACPI table detection > heuristics. This is for historical reasons -- when the linker/loader commands were invented, it wasn't considered that in UEFI, ACPI tables cannot just be linked together in-place, instead they'd have to be passed to EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() one by one, for copying. The commands didn't provide dedicated means for identifying individual tables in blobs. Hence the heuristics built upon ADD_POINTER. And once you have heuristics, you want to suppress them occasionally, if you can find a way. > Can we add and use explicit flag to mark blobs as ACPI tables, > so that OVMF won't have to guess whether to hand off table > as ACPI to UEFI stack or just keep it to yourself? The ADD_POINTER-based heuristics cannot be turned off for ACPI table identification, because a single fw_cfg blob can (and does) contain multiple ACPI tables, and OVMF needs to figure out, somehow, where each of those tables start. Blob-level hints won't help with this. The following could be an improvement though: a blob-level hint (perhaps in the ALLOCATE command) that the blob contains *no* ACPI tables. In this case, OVMF could turn off the table recognition heuristics for those ADD_POINTER commands that point into such blobs. (Plus mark the blob for permanent preservation at once, and not only when an ADD_POINTER "probe" into the blob fails.) OVMF currently ignores the Zone field in the ALLOCATE command, so that could be extended / abused for such a hint, without breaking compatibility with OVMF. (Not sure about SeaBIOS.) Otherwise, a new allocation command will be necessary. (Which should embed the current ALLOCATE command structure fully, at some offset.) Thanks Laszlo
> On Feb 9, 2017, at 11:27 AM, Laszlo Ersek <lersek@redhat.com> wrote: > > On 02/09/17 18:23, Igor Mammedov wrote: >> On Wed, 8 Feb 2017 01:48:42 +0100 >> Laszlo Ersek <lersek@redhat.com> wrote: >> >>> On 02/05/17 10:12, ben@skyportsystems.com wrote: >>>> From: Ben Warren <ben@skyportsystems.com> >> [...] >> >>> (2) Structure of the vmgenid fw_cfg blob, AKA "why that darn offset 40 >>> decimal". >>> >>> I explained it under points (6) and (7) in the following message: >>> >>> Message-Id: <c16a03d5-820a-f719-81ea-43858f903395@redhat.com> >>> URL: https://www.mail-archive.com/qemu-devel@nongnu.org/msg425226.html >>> >>> The story is that *wherever* an ADD_POINTER command points to -- that >>> is, at the *exact* target address --, OVMF will look for an ACPI table >>> header, somewhat heuristically. If it finds a byte pattern that (a) fits >>> into the remaining blob and (b) passes some superficial ACPI table >>> header checks, such as length and checksum, then OVMF assumes that the >>> blob contains an ACPI table there, and passes the address (again, the >>> exact, relocated, absolute target address of ADD_POINTER) to >>> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(). >>> >>> We want to disable this heuristic for the vmgenid blob. *If* the blob >>> contained only 16 bytes (for the GUID), then the heuristic would >>> automatically disable itself, because the ACPI table header (36 bytes) >>> is larger than 16 bytes, so OVMF wouldn't even look. However, for the >>> caching and other VMGENID requirements, we need to allocate a full page >>> with the blob (4KB), hence OVMF *will* look. If we placed the GUID right >>> at the start of the page, then OVMF would sanity-check it as an ACPI >>> table header. The check would *most likely* not pass, so things would be >>> fine in practice, but we can do better than that: just put 40 zero bytes >>> at the front of the blob. >>> >>> And this is why the ADD_POINTER command has to point to the beginning of >>> the blob (i.e., 36 zero bytes), to "suppress the OVMF ACPI SDT header >>> detection". (The other 4 bytes are for arriving at an address divisible >>> by 8, which is a VMGENID requirement for the GUID.) >>> >>> The consequence is that both the ADDR method and QEMU's guest memory >>> access code have to add 40 manually. >> The longer I look "suppress the OVMF ACPI SDT header detection", >> the less I like approach. >> >> It looks somewhat backwards where a firmware forces QEMU >> to use non obvious offsets to workaround OVMF ACPI table detection >> heuristics. > > This is for historical reasons -- when the linker/loader commands were > invented, it wasn't considered that in UEFI, ACPI tables cannot just be > linked together in-place, instead they'd have to be passed to > EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() one by one, for copying. The > commands didn't provide dedicated means for identifying individual > tables in blobs. Hence the heuristics built upon ADD_POINTER. > > And once you have heuristics, you want to suppress them occasionally, if > you can find a way. > >> Can we add and use explicit flag to mark blobs as ACPI tables, >> so that OVMF won't have to guess whether to hand off table >> as ACPI to UEFI stack or just keep it to yourself? > > The ADD_POINTER-based heuristics cannot be turned off for ACPI table > identification, because a single fw_cfg blob can (and does) contain > multiple ACPI tables, and OVMF needs to figure out, somehow, where each > of those tables start. Blob-level hints won't help with this. > > The following could be an improvement though: a blob-level hint (perhaps > in the ALLOCATE command) that the blob contains *no* ACPI tables. In > this case, OVMF could turn off the table recognition heuristics for > those ADD_POINTER commands that point into such blobs. (Plus mark the > blob for permanent preservation at once, and not only when an > ADD_POINTER "probe" into the blob fails.) > > OVMF currently ignores the Zone field in the ALLOCATE command, so that > could be extended / abused for such a hint, without breaking > compatibility with OVMF. (Not sure about SeaBIOS.) > Overloading the ALLOCATE command in theory could be done with a simple change to SeaBIOS. Here’s where the zone is handled: switch (entry->alloc_zone) { case ROMFILE_LOADER_ALLOC_ZONE_HIGH: zone = &ZoneHigh; break; case ROMFILE_LOADER_ALLOC_ZONE_FSEG: zone = &ZoneFSeg; break; default: goto err; } ZONE_HIGH = 1, and ZONE_FSEG = 2 and alloc_zone is 8 bits. Stealing the MSB and masking it off would be dirty, but could work. > Otherwise, a new allocation command will be necessary. (Which should > embed the current ALLOCATE command structure fully, at some offset.) > > Thanks > Laszlo
On 02/09/17 21:02, Ben Warren wrote: > >> On Feb 9, 2017, at 11:27 AM, Laszlo Ersek <lersek@redhat.com >> <mailto:lersek@redhat.com>> wrote: >> >> On 02/09/17 18:23, Igor Mammedov wrote: >>> On Wed, 8 Feb 2017 01:48:42 +0100 >>> Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>> wrote: >>> >>>> On 02/05/17 10:12, ben@skyportsystems.com >>>> <mailto:ben@skyportsystems.com> wrote: >>>>> From: Ben Warren <ben@skyportsystems.com >>>>> <mailto:ben@skyportsystems.com>> >>> [...] >>> >>>> (2) Structure of the vmgenid fw_cfg blob, AKA "why that darn offset 40 >>>> decimal". >>>> >>>> I explained it under points (6) and (7) in the following message: >>>> >>>> Message-Id: <c16a03d5-820a-f719-81ea-43858f903395@redhat.com >>>> <mailto:c16a03d5-820a-f719-81ea-43858f903395@redhat.com>> >>>> URL: https://www.mail-archive.com/qemu-devel@nongnu.org/msg425226.html >>>> >>>> The story is that *wherever* an ADD_POINTER command points to -- that >>>> is, at the *exact* target address --, OVMF will look for an ACPI table >>>> header, somewhat heuristically. If it finds a byte pattern that (a) fits >>>> into the remaining blob and (b) passes some superficial ACPI table >>>> header checks, such as length and checksum, then OVMF assumes that the >>>> blob contains an ACPI table there, and passes the address (again, the >>>> exact, relocated, absolute target address of ADD_POINTER) to >>>> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(). >>>> >>>> We want to disable this heuristic for the vmgenid blob. *If* the blob >>>> contained only 16 bytes (for the GUID), then the heuristic would >>>> automatically disable itself, because the ACPI table header (36 bytes) >>>> is larger than 16 bytes, so OVMF wouldn't even look. However, for the >>>> caching and other VMGENID requirements, we need to allocate a full page >>>> with the blob (4KB), hence OVMF *will* look. If we placed the GUID right >>>> at the start of the page, then OVMF would sanity-check it as an ACPI >>>> table header. The check would *most likely* not pass, so things would be >>>> fine in practice, but we can do better than that: just put 40 zero bytes >>>> at the front of the blob. >>>> >>>> And this is why the ADD_POINTER command has to point to the beginning of >>>> the blob (i.e., 36 zero bytes), to "suppress the OVMF ACPI SDT header >>>> detection". (The other 4 bytes are for arriving at an address divisible >>>> by 8, which is a VMGENID requirement for the GUID.) >>>> >>>> The consequence is that both the ADDR method and QEMU's guest memory >>>> access code have to add 40 manually. >>> The longer I look "suppress the OVMF ACPI SDT header detection", >>> the less I like approach. >>> >>> It looks somewhat backwards where a firmware forces QEMU >>> to use non obvious offsets to workaround OVMF ACPI table detection >>> heuristics. >> >> This is for historical reasons -- when the linker/loader commands were >> invented, it wasn't considered that in UEFI, ACPI tables cannot just be >> linked together in-place, instead they'd have to be passed to >> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() one by one, for copying. The >> commands didn't provide dedicated means for identifying individual >> tables in blobs. Hence the heuristics built upon ADD_POINTER. >> >> And once you have heuristics, you want to suppress them occasionally, if >> you can find a way. >> >>> Can we add and use explicit flag to mark blobs as ACPI tables, >>> so that OVMF won't have to guess whether to hand off table >>> as ACPI to UEFI stack or just keep it to yourself? >> >> The ADD_POINTER-based heuristics cannot be turned off for ACPI table >> identification, because a single fw_cfg blob can (and does) contain >> multiple ACPI tables, and OVMF needs to figure out, somehow, where each >> of those tables start. Blob-level hints won't help with this. >> >> The following could be an improvement though: a blob-level hint (perhaps >> in the ALLOCATE command) that the blob contains *no* ACPI tables. In >> this case, OVMF could turn off the table recognition heuristics for >> those ADD_POINTER commands that point into such blobs. (Plus mark the >> blob for permanent preservation at once, and not only when an >> ADD_POINTER "probe" into the blob fails.) >> >> OVMF currently ignores the Zone field in the ALLOCATE command, so that >> could be extended / abused for such a hint, without breaking >> compatibility with OVMF. (Not sure about SeaBIOS.) >> > Overloading the ALLOCATE command in theory could be done with a simple > change to SeaBIOS. Here’s where the zone is handled: > > switch (entry->alloc_zone) { > case ROMFILE_LOADER_ALLOC_ZONE_HIGH: > zone = &ZoneHigh; > break; > case ROMFILE_LOADER_ALLOC_ZONE_FSEG: > zone = &ZoneFSeg; > break; > default: > goto err; > } > > ZONE_HIGH = 1, and ZONE_FSEG = 2 and alloc_zone is 8 bits. Stealing the > MSB and masking it off would be dirty, but could work. Even assuming that we don't horribly break cross-version compatibility with tricks like this, the problem is that such changes only look simple and easy in isolation. They add up, and their testing impact is not small. This week I've been working practically every waking moment -- I wanted to provide quick and detailed feedback (as far as OVMF was concerned) under both this series and on the topic of DSDT / X_DSDT / multiply-pointed-to tables. I'm also "racing you" to create a good quality OVMF series for WRITE_POINTER (including replay at S3 resume), so that as soon you post v6, we can test it together with OVMF too. (Last night I thought I had WRITE_POINTER ready, and started testing it against your v5, but then I suddenly thought of device reset and S3... Sigh.) We can certainly file RFEs for stuff like the above (maybe in RHBZ, maybe in Launchpad), but the scope creep is already significant for VMGENID (it's noone's fault, VMGENID has tentacles into everything). Igor's suggestion is good, it aims to decrease technical debt, but I'm grasping at cycles -- review is very time consuming (albeit just as necessary). Also, for QEMU, the 2.9 soft freeze is around the corner... Best I can recommend now is filing an RFE for this. Thanks Laszlo >> Otherwise, a new allocation command will be necessary. (Which should >> embed the current ALLOCATE command structure fully, at some offset.) >> >> Thanks >> Laszlo >
> On Feb 9, 2017, at 12:24 PM, Laszlo Ersek <lersek@redhat.com> wrote: > > On 02/09/17 21:02, Ben Warren wrote: >> >>> On Feb 9, 2017, at 11:27 AM, Laszlo Ersek <lersek@redhat.com >>> <mailto:lersek@redhat.com>> wrote: >>> >>> On 02/09/17 18:23, Igor Mammedov wrote: >>>> On Wed, 8 Feb 2017 01:48:42 +0100 >>>> Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>> wrote: >>>> >>>>> On 02/05/17 10:12, ben@skyportsystems.com >>>>> <mailto:ben@skyportsystems.com> wrote: >>>>>> From: Ben Warren <ben@skyportsystems.com >>>>>> <mailto:ben@skyportsystems.com>> >>>> [...] >>>> >>>>> (2) Structure of the vmgenid fw_cfg blob, AKA "why that darn offset 40 >>>>> decimal". >>>>> >>>>> I explained it under points (6) and (7) in the following message: >>>>> >>>>> Message-Id: <c16a03d5-820a-f719-81ea-43858f903395@redhat.com >>>>> <mailto:c16a03d5-820a-f719-81ea-43858f903395@redhat.com>> >>>>> URL: https://www.mail-archive.com/qemu-devel@nongnu.org/msg425226.html >>>>> >>>>> The story is that *wherever* an ADD_POINTER command points to -- that >>>>> is, at the *exact* target address --, OVMF will look for an ACPI table >>>>> header, somewhat heuristically. If it finds a byte pattern that (a) fits >>>>> into the remaining blob and (b) passes some superficial ACPI table >>>>> header checks, such as length and checksum, then OVMF assumes that the >>>>> blob contains an ACPI table there, and passes the address (again, the >>>>> exact, relocated, absolute target address of ADD_POINTER) to >>>>> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(). >>>>> >>>>> We want to disable this heuristic for the vmgenid blob. *If* the blob >>>>> contained only 16 bytes (for the GUID), then the heuristic would >>>>> automatically disable itself, because the ACPI table header (36 bytes) >>>>> is larger than 16 bytes, so OVMF wouldn't even look. However, for the >>>>> caching and other VMGENID requirements, we need to allocate a full page >>>>> with the blob (4KB), hence OVMF *will* look. If we placed the GUID right >>>>> at the start of the page, then OVMF would sanity-check it as an ACPI >>>>> table header. The check would *most likely* not pass, so things would be >>>>> fine in practice, but we can do better than that: just put 40 zero bytes >>>>> at the front of the blob. >>>>> >>>>> And this is why the ADD_POINTER command has to point to the beginning of >>>>> the blob (i.e., 36 zero bytes), to "suppress the OVMF ACPI SDT header >>>>> detection". (The other 4 bytes are for arriving at an address divisible >>>>> by 8, which is a VMGENID requirement for the GUID.) >>>>> >>>>> The consequence is that both the ADDR method and QEMU's guest memory >>>>> access code have to add 40 manually. >>>> The longer I look "suppress the OVMF ACPI SDT header detection", >>>> the less I like approach. >>>> >>>> It looks somewhat backwards where a firmware forces QEMU >>>> to use non obvious offsets to workaround OVMF ACPI table detection >>>> heuristics. >>> >>> This is for historical reasons -- when the linker/loader commands were >>> invented, it wasn't considered that in UEFI, ACPI tables cannot just be >>> linked together in-place, instead they'd have to be passed to >>> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() one by one, for copying. The >>> commands didn't provide dedicated means for identifying individual >>> tables in blobs. Hence the heuristics built upon ADD_POINTER. >>> >>> And once you have heuristics, you want to suppress them occasionally, if >>> you can find a way. >>> >>>> Can we add and use explicit flag to mark blobs as ACPI tables, >>>> so that OVMF won't have to guess whether to hand off table >>>> as ACPI to UEFI stack or just keep it to yourself? >>> >>> The ADD_POINTER-based heuristics cannot be turned off for ACPI table >>> identification, because a single fw_cfg blob can (and does) contain >>> multiple ACPI tables, and OVMF needs to figure out, somehow, where each >>> of those tables start. Blob-level hints won't help with this. >>> >>> The following could be an improvement though: a blob-level hint (perhaps >>> in the ALLOCATE command) that the blob contains *no* ACPI tables. In >>> this case, OVMF could turn off the table recognition heuristics for >>> those ADD_POINTER commands that point into such blobs. (Plus mark the >>> blob for permanent preservation at once, and not only when an >>> ADD_POINTER "probe" into the blob fails.) >>> >>> OVMF currently ignores the Zone field in the ALLOCATE command, so that >>> could be extended / abused for such a hint, without breaking >>> compatibility with OVMF. (Not sure about SeaBIOS.) >>> >> Overloading the ALLOCATE command in theory could be done with a simple >> change to SeaBIOS. Here’s where the zone is handled: >> >> switch (entry->alloc_zone) { >> case ROMFILE_LOADER_ALLOC_ZONE_HIGH: >> zone = &ZoneHigh; >> break; >> case ROMFILE_LOADER_ALLOC_ZONE_FSEG: >> zone = &ZoneFSeg; >> break; >> default: >> goto err; >> } >> >> ZONE_HIGH = 1, and ZONE_FSEG = 2 and alloc_zone is 8 bits. Stealing the >> MSB and masking it off would be dirty, but could work. > > Even assuming that we don't horribly break cross-version compatibility > with tricks like this, the problem is that such changes only look simple > and easy in isolation. They add up, and their testing impact is not small. > > This week I've been working practically every waking moment -- I wanted > to provide quick and detailed feedback (as far as OVMF was concerned) > under both this series and on the topic of DSDT / X_DSDT / > multiply-pointed-to tables. I'm also "racing you" to create a good > quality OVMF series for WRITE_POINTER (including replay at S3 resume), > so that as soon you post v6, we can test it together with OVMF too. > (Last night I thought I had WRITE_POINTER ready, and started testing it > against your v5, but then I suddenly thought of device reset and S3... > Sigh.) > > We can certainly file RFEs for stuff like the above (maybe in RHBZ, > maybe in Launchpad), but the scope creep is already significant for > VMGENID (it's noone's fault, VMGENID has tentacles into everything). > Igor's suggestion is good, it aims to decrease technical debt, but I'm > grasping at cycles -- review is very time consuming (albeit just as > necessary). Also, for QEMU, the 2.9 soft freeze is around the corner… OK, well I’m fine with things as they are. I’ll try to get v6 out tomorrow. This optimization can wait until later. > Best I can recommend now is filing an RFE for this. > > Thanks > Laszlo > >>> Otherwise, a new allocation command will be necessary. (Which should >>> embed the current ALLOCATE command structure fully, at some offset.) >>> >>> Thanks >>> Laszlo >> >
On Thu, 9 Feb 2017 12:39:30 -0800 Ben Warren <ben@skyportsystems.com> wrote: > > On Feb 9, 2017, at 12:24 PM, Laszlo Ersek <lersek@redhat.com> wrote: > > > > On 02/09/17 21:02, Ben Warren wrote: > >> > >>> On Feb 9, 2017, at 11:27 AM, Laszlo Ersek <lersek@redhat.com > >>> <mailto:lersek@redhat.com>> wrote: > >>> > >>> On 02/09/17 18:23, Igor Mammedov wrote: > >>>> On Wed, 8 Feb 2017 01:48:42 +0100 > >>>> Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>> wrote: > >>>> > >>>>> On 02/05/17 10:12, ben@skyportsystems.com > >>>>> <mailto:ben@skyportsystems.com> wrote: > >>>>>> From: Ben Warren <ben@skyportsystems.com > >>>>>> <mailto:ben@skyportsystems.com>> > >>>> [...] > >>>> > >>>>> (2) Structure of the vmgenid fw_cfg blob, AKA "why that darn offset 40 > >>>>> decimal". > >>>>> > >>>>> I explained it under points (6) and (7) in the following message: > >>>>> > >>>>> Message-Id: <c16a03d5-820a-f719-81ea-43858f903395@redhat.com > >>>>> <mailto:c16a03d5-820a-f719-81ea-43858f903395@redhat.com>> > >>>>> URL: https://www.mail-archive.com/qemu-devel@nongnu.org/msg425226.html > >>>>> > >>>>> The story is that *wherever* an ADD_POINTER command points to -- that > >>>>> is, at the *exact* target address --, OVMF will look for an ACPI table > >>>>> header, somewhat heuristically. If it finds a byte pattern that (a) fits > >>>>> into the remaining blob and (b) passes some superficial ACPI table > >>>>> header checks, such as length and checksum, then OVMF assumes that the > >>>>> blob contains an ACPI table there, and passes the address (again, the > >>>>> exact, relocated, absolute target address of ADD_POINTER) to > >>>>> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(). > >>>>> > >>>>> We want to disable this heuristic for the vmgenid blob. *If* the blob > >>>>> contained only 16 bytes (for the GUID), then the heuristic would > >>>>> automatically disable itself, because the ACPI table header (36 bytes) > >>>>> is larger than 16 bytes, so OVMF wouldn't even look. However, for the > >>>>> caching and other VMGENID requirements, we need to allocate a full page > >>>>> with the blob (4KB), hence OVMF *will* look. If we placed the GUID right > >>>>> at the start of the page, then OVMF would sanity-check it as an ACPI > >>>>> table header. The check would *most likely* not pass, so things would be > >>>>> fine in practice, but we can do better than that: just put 40 zero bytes > >>>>> at the front of the blob. > >>>>> > >>>>> And this is why the ADD_POINTER command has to point to the beginning of > >>>>> the blob (i.e., 36 zero bytes), to "suppress the OVMF ACPI SDT header > >>>>> detection". (The other 4 bytes are for arriving at an address divisible > >>>>> by 8, which is a VMGENID requirement for the GUID.) > >>>>> > >>>>> The consequence is that both the ADDR method and QEMU's guest memory > >>>>> access code have to add 40 manually. > >>>> The longer I look "suppress the OVMF ACPI SDT header detection", > >>>> the less I like approach. > >>>> > >>>> It looks somewhat backwards where a firmware forces QEMU > >>>> to use non obvious offsets to workaround OVMF ACPI table detection > >>>> heuristics. > >>> > >>> This is for historical reasons -- when the linker/loader commands were > >>> invented, it wasn't considered that in UEFI, ACPI tables cannot just be > >>> linked together in-place, instead they'd have to be passed to > >>> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() one by one, for copying. The > >>> commands didn't provide dedicated means for identifying individual > >>> tables in blobs. Hence the heuristics built upon ADD_POINTER. > >>> > >>> And once you have heuristics, you want to suppress them occasionally, if > >>> you can find a way. > >>> > >>>> Can we add and use explicit flag to mark blobs as ACPI tables, > >>>> so that OVMF won't have to guess whether to hand off table > >>>> as ACPI to UEFI stack or just keep it to yourself? > >>> > >>> The ADD_POINTER-based heuristics cannot be turned off for ACPI table > >>> identification, because a single fw_cfg blob can (and does) contain > >>> multiple ACPI tables, and OVMF needs to figure out, somehow, where each > >>> of those tables start. Blob-level hints won't help with this. > >>> > >>> The following could be an improvement though: a blob-level hint (perhaps > >>> in the ALLOCATE command) that the blob contains *no* ACPI tables. In > >>> this case, OVMF could turn off the table recognition heuristics for > >>> those ADD_POINTER commands that point into such blobs. (Plus mark the > >>> blob for permanent preservation at once, and not only when an > >>> ADD_POINTER "probe" into the blob fails.) > >>> > >>> OVMF currently ignores the Zone field in the ALLOCATE command, so that > >>> could be extended / abused for such a hint, without breaking > >>> compatibility with OVMF. (Not sure about SeaBIOS.) > >>> > >> Overloading the ALLOCATE command in theory could be done with a simple > >> change to SeaBIOS. Here’s where the zone is handled: > >> > >> switch (entry->alloc_zone) { > >> case ROMFILE_LOADER_ALLOC_ZONE_HIGH: > >> zone = &ZoneHigh; > >> break; > >> case ROMFILE_LOADER_ALLOC_ZONE_FSEG: > >> zone = &ZoneFSeg; > >> break; > >> default: > >> goto err; > >> } > >> > >> ZONE_HIGH = 1, and ZONE_FSEG = 2 and alloc_zone is 8 bits. Stealing the > >> MSB and masking it off would be dirty, but could work. > > > > Even assuming that we don't horribly break cross-version compatibility > > with tricks like this, the problem is that such changes only look simple > > and easy in isolation. They add up, and their testing impact is not small. > > > > This week I've been working practically every waking moment -- I wanted > > to provide quick and detailed feedback (as far as OVMF was concerned) > > under both this series and on the topic of DSDT / X_DSDT / > > multiply-pointed-to tables. I'm also "racing you" to create a good > > quality OVMF series for WRITE_POINTER (including replay at S3 resume), > > so that as soon you post v6, we can test it together with OVMF too. > > (Last night I thought I had WRITE_POINTER ready, and started testing it > > against your v5, but then I suddenly thought of device reset and S3... > > Sigh.) > > > > We can certainly file RFEs for stuff like the above (maybe in RHBZ, > > maybe in Launchpad), but the scope creep is already significant for > > VMGENID (it's noone's fault, VMGENID has tentacles into everything). > > Igor's suggestion is good, it aims to decrease technical debt, but I'm > > grasping at cycles -- review is very time consuming (albeit just as > > necessary). Also, for QEMU, the 2.9 soft freeze is around the corner… > > OK, well I’m fine with things as they are. I’ll try to get v6 out tomorrow. This optimization can wait until later. Agreed, let forget about this for now, we can later revisit this issue. (vmgeid is probably not the only one that needs it, TPM comes in mind and maybe there are other blobs.) > > > Best I can recommend now is filing an RFE for this. > > > > Thanks > > Laszlo > > > >>> Otherwise, a new allocation command will be necessary. (Which should > >>> embed the current ALLOCATE command structure fully, at some offset.) > >>> > >>> Thanks > >>> Laszlo > >> > > >
diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak index 384cefb..1a43542 100644 --- a/default-configs/i386-softmmu.mak +++ b/default-configs/i386-softmmu.mak @@ -58,3 +58,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 491a191..aee8b08 100644 --- a/default-configs/x86_64-softmmu.mak +++ b/default-configs/x86_64-softmmu.mak @@ -58,3 +58,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..6c9ecfd --- /dev/null +++ b/hw/acpi/vmgenid.c @@ -0,0 +1,206 @@ +/* + * Virtual Machine Generation ID Device + * + * Copyright (C) 2017 Skyport Systems. + * + * Author: Ben Warren <ben@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(GArray *table_data, GArray *guid, BIOSLinker *linker) +{ + Object *obj; + VmGenIdState *s; + Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx; + uint32_t vgia_offset; + + obj = find_vmgenid_dev(NULL); + assert(obj); + s = VMGENID(obj); + + /* Fill in the GUID values */ + if (guid->len != VMGENID_FW_CFG_SIZE) { + g_array_set_size(guid, VMGENID_FW_CFG_SIZE); + } + g_array_insert_vals(guid, VMGENID_GUID_OFFSET, s->guid.data, 16); + + /* 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)))); + 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 */ + bios_linker_loader_add_pointer(linker, + VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint32_t), + VMGENID_GUID_FW_CFG_FILE, 0, true); + + /* Patch address of GUID fw_cfg blob into the AML */ + bios_linker_loader_add_pointer(linker, + ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t), + VMGENID_GUID_FW_CFG_FILE, 0, false); + + 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(FWCfgState *s, GArray *guid) +{ + Object *obj = find_vmgenid_dev(NULL); + assert(obj); + VmGenIdState *vms = VMGENID(obj); + + /* 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->vgia_le, sizeof(uint32_t), false); +} + +static void vmgenid_update_guest(VmGenIdState *s) +{ + Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL); + uint32_t vgia; + + if (obj) { + /* Write the GUID to guest memory */ + memcpy(&vgia, s->vgia_le, sizeof(vgia)); + vgia = le32_to_cpu(vgia); + if (vgia) { + cpu_physical_memory_write(vgia + VMGENID_GUID_OFFSET, + s->guid.data, sizeof(s->guid.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 *s = VMGENID(obj); + + if (!strncmp(value, "auto", 4)) { + qemu_uuid_generate(&s->guid); + } else if (qemu_uuid_parse(value, &s->guid) < 0) { + error_setg(errp, "'%s. %s': Failed to parse GUID string: %s", + object_get_typename(OBJECT(s)), VMGENID_GUID, value); + return; + } + /* 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, so store that way internally. Make + * sure to swap back whenever reporting via monitor */ + qemu_uuid_bswap(&s->guid); + + /* Send the ACPI notify */ + vmgenid_update_guest(s); +} + +/* 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 *s = opaque; + vmgenid_update_guest(s); + 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(vgia_le, VmGenIdState, sizeof(uint32_t)), + VMSTATE_END_OF_LIST() + }, +}; + +static void vmgenid_initfn(Object *obj) +{ + object_property_add_str(obj, VMGENID_GUID, NULL, vmgenid_set_guid, NULL); +} + +static void vmgenid_device_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->vmsd = &vmstate_vmgenid; +} + +static const TypeInfo vmgenid_device_info = { + .name = VMGENID_DEVICE, + .parent = TYPE_SYS_BUS_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 78a1d84..4c40f76 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" @@ -2653,6 +2654,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) acpi_add_table(table_offsets, tables_blob); build_madt(tables_blob, tables->linker, pcms); + if (find_vmgenid_dev(NULL)) { + acpi_add_table(table_offsets, tables_blob); + vmgenid_build_acpi(tables_blob, tables->vmgenid, tables->linker); + } + if (misc.has_hpet) { acpi_add_table(table_offsets, tables_blob); build_hpet(tables_blob, tables->linker); @@ -2859,6 +2865,10 @@ void acpi_setup(void) fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, tables.tcpalog->data, acpi_data_len(tables.tcpalog)); + if (find_vmgenid_dev(NULL)) { + vmgenid_add_fw_cfg(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..b60437a --- /dev/null +++ b/include/hw/acpi/vmgenid.h @@ -0,0 +1,37 @@ +#ifndef ACPI_VMGENID_H +#define ACPI_VMGENID_H + +#include "hw/acpi/bios-linker-loader.h" +#include "hw/sysbus.h" +#include "qemu/uuid.h" + +#define VMGENID_DEVICE "vmgenid" +#define VMGENID_GUID "guid" +#define VMGENID_GUID_FW_CFG_FILE "etc/vmgenid" +#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 */ + +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid); +void vmgenid_build_acpi(GArray *table_data, GArray *guid, BIOSLinker *linker); + +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE) + +typedef struct VmGenIdState { + SysBusDevice parent_obj; + QemuUUID guid; + uint8_t vgia_le[4]; +} VmGenIdState; + +static Object *find_vmgenid_dev(Error **errp) +{ + Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL); + if (!obj && errp) { + error_setg(errp, "%s is not found", VMGENID_DEVICE); + } + return obj; +} + +#endif