* [Qemu-devel] [PATCH V11 0/3] Virtual Machine Generation ID
@ 2014-12-16 15:50 Gal Hammer
2014-12-16 15:50 ` [Qemu-devel] [PATCH V11 1/3] docs: vm generation id device's description Gal Hammer
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Gal Hammer @ 2014-12-16 15:50 UTC (permalink / raw)
To: qemu-devel; +Cc: Gal Hammer, mst
Hi,
Resending patches after the release of version 2.2 and after I've noticed
that some of the required files are missing.
Please note that this patch set doesn't include the *.hex.generated
files and the binary ACPI tables (make check will fail).
Thanks,
Gal.
V11 - Add required missing files.
V10 - Fixed typos in docs and a few clarification.
V9 - Add a unit test.
- Rebased to version 2.2.
- Removed hex.generated the binary files from patch.
V8 - Add a device's description file.
- GUID is stored in fw cfg file and the guest writes the
physical address to the device (reduces vmexits).
V7 - Move the device's description back to the static SSDT table.
- The GUID is store in a "hard coded" physical address and not
in the ACPI table itself.
- ACPI notification is triggered when the GUID is changed.
V6 - include the pre-compiled ASL file
- remove an empty line at end of files.
V5 - Move device's description to SSDT table (dynamic).
V4 - Fix a typo in error message string.
- Move device's description from DSDT back to SSDT table.
V3 - Remove "-uuid" command line parameter.
- Move device's description from SSDT to DSDT table.
- Add new "vmgenid" sysbus device.
Gal Hammer (3):
docs: vm generation id device's description
i386: Add a Virtual Machine Generation ID device
tests: add a unit test for the vmgenid device.
default-configs/i386-softmmu.mak | 1 +
default-configs/x86_64-softmmu.mak | 1 +
docs/specs/vmgenid.txt | 38 ++++++++++
hw/acpi/core.c | 8 +++
hw/acpi/ich9.c | 8 +++
hw/acpi/piix4.c | 8 +++
hw/i386/acpi-build.c | 26 +++++++
hw/i386/acpi-dsdt.dsl | 4 +-
hw/i386/pc.c | 8 +++
hw/i386/q35-acpi-dsdt.dsl | 5 +-
hw/i386/ssdt-misc.dsl | 43 ++++++++++++
hw/isa/lpc_ich9.c | 1 +
hw/misc/Makefile.objs | 1 +
hw/misc/vmgenid.c | 131 +++++++++++++++++++++++++++++++++++
include/hw/acpi/acpi.h | 2 +
include/hw/acpi/acpi_dev_interface.h | 4 ++
include/hw/acpi/ich9.h | 2 +
include/hw/i386/pc.h | 3 +
include/hw/misc/vmgenid.h | 21 ++++++
tests/Makefile | 2 +
tests/vmgenid-test.c | 48 +++++++++++++
21 files changed, 363 insertions(+), 2 deletions(-)
create mode 100644 docs/specs/vmgenid.txt
create mode 100644 hw/misc/vmgenid.c
create mode 100644 include/hw/misc/vmgenid.h
create mode 100644 tests/vmgenid-test.c
--
2.1.0
^ permalink raw reply [flat|nested] 13+ messages in thread* [Qemu-devel] [PATCH V11 1/3] docs: vm generation id device's description 2014-12-16 15:50 [Qemu-devel] [PATCH V11 0/3] Virtual Machine Generation ID Gal Hammer @ 2014-12-16 15:50 ` Gal Hammer 2014-12-16 15:50 ` [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device Gal Hammer 2014-12-16 15:50 ` [Qemu-devel] [PATCH V11 3/3] tests: add a unit test for the vmgenid device Gal Hammer 2 siblings, 0 replies; 13+ messages in thread From: Gal Hammer @ 2014-12-16 15:50 UTC (permalink / raw) To: qemu-devel; +Cc: Gal Hammer, mst Signed-off-by: Gal Hammer <ghammer@redhat.com> --- docs/specs/vmgenid.txt | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 docs/specs/vmgenid.txt diff --git a/docs/specs/vmgenid.txt b/docs/specs/vmgenid.txt new file mode 100644 index 0000000..656d598 --- /dev/null +++ b/docs/specs/vmgenid.txt @@ -0,0 +1,38 @@ +VIRTUAL MACHINE GENERATION ID +============================= + +Copyright (C) 2014 Red Hat, Inc. + +This work is licensed under the terms of the GNU GPL, version 2 or later. +See the COPYING file in the top-level directory. + +=== + +The VM generation ID (vmgenid) device is an emulated device which +exposes a 128-bit, cryptographically random, integer value identifier. +This allows management applications (e.g. libvirt) to notify the guest +operating system when the virtual machine is executed with a different +configuration (e.g. snapshot execution or creation from a template). + +This is specified on the web at: http://go.microsoft.com/fwlink/?LinkId=260709 + +--- + +The vmgenid device is a sysbus device with the following ACPI ID: +"QEMU0002". + +The device adds a "vmgenid.uuid" property, which can be modified using +the -global command line argument or the QMP interface. + +The device uses a fixed memory resource: 0xfedf0000-0xfedf0003. The +guest is expected to write the physical address of the GUID's buffer +to that memory resource. This allows the device to modify the GUID if +requested by the management application. Current device's implementation +supports a 32-bit address. + +According to the specification, any change to the GUID executes an +ACPI notification. The vmgenid device triggers the GPE._E00 which +executes the ACPI Notify operation. + +Although not specified in Microsoft's document, it is assumed that the +device is expected to use the little-endian system. -- 2.1.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device 2014-12-16 15:50 [Qemu-devel] [PATCH V11 0/3] Virtual Machine Generation ID Gal Hammer 2014-12-16 15:50 ` [Qemu-devel] [PATCH V11 1/3] docs: vm generation id device's description Gal Hammer @ 2014-12-16 15:50 ` Gal Hammer 2015-01-22 13:52 ` Igor Mammedov 2014-12-16 15:50 ` [Qemu-devel] [PATCH V11 3/3] tests: add a unit test for the vmgenid device Gal Hammer 2 siblings, 1 reply; 13+ messages in thread From: Gal Hammer @ 2014-12-16 15:50 UTC (permalink / raw) To: qemu-devel; +Cc: Gal Hammer, mst Based on Microsoft's sepecifications (paper can be dowloaded from http://go.microsoft.com/fwlink/?LinkId=260709), add a device description to the SSDT ACPI table and its implementation. The GUID is set using a global "vmgenid.uuid" parameter. Signed-off-by: Gal Hammer <ghammer@redhat.com> --- default-configs/i386-softmmu.mak | 1 + default-configs/x86_64-softmmu.mak | 1 + hw/acpi/core.c | 8 +++ hw/acpi/ich9.c | 8 +++ hw/acpi/piix4.c | 8 +++ hw/i386/acpi-build.c | 26 +++++++ hw/i386/acpi-dsdt.dsl | 4 +- hw/i386/pc.c | 8 +++ hw/i386/q35-acpi-dsdt.dsl | 5 +- hw/i386/ssdt-misc.dsl | 43 ++++++++++++ hw/isa/lpc_ich9.c | 1 + hw/misc/Makefile.objs | 1 + hw/misc/vmgenid.c | 131 +++++++++++++++++++++++++++++++++++ include/hw/acpi/acpi.h | 2 + include/hw/acpi/acpi_dev_interface.h | 4 ++ include/hw/acpi/ich9.h | 2 + include/hw/i386/pc.h | 3 + include/hw/misc/vmgenid.h | 21 ++++++ 18 files changed, 275 insertions(+), 2 deletions(-) create mode 100644 hw/misc/vmgenid.c create mode 100644 include/hw/misc/vmgenid.h diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak index 8e08841..bd33c75 100644 --- a/default-configs/i386-softmmu.mak +++ b/default-configs/i386-softmmu.mak @@ -45,3 +45,4 @@ CONFIG_IOAPIC=y CONFIG_ICC_BUS=y CONFIG_PVPANIC=y CONFIG_MEM_HOTPLUG=y +CONFIG_VMGENID=y diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak index 66557ac..006fc7c 100644 --- a/default-configs/x86_64-softmmu.mak +++ b/default-configs/x86_64-softmmu.mak @@ -45,3 +45,4 @@ CONFIG_IOAPIC=y CONFIG_ICC_BUS=y CONFIG_PVPANIC=y CONFIG_MEM_HOTPLUG=y +CONFIG_VMGENID=y diff --git a/hw/acpi/core.c b/hw/acpi/core.c index 51913d6..d4597c6 100644 --- a/hw/acpi/core.c +++ b/hw/acpi/core.c @@ -28,6 +28,8 @@ #include "qapi-visit.h" #include "qapi-event.h" +#define ACPI_VM_GENERATION_ID_CHANGED_STATUS 1 + struct acpi_table_header { uint16_t _length; /* our length, not actual part of the hdr */ /* allows easier parsing for fw_cfg clients */ @@ -683,3 +685,9 @@ void acpi_update_sci(ACPIREGS *regs, qemu_irq irq) (regs->pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) && !(pm1a_sts & ACPI_BITMASK_TIMER_STATUS)); } + +void acpi_vm_generation_id_changed(ACPIREGS *acpi_regs, qemu_irq irq) +{ + acpi_regs->gpe.sts[0] |= ACPI_VM_GENERATION_ID_CHANGED_STATUS; + acpi_update_sci(acpi_regs, irq); +} diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c index ea991a3..12a9387 100644 --- a/hw/acpi/ich9.c +++ b/hw/acpi/ich9.c @@ -307,3 +307,11 @@ void ich9_pm_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list) acpi_memory_ospm_status(&s->pm.acpi_memory_hotplug, list); } + +void ich9_vm_generation_id_changed(AcpiDeviceIf *adev) +{ + ICH9LPCState *s = ICH9_LPC_DEVICE(adev); + ICH9LPCPMRegs *pm = &s->pm; + + acpi_vm_generation_id_changed(&pm->acpi_regs, pm->irq); +} diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c index 481a16c..41b6eb6 100644 --- a/hw/acpi/piix4.c +++ b/hw/acpi/piix4.c @@ -574,6 +574,13 @@ static void piix4_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list) acpi_memory_ospm_status(&s->acpi_memory_hotplug, list); } +static void piix4_vm_generation_id_changed(AcpiDeviceIf *adev) +{ + PIIX4PMState *s = PIIX4_PM(adev); + + acpi_vm_generation_id_changed(&s->ar, s->irq); +} + static Property piix4_pm_properties[] = { DEFINE_PROP_UINT32("smb_io_base", PIIX4PMState, smb_io_base, 0), DEFINE_PROP_UINT8(ACPI_PM_PROP_S3_DISABLED, PIIX4PMState, disable_s3, 0), @@ -611,6 +618,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data) hc->plug = piix4_device_plug_cb; hc->unplug_request = piix4_device_unplug_request_cb; adevc->ospm_status = piix4_ospm_status; + adevc->vm_generation_id_changed = piix4_vm_generation_id_changed; } static const TypeInfo piix4_pm_info = { diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index a4d0c0c..f20a6a5 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -257,6 +257,7 @@ static void acpi_get_pci_info(PcPciInfo *info) #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables" #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp" #define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log" +#define ACPI_BUILD_VMGENID_FILE "etc/vm-generation-id" static void build_header(GArray *linker, GArray *table_data, @@ -1068,6 +1069,8 @@ build_ssdt(GArray *table_data, GArray *linker, { MachineState *machine = MACHINE(qdev_get_machine()); uint32_t nr_mem = machine->ram_slots; + uint32_t vm_gid_physical_address; + uint32_t vm_gid_offset = 0; unsigned acpi_cpus = guest_info->apic_id_limit; int ssdt_start = table_data->len; uint8_t *ssdt_ptr; @@ -1096,6 +1099,21 @@ build_ssdt(GArray *table_data, GArray *linker, ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), ssdt_isa_pest[0], 16, misc->pvpanic_port); + if (vm_generation_id_set()) { + vm_gid_physical_address = ssdt_start + ssdt_acpi_vm_gid_addr[0]; + bios_linker_loader_alloc(linker, ACPI_BUILD_VMGENID_FILE, 8, true); + bios_linker_loader_add_pointer(linker, ACPI_BUILD_VMGENID_FILE, + ACPI_BUILD_TABLE_FILE, + table_data, + &vm_gid_offset, + sizeof(vm_gid_offset)); + } else { + vm_gid_physical_address = 0; + } + + ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), + ssdt_acpi_vm_gid_addr[0], 32, vm_gid_physical_address); + ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), ssdt_mctrl_nr_slots[0], 32, nr_mem); @@ -1490,6 +1508,7 @@ struct AcpiBuildTables { GArray *table_data; GArray *rsdp; GArray *tcpalog; + GArray *vmgenid; GArray *linker; } AcpiBuildTables; @@ -1498,6 +1517,7 @@ static inline void acpi_build_tables_init(AcpiBuildTables *tables) tables->rsdp = g_array_new(false, true /* clear */, 1); tables->table_data = g_array_new(false, true /* clear */, 1); tables->tcpalog = g_array_new(false, true /* clear */, 1); + tables->vmgenid = g_array_new(false, true /* clear */, 1); tables->linker = bios_linker_loader_init(); } @@ -1508,6 +1528,7 @@ static inline void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre) g_array_free(tables->rsdp, mfre); g_array_free(tables->table_data, true); g_array_free(tables->tcpalog, mfre); + g_array_free(tables->vmgenid, mfre); } typedef @@ -1790,6 +1811,11 @@ void acpi_setup(PcGuestInfo *guest_info) fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE, tables.tcpalog->data, acpi_data_len(tables.tcpalog)); + /* Add a 128-bit fw cfg file which stores the VM generation id. */ + g_array_set_size(tables.vmgenid, 16); + fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_VMGENID_FILE, + tables.vmgenid->data, tables.vmgenid->len); + /* * RSDP is small so it's easy to keep it immutable, no need to * bother with ROM blobs. diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl index a611e07..5a72820 100644 --- a/hw/i386/acpi-dsdt.dsl +++ b/hw/i386/acpi-dsdt.dsl @@ -306,7 +306,9 @@ DefinitionBlock ( Scope(\_GPE) { Name(_HID, "ACPI0006") - Method(_L00) { + External(\_SB.VMGI, DeviceObj) + Method(_E00) { + Notify(\_SB.VMGI, 0x80) } Method(_E01) { // PCI hotplug event diff --git a/hw/i386/pc.c b/hw/i386/pc.c index f31d55e..c8abe43 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -59,6 +59,7 @@ #include "hw/pci/pci_host.h" #include "acpi-build.h" #include "hw/mem/pc-dimm.h" +#include "hw/misc/vmgenid.h" #include "trace.h" #include "qapi/visitor.h" #include "qapi-visit.h" @@ -1356,6 +1357,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, int i; DriveInfo *fd[MAX_FD]; DeviceState *hpet = NULL; + DeviceState *vmgenid; int pit_isa_irq = 0; qemu_irq pit_alt_irq = NULL; qemu_irq rtc_irq = NULL; @@ -1454,6 +1456,12 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, fd[i] = drive_get(IF_FLOPPY, 0, i); } *floppy = fdctrl_init_isa(isa_bus, fd); + + vmgenid = qdev_try_create(NULL, VMGENID_DEVICE); + if (vmgenid) { + qdev_init_nofail(vmgenid); + sysbus_mmio_map(SYS_BUS_DEVICE(vmgenid), 0, VMGENID_BASE_ADDRESS); + } } void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus) diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl index e1cee5d..acc6e8d 100644 --- a/hw/i386/q35-acpi-dsdt.dsl +++ b/hw/i386/q35-acpi-dsdt.dsl @@ -414,7 +414,10 @@ DefinitionBlock ( Scope(\_GPE) { Name(_HID, "ACPI0006") - Method(_L00) { + External(\_SB.VMGI, DeviceObj) + Method(_E00) + { + Notify(\_SB.VMGI, 0x80) } Method(_L01) { } diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl index 1e3baae..9c7de2c 100644 --- a/hw/i386/ssdt-misc.dsl +++ b/hw/i386/ssdt-misc.dsl @@ -13,6 +13,7 @@ * with this program; if not, see <http://www.gnu.org/licenses/>. */ #include "hw/acpi/pc-hotplug.h" +#include "hw/misc/vmgenid.h" ACPI_EXTRACT_ALL_CODE ssdp_misc_aml @@ -119,4 +120,46 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1) } } } + + Scope(\_SB) { + Device(VMGI) { + Name(_HID, "QEMU0002") + Name(_CID, "VM_Gen_Counter") + Name(_DDN, "VM_Gen_Counter") + + ACPI_EXTRACT_NAME_DWORD_CONST ssdt_acpi_vm_gid_addr + Name(VGIA, 0x12345678) + + OperationRegion(OPRG, SystemMemory, + VMGENID_BASE_ADDRESS, VMGENID_BASE_ADDR_LEN) + Field(OPRG, DWordAcc, NoLock, Preserve) { + PHYA, 32 + } + + Name(_CRS, ResourceTemplate() { + Memory32Fixed(ReadOnly, + VMGENID_BASE_ADDRESS, VMGENID_BASE_ADDR_LEN) + }) + + Method(_INI, 0, NotSerialized) { + Store(VGIA, PHYA) + } + + Method(_STA, 0, NotSerialized) { + Store(VGIA, Local0) + If (LEqual(Local0, Zero)) { + Return (0x00) + } Else { + Return (0x0F) + } + } + + Method(ADDR, 0, Serialized) { + Store(Package(2) { }, Local0) + Store(VGIA, Index(Local0, 0)) + Store(0x0000, Index(Local0, 1)) + return (Local0) + } + } + } } diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c index 530b074..3a012fa 100644 --- a/hw/isa/lpc_ich9.c +++ b/hw/isa/lpc_ich9.c @@ -678,6 +678,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data) hc->plug = ich9_device_plug_cb; hc->unplug_request = ich9_device_unplug_request_cb; adevc->ospm_status = ich9_pm_ospm_status; + adevc->vm_generation_id_changed = ich9_vm_generation_id_changed; } static const TypeInfo ich9_lpc_info = { diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs index 979e532..c18b800 100644 --- a/hw/misc/Makefile.objs +++ b/hw/misc/Makefile.objs @@ -41,3 +41,4 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o obj-$(CONFIG_ZYNQ) += zynq_slcr.o obj-$(CONFIG_PVPANIC) += pvpanic.o +obj-$(CONFIG_VMGENID) += vmgenid.o diff --git a/hw/misc/vmgenid.c b/hw/misc/vmgenid.c new file mode 100644 index 0000000..ff09243 --- /dev/null +++ b/hw/misc/vmgenid.c @@ -0,0 +1,131 @@ +/* + * Virtual Machine Generation ID Device + * + * Copyright (C) 2014 Red Hat Inc. + * + * Authors: Gal Hammer <ghammer@redhat.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ + +#include "hw/i386/pc.h" +#include "hw/sysbus.h" +#include "hw/misc/vmgenid.h" +#include "hw/acpi/acpi_dev_interface.h" + +#define PROPERTY_UUID "uuid" + +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE) + +typedef struct VmGenIdState { + SysBusDevice parent_obj; + MemoryRegion iomem; + hwaddr guid_physical_addr; + uint8_t guid[16]; + bool guid_set; +} VmGenIdState; + +bool vm_generation_id_set(void) +{ + Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL); + VmGenIdState *s = VMGENID(obj); + + if (!obj) { + return false; + } + return s->guid_set; +} + +static void vmgenid_update_guest(VmGenIdState *s) +{ + Object *acpi_obj; + + if (s->guid_physical_addr) { + cpu_physical_memory_write(s->guid_physical_addr, &s->guid, + sizeof(s->guid)); + + acpi_obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL); + if (acpi_obj) { + AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(acpi_obj); + AcpiDeviceIf *adev = ACPI_DEVICE_IF(acpi_obj); + + adevc->vm_generation_id_changed(adev); + } + } +} + +static uint64_t vmgenid_ram_read(void *opaque, hwaddr addr, + unsigned size) +{ + VmGenIdState *s = VMGENID(opaque); + + return s->guid_physical_addr; +} + +static void vmgenid_ram_write(void *opaque, hwaddr addr, uint64_t value, + unsigned size) +{ + VmGenIdState *s = VMGENID(opaque); + + s->guid_physical_addr = value; + vmgenid_update_guest(s); +} + +static const MemoryRegionOps vmgenid_ram_ops = { + .read = vmgenid_ram_read, + .write = vmgenid_ram_write, + .valid = { + .min_access_size = 4, + .max_access_size = 4, + }, + .endianness = DEVICE_NATIVE_ENDIAN, +}; + +static void vmgenid_set_uuid(Object *obj, const char *value, Error **errp) +{ + VmGenIdState *s = VMGENID(obj); + + if (qemu_uuid_parse(value, s->guid) < 0) { + error_setg(errp, "Fail to parse UUID string."); + return; + } + + s->guid_set = true; + vmgenid_update_guest(s); +} + +static void vmgenid_init(Object *obj) +{ + SysBusDevice *sbd = SYS_BUS_DEVICE(obj); + VmGenIdState *s = VMGENID(obj); + + memory_region_init_io(&s->iomem, obj, &vmgenid_ram_ops, s, "vgid", + VMGENID_BASE_ADDR_LEN); + sysbus_init_mmio(sbd, &s->iomem); + + object_property_add_str(obj, PROPERTY_UUID, NULL, vmgenid_set_uuid, NULL); +} + +static void vmgenid_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + set_bit(DEVICE_CATEGORY_MISC, dc->categories); +} + +static const TypeInfo vmgenid_device_info = { + .name = VMGENID_DEVICE, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(VmGenIdState), + .instance_init = vmgenid_init, + .class_init = vmgenid_class_init, +}; + +static void vmgenid_register_types(void) +{ + type_register_static(&vmgenid_device_info); +} + +type_init(vmgenid_register_types) diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h index 1f678b4..9373b4d 100644 --- a/include/hw/acpi/acpi.h +++ b/include/hw/acpi/acpi.h @@ -185,4 +185,6 @@ unsigned acpi_table_len(void *current); void acpi_table_add(const QemuOpts *opts, Error **errp); void acpi_table_add_builtin(const QemuOpts *opts, Error **errp); +void acpi_vm_generation_id_changed(ACPIREGS *acpi_regs, qemu_irq irq); + #endif /* !QEMU_HW_ACPI_H */ diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h index f245f8d..757ce60 100644 --- a/include/hw/acpi/acpi_dev_interface.h +++ b/include/hw/acpi/acpi_dev_interface.h @@ -28,6 +28,9 @@ typedef struct AcpiDeviceIf { * ospm_status: returns status of ACPI device objects, reported * via _OST method if device supports it. * + * vm_generation_id_changed: notify the guest that it generation + * id was changed. + * * Interface is designed for providing unified interface * to generic ACPI functionality that could be used without * knowledge about internals of actual device that implements @@ -39,5 +42,6 @@ typedef struct AcpiDeviceIfClass { /* <public> */ void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list); + void (*vm_generation_id_changed)(AcpiDeviceIf *adev); } AcpiDeviceIfClass; #endif diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h index fe975e6..923e711 100644 --- a/include/hw/acpi/ich9.h +++ b/include/hw/acpi/ich9.h @@ -61,4 +61,6 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp); void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp); void ich9_pm_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list); + +void ich9_vm_generation_id_changed(AcpiDeviceIf *adev); #endif /* HW_ACPI_ICH9_H */ diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 69d9cf8..7e7668b 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -298,6 +298,9 @@ void pc_system_firmware_init(MemoryRegion *rom_memory, /* pvpanic.c */ uint16_t pvpanic_port(void); +/* vmgenid.c */ +bool vm_generation_id_set(void); + /* e820 types */ #define E820_RAM 1 #define E820_RESERVED 2 diff --git a/include/hw/misc/vmgenid.h b/include/hw/misc/vmgenid.h new file mode 100644 index 0000000..7b9a2b2 --- /dev/null +++ b/include/hw/misc/vmgenid.h @@ -0,0 +1,21 @@ +/* + * Virtual Machine Generation ID Device + * + * Copyright (C) 2014 Red Hat Inc. + * + * Authors: Gal Hammer <ghammer@redhat.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ + +#ifndef HW_MISC_VMGENID_H +#define HW_MISC_VMGENID_H + +#define VMGENID_DEVICE "vmgenid" + +#define VMGENID_BASE_ADDRESS 0xfedf0000 +#define VMGENID_BASE_ADDR_LEN 4 + +#endif -- 2.1.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device 2014-12-16 15:50 ` [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device Gal Hammer @ 2015-01-22 13:52 ` Igor Mammedov 2015-01-22 21:21 ` Michael S. Tsirkin 2015-02-01 12:56 ` Gal Hammer 0 siblings, 2 replies; 13+ messages in thread From: Igor Mammedov @ 2015-01-22 13:52 UTC (permalink / raw) To: Gal Hammer; +Cc: qemu-devel, mst On Tue, 16 Dec 2014 17:50:43 +0200 Gal Hammer <ghammer@redhat.com> wrote: > Based on Microsoft's sepecifications (paper can be dowloaded from > http://go.microsoft.com/fwlink/?LinkId=260709), add a device > description to the SSDT ACPI table and its implementation. > > The GUID is set using a global "vmgenid.uuid" parameter. > > Signed-off-by: Gal Hammer <ghammer@redhat.com> > > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -257,6 +257,7 @@ static void acpi_get_pci_info(PcPciInfo *info) > #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables" > #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp" > #define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log" > +#define ACPI_BUILD_VMGENID_FILE "etc/vm-generation-id" > > static void > build_header(GArray *linker, GArray *table_data, > @@ -1068,6 +1069,8 @@ build_ssdt(GArray *table_data, GArray *linker, > { > MachineState *machine = MACHINE(qdev_get_machine()); > uint32_t nr_mem = machine->ram_slots; > + uint32_t vm_gid_physical_address; > + uint32_t vm_gid_offset = 0; > unsigned acpi_cpus = guest_info->apic_id_limit; > int ssdt_start = table_data->len; > uint8_t *ssdt_ptr; > @@ -1096,6 +1099,21 @@ build_ssdt(GArray *table_data, GArray *linker, > ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), > ssdt_isa_pest[0], 16, misc->pvpanic_port); > > + if (vm_generation_id_set()) { > + vm_gid_physical_address = ssdt_start + ssdt_acpi_vm_gid_addr[0]; > + bios_linker_loader_alloc(linker, ACPI_BUILD_VMGENID_FILE, 8, true); > + bios_linker_loader_add_pointer(linker, ACPI_BUILD_VMGENID_FILE, > + ACPI_BUILD_TABLE_FILE, > + table_data, > + &vm_gid_offset, > + sizeof(vm_gid_offset)); could some explain how this pointer magic works, From my weak understanding it seems broken. Lets see: [1] &vm_gid_offset - must be pointer inside of dest_file blob (ACPI_BUILD_VMGENID_FILE) [2] vm_gid_offset - should hold offset of the place inside of src_file (ACPI_BUILD_TABLE_FILE) where to pointer inside of dest_file should point to now: vm_gid_physical_address - holds [2] i.e. offset of VGIA constant in inside SSDT in ACPI_BUILD_TABLE_FILE. > + ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), > + ssdt_acpi_vm_gid_addr[0], 32, vm_gid_physical_address); Then we write this offset into VGIA in ACPI_BUILD_TABLE_FILE. After BIOS loads tables it's going to patch at [3] ACPI_BUILD_VMGENID_FILE + (&vm_gid_offset - table_data->data) /* only god knows where it will be/ and on top of it write in it value: *(ACPI_BUILD_TABLE_FILE + *[3]) This approach in general of patching arbitrary place in AML blob to get PHY addr of buffer with UUID, is quite a hack, especially in light of that we are trying to hide all direct access to AML blobs with related pointer arithmetic and manual patching. Why not reserve some potion of RAM and pass to BIOS/guest a reservation so it won't be part of AddressRangeMemory or AddressRangeACPI as MS spec requires? Then you won't need jump all above hoops to just get buffer's PHY addr. > [...] > typedef > @@ -1790,6 +1811,11 @@ void acpi_setup(PcGuestInfo *guest_info) > fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE, > tables.tcpalog->data, acpi_data_len(tables.tcpalog)); > > + /* Add a 128-bit fw cfg file which stores the VM generation id. */ > + g_array_set_size(tables.vmgenid, 16); > + fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_VMGENID_FILE, > + tables.vmgenid->data, tables.vmgenid->len); shouldn't it be migratable? /i.e. acpi_add_rom_blob(...)/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device 2015-01-22 13:52 ` Igor Mammedov @ 2015-01-22 21:21 ` Michael S. Tsirkin 2015-02-01 12:56 ` Gal Hammer 1 sibling, 0 replies; 13+ messages in thread From: Michael S. Tsirkin @ 2015-01-22 21:21 UTC (permalink / raw) To: Igor Mammedov; +Cc: Gal Hammer, qemu-devel On Thu, Jan 22, 2015 at 02:52:46PM +0100, Igor Mammedov wrote: > On Tue, 16 Dec 2014 17:50:43 +0200 > Gal Hammer <ghammer@redhat.com> wrote: > > > Based on Microsoft's sepecifications (paper can be dowloaded from > > http://go.microsoft.com/fwlink/?LinkId=260709), add a device > > description to the SSDT ACPI table and its implementation. > > > > The GUID is set using a global "vmgenid.uuid" parameter. > > > > Signed-off-by: Gal Hammer <ghammer@redhat.com> > > > > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -257,6 +257,7 @@ static void acpi_get_pci_info(PcPciInfo *info) > > #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables" > > #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp" > > #define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log" > > +#define ACPI_BUILD_VMGENID_FILE "etc/vm-generation-id" > > > > static void > > build_header(GArray *linker, GArray *table_data, > > @@ -1068,6 +1069,8 @@ build_ssdt(GArray *table_data, GArray *linker, > > { > > MachineState *machine = MACHINE(qdev_get_machine()); > > uint32_t nr_mem = machine->ram_slots; > > + uint32_t vm_gid_physical_address; > > + uint32_t vm_gid_offset = 0; > > unsigned acpi_cpus = guest_info->apic_id_limit; > > int ssdt_start = table_data->len; > > uint8_t *ssdt_ptr; > > @@ -1096,6 +1099,21 @@ build_ssdt(GArray *table_data, GArray *linker, > > ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), > > ssdt_isa_pest[0], 16, misc->pvpanic_port); > > > > + if (vm_generation_id_set()) { > > + vm_gid_physical_address = ssdt_start + ssdt_acpi_vm_gid_addr[0]; > > + bios_linker_loader_alloc(linker, ACPI_BUILD_VMGENID_FILE, 8, true); > > + bios_linker_loader_add_pointer(linker, ACPI_BUILD_VMGENID_FILE, > > + ACPI_BUILD_TABLE_FILE, > > + table_data, > > + &vm_gid_offset, > > + sizeof(vm_gid_offset)); > could some explain how this pointer magic works, > From my weak understanding it seems broken. > Lets see: > > [1] &vm_gid_offset - must be pointer inside of dest_file blob (ACPI_BUILD_VMGENID_FILE) > [2] vm_gid_offset - should hold offset of the place inside of src_file > (ACPI_BUILD_TABLE_FILE) where to pointer inside of dest_file should point to > > now: > vm_gid_physical_address - holds [2] i.e. offset of VGIA constant in inside SSDT in ACPI_BUILD_TABLE_FILE. > > > + ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), > > + ssdt_acpi_vm_gid_addr[0], 32, vm_gid_physical_address); > Then we write this offset into VGIA in ACPI_BUILD_TABLE_FILE. > > After BIOS loads tables it's going to patch at > [3] ACPI_BUILD_VMGENID_FILE + (&vm_gid_offset - table_data->data) /* only god knows where it will be/ > > and on top of it write in it value: > *(ACPI_BUILD_TABLE_FILE + *[3]) Too late today - I'll need to re-review this, will do next week. > This approach in general of patching arbitrary place in AML blob But I think this isn't very different from other pointers we have though. > to get PHY addr of buffer with UUID, is quite a hack, especially > in light of that we are trying to hide all direct access to AML > blobs with related pointer arithmetic and manual patching. > > Why not reserve some potion of RAM and pass to BIOS/guest > a reservation so it won't be part of AddressRangeMemory or > AddressRangeACPI as MS spec requires? Then you won't need > jump all above hoops to just get buffer's PHY addr. Yea, look at the pain we are in each time we try. Allocating by guest seems cleaner. > > > [...] > > typedef > > @@ -1790,6 +1811,11 @@ void acpi_setup(PcGuestInfo *guest_info) > > fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE, > > tables.tcpalog->data, acpi_data_len(tables.tcpalog)); > > > > + /* Add a 128-bit fw cfg file which stores the VM generation id. */ > > + g_array_set_size(tables.vmgenid, 16); > > + fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_VMGENID_FILE, > > + tables.vmgenid->data, tables.vmgenid->len); > shouldn't it be migratable? /i.e. acpi_add_rom_blob(...)/ Not necessarily because there's nothing there: it's only purpose in life is to make guest allocate and zero out memory. -- MST ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device 2015-01-22 13:52 ` Igor Mammedov 2015-01-22 21:21 ` Michael S. Tsirkin @ 2015-02-01 12:56 ` Gal Hammer 2015-02-02 12:46 ` Igor Mammedov 1 sibling, 1 reply; 13+ messages in thread From: Gal Hammer @ 2015-02-01 12:56 UTC (permalink / raw) To: Igor Mammedov; +Cc: qemu-devel, mst On 22/01/2015 15:52, Igor Mammedov wrote: > On Tue, 16 Dec 2014 17:50:43 +0200 > Gal Hammer <ghammer@redhat.com> wrote: > >> Based on Microsoft's sepecifications (paper can be dowloaded from >> http://go.microsoft.com/fwlink/?LinkId=260709), add a device >> description to the SSDT ACPI table and its implementation. >> >> The GUID is set using a global "vmgenid.uuid" parameter. >> >> Signed-off-by: Gal Hammer <ghammer@redhat.com> >> > >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -257,6 +257,7 @@ static void acpi_get_pci_info(PcPciInfo *info) >> #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables" >> #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp" >> #define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log" >> +#define ACPI_BUILD_VMGENID_FILE "etc/vm-generation-id" >> >> static void >> build_header(GArray *linker, GArray *table_data, >> @@ -1068,6 +1069,8 @@ build_ssdt(GArray *table_data, GArray *linker, >> { >> MachineState *machine = MACHINE(qdev_get_machine()); >> uint32_t nr_mem = machine->ram_slots; >> + uint32_t vm_gid_physical_address; >> + uint32_t vm_gid_offset = 0; >> unsigned acpi_cpus = guest_info->apic_id_limit; >> int ssdt_start = table_data->len; >> uint8_t *ssdt_ptr; >> @@ -1096,6 +1099,21 @@ build_ssdt(GArray *table_data, GArray *linker, >> ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), >> ssdt_isa_pest[0], 16, misc->pvpanic_port); >> >> + if (vm_generation_id_set()) { >> + vm_gid_physical_address = ssdt_start + ssdt_acpi_vm_gid_addr[0]; >> + bios_linker_loader_alloc(linker, ACPI_BUILD_VMGENID_FILE, 8, true); >> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_VMGENID_FILE, >> + ACPI_BUILD_TABLE_FILE, >> + table_data, >> + &vm_gid_offset, >> + sizeof(vm_gid_offset)); > could some explain how this pointer magic works, I can try, but don't you think that a magic is gone once explained? ;-) > From my weak understanding it seems broken. > Lets see: > > [1] &vm_gid_offset - must be pointer inside of dest_file blob (ACPI_BUILD_VMGENID_FILE) > [2] vm_gid_offset - should hold offset of the place inside of src_file > (ACPI_BUILD_TABLE_FILE) where to pointer inside of dest_file should point to The vm_gid_offset should point where in the ACPI_BUILD_VMGENID_FILE the VM's GUID is stored. At the moment, it should always be zero because the GUID is stored at the begging of the ACPI_BUILD_VMGENID_FILE. > > now: > vm_gid_physical_address - holds [2] i.e. offset of VGIA constant in inside SSDT in ACPI_BUILD_TABLE_FILE. > >> + ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), >> + ssdt_acpi_vm_gid_addr[0], 32, vm_gid_physical_address); > Then we write this offset into VGIA in ACPI_BUILD_TABLE_FILE. Yes. This offset is later patched by the linker to the full physical address. > After BIOS loads tables it's going to patch at > [3] ACPI_BUILD_VMGENID_FILE + (&vm_gid_offset - table_data->data) /* only god knows where it will be/ > > and on top of it write in it value: > *(ACPI_BUILD_TABLE_FILE + *[3]) We know exactly where it is, no need to call for god's help :-). > This approach in general of patching arbitrary place in AML blob > to get PHY addr of buffer with UUID, is quite a hack, especially > in light of that we are trying to hide all direct access to AML > blobs with related pointer arithmetic and manual patching. > > Why not reserve some potion of RAM and pass to BIOS/guest > a reservation so it won't be part of AddressRangeMemory or > AddressRangeACPI as MS spec requires? Then you won't need > jump all above hoops to just get buffer's PHY addr. I'll be glad to hear a new idea that I didn't already try in one of other previous patches. The problem is that the specification requires working with a physical address, so it must be allocated from inside the guest. Since the OS is not exist in this stage and I also don't want to write a special driver just to allocate this buffer I had to choose this approach. >> > [...] >> typedef >> @@ -1790,6 +1811,11 @@ void acpi_setup(PcGuestInfo *guest_info) >> fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE, >> tables.tcpalog->data, acpi_data_len(tables.tcpalog)); >> >> + /* Add a 128-bit fw cfg file which stores the VM generation id. */ >> + g_array_set_size(tables.vmgenid, 16); >> + fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_VMGENID_FILE, >> + tables.vmgenid->data, tables.vmgenid->len); > shouldn't it be migratable? /i.e. acpi_add_rom_blob(...)/ > I'm not too familiar with the migration process, but I assume that this memory will be copied as part of the guest memory. Gal. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device 2015-02-01 12:56 ` Gal Hammer @ 2015-02-02 12:46 ` Igor Mammedov 2015-02-02 13:13 ` Gal Hammer 0 siblings, 1 reply; 13+ messages in thread From: Igor Mammedov @ 2015-02-02 12:46 UTC (permalink / raw) To: Gal Hammer; +Cc: qemu-devel, mst On Sun, 01 Feb 2015 14:56:26 +0200 Gal Hammer <ghammer@redhat.com> wrote: > On 22/01/2015 15:52, Igor Mammedov wrote: > > On Tue, 16 Dec 2014 17:50:43 +0200 > > Gal Hammer <ghammer@redhat.com> wrote: > > > >> Based on Microsoft's sepecifications (paper can be dowloaded from > >> http://go.microsoft.com/fwlink/?LinkId=260709), add a device > >> description to the SSDT ACPI table and its implementation. > >> > >> The GUID is set using a global "vmgenid.uuid" parameter. > >> > >> Signed-off-by: Gal Hammer <ghammer@redhat.com> > >> > > > >> --- a/hw/i386/acpi-build.c > >> +++ b/hw/i386/acpi-build.c > >> @@ -257,6 +257,7 @@ static void acpi_get_pci_info(PcPciInfo *info) > >> #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables" > >> #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp" > >> #define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log" > >> +#define ACPI_BUILD_VMGENID_FILE "etc/vm-generation-id" > >> > >> static void > >> build_header(GArray *linker, GArray *table_data, > >> @@ -1068,6 +1069,8 @@ build_ssdt(GArray *table_data, GArray *linker, > >> { > >> MachineState *machine = MACHINE(qdev_get_machine()); > >> uint32_t nr_mem = machine->ram_slots; > >> + uint32_t vm_gid_physical_address; > >> + uint32_t vm_gid_offset = 0; > >> unsigned acpi_cpus = guest_info->apic_id_limit; > >> int ssdt_start = table_data->len; > >> uint8_t *ssdt_ptr; > >> @@ -1096,6 +1099,21 @@ build_ssdt(GArray *table_data, GArray *linker, > >> ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), > >> ssdt_isa_pest[0], 16, misc->pvpanic_port); > >> > >> + if (vm_generation_id_set()) { > >> + vm_gid_physical_address = ssdt_start + ssdt_acpi_vm_gid_addr[0]; > >> + bios_linker_loader_alloc(linker, ACPI_BUILD_VMGENID_FILE, 8, true); > >> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_VMGENID_FILE, > >> + ACPI_BUILD_TABLE_FILE, > >> + table_data, > >> + &vm_gid_offset, > >> + sizeof(vm_gid_offset)); > > could some explain how this pointer magic works, > > I can try, but don't you think that a magic is gone once explained? ;-) > > > From my weak understanding it seems broken. > > Lets see: > > > > [1] &vm_gid_offset - must be pointer inside of dest_file blob (ACPI_BUILD_VMGENID_FILE) > > [2] vm_gid_offset - should hold offset of the place inside of src_file > > (ACPI_BUILD_TABLE_FILE) where to pointer inside of dest_file should point to > > The vm_gid_offset should point where in the ACPI_BUILD_VMGENID_FILE the > VM's GUID is stored. At the moment, it should always be zero because the > GUID is stored at the begging of the ACPI_BUILD_VMGENID_FILE. > > > > > now: > > vm_gid_physical_address - holds [2] i.e. offset of VGIA constant in inside SSDT in ACPI_BUILD_TABLE_FILE. > > > >> + ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), > >> + ssdt_acpi_vm_gid_addr[0], 32, vm_gid_physical_address); > > Then we write this offset into VGIA in ACPI_BUILD_TABLE_FILE. > > Yes. This offset is later patched by the linker to the full physical > address. > > > After BIOS loads tables it's going to patch at > > [3] ACPI_BUILD_VMGENID_FILE + (&vm_gid_offset - table_data->data) /* only god knows where it will be/ > > > > and on top of it write in it value: > > *(ACPI_BUILD_TABLE_FILE + *[3]) > > We know exactly where it is, no need to call for god's help :-). > > > This approach in general of patching arbitrary place in AML blob > > to get PHY addr of buffer with UUID, is quite a hack, especially > > in light of that we are trying to hide all direct access to AML > > blobs with related pointer arithmetic and manual patching. > > > > Why not reserve some potion of RAM and pass to BIOS/guest > > a reservation so it won't be part of AddressRangeMemory or > > AddressRangeACPI as MS spec requires? Then you won't need > > jump all above hoops to just get buffer's PHY addr. > > I'll be glad to hear a new idea that I didn't already try in one of > other previous patches. The problem is that the specification requires > working with a physical address, so it must be allocated from inside the > guest. Since the OS is not exist in this stage and I also don't want to > write a special driver just to allocate this buffer I had to choose this > approach. how about creating device which will map 4K MMIO region in PCI hole address space and passing it as a reservation via e820 table we have in QEMU. Then address could be directly built in ACPI tables as constant value at the time of ACPI tables creation. That way it would be possible to get address of buffer without firmware + guest OS doing anything and going through quite complex chain for getting buffer address (qemu->bios->OSPM->qemu). If you go current route, it would be needed to teach linker a new command to make reservation in E820 so that allocated buffer won't be part of of AddressRangeMemory as required by spec or anything else. Which would make already hard to understand/use correctly linker API even more complex. > > >> > > [...] > >> typedef > >> @@ -1790,6 +1811,11 @@ void acpi_setup(PcGuestInfo *guest_info) > >> fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE, > >> tables.tcpalog->data, acpi_data_len(tables.tcpalog)); > >> > >> + /* Add a 128-bit fw cfg file which stores the VM generation id. */ > >> + g_array_set_size(tables.vmgenid, 16); > >> + fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_VMGENID_FILE, > >> + tables.vmgenid->data, tables.vmgenid->len); > > shouldn't it be migratable? /i.e. acpi_add_rom_blob(...)/ > > > > I'm not too familiar with the migration process, but I assume that this > memory will be copied as part of the guest memory. > > Gal. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device 2015-02-02 12:46 ` Igor Mammedov @ 2015-02-02 13:13 ` Gal Hammer 2015-02-02 13:55 ` Igor Mammedov 0 siblings, 1 reply; 13+ messages in thread From: Gal Hammer @ 2015-02-02 13:13 UTC (permalink / raw) To: Igor Mammedov; +Cc: qemu-devel, mst On 02/02/2015 14:46, Igor Mammedov wrote: > On Sun, 01 Feb 2015 14:56:26 +0200 > Gal Hammer <ghammer@redhat.com> wrote: > >> On 22/01/2015 15:52, Igor Mammedov wrote: >>> On Tue, 16 Dec 2014 17:50:43 +0200 >>> Gal Hammer <ghammer@redhat.com> wrote: >>> >>>> Based on Microsoft's sepecifications (paper can be dowloaded from >>>> http://go.microsoft.com/fwlink/?LinkId=260709), add a device >>>> description to the SSDT ACPI table and its implementation. >>>> >>>> The GUID is set using a global "vmgenid.uuid" parameter. >>>> >>>> Signed-off-by: Gal Hammer <ghammer@redhat.com> >>>> >>> >>>> --- a/hw/i386/acpi-build.c >>>> +++ b/hw/i386/acpi-build.c >>>> @@ -257,6 +257,7 @@ static void acpi_get_pci_info(PcPciInfo *info) >>>> #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables" >>>> #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp" >>>> #define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log" >>>> +#define ACPI_BUILD_VMGENID_FILE "etc/vm-generation-id" >>>> >>>> static void >>>> build_header(GArray *linker, GArray *table_data, >>>> @@ -1068,6 +1069,8 @@ build_ssdt(GArray *table_data, GArray *linker, >>>> { >>>> MachineState *machine = MACHINE(qdev_get_machine()); >>>> uint32_t nr_mem = machine->ram_slots; >>>> + uint32_t vm_gid_physical_address; >>>> + uint32_t vm_gid_offset = 0; >>>> unsigned acpi_cpus = guest_info->apic_id_limit; >>>> int ssdt_start = table_data->len; >>>> uint8_t *ssdt_ptr; >>>> @@ -1096,6 +1099,21 @@ build_ssdt(GArray *table_data, GArray *linker, >>>> ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), >>>> ssdt_isa_pest[0], 16, misc->pvpanic_port); >>>> >>>> + if (vm_generation_id_set()) { >>>> + vm_gid_physical_address = ssdt_start + ssdt_acpi_vm_gid_addr[0]; >>>> + bios_linker_loader_alloc(linker, ACPI_BUILD_VMGENID_FILE, 8, true); >>>> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_VMGENID_FILE, >>>> + ACPI_BUILD_TABLE_FILE, >>>> + table_data, >>>> + &vm_gid_offset, >>>> + sizeof(vm_gid_offset)); >>> could some explain how this pointer magic works, >> >> I can try, but don't you think that a magic is gone once explained? ;-) >> >>> From my weak understanding it seems broken. >>> Lets see: >>> >>> [1] &vm_gid_offset - must be pointer inside of dest_file blob (ACPI_BUILD_VMGENID_FILE) >>> [2] vm_gid_offset - should hold offset of the place inside of src_file >>> (ACPI_BUILD_TABLE_FILE) where to pointer inside of dest_file should point to >> >> The vm_gid_offset should point where in the ACPI_BUILD_VMGENID_FILE the >> VM's GUID is stored. At the moment, it should always be zero because the >> GUID is stored at the begging of the ACPI_BUILD_VMGENID_FILE. >> >>> >>> now: >>> vm_gid_physical_address - holds [2] i.e. offset of VGIA constant in inside SSDT in ACPI_BUILD_TABLE_FILE. >>> >>>> + ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), >>>> + ssdt_acpi_vm_gid_addr[0], 32, vm_gid_physical_address); >>> Then we write this offset into VGIA in ACPI_BUILD_TABLE_FILE. >> >> Yes. This offset is later patched by the linker to the full physical >> address. >> >>> After BIOS loads tables it's going to patch at >>> [3] ACPI_BUILD_VMGENID_FILE + (&vm_gid_offset - table_data->data) /* only god knows where it will be/ >>> >>> and on top of it write in it value: >>> *(ACPI_BUILD_TABLE_FILE + *[3]) >> >> We know exactly where it is, no need to call for god's help :-). >> >>> This approach in general of patching arbitrary place in AML blob >>> to get PHY addr of buffer with UUID, is quite a hack, especially >>> in light of that we are trying to hide all direct access to AML >>> blobs with related pointer arithmetic and manual patching. >>> >>> Why not reserve some potion of RAM and pass to BIOS/guest >>> a reservation so it won't be part of AddressRangeMemory or >>> AddressRangeACPI as MS spec requires? Then you won't need >>> jump all above hoops to just get buffer's PHY addr. >> >> I'll be glad to hear a new idea that I didn't already try in one of >> other previous patches. The problem is that the specification requires >> working with a physical address, so it must be allocated from inside the >> guest. Since the OS is not exist in this stage and I also don't want to >> write a special driver just to allocate this buffer I had to choose this >> approach. > how about creating device which will map 4K MMIO region in PCI hole > address space and passing it as a reservation via e820 table we have in QEMU. > Then address could be directly built in ACPI tables as constant value > at the time of ACPI tables creation. Isn't this will cause a VMEXIT when the guest is reading the GUID? If it is then this idea was already presented and Michael didn't approve it. > That way it would be possible to get address of buffer without > firmware + guest OS doing anything and going through quite complex > chain for getting buffer address (qemu->bios->OSPM->qemu). > If you go current route, it would be needed to teach linker a new command > to make reservation in E820 so that allocated buffer won't be part of > of AddressRangeMemory as required by spec or anything else. > Which would make already hard to understand/use correctly linker API > even more complex. > > >> >>>> >>> [...] >>>> typedef >>>> @@ -1790,6 +1811,11 @@ void acpi_setup(PcGuestInfo *guest_info) >>>> fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE, >>>> tables.tcpalog->data, acpi_data_len(tables.tcpalog)); >>>> >>>> + /* Add a 128-bit fw cfg file which stores the VM generation id. */ >>>> + g_array_set_size(tables.vmgenid, 16); >>>> + fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_VMGENID_FILE, >>>> + tables.vmgenid->data, tables.vmgenid->len); >>> shouldn't it be migratable? /i.e. acpi_add_rom_blob(...)/ >>> >> >> I'm not too familiar with the migration process, but I assume that this >> memory will be copied as part of the guest memory. >> >> Gal. >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device 2015-02-02 13:13 ` Gal Hammer @ 2015-02-02 13:55 ` Igor Mammedov 2015-02-04 15:09 ` Gal Hammer 0 siblings, 1 reply; 13+ messages in thread From: Igor Mammedov @ 2015-02-02 13:55 UTC (permalink / raw) To: Gal Hammer; +Cc: qemu-devel, mst On Mon, 02 Feb 2015 15:13:39 +0200 Gal Hammer <ghammer@redhat.com> wrote: > On 02/02/2015 14:46, Igor Mammedov wrote: > > On Sun, 01 Feb 2015 14:56:26 +0200 > > Gal Hammer <ghammer@redhat.com> wrote: > > > >> On 22/01/2015 15:52, Igor Mammedov wrote: > >>> On Tue, 16 Dec 2014 17:50:43 +0200 > >>> Gal Hammer <ghammer@redhat.com> wrote: > >>> > >>>> Based on Microsoft's sepecifications (paper can be dowloaded from > >>>> http://go.microsoft.com/fwlink/?LinkId=260709), add a device > >>>> description to the SSDT ACPI table and its implementation. > >>>> > >>>> The GUID is set using a global "vmgenid.uuid" parameter. > >>>> > >>>> Signed-off-by: Gal Hammer <ghammer@redhat.com> > >>>> > >>> > >>>> --- a/hw/i386/acpi-build.c > >>>> +++ b/hw/i386/acpi-build.c > >>>> @@ -257,6 +257,7 @@ static void acpi_get_pci_info(PcPciInfo *info) > >>>> #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables" > >>>> #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp" > >>>> #define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log" > >>>> +#define ACPI_BUILD_VMGENID_FILE "etc/vm-generation-id" > >>>> > >>>> static void > >>>> build_header(GArray *linker, GArray *table_data, > >>>> @@ -1068,6 +1069,8 @@ build_ssdt(GArray *table_data, GArray *linker, > >>>> { > >>>> MachineState *machine = MACHINE(qdev_get_machine()); > >>>> uint32_t nr_mem = machine->ram_slots; > >>>> + uint32_t vm_gid_physical_address; > >>>> + uint32_t vm_gid_offset = 0; > >>>> unsigned acpi_cpus = guest_info->apic_id_limit; > >>>> int ssdt_start = table_data->len; > >>>> uint8_t *ssdt_ptr; > >>>> @@ -1096,6 +1099,21 @@ build_ssdt(GArray *table_data, GArray *linker, > >>>> ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), > >>>> ssdt_isa_pest[0], 16, misc->pvpanic_port); > >>>> > >>>> + if (vm_generation_id_set()) { > >>>> + vm_gid_physical_address = ssdt_start + ssdt_acpi_vm_gid_addr[0]; > >>>> + bios_linker_loader_alloc(linker, ACPI_BUILD_VMGENID_FILE, 8, true); > >>>> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_VMGENID_FILE, > >>>> + ACPI_BUILD_TABLE_FILE, > >>>> + table_data, > >>>> + &vm_gid_offset, > >>>> + sizeof(vm_gid_offset)); > >>> could some explain how this pointer magic works, > >> > >> I can try, but don't you think that a magic is gone once explained? ;-) > >> > >>> From my weak understanding it seems broken. > >>> Lets see: > >>> > >>> [1] &vm_gid_offset - must be pointer inside of dest_file blob (ACPI_BUILD_VMGENID_FILE) > >>> [2] vm_gid_offset - should hold offset of the place inside of src_file > >>> (ACPI_BUILD_TABLE_FILE) where to pointer inside of dest_file should point to > >> > >> The vm_gid_offset should point where in the ACPI_BUILD_VMGENID_FILE the > >> VM's GUID is stored. At the moment, it should always be zero because the > >> GUID is stored at the begging of the ACPI_BUILD_VMGENID_FILE. > >> > >>> > >>> now: > >>> vm_gid_physical_address - holds [2] i.e. offset of VGIA constant in inside SSDT in ACPI_BUILD_TABLE_FILE. > >>> > >>>> + ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), > >>>> + ssdt_acpi_vm_gid_addr[0], 32, vm_gid_physical_address); > >>> Then we write this offset into VGIA in ACPI_BUILD_TABLE_FILE. > >> > >> Yes. This offset is later patched by the linker to the full physical > >> address. > >> > >>> After BIOS loads tables it's going to patch at > >>> [3] ACPI_BUILD_VMGENID_FILE + (&vm_gid_offset - table_data->data) /* only god knows where it will be/ > >>> > >>> and on top of it write in it value: > >>> *(ACPI_BUILD_TABLE_FILE + *[3]) > >> > >> We know exactly where it is, no need to call for god's help :-). > >> > >>> This approach in general of patching arbitrary place in AML blob > >>> to get PHY addr of buffer with UUID, is quite a hack, especially > >>> in light of that we are trying to hide all direct access to AML > >>> blobs with related pointer arithmetic and manual patching. > >>> > >>> Why not reserve some potion of RAM and pass to BIOS/guest > >>> a reservation so it won't be part of AddressRangeMemory or > >>> AddressRangeACPI as MS spec requires? Then you won't need > >>> jump all above hoops to just get buffer's PHY addr. > >> > >> I'll be glad to hear a new idea that I didn't already try in one of > >> other previous patches. The problem is that the specification requires > >> working with a physical address, so it must be allocated from inside the > >> guest. Since the OS is not exist in this stage and I also don't want to > >> write a special driver just to allocate this buffer I had to choose this > >> approach. > > how about creating device which will map 4K MMIO region in PCI hole > > address space and passing it as a reservation via e820 table we have in QEMU. > > Then address could be directly built in ACPI tables as constant value > > at the time of ACPI tables creation. > > Isn't this will cause a VMEXIT when the guest is reading the GUID? If it > is then this idea was already presented and Michael didn't approve it. It will, but is it performance critical? VM supposed to read it at start-up and on getting notification. So I think VMEXIT in this case is not sufficient to drop simple and strait-forward design. BTW: For start-up fw_cfg file is not any way better, it's also causes VMEXIT for every byte it reads from it. > > > That way it would be possible to get address of buffer without > > firmware + guest OS doing anything and going through quite complex > > chain for getting buffer address (qemu->bios->OSPM->qemu). > > If you go current route, it would be needed to teach linker a new command > > to make reservation in E820 so that allocated buffer won't be part of > > of AddressRangeMemory as required by spec or anything else. > > Which would make already hard to understand/use correctly linker API > > even more complex. > > > > > >> > >>>> > >>> [...] > >>>> typedef > >>>> @@ -1790,6 +1811,11 @@ void acpi_setup(PcGuestInfo *guest_info) > >>>> fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE, > >>>> tables.tcpalog->data, acpi_data_len(tables.tcpalog)); > >>>> > >>>> + /* Add a 128-bit fw cfg file which stores the VM generation id. */ > >>>> + g_array_set_size(tables.vmgenid, 16); > >>>> + fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_VMGENID_FILE, > >>>> + tables.vmgenid->data, tables.vmgenid->len); > >>> shouldn't it be migratable? /i.e. acpi_add_rom_blob(...)/ > >>> > >> > >> I'm not too familiar with the migration process, but I assume that this > >> memory will be copied as part of the guest memory. > >> > >> Gal. > >> > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device 2015-02-02 13:55 ` Igor Mammedov @ 2015-02-04 15:09 ` Gal Hammer 2015-02-04 15:53 ` Igor Mammedov 0 siblings, 1 reply; 13+ messages in thread From: Gal Hammer @ 2015-02-04 15:09 UTC (permalink / raw) To: Igor Mammedov; +Cc: qemu-devel, mst Hi Igor, ----- Original Message ----- > From: "Igor Mammedov" <imammedo@redhat.com> > To: "Gal Hammer" <ghammer@redhat.com> > Cc: qemu-devel@nongnu.org, mst@redhat.com > Sent: Monday, February 2, 2015 3:55:02 PM > Subject: Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device > > On Mon, 02 Feb 2015 15:13:39 +0200 > Gal Hammer <ghammer@redhat.com> wrote: > > > On 02/02/2015 14:46, Igor Mammedov wrote: > > > On Sun, 01 Feb 2015 14:56:26 +0200 > > > Gal Hammer <ghammer@redhat.com> wrote: > > > > > >> On 22/01/2015 15:52, Igor Mammedov wrote: > > >>> On Tue, 16 Dec 2014 17:50:43 +0200 > > >>> Gal Hammer <ghammer@redhat.com> wrote: > > >>> > > >>>> Based on Microsoft's sepecifications (paper can be dowloaded from > > >>>> http://go.microsoft.com/fwlink/?LinkId=260709), add a device > > >>>> description to the SSDT ACPI table and its implementation. > > >>>> > > >>>> The GUID is set using a global "vmgenid.uuid" parameter. > > >>>> > > >>>> Signed-off-by: Gal Hammer <ghammer@redhat.com> > > >>>> > > >>> > > >>>> --- a/hw/i386/acpi-build.c > > >>>> +++ b/hw/i386/acpi-build.c > > >>>> @@ -257,6 +257,7 @@ static void acpi_get_pci_info(PcPciInfo *info) > > >>>> #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables" > > >>>> #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp" > > >>>> #define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log" > > >>>> +#define ACPI_BUILD_VMGENID_FILE "etc/vm-generation-id" > > >>>> > > >>>> static void > > >>>> build_header(GArray *linker, GArray *table_data, > > >>>> @@ -1068,6 +1069,8 @@ build_ssdt(GArray *table_data, GArray *linker, > > >>>> { > > >>>> MachineState *machine = MACHINE(qdev_get_machine()); > > >>>> uint32_t nr_mem = machine->ram_slots; > > >>>> + uint32_t vm_gid_physical_address; > > >>>> + uint32_t vm_gid_offset = 0; > > >>>> unsigned acpi_cpus = guest_info->apic_id_limit; > > >>>> int ssdt_start = table_data->len; > > >>>> uint8_t *ssdt_ptr; > > >>>> @@ -1096,6 +1099,21 @@ build_ssdt(GArray *table_data, GArray *linker, > > >>>> ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), > > >>>> ssdt_isa_pest[0], 16, misc->pvpanic_port); > > >>>> > > >>>> + if (vm_generation_id_set()) { > > >>>> + vm_gid_physical_address = ssdt_start + > > >>>> ssdt_acpi_vm_gid_addr[0]; > > >>>> + bios_linker_loader_alloc(linker, ACPI_BUILD_VMGENID_FILE, 8, > > >>>> true); > > >>>> + bios_linker_loader_add_pointer(linker, > > >>>> ACPI_BUILD_VMGENID_FILE, > > >>>> + ACPI_BUILD_TABLE_FILE, > > >>>> + table_data, > > >>>> + &vm_gid_offset, > > >>>> + sizeof(vm_gid_offset)); > > >>> could some explain how this pointer magic works, > > >> > > >> I can try, but don't you think that a magic is gone once explained? ;-) > > >> > > >>> From my weak understanding it seems broken. > > >>> Lets see: > > >>> > > >>> [1] &vm_gid_offset - must be pointer inside of dest_file blob > > >>> (ACPI_BUILD_VMGENID_FILE) > > >>> [2] vm_gid_offset - should hold offset of the place inside of > > >>> src_file > > >>> (ACPI_BUILD_TABLE_FILE) where to pointer inside > > >>> of dest_file should point to > > >> > > >> The vm_gid_offset should point where in the ACPI_BUILD_VMGENID_FILE the > > >> VM's GUID is stored. At the moment, it should always be zero because the > > >> GUID is stored at the begging of the ACPI_BUILD_VMGENID_FILE. > > >> > > >>> > > >>> now: > > >>> vm_gid_physical_address - holds [2] i.e. offset of VGIA constant in > > >>> inside SSDT in ACPI_BUILD_TABLE_FILE. > > >>> > > >>>> + ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), > > >>>> + ssdt_acpi_vm_gid_addr[0], 32, > > >>>> vm_gid_physical_address); > > >>> Then we write this offset into VGIA in ACPI_BUILD_TABLE_FILE. > > >> > > >> Yes. This offset is later patched by the linker to the full physical > > >> address. > > >> > > >>> After BIOS loads tables it's going to patch at > > >>> [3] ACPI_BUILD_VMGENID_FILE + (&vm_gid_offset - table_data->data) /* > > >>> only god knows where it will be/ > > >>> > > >>> and on top of it write in it value: > > >>> *(ACPI_BUILD_TABLE_FILE + *[3]) > > >> > > >> We know exactly where it is, no need to call for god's help :-). > > >> > > >>> This approach in general of patching arbitrary place in AML blob > > >>> to get PHY addr of buffer with UUID, is quite a hack, especially > > >>> in light of that we are trying to hide all direct access to AML > > >>> blobs with related pointer arithmetic and manual patching. > > >>> > > >>> Why not reserve some potion of RAM and pass to BIOS/guest > > >>> a reservation so it won't be part of AddressRangeMemory or > > >>> AddressRangeACPI as MS spec requires? Then you won't need > > >>> jump all above hoops to just get buffer's PHY addr. > > >> > > >> I'll be glad to hear a new idea that I didn't already try in one of > > >> other previous patches. The problem is that the specification requires > > >> working with a physical address, so it must be allocated from inside the > > >> guest. Since the OS is not exist in this stage and I also don't want to > > >> write a special driver just to allocate this buffer I had to choose this > > >> approach. > > > how about creating device which will map 4K MMIO region in PCI hole > > > address space and passing it as a reservation via e820 table we have in > > > QEMU. > > > Then address could be directly built in ACPI tables as constant value > > > at the time of ACPI tables creation. > > > > Isn't this will cause a VMEXIT when the guest is reading the GUID? If it > > is then this idea was already presented and Michael didn't approve it. > It will, but is it performance critical? VM supposed to read it > at start-up and on getting notification. So I think VMEXIT in this case > is not sufficient to drop simple and strait-forward design. I agree with you on that and one of the previous patches did used a fixed-address to store the GUID while read/write access were handled by qemu driver code. But as I wrote before, it was Michael who didn't approved it so I proposed this method although it is a bit more complicated. I don't know how to break out of this dead-lock... :( > > BTW: > For start-up fw_cfg file is not any way better, it's also causes VMEXIT > for every byte it reads from it. I don't understand your claim. Accessing the fw_cfg "file" doesn't cause VMEXIT as it located somewhere in the guest's memory range. > > > > > > That way it would be possible to get address of buffer without > > > firmware + guest OS doing anything and going through quite complex > > > chain for getting buffer address (qemu->bios->OSPM->qemu). > > > If you go current route, it would be needed to teach linker a new command > > > to make reservation in E820 so that allocated buffer won't be part of > > > of AddressRangeMemory as required by spec or anything else. > > > Which would make already hard to understand/use correctly linker API > > > even more complex. > > > > > > > > >> > > >>>> > > >>> [...] > > >>>> typedef > > >>>> @@ -1790,6 +1811,11 @@ void acpi_setup(PcGuestInfo *guest_info) > > >>>> fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE, > > >>>> tables.tcpalog->data, > > >>>> acpi_data_len(tables.tcpalog)); > > >>>> > > >>>> + /* Add a 128-bit fw cfg file which stores the VM generation id. > > >>>> */ > > >>>> + g_array_set_size(tables.vmgenid, 16); > > >>>> + fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_VMGENID_FILE, > > >>>> + tables.vmgenid->data, tables.vmgenid->len); > > >>> shouldn't it be migratable? /i.e. acpi_add_rom_blob(...)/ > > >>> > > >> > > >> I'm not too familiar with the migration process, but I assume that this > > >> memory will be copied as part of the guest memory. > > >> > > >> Gal. > > >> > > > > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device 2015-02-04 15:09 ` Gal Hammer @ 2015-02-04 15:53 ` Igor Mammedov 2015-02-04 16:07 ` Michael S. Tsirkin 0 siblings, 1 reply; 13+ messages in thread From: Igor Mammedov @ 2015-02-04 15:53 UTC (permalink / raw) To: Gal Hammer; +Cc: qemu-devel, mst On Wed, 4 Feb 2015 10:09:38 -0500 (EST) Gal Hammer <ghammer@redhat.com> wrote: > Hi Igor, > > ----- Original Message ----- > > From: "Igor Mammedov" <imammedo@redhat.com> > > To: "Gal Hammer" <ghammer@redhat.com> > > Cc: qemu-devel@nongnu.org, mst@redhat.com > > Sent: Monday, February 2, 2015 3:55:02 PM > > Subject: Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device > > > > On Mon, 02 Feb 2015 15:13:39 +0200 > > Gal Hammer <ghammer@redhat.com> wrote: > > > > > On 02/02/2015 14:46, Igor Mammedov wrote: > > > > On Sun, 01 Feb 2015 14:56:26 +0200 > > > > Gal Hammer <ghammer@redhat.com> wrote: > > > > > > > >> On 22/01/2015 15:52, Igor Mammedov wrote: > > > >>> On Tue, 16 Dec 2014 17:50:43 +0200 > > > >>> Gal Hammer <ghammer@redhat.com> wrote: > > > >>> > > > >>>> Based on Microsoft's sepecifications (paper can be dowloaded from > > > >>>> http://go.microsoft.com/fwlink/?LinkId=260709), add a device > > > >>>> description to the SSDT ACPI table and its implementation. > > > >>>> > > > >>>> The GUID is set using a global "vmgenid.uuid" parameter. > > > >>>> > > > >>>> Signed-off-by: Gal Hammer <ghammer@redhat.com> > > > >>>> > > > >>> > > > >>>> --- a/hw/i386/acpi-build.c > > > >>>> +++ b/hw/i386/acpi-build.c > > > >>>> @@ -257,6 +257,7 @@ static void acpi_get_pci_info(PcPciInfo *info) > > > >>>> #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables" > > > >>>> #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp" > > > >>>> #define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log" > > > >>>> +#define ACPI_BUILD_VMGENID_FILE "etc/vm-generation-id" > > > >>>> > > > >>>> static void > > > >>>> build_header(GArray *linker, GArray *table_data, > > > >>>> @@ -1068,6 +1069,8 @@ build_ssdt(GArray *table_data, GArray *linker, > > > >>>> { > > > >>>> MachineState *machine = MACHINE(qdev_get_machine()); > > > >>>> uint32_t nr_mem = machine->ram_slots; > > > >>>> + uint32_t vm_gid_physical_address; > > > >>>> + uint32_t vm_gid_offset = 0; > > > >>>> unsigned acpi_cpus = guest_info->apic_id_limit; > > > >>>> int ssdt_start = table_data->len; > > > >>>> uint8_t *ssdt_ptr; > > > >>>> @@ -1096,6 +1099,21 @@ build_ssdt(GArray *table_data, GArray *linker, > > > >>>> ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), > > > >>>> ssdt_isa_pest[0], 16, misc->pvpanic_port); > > > >>>> > > > >>>> + if (vm_generation_id_set()) { > > > >>>> + vm_gid_physical_address = ssdt_start + > > > >>>> ssdt_acpi_vm_gid_addr[0]; > > > >>>> + bios_linker_loader_alloc(linker, ACPI_BUILD_VMGENID_FILE, 8, > > > >>>> true); > > > >>>> + bios_linker_loader_add_pointer(linker, > > > >>>> ACPI_BUILD_VMGENID_FILE, > > > >>>> + ACPI_BUILD_TABLE_FILE, > > > >>>> + table_data, > > > >>>> + &vm_gid_offset, > > > >>>> + sizeof(vm_gid_offset)); > > > >>> could some explain how this pointer magic works, > > > >> > > > >> I can try, but don't you think that a magic is gone once explained? ;-) > > > >> > > > >>> From my weak understanding it seems broken. > > > >>> Lets see: > > > >>> > > > >>> [1] &vm_gid_offset - must be pointer inside of dest_file blob > > > >>> (ACPI_BUILD_VMGENID_FILE) > > > >>> [2] vm_gid_offset - should hold offset of the place inside of > > > >>> src_file > > > >>> (ACPI_BUILD_TABLE_FILE) where to pointer inside > > > >>> of dest_file should point to > > > >> > > > >> The vm_gid_offset should point where in the ACPI_BUILD_VMGENID_FILE the > > > >> VM's GUID is stored. At the moment, it should always be zero because the > > > >> GUID is stored at the begging of the ACPI_BUILD_VMGENID_FILE. > > > >> > > > >>> > > > >>> now: > > > >>> vm_gid_physical_address - holds [2] i.e. offset of VGIA constant in > > > >>> inside SSDT in ACPI_BUILD_TABLE_FILE. > > > >>> > > > >>>> + ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), > > > >>>> + ssdt_acpi_vm_gid_addr[0], 32, > > > >>>> vm_gid_physical_address); > > > >>> Then we write this offset into VGIA in ACPI_BUILD_TABLE_FILE. > > > >> > > > >> Yes. This offset is later patched by the linker to the full physical > > > >> address. > > > >> > > > >>> After BIOS loads tables it's going to patch at > > > >>> [3] ACPI_BUILD_VMGENID_FILE + (&vm_gid_offset - table_data->data) /* > > > >>> only god knows where it will be/ > > > >>> > > > >>> and on top of it write in it value: > > > >>> *(ACPI_BUILD_TABLE_FILE + *[3]) > > > >> > > > >> We know exactly where it is, no need to call for god's help :-). > > > >> > > > >>> This approach in general of patching arbitrary place in AML blob > > > >>> to get PHY addr of buffer with UUID, is quite a hack, especially > > > >>> in light of that we are trying to hide all direct access to AML > > > >>> blobs with related pointer arithmetic and manual patching. > > > >>> > > > >>> Why not reserve some potion of RAM and pass to BIOS/guest > > > >>> a reservation so it won't be part of AddressRangeMemory or > > > >>> AddressRangeACPI as MS spec requires? Then you won't need > > > >>> jump all above hoops to just get buffer's PHY addr. > > > >> > > > >> I'll be glad to hear a new idea that I didn't already try in one of > > > >> other previous patches. The problem is that the specification requires > > > >> working with a physical address, so it must be allocated from inside the > > > >> guest. Since the OS is not exist in this stage and I also don't want to > > > >> write a special driver just to allocate this buffer I had to choose this > > > >> approach. > > > > how about creating device which will map 4K MMIO region in PCI hole > > > > address space and passing it as a reservation via e820 table we have in > > > > QEMU. > > > > Then address could be directly built in ACPI tables as constant value > > > > at the time of ACPI tables creation. > > > > > > Isn't this will cause a VMEXIT when the guest is reading the GUID? If it > > > is then this idea was already presented and Michael didn't approve it. > > It will, but is it performance critical? VM supposed to read it > > at start-up and on getting notification. So I think VMEXIT in this case > > is not sufficient to drop simple and strait-forward design. > > I agree with you on that and one of the previous patches did used a fixed-address to store the GUID while read/write access were handled by qemu driver code. But as I wrote before, it was Michael who didn't approved it so I proposed this method although it is a bit more complicated. > > I don't know how to break out of this dead-lock... :( Could you post a link to driver based version of series. Perhaps we could address Michael's comments and still stay with a simple implementation. > > > > > BTW: > > For start-up fw_cfg file is not any way better, it's also causes VMEXIT > > for every byte it reads from it. > > I don't understand your claim. Accessing the fw_cfg "file" doesn't cause VMEXIT as it located somewhere in the guest's memory range. As far as I'm aware MMIO or ioport is used for reading fw_cfg contents on guest side, one byte at a time and every such access causes VMEXIT into QEMU callback. > > > > > > > > > > That way it would be possible to get address of buffer without > > > > firmware + guest OS doing anything and going through quite complex > > > > chain for getting buffer address (qemu->bios->OSPM->qemu). > > > > If you go current route, it would be needed to teach linker a new command > > > > to make reservation in E820 so that allocated buffer won't be part of > > > > of AddressRangeMemory as required by spec or anything else. > > > > Which would make already hard to understand/use correctly linker API > > > > even more complex. > > > > > > > > > > > >> > > > >>>> > > > >>> [...] > > > >>>> typedef > > > >>>> @@ -1790,6 +1811,11 @@ void acpi_setup(PcGuestInfo *guest_info) > > > >>>> fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE, > > > >>>> tables.tcpalog->data, > > > >>>> acpi_data_len(tables.tcpalog)); > > > >>>> > > > >>>> + /* Add a 128-bit fw cfg file which stores the VM generation id. > > > >>>> */ > > > >>>> + g_array_set_size(tables.vmgenid, 16); > > > >>>> + fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_VMGENID_FILE, > > > >>>> + tables.vmgenid->data, tables.vmgenid->len); > > > >>> shouldn't it be migratable? /i.e. acpi_add_rom_blob(...)/ > > > >>> > > > >> > > > >> I'm not too familiar with the migration process, but I assume that this > > > >> memory will be copied as part of the guest memory. > > > >> > > > >> Gal. > > > >> > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device 2015-02-04 15:53 ` Igor Mammedov @ 2015-02-04 16:07 ` Michael S. Tsirkin 0 siblings, 0 replies; 13+ messages in thread From: Michael S. Tsirkin @ 2015-02-04 16:07 UTC (permalink / raw) To: Igor Mammedov; +Cc: Gal Hammer, qemu-devel On Wed, Feb 04, 2015 at 04:53:54PM +0100, Igor Mammedov wrote: > > > > Isn't this will cause a VMEXIT when the guest is reading the GUID? If it > > > > is then this idea was already presented and Michael didn't approve it. > > > It will, but is it performance critical? VM supposed to read it > > > at start-up and on getting notification. So I think VMEXIT in this case > > > is not sufficient to drop simple and strait-forward design. > > > > I agree with you on that and one of the previous patches did used a fixed-address to store the GUID while read/write access were handled by qemu driver code. But as I wrote before, it was Michael who didn't approved it so I proposed this method although it is a bit more complicated. > > > > I don't know how to break out of this dead-lock... :( > Could you post a link to driver based version of series. > Perhaps we could address Michael's comments and still stay > with a simple implementation. The point is to keep all allocations in guest. I don't want to "steal" a page from guest. > > > > > > > > BTW: > > > For start-up fw_cfg file is not any way better, it's also causes VMEXIT > > > for every byte it reads from it. > > > > I don't understand your claim. Accessing the fw_cfg "file" doesn't cause VMEXIT as it located somewhere in the guest's memory range. > As far as I'm aware MMIO or ioport is used for reading fw_cfg contents > on guest side, one byte at a time and every such access causes VMEXIT > into QEMU callback. It's highly unlikely to be measureable. Prove me wrong if you like. -- MST ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH V11 3/3] tests: add a unit test for the vmgenid device. 2014-12-16 15:50 [Qemu-devel] [PATCH V11 0/3] Virtual Machine Generation ID Gal Hammer 2014-12-16 15:50 ` [Qemu-devel] [PATCH V11 1/3] docs: vm generation id device's description Gal Hammer 2014-12-16 15:50 ` [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device Gal Hammer @ 2014-12-16 15:50 ` Gal Hammer 2 siblings, 0 replies; 13+ messages in thread From: Gal Hammer @ 2014-12-16 15:50 UTC (permalink / raw) To: qemu-devel; +Cc: Gal Hammer, mst Signed-off-by: Gal Hammer <ghammer@redhat.com> --- tests/Makefile | 2 ++ tests/vmgenid-test.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100644 tests/vmgenid-test.c diff --git a/tests/Makefile b/tests/Makefile index 16f0e4c..612441a 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -64,6 +64,7 @@ gcov-files-check-qom-interface-y = qom/object.c check-unit-$(CONFIG_POSIX) += tests/test-vmstate$(EXESUF) check-unit-y += tests/test-qemu-opts$(EXESUF) gcov-files-test-qemu-opts-y = qom/test-qemu-opts.c +check-qtest-i386-y += tests/vmgenid-test$(EXESUF) check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh @@ -351,6 +352,7 @@ tests/usb-hcd-xhci-test$(EXESUF): tests/usb-hcd-xhci-test.o $(libqos-usb-obj-y) tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o qemu-timer.o $(qtest-obj-y) tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o libqemuutil.a libqemustub.a +tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o ifeq ($(CONFIG_POSIX),y) LIBS += -lutil diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c new file mode 100644 index 0000000..d9c3e29 --- /dev/null +++ b/tests/vmgenid-test.c @@ -0,0 +1,48 @@ +/* + * QTest testcase for VM Generation ID + * + * Copyright (c) 2014 Red Hat, Inc. + * + * 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 <string.h> +#include "libqtest.h" + +static void vmgenid_test(void) +{ + static const uint8_t expected[16] = { + 0x32, 0x4e, 0x6e, 0xaf, 0xd1, 0xd1, 0x4b, 0xf6, + 0xbf, 0x41, 0xb9, 0xbb, 0x6c, 0x91, 0xfb, 0x87 + }; + uint8_t guid[16]; + uint32_t i; + + // Emulate the ACPI _INI method (tells the device where the physical + // memory was "allocated". + writel(0xfedf0000, 0x0001f000); + + // Skip the ACPI ADDR method and read the GUID directly from memory. + for (i = 0; i < 16; i++) { + guid[i] = readb(0x0001f000 + i); + } + + g_assert_cmpuint(sizeof(guid), ==, sizeof(expected)); + g_assert(memcmp(guid, expected, sizeof(guid)) == 0); +} + +int main(int argc, char **argv) +{ + int ret; + + g_test_init(&argc, &argv, NULL); + qtest_add_func("/vmgenid/vmgenid", vmgenid_test); + + qtest_start("-global vmgenid.uuid=324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"); + ret = g_test_run(); + + qtest_end(); + + return ret; +} -- 2.1.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-02-04 16:07 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-12-16 15:50 [Qemu-devel] [PATCH V11 0/3] Virtual Machine Generation ID Gal Hammer 2014-12-16 15:50 ` [Qemu-devel] [PATCH V11 1/3] docs: vm generation id device's description Gal Hammer 2014-12-16 15:50 ` [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device Gal Hammer 2015-01-22 13:52 ` Igor Mammedov 2015-01-22 21:21 ` Michael S. Tsirkin 2015-02-01 12:56 ` Gal Hammer 2015-02-02 12:46 ` Igor Mammedov 2015-02-02 13:13 ` Gal Hammer 2015-02-02 13:55 ` Igor Mammedov 2015-02-04 15:09 ` Gal Hammer 2015-02-04 15:53 ` Igor Mammedov 2015-02-04 16:07 ` Michael S. Tsirkin 2014-12-16 15:50 ` [Qemu-devel] [PATCH V11 3/3] tests: add a unit test for the vmgenid device Gal Hammer
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.