* [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID
@ 2017-02-16 6:18 ben
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 1/8] linker-loader: Add new 'write pointer' command ben
` (9 more replies)
0 siblings, 10 replies; 27+ messages in thread
From: ben @ 2017-02-16 6:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, mst, imammedo, Ben Warren
From: Ben Warren <ben@skyportsystems.com>
This patch set adds support for passing a GUID to Windows guests. It is a
re-implementation of previous patch sets written by Igor Mammedov et al, but
this time passing the GUID data as a fw_cfg blob.
This patch set has dependencies on new guest functionality, in particular the
support for a new linker-loader command and the ability to write back data
to QEMU over a DMA link. Work is in flight in both SeaBIOS and OVMF to support this.
v6->v7:
- Rebased to top of tree.
- Added 'src_offset' field to "write pointer" command
- Reworked unit tests based on feedback
- various minor changes based on feedback
- Added entries to MAINTAINERS file
v5->v6:
- Rebased to top of tree.
- Changed device from sysbus to a simple device. This removed the need for
adding dynamic sysbus support to pc_piix boards.
- Removed patch that introduced QWORD patching of AML.
- Removed ability to set GUID via QMP/HMP.
- Improved comments/documentation in code.
v4->v5:
- Added significantly more detail to the documentation.
- Replaced the previously-implemented linker-loader command with a new one:
"write pointer". This allows writing the guest address of a fw_cfg blob back
to an arbitrary offset in a writeable fw_cfg file visible to QEMU. This will
require support in SeaBIOS and OVMF (ongoing).
- Fixed endianness issues throughout.
- Several styling cleanups.
v3->v4:
- Rebased to top of tree.
- Re-added document patch that was accidentally dropped from the last revision.
- Added VMState functionality so that VGIA is restored properly.
- Added Unit tests
v2->v3:
- Added second writeable fw_cfg for storing the VM Generaiton ID
address. This uses a new linker-loader command for instructing the
guest to write back the allocated address. A patch for SeaBIOS has been
submitted (https://www.seabios.org/pipermail/seabios/2017-January/011079.html)
and the resulting binary will need to be pulled into QEMU once accepted.
- Setting VM Generation ID by command line or qmp/hmp now accepts an "auto"
value, whereby QEMU generates a random GUID.
- Incorporated review comments from v2 mainly around code styling and AML syntax
- Changed to use the E05 ACPI event instead of E00
v1->v2:
- Removed "changed" boolean parameter as it is unneeded
- Added ACPI Notify logic
- Style changes to pass checkpatch.pl
- Added support for dynamic sysbus to pc_piix boards
Ben Warren (7):
linker-loader: Add new 'write pointer' command
docs: VM Generation ID device description
ACPI: Add vmgenid blob storage to the build tables
ACPI: Add Virtual Machine Generation ID support
tests: Move reusable ACPI code into a utility file
tests: Add unit tests for the VM Generation ID feature
MAINTAINERS: Add VM Generation ID entry
Igor Mammedov (1):
qmp/hmp: add query-vm-generation-id and 'info vm-generation-id'
commands
MAINTAINERS | 8 ++
default-configs/i386-softmmu.mak | 1 +
default-configs/x86_64-softmmu.mak | 1 +
docs/specs/vmgenid.txt | 245 +++++++++++++++++++++++++++++++++
hmp-commands-info.hx | 14 ++
hmp.c | 9 ++
hmp.h | 1 +
hw/acpi/Makefile.objs | 1 +
hw/acpi/aml-build.c | 2 +
hw/acpi/bios-linker-loader.c | 66 ++++++++-
hw/acpi/vmgenid.c | 255 +++++++++++++++++++++++++++++++++++
hw/i386/acpi-build.c | 16 +++
include/hw/acpi/acpi_dev_interface.h | 1 +
include/hw/acpi/aml-build.h | 1 +
include/hw/acpi/bios-linker-loader.h | 7 +
include/hw/acpi/vmgenid.h | 35 +++++
qapi-schema.json | 20 +++
stubs/Makefile.objs | 1 +
stubs/vmgenid.c | 9 ++
tests/Makefile.include | 4 +-
tests/acpi-utils.c | 65 +++++++++
tests/acpi-utils.h | 94 +++++++++++++
tests/bios-tables-test.c | 132 +++---------------
tests/vmgenid-test.c | 174 ++++++++++++++++++++++++
24 files changed, 1041 insertions(+), 121 deletions(-)
create mode 100644 docs/specs/vmgenid.txt
create mode 100644 hw/acpi/vmgenid.c
create mode 100644 include/hw/acpi/vmgenid.h
create mode 100644 stubs/vmgenid.c
create mode 100644 tests/acpi-utils.c
create mode 100644 tests/acpi-utils.h
create mode 100644 tests/vmgenid-test.c
--
2.7.4
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v7 1/8] linker-loader: Add new 'write pointer' command
2017-02-16 6:18 [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID ben
@ 2017-02-16 6:18 ` ben
2017-02-16 9:43 ` Igor Mammedov
2017-02-16 17:01 ` Laszlo Ersek
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 2/8] docs: VM Generation ID device description ben
` (8 subsequent siblings)
9 siblings, 2 replies; 27+ messages in thread
From: ben @ 2017-02-16 6:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, mst, imammedo, Ben Warren
From: Ben Warren <ben@skyportsystems.com>
This is similar to the existing 'add pointer' functionality, but instead
of instructing the guest (BIOS or UEFI) to patch memory, it instructs
the guest to write the pointer back to QEMU via a writeable fw_cfg file.
Signed-off-by: Ben Warren <ben@skyportsystems.com>
---
hw/acpi/bios-linker-loader.c | 66 ++++++++++++++++++++++++++++++++++--
include/hw/acpi/bios-linker-loader.h | 7 ++++
2 files changed, 70 insertions(+), 3 deletions(-)
diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
index d963ebe..d5fb703 100644
--- a/hw/acpi/bios-linker-loader.c
+++ b/hw/acpi/bios-linker-loader.c
@@ -78,6 +78,21 @@ struct BiosLinkerLoaderEntry {
uint32_t length;
} cksum;
+ /*
+ * COMMAND_WRITE_POINTER - write the fw_cfg file (originating from
+ * @dest_file) at @wr_pointer.offset, by adding a pointer to
+ * @src_offset within the table originating from @src_file.
+ * 1,2,4 or 8 byte unsigned addition is used depending on
+ * @wr_pointer.size.
+ */
+ struct {
+ char dest_file[BIOS_LINKER_LOADER_FILESZ];
+ char src_file[BIOS_LINKER_LOADER_FILESZ];
+ uint32_t dst_offset;
+ uint32_t src_offset;
+ uint8_t size;
+ } wr_pointer;
+
/* padding */
char pad[124];
};
@@ -85,9 +100,10 @@ struct BiosLinkerLoaderEntry {
typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry;
enum {
- BIOS_LINKER_LOADER_COMMAND_ALLOCATE = 0x1,
- BIOS_LINKER_LOADER_COMMAND_ADD_POINTER = 0x2,
- BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
+ BIOS_LINKER_LOADER_COMMAND_ALLOCATE = 0x1,
+ BIOS_LINKER_LOADER_COMMAND_ADD_POINTER = 0x2,
+ BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
+ BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER = 0x4,
};
enum {
@@ -278,3 +294,47 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
}
+
+/*
+ * bios_linker_loader_write_pointer: ask guest to write a pointer to the
+ * source file into the destination file, and write it back to QEMU via
+ * fw_cfg DMA.
+ *
+ * @linker: linker object instance
+ * @dest_file: destination file that must be written
+ * @dst_patched_offset: location within destination file blob to be patched
+ * with the pointer to @src_file, in bytes
+ * @dst_patched_offset_size: size of the pointer to be patched
+ * at @dst_patched_offset in @dest_file blob, in bytes
+ * @src_file: source file who's address must be taken
+ * @src_offset: location within source file blob to which
+ * @dest_file+@dst_patched_offset will point to after
+ * firmware's executed WRITE_POINTER command
+ */
+void bios_linker_loader_write_pointer(BIOSLinker *linker,
+ const char *dest_file,
+ uint32_t dst_patched_offset,
+ uint8_t dst_patched_size,
+ const char *src_file,
+ uint32_t src_offset)
+{
+ BiosLinkerLoaderEntry entry;
+ const BiosLinkerFileEntry *source_file =
+ bios_linker_find_file(linker, src_file);
+
+ assert(source_file);
+ assert(src_offset <= source_file->blob->len);
+ memset(&entry, 0, sizeof entry);
+ strncpy(entry.wr_pointer.dest_file, dest_file,
+ sizeof entry.wr_pointer.dest_file - 1);
+ strncpy(entry.wr_pointer.src_file, src_file,
+ sizeof entry.wr_pointer.src_file - 1);
+ entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER);
+ entry.wr_pointer.dst_offset = cpu_to_le32(dst_patched_offset);
+ entry.wr_pointer.src_offset = cpu_to_le32(dst_patched_offset);
+ entry.wr_pointer.size = dst_patched_size;
+ assert(dst_patched_size == 1 || dst_patched_size == 2 ||
+ dst_patched_size == 4 || dst_patched_size == 8);
+
+ g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
+}
diff --git a/include/hw/acpi/bios-linker-loader.h b/include/hw/acpi/bios-linker-loader.h
index fa1e5d1..efe17b0 100644
--- a/include/hw/acpi/bios-linker-loader.h
+++ b/include/hw/acpi/bios-linker-loader.h
@@ -26,5 +26,12 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
const char *src_file,
uint32_t src_offset);
+void bios_linker_loader_write_pointer(BIOSLinker *linker,
+ const char *dest_file,
+ uint32_t dst_patched_offset,
+ uint8_t dst_patched_size,
+ const char *src_file,
+ uint32_t src_offset);
+
void bios_linker_loader_cleanup(BIOSLinker *linker);
#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v7 2/8] docs: VM Generation ID device description
2017-02-16 6:18 [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID ben
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 1/8] linker-loader: Add new 'write pointer' command ben
@ 2017-02-16 6:18 ` ben
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 3/8] ACPI: Add vmgenid blob storage to the build tables ben
` (7 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: ben @ 2017-02-16 6:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, mst, imammedo, Ben Warren, Gal Hammer
From: Ben Warren <ben@skyportsystems.com>
This patch is based off an earlier version by
Gal Hammer (ghammer@redhat.com)
Requirements section, ASCII diagrams and overall help
provided by Laszlo Ersek (lersek@redhat.com)
Signed-off-by: Gal Hammer <ghammer@redhat.com>
Signed-off-by: Ben Warren <ben@skyportsystems.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
docs/specs/vmgenid.txt | 245 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 245 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..aa9f518
--- /dev/null
+++ b/docs/specs/vmgenid.txt
@@ -0,0 +1,245 @@
+VIRTUAL MACHINE GENERATION ID
+=============================
+
+Copyright (C) 2016 Red Hat, Inc.
+Copyright (C) 2017 Skyport Systems, 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,
+referred to as a Globally Unique Identifier, or GUID.
+
+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). The
+guest operating system notices the change, and is then able to react as
+appropriate by marking its copies of distributed databases as dirty,
+re-initializing its random number generator etc.
+
+
+Requirements
+------------
+
+These requirements are extracted from the "How to implement virtual machine
+generation ID support in a virtualization platform" section of the
+specification, dated August 1, 2012.
+
+
+The document may be found on the web at:
+ http://go.microsoft.com/fwlink/?LinkId=260709
+
+R1a. The generation ID shall live in an 8-byte aligned buffer.
+
+R1b. The buffer holding the generation ID shall be in guest RAM, ROM, or device
+ MMIO range.
+
+R1c. The buffer holding the generation ID shall be kept separate from areas
+ used by the operating system.
+
+R1d. The buffer shall not be covered by an AddressRangeMemory or
+ AddressRangeACPI entry in the E820 or UEFI memory map.
+
+R1e. The generation ID shall not live in a page frame that could be mapped with
+ caching disabled. (In other words, regardless of whether the generation ID
+ lives in RAM, ROM or MMIO, it shall only be mapped as cacheable.)
+
+R2 to R5. [These AML requirements are isolated well enough in the Microsoft
+ specification for us to simply refer to them here.]
+
+R6. The hypervisor shall expose a _HID (hardware identifier) object in the
+ VMGenId device's scope that is unique to the hypervisor vendor.
+
+
+QEMU Implementation
+-------------------
+
+The above-mentioned specification does not dictate which ACPI descriptor table
+will contain the VM Generation ID device. Other implementations (Hyper-V and
+Xen) put it in the main descriptor table (Differentiated System Description
+Table or DSDT). For ease of debugging and implementation, we have decided to
+put it in its own Secondary System Description Table, or SSDT.
+
+The following is a dump of the contents from a running system:
+
+# iasl -p ./SSDT -d /sys/firmware/acpi/tables/SSDT
+
+Intel ACPI Component Architecture
+ASL+ Optimizing Compiler version 20150717-64
+Copyright (c) 2000 - 2015 Intel Corporation
+
+Reading ACPI table from file /sys/firmware/acpi/tables/SSDT - Length
+00000198 (0x0000C6)
+ACPI: SSDT 0x0000000000000000 0000C6 (v01 BOCHS VMGENID 00000001 BXPC
+00000001)
+Acpi table [SSDT] successfully installed and loaded
+Pass 1 parse of [SSDT]
+Pass 2 parse of [SSDT]
+Parsing Deferred Opcodes (Methods/Buffers/Packages/Regions)
+
+Parsing completed
+Disassembly completed
+ASL Output: ./SSDT.dsl - 1631 bytes
+# cat SSDT.dsl
+/*
+ * Intel ACPI Component Architecture
+ * AML/ASL+ Disassembler version 20150717-64
+ * Copyright (c) 2000 - 2015 Intel Corporation
+ *
+ * Disassembling to symbolic ASL+ operators
+ *
+ * Disassembly of /sys/firmware/acpi/tables/SSDT, Sun Feb 5 00:19:37 2017
+ *
+ * Original Table Header:
+ * Signature "SSDT"
+ * Length 0x000000CA (202)
+ * Revision 0x01
+ * Checksum 0x4B
+ * OEM ID "BOCHS "
+ * OEM Table ID "VMGENID"
+ * OEM Revision 0x00000001 (1)
+ * Compiler ID "BXPC"
+ * Compiler Version 0x00000001 (1)
+ */
+DefinitionBlock ("/sys/firmware/acpi/tables/SSDT.aml", "SSDT", 1, "BOCHS ",
+"VMGENID", 0x00000001)
+{
+ Name (VGIA, 0x07FFF000)
+ Scope (\_SB)
+ {
+ Device (VGEN)
+ {
+ Name (_HID, "QEMUVGID") // _HID: Hardware ID
+ Name (_CID, "VM_Gen_Counter") // _CID: Compatible ID
+ Name (_DDN, "VM_Gen_Counter") // _DDN: DOS Device Name
+ Method (_STA, 0, NotSerialized) // _STA: Status
+ {
+ Local0 = 0x0F
+ If ((VGIA == Zero))
+ {
+ Local0 = Zero
+ }
+
+ Return (Local0)
+ }
+
+ Method (ADDR, 0, NotSerialized)
+ {
+ Local0 = Package (0x02) {}
+ Index (Local0, Zero) = (VGIA + 0x28)
+ Index (Local0, One) = Zero
+ Return (Local0)
+ }
+ }
+ }
+
+ Method (\_GPE._E05, 0, NotSerialized) // _Exx: Edge-Triggered GPE
+ {
+ Notify (\_SB.VGEN, 0x80) // Status Change
+ }
+}
+
+
+Design Details:
+---------------
+
+Requirements R1a through R1e dictate that the memory holding the
+VM Generation ID must be allocated and owned by the guest firmware,
+in this case BIOS or UEFI. However, to be useful, QEMU must be able to
+change the contents of the memory at runtime, specifically when starting a
+backed-up or snapshotted image. In order to do this, QEMU must know the
+address that has been allocated.
+
+The mechanism chosen for this memory sharing is writeable fw_cfg blobs.
+These are data object that are visible to both QEMU and guests, and are
+addressable as sequential files.
+
+More information about fw_cfg can be found in "docs/specs/fw_cfg.txt"
+
+Two fw_cfg blobs are used in this case:
+
+/etc/vmgenid_guid - contains the actual VM Generation ID GUID
+ - read-only to the guest
+/etc/vmgenid_addr - contains the address of the downloaded vmgenid blob
+ - writeable by the guest
+
+
+QEMU sends the following commands to the guest at startup:
+
+1. Allocate memory for vmgenid_guid fw_cfg blob.
+2. Write the address of vmgenid_guid into the SSDT (VGIA ACPI variable as
+ shown above in the iasl dump). Note that this change is not propagated
+ back to QEMU.
+3. Write the address of vmgenid_guid back to QEMU's copy of vmgenid_addr
+ via the fw_cfg DMA interface.
+
+After step 3, QEMU is able to update the contents of vmgenid_guid at will.
+
+Since BIOS or UEFI does not necessarily run when we wish to change the GUID,
+the value of VGIA is persisted via the VMState mechanism.
+
+As spelled out in the specification, any change to the GUID executes an
+ACPI notification. The exact handler to use is not specified, so the vmgenid
+device uses the first unused one: \_GPE._E05.
+
+
+Endian-ness Considerations:
+---------------------------
+
+Although not specified in Microsoft's document, it is assumed that the
+device is expected to use little-endian format.
+
+All GUID passed in via command line or monitor are treated as big-endian.
+GUID values displayed via monitor are shown in big-endian format.
+
+
+GUID Storage Format:
+--------------------
+
+In order to implement an OVMF "SDT Header Probe Suppressor", the contents of
+the vmgenid_guid fw_cfg blob are not simply a 128-bit GUID. There is also
+significant padding in order to align and fill a memory page, as shown in the
+following diagram:
+
++----------------------------------+
+| SSDT with OEM Table ID = VMGENID |
++----------------------------------+
+| ... | TOP OF PAGE
+| VGIA dword object ---------------|-----> +---------------------------+
+| ... | | fw-allocated array for |
+| _STA method referring to VGIA | | "etc/vmgenid_guid" |
+| ... | +---------------------------+
+| ADDR method referring to VGIA | | 0: OVMF SDT Header probe |
+| ... | | suppressor |
++----------------------------------+ | 36: padding for 8-byte |
+ | alignment |
+ | 40: GUID |
+ | 56: padding to page size |
+ +---------------------------+
+ END OF PAGE
+
+
+Device Usage:
+-------------
+
+The device has one property, which may be only be set using the command line:
+
+ guid - sets the value of the GUID. A special value "auto" instructs
+ QEMU to generate a new random GUID.
+
+For example:
+
+ QEMU -device vmgenid,guid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
+ QEMU -device vmgenid,guid=auto
+
+The property may be queried via QMP/HMP:
+
+ (QEMU) query-vm-generation-id
+ {"return": {"guid": "324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"}}
+
+Setting of this parameter is intentionally left out from the QMP/HMP
+interfaces. There are no known use cases for changing the GUID once QEMU is
+running, and adding this capability would greatly increase the complexity.
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v7 3/8] ACPI: Add vmgenid blob storage to the build tables
2017-02-16 6:18 [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID ben
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 1/8] linker-loader: Add new 'write pointer' command ben
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 2/8] docs: VM Generation ID device description ben
@ 2017-02-16 6:18 ` ben
2017-02-16 17:05 ` Laszlo Ersek
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support ben
` (6 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: ben @ 2017-02-16 6:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, mst, imammedo, Ben Warren
From: Ben Warren <ben@skyportsystems.com>
This allows them to be centrally initialized and destroyed
The "AcpiBuildTables.vmgenid" array will be used to construct the
"etc/vmgenid_guid" fw_cfg blob.
Its contents will be linked into fw_cfg after being built on the
pc_machine_done() -> acpi_setup() -> acpi_build() call path, and dropped
without use on the subsequent, guest triggered, acpi_build_update() ->
acpi_build() call path.
Signed-off-by: Ben Warren <ben@skyportsystems.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/aml-build.c | 2 ++
include/hw/acpi/aml-build.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index b2a1e40..c6f2032 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1559,6 +1559,7 @@ 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();
}
@@ -1568,6 +1569,7 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre)
g_array_free(tables->rsdp, true);
g_array_free(tables->table_data, true);
g_array_free(tables->tcpalog, mfre);
+ g_array_free(tables->vmgenid, mfre);
}
/* Build rsdt table */
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 559326c..00c21f1 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -210,6 +210,7 @@ struct AcpiBuildTables {
GArray *table_data;
GArray *rsdp;
GArray *tcpalog;
+ GArray *vmgenid;
BIOSLinker *linker;
} AcpiBuildTables;
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support
2017-02-16 6:18 [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID ben
` (2 preceding siblings ...)
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 3/8] ACPI: Add vmgenid blob storage to the build tables ben
@ 2017-02-16 6:18 ` ben
2017-02-16 9:56 ` Igor Mammedov
2017-02-16 17:11 ` Laszlo Ersek
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 5/8] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands ben
` (5 subsequent siblings)
9 siblings, 2 replies; 27+ messages in thread
From: ben @ 2017-02-16 6:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, mst, imammedo, Ben Warren
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 | 239 +++++++++++++++++++++++++++++++++++
hw/i386/acpi-build.c | 16 +++
include/hw/acpi/acpi_dev_interface.h | 1 +
include/hw/acpi/vmgenid.h | 35 +++++
7 files changed, 294 insertions(+)
create mode 100644 hw/acpi/vmgenid.c
create mode 100644 include/hw/acpi/vmgenid.h
diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 48b07a4..029e952 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -59,3 +59,4 @@ CONFIG_I82801B11=y
CONFIG_SMBIOS=y
CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
CONFIG_PXB=y
+CONFIG_ACPI_VMGENID=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index fd96345..d1d7432 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -59,3 +59,4 @@ CONFIG_I82801B11=y
CONFIG_SMBIOS=y
CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
CONFIG_PXB=y
+CONFIG_ACPI_VMGENID=y
diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index 6acf798..11c35bc 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -5,6 +5,7 @@ common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
+common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
common-obj-y += acpi_interface.o
diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
new file mode 100644
index 0000000..8fba7e0
--- /dev/null
+++ b/hw/acpi/vmgenid.c
@@ -0,0 +1,239 @@
+/*
+ * 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(VmGenIdState *vms, GArray *table_data, GArray *guid,
+ BIOSLinker *linker)
+{
+ Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx;
+ uint32_t vgia_offset;
+ QemuUUID guid_le;
+
+ /* Fill in the GUID values. These need to be converted to little-endian
+ * first, since that's what the guest expects
+ */
+ g_array_set_size(guid, VMGENID_FW_CFG_SIZE - ARRAY_SIZE(guid_le.data));
+ guid_le = vms->guid;
+ qemu_uuid_bswap(&guid_le);
+ /* The GUID is written at a fixed offset into the fw_cfg file
+ * in order to implement the "OVMF SDT Header probe suppressor"
+ * see docs/specs/vmgenid.txt for more details
+ */
+ g_array_insert_vals(guid, VMGENID_GUID_OFFSET, guid_le.data,
+ ARRAY_SIZE(guid_le.data));
+
+ /* Put this in a separate SSDT table */
+ ssdt = init_aml_allocator();
+
+ /* Reserve space for header */
+ acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
+
+ /* Storage for the GUID address */
+ vgia_offset = table_data->len +
+ build_append_named_dword(ssdt->buf, "VGIA");
+ scope = aml_scope("\\_SB");
+ dev = aml_device("VGEN");
+ aml_append(dev, aml_name_decl("_HID", aml_string("QEMUVGID")));
+ aml_append(dev, aml_name_decl("_CID", aml_string("VM_Gen_Counter")));
+ aml_append(dev, aml_name_decl("_DDN", aml_string("VM_Gen_Counter")));
+
+ /* Simple status method to check that address is linked and non-zero */
+ method = aml_method("_STA", 0, AML_NOTSERIALIZED);
+ addr = aml_local(0);
+ aml_append(method, aml_store(aml_int(0xf), addr));
+ if_ctx = aml_if(aml_equal(aml_name("VGIA"), aml_int(0)));
+ aml_append(if_ctx, aml_store(aml_int(0), addr));
+ aml_append(method, if_ctx);
+ aml_append(method, aml_return(addr));
+ aml_append(dev, method);
+
+ /* the ADDR method returns two 32-bit words representing the lower and
+ * upper halves * of the physical address of the fw_cfg blob
+ * (holding the GUID)
+ */
+ method = aml_method("ADDR", 0, AML_NOTSERIALIZED);
+
+ addr = aml_local(0);
+ aml_append(method, aml_store(aml_package(2), addr));
+
+ aml_append(method, aml_store(aml_add(aml_name("VGIA"),
+ aml_int(VMGENID_GUID_OFFSET), NULL),
+ aml_index(addr, aml_int(0))));
+ aml_append(method, aml_store(aml_int(0), aml_index(addr, aml_int(1))));
+ aml_append(method, aml_return(addr));
+
+ aml_append(dev, method);
+ aml_append(scope, dev);
+ aml_append(ssdt, scope);
+
+ /* attach an ACPI notify */
+ method = aml_method("\\_GPE._E05", 0, AML_NOTSERIALIZED);
+ aml_append(method, aml_notify(aml_name("\\_SB.VGEN"), aml_int(0x80)));
+ aml_append(ssdt, method);
+
+ g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
+
+ /* Allocate guest memory for the Data fw_cfg blob */
+ bios_linker_loader_alloc(linker, VMGENID_GUID_FW_CFG_FILE, guid, 4096,
+ false /* page boundary, high memory */);
+
+ /* Patch address of GUID fw_cfg blob into the ADDR fw_cfg blob
+ * so QEMU can write the GUID there. The address is expected to be
+ * < 4GB, but write 64 bits anyway.
+ */
+ bios_linker_loader_write_pointer(linker,
+ VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint64_t),
+ VMGENID_GUID_FW_CFG_FILE, VMGENID_GUID_OFFSET);
+
+ /* Patch address of GUID fw_cfg blob into the AML so OSPM can retrieve
+ * and read it. Note that while we provide storage for 64 bits, only
+ * the least-signficant 32 get patched into AML.
+ */
+ bios_linker_loader_add_pointer(linker,
+ ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t),
+ VMGENID_GUID_FW_CFG_FILE, 0);
+
+ build_header(linker, table_data,
+ (void *)(table_data->data + table_data->len - ssdt->buf->len),
+ "SSDT", ssdt->buf->len, 1, NULL, "VMGENID");
+ free_aml_allocator();
+}
+
+void vmgenid_add_fw_cfg(VmGenIdState *vms, FWCfgState *s, GArray *guid)
+{
+ /* Create a read-only fw_cfg file for GUID */
+ fw_cfg_add_file(s, VMGENID_GUID_FW_CFG_FILE, guid->data,
+ VMGENID_FW_CFG_SIZE);
+ /* Create a read-write fw_cfg file for Address */
+ fw_cfg_add_file_callback(s, VMGENID_ADDR_FW_CFG_FILE, NULL, NULL,
+ vms->vmgenid_addr_le,
+ ARRAY_SIZE(vms->vmgenid_addr_le), false);
+}
+
+static void vmgenid_update_guest(VmGenIdState *vms)
+{
+ Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL);
+ uint32_t vmgenid_addr;
+ QemuUUID guid_le;
+
+ if (obj) {
+ /* Write the GUID to guest memory */
+ memcpy(&vmgenid_addr, vms->vmgenid_addr_le, sizeof(vmgenid_addr));
+ vmgenid_addr = le32_to_cpu(vmgenid_addr);
+ /* A zero value in vmgenid_addr means that BIOS has not yet written
+ * the address
+ */
+ if (vmgenid_addr) {
+ /* QemuUUID has the first three words as big-endian, and expect
+ * that any GUIDs passed in will always be BE. The guest,
+ * however, will expect the fields to be little-endian.
+ * Perform a byte swap immediately before writing.
+ */
+ guid_le = vms->guid;
+ qemu_uuid_bswap(&guid_le);
+ /* The GUID is written at a fixed offset into the fw_cfg file
+ * in order to implement the "OVMF SDT Header probe suppressor"
+ * see docs/specs/vmgenid.txt for more details
+ */
+ cpu_physical_memory_write(vmgenid_addr, guid_le.data,
+ sizeof(guid_le.data));
+ /* Send _GPE.E05 event */
+ acpi_send_event(DEVICE(obj), ACPI_VMGENID_CHANGE_STATUS);
+ }
+ }
+}
+
+static void vmgenid_set_guid(Object *obj, const char *value, Error **errp)
+{
+ VmGenIdState *vms = VMGENID(obj);
+
+ if (!strcmp(value, "auto")) {
+ qemu_uuid_generate(&vms->guid);
+ } else if (qemu_uuid_parse(value, &vms->guid) < 0) {
+ error_setg(errp, "'%s. %s': Failed to parse GUID string: %s",
+ object_get_typename(OBJECT(vms)), VMGENID_GUID, value);
+ return;
+ }
+
+ vmgenid_update_guest(vms);
+}
+
+/* After restoring an image, we need to update the guest memory and notify
+ * it of a potential change to VM Generation ID
+ */
+static int vmgenid_post_load(void *opaque, int version_id)
+{
+ VmGenIdState *vms = opaque;
+ vmgenid_update_guest(vms);
+ return 0;
+}
+
+static const VMStateDescription vmstate_vmgenid = {
+ .name = "vmgenid",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .post_load = vmgenid_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8_ARRAY(vmgenid_addr_le, VmGenIdState, sizeof(uint64_t)),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+static void vmgenid_handle_reset(void *opaque)
+{
+ VmGenIdState *vms = VMGENID(opaque);
+ /* Clear the guest-allocated GUID address when the VM resets */
+ memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le));
+}
+
+static void vmgenid_realize(DeviceState *dev, Error **errp)
+{
+ VmGenIdState *vms = VMGENID(dev);
+ qemu_register_reset(vmgenid_handle_reset, vms);
+}
+
+static void vmgenid_device_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->vmsd = &vmstate_vmgenid;
+ dc->realize = vmgenid_realize;
+ dc->hotpluggable = false;
+
+ object_class_property_add_str(klass, VMGENID_GUID, NULL,
+ vmgenid_set_guid, NULL);
+ object_class_property_set_description(klass, VMGENID_GUID,
+ "Set Global Unique Identifier "
+ "(big-endian) or auto for random value",
+ NULL);
+}
+
+static const TypeInfo vmgenid_device_info = {
+ .name = VMGENID_DEVICE,
+ .parent = TYPE_DEVICE,
+ .instance_size = sizeof(VmGenIdState),
+ .class_init = vmgenid_device_class_init,
+};
+
+static void vmgenid_register_types(void)
+{
+ type_register_static(&vmgenid_device_info);
+}
+
+type_init(vmgenid_register_types)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 1c928ab..db04cf5 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -42,6 +42,7 @@
#include "hw/acpi/memory_hotplug.h"
#include "sysemu/tpm.h"
#include "hw/acpi/tpm.h"
+#include "hw/acpi/vmgenid.h"
#include "sysemu/tpm_backend.h"
#include "hw/timer/mc146818rtc_regs.h"
#include "sysemu/numa.h"
@@ -2610,6 +2611,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
size_t aml_len = 0;
GArray *tables_blob = tables->table_data;
AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL };
+ Object *vmgenid_dev;
acpi_get_pm_info(&pm);
acpi_get_misc_info(&misc);
@@ -2653,6 +2655,13 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
acpi_add_table(table_offsets, tables_blob);
build_madt(tables_blob, tables->linker, pcms);
+ vmgenid_dev = find_vmgenid_dev();
+ if (vmgenid_dev) {
+ acpi_add_table(table_offsets, tables_blob);
+ vmgenid_build_acpi(VMGENID(vmgenid_dev), tables_blob,
+ tables->vmgenid, tables->linker);
+ }
+
if (misc.has_hpet) {
acpi_add_table(table_offsets, tables_blob);
build_hpet(tables_blob, tables->linker);
@@ -2823,6 +2832,7 @@ void acpi_setup(void)
PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
AcpiBuildTables tables;
AcpiBuildState *build_state;
+ Object *vmgenid_dev;
if (!pcms->fw_cfg) {
ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
@@ -2859,6 +2869,12 @@ void acpi_setup(void)
fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
tables.tcpalog->data, acpi_data_len(tables.tcpalog));
+ vmgenid_dev = find_vmgenid_dev();
+ if (vmgenid_dev) {
+ vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
+ tables.vmgenid);
+ }
+
if (!pcmc->rsdp_in_ram) {
/*
* Keep for compatibility with old machine types.
diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
index 71d3c48..3c2e4e9 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -11,6 +11,7 @@ typedef enum {
ACPI_CPU_HOTPLUG_STATUS = 4,
ACPI_MEMORY_HOTPLUG_STATUS = 8,
ACPI_NVDIMM_HOTPLUG_STATUS = 16,
+ ACPI_VMGENID_CHANGE_STATUS = 32,
} AcpiEventStatusBits;
#define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h
new file mode 100644
index 0000000..db7fa0e
--- /dev/null
+++ b/include/hw/acpi/vmgenid.h
@@ -0,0 +1,35 @@
+#ifndef ACPI_VMGENID_H
+#define ACPI_VMGENID_H
+
+#include "hw/acpi/bios-linker-loader.h"
+#include "hw/qdev.h"
+#include "qemu/uuid.h"
+
+#define VMGENID_DEVICE "vmgenid"
+#define VMGENID_GUID "guid"
+#define VMGENID_GUID_FW_CFG_FILE "etc/vmgenid_guid"
+#define VMGENID_ADDR_FW_CFG_FILE "etc/vmgenid_addr"
+
+#define VMGENID_FW_CFG_SIZE 4096 /* Occupy a page of memory */
+#define VMGENID_GUID_OFFSET 40 /* allow space for
+ * OVMF SDT Header Probe Supressor
+ */
+
+#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE)
+
+typedef struct VmGenIdState {
+ DeviceClass parent_obj;
+ QemuUUID guid; /* The 128-bit GUID seen by the guest */
+ uint8_t vmgenid_addr_le[8]; /* Address of the GUID (little-endian) */
+} VmGenIdState;
+
+static inline Object *find_vmgenid_dev(void)
+{
+ return object_resolve_path_type("", VMGENID_DEVICE, NULL);
+}
+
+void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid,
+ BIOSLinker *linker);
+void vmgenid_add_fw_cfg(VmGenIdState *vms, FWCfgState *s, GArray *guid);
+
+#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v7 5/8] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands
2017-02-16 6:18 [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID ben
` (3 preceding siblings ...)
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support ben
@ 2017-02-16 6:18 ` ben
2017-02-16 17:13 ` Laszlo Ersek
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 6/8] tests: Move reusable ACPI code into a utility file ben
` (4 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: ben @ 2017-02-16 6:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, mst, imammedo, Ben Warren
From: Igor Mammedov <imammedo@redhat.com>
Add commands to query Virtual Machine Generation ID counter.
QMP command example:
{ "execute": "query-vm-generation-id" }
HMP command example:
info vm-generation-id
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Ben Warren <ben@skyportsystems.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
hmp-commands-info.hx | 14 ++++++++++++++
hmp.c | 9 +++++++++
hmp.h | 1 +
hw/acpi/vmgenid.c | 16 ++++++++++++++++
qapi-schema.json | 20 ++++++++++++++++++++
stubs/Makefile.objs | 1 +
stubs/vmgenid.c | 9 +++++++++
7 files changed, 70 insertions(+)
create mode 100644 stubs/vmgenid.c
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index b0f35e6..a53f105 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -802,6 +802,20 @@ Show information about hotpluggable CPUs
ETEXI
STEXI
+@item info vm-generation-id
+@findex vm-generation-id
+Show Virtual Machine Generation ID
+ETEXI
+
+ {
+ .name = "vm-generation-id",
+ .args_type = "",
+ .params = "",
+ .help = "Show Virtual Machine Generation ID",
+ .cmd = hmp_info_vm_generation_id,
+ },
+
+STEXI
@end table
ETEXI
diff --git a/hmp.c b/hmp.c
index 2bc4f06..535613d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2565,3 +2565,12 @@ void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict)
qapi_free_HotpluggableCPUList(saved);
}
+
+void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict)
+{
+ GuidInfo *info = qmp_query_vm_generation_id(NULL);
+ if (info) {
+ monitor_printf(mon, "%s\n", info->guid);
+ }
+ qapi_free_GuidInfo(info);
+}
diff --git a/hmp.h b/hmp.h
index 05daf7c..799fd37 100644
--- a/hmp.h
+++ b/hmp.h
@@ -137,5 +137,6 @@ void hmp_rocker_of_dpa_flows(Monitor *mon, const QDict *qdict);
void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict);
void hmp_info_dump(Monitor *mon, const QDict *qdict);
void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict);
+void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict);
#endif
diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
index 8fba7e0..9f97b72 100644
--- a/hw/acpi/vmgenid.c
+++ b/hw/acpi/vmgenid.c
@@ -237,3 +237,19 @@ static void vmgenid_register_types(void)
}
type_init(vmgenid_register_types)
+
+GuidInfo *qmp_query_vm_generation_id(Error **errp)
+{
+ GuidInfo *info;
+ VmGenIdState *vms;
+ Object *obj = find_vmgenid_dev();
+
+ if (!obj) {
+ return NULL;
+ }
+ vms = VMGENID(obj);
+
+ info = g_malloc0(sizeof(*info));
+ info->guid = qemu_uuid_unparse_strdup(&vms->guid);
+ return info;
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index 5edb08d..396e49c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -6056,3 +6056,23 @@
#
##
{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
+
+##
+# @GuidInfo:
+#
+# GUID information.
+#
+# @guid: the globally unique identifier
+#
+# Since: 2.9
+##
+{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} }
+
+##
+# @query-vm-generation-id:
+#
+# Show Virtual Machine Generation ID
+#
+# Since 2.9
+##
+{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index a187295..0bffca6 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -35,3 +35,4 @@ stub-obj-y += qmp_pc_dimm_device_list.o
stub-obj-y += target-monitor-defs.o
stub-obj-y += target-get-monitor-def.o
stub-obj-y += pc_madt_cpu_entry.o
+stub-obj-y += vmgenid.o
diff --git a/stubs/vmgenid.c b/stubs/vmgenid.c
new file mode 100644
index 0000000..c64eb7a
--- /dev/null
+++ b/stubs/vmgenid.c
@@ -0,0 +1,9 @@
+#include "qemu/osdep.h"
+#include "qmp-commands.h"
+#include "qapi/qmp/qerror.h"
+
+GuidInfo *qmp_query_vm_generation_id(Error **errp)
+{
+ error_setg(errp, QERR_UNSUPPORTED);
+ return NULL;
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v7 6/8] tests: Move reusable ACPI code into a utility file
2017-02-16 6:18 [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID ben
` (4 preceding siblings ...)
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 5/8] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands ben
@ 2017-02-16 6:18 ` ben
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 7/8] tests: Add unit tests for the VM Generation ID feature ben
` (3 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: ben @ 2017-02-16 6:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, mst, imammedo, Ben Warren
From: Ben Warren <ben@skyportsystems.com>
Also usable by upcoming VM Generation ID tests
Signed-off-by: Ben Warren <ben@skyportsystems.com>
---
tests/Makefile.include | 2 +-
tests/acpi-utils.c | 65 +++++++++++++++++++++++
tests/acpi-utils.h | 94 +++++++++++++++++++++++++++++++++
tests/bios-tables-test.c | 132 ++++++-----------------------------------------
4 files changed, 175 insertions(+), 118 deletions(-)
create mode 100644 tests/acpi-utils.c
create mode 100644 tests/acpi-utils.h
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 634394a..143507e 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -667,7 +667,7 @@ tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
tests/boot-order-test$(EXESUF): tests/boot-order-test.o $(libqos-obj-y)
tests/boot-serial-test$(EXESUF): tests/boot-serial-test.o $(libqos-obj-y)
tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \
- tests/boot-sector.o $(libqos-obj-y)
+ tests/boot-sector.o tests/acpi-utils.o $(libqos-obj-y)
tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o $(libqos-obj-y)
tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
tests/ds1338-test$(EXESUF): tests/ds1338-test.o $(libqos-imx-obj-y)
diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
new file mode 100644
index 0000000..c5d1ebd
--- /dev/null
+++ b/tests/acpi-utils.c
@@ -0,0 +1,65 @@
+/*
+ * ACPI Utility Functions
+ *
+ * Copyright (c) 2013 Red Hat Inc.
+ * Copyright (c) 2017 Skyport Systems
+ *
+ * Authors:
+ * Michael S. Tsirkin <mst@redhat.com>,
+ * 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 <glib/gstdio.h>
+#include "qemu-common.h"
+#include "hw/smbios/smbios.h"
+#include "qemu/bitmap.h"
+#include "acpi-utils.h"
+#include "boot-sector.h"
+
+uint8_t acpi_calc_checksum(const uint8_t *data, int len)
+{
+ int i;
+ uint8_t sum = 0;
+
+ for (i = 0; i < len; i++) {
+ sum += data[i];
+ }
+
+ return sum;
+}
+
+uint32_t acpi_find_rsdp_address(void)
+{
+ uint32_t off;
+
+ /* RSDP location can vary across a narrow range */
+ for (off = 0xf0000; off < 0x100000; off += 0x10) {
+ uint8_t sig[] = "RSD PTR ";
+ int i;
+
+ for (i = 0; i < sizeof sig - 1; ++i) {
+ sig[i] = readb(off + i);
+ }
+
+ if (!memcmp(sig, "RSD PTR ", sizeof sig)) {
+ break;
+ }
+ }
+ return off;
+}
+
+void acpi_parse_rsdp_table(uint32_t addr, AcpiRsdpDescriptor *rsdp_table)
+{
+ ACPI_READ_FIELD(rsdp_table->signature, addr);
+ ACPI_ASSERT_CMP64(rsdp_table->signature, "RSD PTR ");
+
+ ACPI_READ_FIELD(rsdp_table->checksum, addr);
+ ACPI_READ_ARRAY(rsdp_table->oem_id, addr);
+ ACPI_READ_FIELD(rsdp_table->revision, addr);
+ ACPI_READ_FIELD(rsdp_table->rsdt_physical_address, addr);
+ ACPI_READ_FIELD(rsdp_table->length, addr);
+}
diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
new file mode 100644
index 0000000..9f9a2d5
--- /dev/null
+++ b/tests/acpi-utils.h
@@ -0,0 +1,94 @@
+/*
+ * Utilities for working with ACPI tables
+ *
+ * Copyright (c) 2013 Red Hat Inc.
+ *
+ * Authors:
+ * Michael S. Tsirkin <mst@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 TEST_ACPI_UTILS_H
+#define TEST_ACPI_UTILS_H
+
+#include "hw/acpi/acpi-defs.h"
+#include "libqtest.h"
+
+/* DSDT and SSDTs format */
+typedef struct {
+ AcpiTableHeader header;
+ gchar *aml; /* aml bytecode from guest */
+ gsize aml_len;
+ gchar *aml_file;
+ gchar *asl; /* asl code generated from aml */
+ gsize asl_len;
+ gchar *asl_file;
+ bool tmp_files_retain; /* do not delete the temp asl/aml */
+} QEMU_PACKED AcpiSdtTable;
+
+#define ACPI_READ_FIELD(field, addr) \
+ do { \
+ switch (sizeof(field)) { \
+ case 1: \
+ field = readb(addr); \
+ break; \
+ case 2: \
+ field = readw(addr); \
+ break; \
+ case 4: \
+ field = readl(addr); \
+ break; \
+ case 8: \
+ field = readq(addr); \
+ break; \
+ default: \
+ g_assert(false); \
+ } \
+ addr += sizeof(field); \
+ } while (0);
+
+#define ACPI_READ_ARRAY_PTR(arr, length, addr) \
+ do { \
+ int idx; \
+ for (idx = 0; idx < length; ++idx) { \
+ ACPI_READ_FIELD(arr[idx], addr); \
+ } \
+ } while (0);
+
+#define ACPI_READ_ARRAY(arr, addr) \
+ ACPI_READ_ARRAY_PTR(arr, sizeof(arr) / sizeof(arr[0]), addr)
+
+#define ACPI_READ_TABLE_HEADER(table, addr) \
+ do { \
+ ACPI_READ_FIELD((table)->signature, addr); \
+ ACPI_READ_FIELD((table)->length, addr); \
+ ACPI_READ_FIELD((table)->revision, addr); \
+ ACPI_READ_FIELD((table)->checksum, addr); \
+ ACPI_READ_ARRAY((table)->oem_id, addr); \
+ ACPI_READ_ARRAY((table)->oem_table_id, addr); \
+ ACPI_READ_FIELD((table)->oem_revision, addr); \
+ ACPI_READ_ARRAY((table)->asl_compiler_id, addr); \
+ ACPI_READ_FIELD((table)->asl_compiler_revision, addr); \
+ } while (0);
+
+#define ACPI_ASSERT_CMP(actual, expected) do { \
+ uint32_t ACPI_ASSERT_CMP_le = cpu_to_le32(actual); \
+ char ACPI_ASSERT_CMP_str[5] = {}; \
+ memcpy(ACPI_ASSERT_CMP_str, &ACPI_ASSERT_CMP_le, 4); \
+ g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
+} while (0)
+
+#define ACPI_ASSERT_CMP64(actual, expected) do { \
+ uint64_t ACPI_ASSERT_CMP_le = cpu_to_le64(actual); \
+ char ACPI_ASSERT_CMP_str[9] = {}; \
+ memcpy(ACPI_ASSERT_CMP_str, &ACPI_ASSERT_CMP_le, 8); \
+ g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
+} while (0)
+
+uint8_t acpi_calc_checksum(const uint8_t *data, int len);
+uint32_t acpi_find_rsdp_address(void);
+void acpi_parse_rsdp_table(uint32_t addr, AcpiRsdpDescriptor *rsdp_table);
+
+#endif /* TEST_ACPI_UTILS_H */
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 5404805..423a6f5 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -13,10 +13,9 @@
#include "qemu/osdep.h"
#include <glib/gstdio.h>
#include "qemu-common.h"
-#include "libqtest.h"
-#include "hw/acpi/acpi-defs.h"
#include "hw/smbios/smbios.h"
#include "qemu/bitmap.h"
+#include "acpi-utils.h"
#include "boot-sector.h"
#define MACHINE_PC "pc"
@@ -24,18 +23,6 @@
#define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML"
-/* DSDT and SSDTs format */
-typedef struct {
- AcpiTableHeader header;
- gchar *aml; /* aml bytecode from guest */
- gsize aml_len;
- gchar *aml_file;
- gchar *asl; /* asl code generated from aml */
- gsize asl_len;
- gchar *asl_file;
- bool tmp_files_retain; /* do not delete the temp asl/aml */
-} QEMU_PACKED AcpiSdtTable;
-
typedef struct {
const char *machine;
const char *variant;
@@ -53,65 +40,6 @@ typedef struct {
int required_struct_types_len;
} test_data;
-#define ACPI_READ_FIELD(field, addr) \
- do { \
- switch (sizeof(field)) { \
- case 1: \
- field = readb(addr); \
- break; \
- case 2: \
- field = readw(addr); \
- break; \
- case 4: \
- field = readl(addr); \
- break; \
- case 8: \
- field = readq(addr); \
- break; \
- default: \
- g_assert(false); \
- } \
- addr += sizeof(field); \
- } while (0);
-
-#define ACPI_READ_ARRAY_PTR(arr, length, addr) \
- do { \
- int idx; \
- for (idx = 0; idx < length; ++idx) { \
- ACPI_READ_FIELD(arr[idx], addr); \
- } \
- } while (0);
-
-#define ACPI_READ_ARRAY(arr, addr) \
- ACPI_READ_ARRAY_PTR(arr, sizeof(arr)/sizeof(arr[0]), addr)
-
-#define ACPI_READ_TABLE_HEADER(table, addr) \
- do { \
- ACPI_READ_FIELD((table)->signature, addr); \
- ACPI_READ_FIELD((table)->length, addr); \
- ACPI_READ_FIELD((table)->revision, addr); \
- ACPI_READ_FIELD((table)->checksum, addr); \
- ACPI_READ_ARRAY((table)->oem_id, addr); \
- ACPI_READ_ARRAY((table)->oem_table_id, addr); \
- ACPI_READ_FIELD((table)->oem_revision, addr); \
- ACPI_READ_ARRAY((table)->asl_compiler_id, addr); \
- ACPI_READ_FIELD((table)->asl_compiler_revision, addr); \
- } while (0);
-
-#define ACPI_ASSERT_CMP(actual, expected) do { \
- uint32_t ACPI_ASSERT_CMP_le = cpu_to_le32(actual); \
- char ACPI_ASSERT_CMP_str[5] = {}; \
- memcpy(ACPI_ASSERT_CMP_str, &ACPI_ASSERT_CMP_le, 4); \
- g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
-} while (0)
-
-#define ACPI_ASSERT_CMP64(actual, expected) do { \
- uint64_t ACPI_ASSERT_CMP_le = cpu_to_le64(actual); \
- char ACPI_ASSERT_CMP_str[9] = {}; \
- memcpy(ACPI_ASSERT_CMP_str, &ACPI_ASSERT_CMP_le, 8); \
- g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
-} while (0)
-
static char disk[] = "tests/acpi-test-disk-XXXXXX";
static const char *data_dir = "tests/acpi-test-data";
#ifdef CONFIG_IASL
@@ -147,36 +75,9 @@ static void free_test_data(test_data *data)
g_array_free(data->tables, false);
}
-static uint8_t acpi_checksum(const uint8_t *data, int len)
-{
- int i;
- uint8_t sum = 0;
-
- for (i = 0; i < len; i++) {
- sum += data[i];
- }
-
- return sum;
-}
-
static void test_acpi_rsdp_address(test_data *data)
{
- uint32_t off;
-
- /* OK, now find RSDP */
- for (off = 0xf0000; off < 0x100000; off += 0x10) {
- uint8_t sig[] = "RSD PTR ";
- int i;
-
- for (i = 0; i < sizeof sig - 1; ++i) {
- sig[i] = readb(off + i);
- }
-
- if (!memcmp(sig, "RSD PTR ", sizeof sig)) {
- break;
- }
- }
-
+ uint32_t off = acpi_find_rsdp_address();
g_assert_cmphex(off, <, 0x100000);
data->rsdp_addr = off;
}
@@ -186,17 +87,10 @@ static void test_acpi_rsdp_table(test_data *data)
AcpiRsdpDescriptor *rsdp_table = &data->rsdp_table;
uint32_t addr = data->rsdp_addr;
- ACPI_READ_FIELD(rsdp_table->signature, addr);
- ACPI_ASSERT_CMP64(rsdp_table->signature, "RSD PTR ");
-
- ACPI_READ_FIELD(rsdp_table->checksum, addr);
- ACPI_READ_ARRAY(rsdp_table->oem_id, addr);
- ACPI_READ_FIELD(rsdp_table->revision, addr);
- ACPI_READ_FIELD(rsdp_table->rsdt_physical_address, addr);
- ACPI_READ_FIELD(rsdp_table->length, addr);
+ acpi_parse_rsdp_table(addr, rsdp_table);
/* rsdp checksum is not for the whole table, but for the first 20 bytes */
- g_assert(!acpi_checksum((uint8_t *)rsdp_table, 20));
+ g_assert(!acpi_calc_checksum((uint8_t *)rsdp_table, 20));
}
static void test_acpi_rsdt_table(test_data *data)
@@ -220,8 +114,9 @@ static void test_acpi_rsdt_table(test_data *data)
tables = g_new0(uint32_t, tables_nr);
ACPI_READ_ARRAY_PTR(tables, tables_nr, addr);
- checksum = acpi_checksum((uint8_t *)rsdt_table, rsdt_table->length) +
- acpi_checksum((uint8_t *)tables, tables_nr * sizeof(uint32_t));
+ checksum = acpi_calc_checksum((uint8_t *)rsdt_table, rsdt_table->length) +
+ acpi_calc_checksum((uint8_t *)tables,
+ tables_nr * sizeof(uint32_t));
g_assert(!checksum);
/* SSDT tables after FADT */
@@ -279,7 +174,7 @@ static void test_acpi_fadt_table(test_data *data)
ACPI_READ_FIELD(fadt_table->flags, addr);
ACPI_ASSERT_CMP(fadt_table->signature, "FACP");
- g_assert(!acpi_checksum((uint8_t *)fadt_table, fadt_table->length));
+ g_assert(!acpi_calc_checksum((uint8_t *)fadt_table, fadt_table->length));
}
static void test_acpi_facs_table(test_data *data)
@@ -308,8 +203,10 @@ static void test_dst_table(AcpiSdtTable *sdt_table, uint32_t addr)
sdt_table->aml = g_malloc0(sdt_table->aml_len);
ACPI_READ_ARRAY_PTR(sdt_table->aml, sdt_table->aml_len, addr);
- checksum = acpi_checksum((uint8_t *)sdt_table, sizeof(AcpiTableHeader)) +
- acpi_checksum((uint8_t *)sdt_table->aml, sdt_table->aml_len);
+ checksum = acpi_calc_checksum((uint8_t *)sdt_table,
+ sizeof(AcpiTableHeader)) +
+ acpi_calc_checksum((uint8_t *)sdt_table->aml,
+ sdt_table->aml_len);
g_assert(!checksum);
}
@@ -608,8 +505,9 @@ static bool smbios_ep_table_ok(test_data *data)
return false;
}
ACPI_READ_FIELD(ep_table->smbios_bcd_revision, addr);
- if (acpi_checksum((uint8_t *)ep_table, sizeof *ep_table) ||
- acpi_checksum((uint8_t *)ep_table + 0x10, sizeof *ep_table - 0x10)) {
+ if (acpi_calc_checksum((uint8_t *)ep_table, sizeof *ep_table) ||
+ acpi_calc_checksum((uint8_t *)ep_table + 0x10,
+ sizeof *ep_table - 0x10)) {
return false;
}
return true;
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v7 7/8] tests: Add unit tests for the VM Generation ID feature
2017-02-16 6:18 [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID ben
` (5 preceding siblings ...)
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 6/8] tests: Move reusable ACPI code into a utility file ben
@ 2017-02-16 6:18 ` ben
2017-02-16 10:36 ` Igor Mammedov
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 8/8] MAINTAINERS: Add VM Generation ID entry ben
` (2 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: ben @ 2017-02-16 6:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, mst, imammedo, Ben Warren
From: Ben Warren <ben@skyportsystems.com>
The following tests are implemented:
* test that a GUID passed in by command line is propagated to the guest.
Read the GUID both from guest memory and from the monitor
* test that the "auto" argument to the GUID generates a valid GUID, as
seen by the guest.
This patch is loosely based on a previous patch from:
Gal Hammer <ghammer@redhat.com> and Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Ben Warren <ben@skyportsystems.com>
---
tests/Makefile.include | 2 +
tests/vmgenid-test.c | 174 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 176 insertions(+)
create mode 100644 tests/vmgenid-test.c
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 143507e..8d36341 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -241,6 +241,7 @@ check-qtest-i386-y += tests/usb-hcd-xhci-test$(EXESUF)
gcov-files-i386-y += hw/usb/hcd-xhci.c
check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
check-qtest-i386-y += tests/q35-test$(EXESUF)
+check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
gcov-files-i386-y += hw/pci-host/q35.c
check-qtest-i386-$(CONFIG_VHOST_NET_TEST_i386) += tests/vhost-user-test$(EXESUF)
ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
@@ -726,6 +727,7 @@ tests/ivshmem-test$(EXESUF): tests/ivshmem-test.o contrib/ivshmem-server/ivshmem
tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o contrib/libvhost-user/libvhost-user.o $(test-util-obj-y)
tests/test-uuid$(EXESUF): tests/test-uuid.o $(test-util-obj-y)
tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
+tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/acpi-utils.o
tests/migration/stress$(EXESUF): tests/migration/stress.o
$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
new file mode 100644
index 0000000..1741455
--- /dev/null
+++ b/tests/vmgenid-test.c
@@ -0,0 +1,174 @@
+/*
+ * QTest testcase for VM Generation ID
+ *
+ * Copyright (c) 2016 Red Hat, Inc.
+ * Copyright (c) 2017 Skyport Systems
+ *
+ * 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 <glib.h>
+#include <string.h>
+#include <unistd.h>
+#include "qemu/osdep.h"
+#include "qemu/bitmap.h"
+#include "qemu/uuid.h"
+#include "hw/acpi/acpi-defs.h"
+#include "acpi-utils.h"
+#include "libqtest.h"
+
+#define VGID_GUID "324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
+#define VMGENID_GUID_OFFSET 40 /* allow space for
+ * OVMF SDT Header Probe Supressor
+ */
+
+typedef struct {
+ AcpiTableHeader header;
+ gchar name_op;
+ gchar vgia[4];
+ gchar val_op;
+ uint32_t vgia_val;
+} QEMU_PACKED VgidTable;
+
+static uint32_t acpi_find_vgia(void)
+{
+ uint32_t off;
+ AcpiRsdpDescriptor rsdp_table;
+ uint32_t rsdt;
+ AcpiRsdtDescriptorRev1 rsdt_table;
+ int tables_nr;
+ uint32_t *tables;
+ AcpiTableHeader ssdt_table;
+ VgidTable vgid_table;
+ int i;
+
+ off = acpi_find_rsdp_address();
+ g_assert_cmphex(off, <, 0x100000);
+
+ acpi_parse_rsdp_table(off, &rsdp_table);
+
+ rsdt = rsdp_table.rsdt_physical_address;
+ /* read the header */
+ ACPI_READ_TABLE_HEADER(&rsdt_table, rsdt);
+ ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
+
+ /* compute the table entries in rsdt */
+ tables_nr = (rsdt_table.length - sizeof(AcpiRsdtDescriptorRev1)) /
+ sizeof(uint32_t);
+ g_assert_cmpint(tables_nr, >, 0);
+
+ /* get the addresses of the tables pointed by rsdt */
+ tables = g_new0(uint32_t, tables_nr);
+ ACPI_READ_ARRAY_PTR(tables, tables_nr, rsdt);
+
+ for (i = 0; i < tables_nr; i++) {
+ ACPI_READ_TABLE_HEADER(&ssdt_table, tables[i]);
+ if (!strncmp((char *)ssdt_table.oem_table_id, "VMGENID", 7)) {
+ /* the first entry in the table should be VGIA
+ * That's all we need
+ */
+ ACPI_READ_FIELD(vgid_table.name_op, tables[i]);
+ g_assert(vgid_table.name_op == 0x08); /* name */
+ ACPI_READ_ARRAY(vgid_table.vgia, tables[i]);
+ g_assert(memcmp(vgid_table.vgia, "VGIA", 4) == 0);
+ ACPI_READ_FIELD(vgid_table.val_op, tables[i]);
+ g_assert(vgid_table.val_op == 0x0C); /* dword */
+ ACPI_READ_FIELD(vgid_table.vgia_val, tables[i]);
+ /* The GUID is written at a fixed offset into the fw_cfg file
+ * in order to implement the "OVMF SDT Header probe suppressor"
+ * see docs/specs/vmgenid.txt for more details
+ */
+ return vgid_table.vgia_val + VMGENID_GUID_OFFSET;
+ }
+ }
+ return 0;
+}
+
+static void read_guid_from_memory(QemuUUID *guid)
+{
+ uint32_t vmgenid_addr;
+ int i;
+
+ vmgenid_addr = acpi_find_vgia();
+ g_assert(vmgenid_addr);
+
+ /* Read the GUID directly from guest memory */
+ for (i = 0; i < 16; i++) {
+ guid->data[i] = readb(vmgenid_addr + i);
+ }
+ /* The GUID is in little-endian format in the guest, while QEMU
+ * uses big-endian. Swap after reading.
+ */
+ qemu_uuid_bswap(guid);
+}
+
+static void read_guid_from_monitor(QemuUUID *guid)
+{
+ QDict *rsp, *rsp_ret;
+ const char *guid_str;
+
+ rsp = qmp("{ 'execute': 'query-vm-generation-id' }");
+ if (qdict_haskey(rsp, "return")) {
+ rsp_ret = qdict_get_qdict(rsp, "return");
+ g_assert(qdict_haskey(rsp_ret, "guid"));
+ guid_str = qdict_get_str(rsp_ret, "guid");
+ g_assert(qemu_uuid_parse(guid_str, guid) == 0);
+ }
+ QDECREF(rsp);
+}
+
+static void vmgenid_test(void)
+{
+ QemuUUID expected, measured;
+ gchar *cmd;
+
+ g_assert(qemu_uuid_parse(VGID_GUID, &expected) == 0);
+
+ cmd = g_strdup_printf("-machine accel=tcg -device vmgenid,id=testvgid,"
+ "guid=%s", VGID_GUID);
+ qtest_start(cmd);
+
+ /* Read the GUID from accessing guest memory */
+ read_guid_from_memory(&measured);
+ g_assert(memcmp(measured.data, expected.data, sizeof(measured.data)) == 0);
+
+ /* Read the GUID via the monitor */
+ read_guid_from_monitor(&measured);
+ g_assert(memcmp(measured.data, expected.data, sizeof(measured.data)) == 0);
+
+ qtest_quit(global_qtest);
+ g_free(cmd);
+}
+
+static void vmgenid_set_guid_auto_test(void)
+{
+ const char *cmd;
+ QemuUUID measured;
+
+ cmd = "-machine accel=tcg -device vmgenid,id=testvgid," "guid=auto";
+ qtest_start(cmd);
+
+ read_guid_from_memory(&measured);
+
+ /* Just check that the GUID is non-null */
+ g_assert(!qemu_uuid_is_null(&measured));
+
+ qtest_quit(global_qtest);
+}
+
+int main(int argc, char **argv)
+{
+ int ret;
+
+ g_test_init(&argc, &argv, NULL);
+
+ qtest_add_func("/vmgenid/vmgenid", vmgenid_test);
+ qtest_add_func("/vmgenid/vmgenid/set-guid-auto",
+ vmgenid_set_guid_auto_test);
+ ret = g_test_run();
+
+ qtest_end();
+
+ return ret;
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v7 8/8] MAINTAINERS: Add VM Generation ID entry
2017-02-16 6:18 [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID ben
` (6 preceding siblings ...)
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 7/8] tests: Add unit tests for the VM Generation ID feature ben
@ 2017-02-16 6:18 ` ben
2017-02-16 10:44 ` Laszlo Ersek
2017-02-16 14:29 ` [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID Igor Mammedov
2017-02-16 20:55 ` Laszlo Ersek
9 siblings, 1 reply; 27+ messages in thread
From: ben @ 2017-02-16 6:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, mst, imammedo, Ben Warren
From: Ben Warren <ben@skyportsystems.com>
Also add BIOS tables entry
Signed-off-by: Ben Warren <ben@skyportsystems.com>
---
MAINTAINERS | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index fb57d8e..e2e4b4f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -909,6 +909,7 @@ F: hw/acpi/*
F: hw/smbios/*
F: hw/i386/acpi-build.[hc]
F: hw/arm/virt-acpi-build.c
+F: tests/bios-tables-test.c
ppc4xx
M: Alexander Graf <agraf@suse.de>
@@ -1123,6 +1124,13 @@ F: hw/nvram/chrp_nvram.c
F: include/hw/nvram/chrp_nvram.h
F: tests/prom-env-test.c
+VM Generation ID
+M: Ben Warren <ben@skyportsystems.com>
+S: Maintained
+F: hw/acpi/vmgenid.c
+F: include/hw/acpi/vmgenid.h
+F: tests/vmgenid-test.c
+
Subsystems
----------
Audio
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/8] linker-loader: Add new 'write pointer' command
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 1/8] linker-loader: Add new 'write pointer' command ben
@ 2017-02-16 9:43 ` Igor Mammedov
2017-02-16 14:43 ` Michael S. Tsirkin
2017-02-16 15:48 ` Eric Blake
2017-02-16 17:01 ` Laszlo Ersek
1 sibling, 2 replies; 27+ messages in thread
From: Igor Mammedov @ 2017-02-16 9:43 UTC (permalink / raw)
To: ben; +Cc: qemu-devel, lersek, mst
On Wed, 15 Feb 2017 22:18:11 -0800
ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
>
> This is similar to the existing 'add pointer' functionality, but instead
> of instructing the guest (BIOS or UEFI) to patch memory, it instructs
> the guest to write the pointer back to QEMU via a writeable fw_cfg file.
>
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> ---
> hw/acpi/bios-linker-loader.c | 66 ++++++++++++++++++++++++++++++++++--
> include/hw/acpi/bios-linker-loader.h | 7 ++++
> 2 files changed, 70 insertions(+), 3 deletions(-)
>
> diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
> index d963ebe..d5fb703 100644
> --- a/hw/acpi/bios-linker-loader.c
> +++ b/hw/acpi/bios-linker-loader.c
> @@ -78,6 +78,21 @@ struct BiosLinkerLoaderEntry {
> uint32_t length;
> } cksum;
>
> + /*
> + * COMMAND_WRITE_POINTER - write the fw_cfg file (originating from
> + * @dest_file) at @wr_pointer.offset, by adding a pointer to
> + * @src_offset within the table originating from @src_file.
> + * 1,2,4 or 8 byte unsigned addition is used depending on
> + * @wr_pointer.size.
> + */
> + struct {
> + char dest_file[BIOS_LINKER_LOADER_FILESZ];
> + char src_file[BIOS_LINKER_LOADER_FILESZ];
> + uint32_t dst_offset;
> + uint32_t src_offset;
> + uint8_t size;
> + } wr_pointer;
> +
> /* padding */
> char pad[124];
Shouldn't padding be reduced by 4 bytes to keep
sizeof(BiosLinkerLoaderEntry) the same as before patch,
so that old bios would be able to skip this unknown command
and read the next at the right offset?
> };
> @@ -85,9 +100,10 @@ struct BiosLinkerLoaderEntry {
> typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry;
>
> enum {
> - BIOS_LINKER_LOADER_COMMAND_ALLOCATE = 0x1,
> - BIOS_LINKER_LOADER_COMMAND_ADD_POINTER = 0x2,
> - BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> + BIOS_LINKER_LOADER_COMMAND_ALLOCATE = 0x1,
> + BIOS_LINKER_LOADER_COMMAND_ADD_POINTER = 0x2,
> + BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> + BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER = 0x4,
> };
>
> enum {
> @@ -278,3 +294,47 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
>
> g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
> }
> +
> +/*
> + * bios_linker_loader_write_pointer: ask guest to write a pointer to the
> + * source file into the destination file, and write it back to QEMU via
> + * fw_cfg DMA.
> + *
> + * @linker: linker object instance
> + * @dest_file: destination file that must be written
> + * @dst_patched_offset: location within destination file blob to be patched
> + * with the pointer to @src_file, in bytes
> + * @dst_patched_offset_size: size of the pointer to be patched
> + * at @dst_patched_offset in @dest_file blob, in bytes
> + * @src_file: source file who's address must be taken
> + * @src_offset: location within source file blob to which
> + * @dest_file+@dst_patched_offset will point to after
> + * firmware's executed WRITE_POINTER command
> + */
> +void bios_linker_loader_write_pointer(BIOSLinker *linker,
> + const char *dest_file,
> + uint32_t dst_patched_offset,
> + uint8_t dst_patched_size,
> + const char *src_file,
> + uint32_t src_offset)
> +{
> + BiosLinkerLoaderEntry entry;
> + const BiosLinkerFileEntry *source_file =
> + bios_linker_find_file(linker, src_file);
> +
> + assert(source_file);
> + assert(src_offset <= source_file->blob->len);
off by one, should be '<'
> + memset(&entry, 0, sizeof entry);
> + strncpy(entry.wr_pointer.dest_file, dest_file,
> + sizeof entry.wr_pointer.dest_file - 1);
> + strncpy(entry.wr_pointer.src_file, src_file,
> + sizeof entry.wr_pointer.src_file - 1);
> + entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER);
> + entry.wr_pointer.dst_offset = cpu_to_le32(dst_patched_offset);
> + entry.wr_pointer.src_offset = cpu_to_le32(dst_patched_offset);
> + entry.wr_pointer.size = dst_patched_size;
> + assert(dst_patched_size == 1 || dst_patched_size == 2 ||
> + dst_patched_size == 4 || dst_patched_size == 8);
> +
> + g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
> +}
> diff --git a/include/hw/acpi/bios-linker-loader.h b/include/hw/acpi/bios-linker-loader.h
> index fa1e5d1..efe17b0 100644
> --- a/include/hw/acpi/bios-linker-loader.h
> +++ b/include/hw/acpi/bios-linker-loader.h
> @@ -26,5 +26,12 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
> const char *src_file,
> uint32_t src_offset);
>
> +void bios_linker_loader_write_pointer(BIOSLinker *linker,
> + const char *dest_file,
> + uint32_t dst_patched_offset,
> + uint8_t dst_patched_size,
> + const char *src_file,
> + uint32_t src_offset);
> +
> void bios_linker_loader_cleanup(BIOSLinker *linker);
> #endif
Taking in account comments above are corner cases.
1: old bios running on new QEMU with vmgenid device and OLD/not supported bios
2: assert check
It's ok fixes for above issues being fixed in follow up patch
or as fixup while patch is staged in pci tree
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support ben
@ 2017-02-16 9:56 ` Igor Mammedov
2017-02-16 18:32 ` Ben Warren
2017-02-16 17:11 ` Laszlo Ersek
1 sibling, 1 reply; 27+ messages in thread
From: Igor Mammedov @ 2017-02-16 9:56 UTC (permalink / raw)
To: ben; +Cc: qemu-devel, lersek, mst
On Wed, 15 Feb 2017 22:18:14 -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>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Thing to get fixed on top is to check
that there isn't vmgenid device present already
into realizefn() and fail gracefully is there is.
> ---
> default-configs/i386-softmmu.mak | 1 +
> default-configs/x86_64-softmmu.mak | 1 +
> hw/acpi/Makefile.objs | 1 +
> hw/acpi/vmgenid.c | 239 +++++++++++++++++++++++++++++++++++
> hw/i386/acpi-build.c | 16 +++
> include/hw/acpi/acpi_dev_interface.h | 1 +
> include/hw/acpi/vmgenid.h | 35 +++++
> 7 files changed, 294 insertions(+)
> create mode 100644 hw/acpi/vmgenid.c
> create mode 100644 include/hw/acpi/vmgenid.h
>
> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
> index 48b07a4..029e952 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -59,3 +59,4 @@ CONFIG_I82801B11=y
> CONFIG_SMBIOS=y
> CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
> CONFIG_PXB=y
> +CONFIG_ACPI_VMGENID=y
> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
> index fd96345..d1d7432 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -59,3 +59,4 @@ CONFIG_I82801B11=y
> CONFIG_SMBIOS=y
> CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
> CONFIG_PXB=y
> +CONFIG_ACPI_VMGENID=y
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index 6acf798..11c35bc 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -5,6 +5,7 @@ common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
> common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
> common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
> common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
> +common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
> common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>
> common-obj-y += acpi_interface.o
> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> new file mode 100644
> index 0000000..8fba7e0
> --- /dev/null
> +++ b/hw/acpi/vmgenid.c
> @@ -0,0 +1,239 @@
> +/*
> + * 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(VmGenIdState *vms, GArray *table_data, GArray *guid,
> + BIOSLinker *linker)
> +{
> + Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx;
> + uint32_t vgia_offset;
> + QemuUUID guid_le;
> +
> + /* Fill in the GUID values. These need to be converted to little-endian
> + * first, since that's what the guest expects
> + */
> + g_array_set_size(guid, VMGENID_FW_CFG_SIZE - ARRAY_SIZE(guid_le.data));
> + guid_le = vms->guid;
> + qemu_uuid_bswap(&guid_le);
> + /* The GUID is written at a fixed offset into the fw_cfg file
> + * in order to implement the "OVMF SDT Header probe suppressor"
> + * see docs/specs/vmgenid.txt for more details
> + */
> + g_array_insert_vals(guid, VMGENID_GUID_OFFSET, guid_le.data,
> + ARRAY_SIZE(guid_le.data));
> +
> + /* Put this in a separate SSDT table */
> + ssdt = init_aml_allocator();
> +
> + /* Reserve space for header */
> + acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
> +
> + /* Storage for the GUID address */
> + vgia_offset = table_data->len +
> + build_append_named_dword(ssdt->buf, "VGIA");
> + scope = aml_scope("\\_SB");
> + dev = aml_device("VGEN");
> + aml_append(dev, aml_name_decl("_HID", aml_string("QEMUVGID")));
> + aml_append(dev, aml_name_decl("_CID", aml_string("VM_Gen_Counter")));
> + aml_append(dev, aml_name_decl("_DDN", aml_string("VM_Gen_Counter")));
> +
> + /* Simple status method to check that address is linked and non-zero */
> + method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> + addr = aml_local(0);
> + aml_append(method, aml_store(aml_int(0xf), addr));
> + if_ctx = aml_if(aml_equal(aml_name("VGIA"), aml_int(0)));
> + aml_append(if_ctx, aml_store(aml_int(0), addr));
> + aml_append(method, if_ctx);
> + aml_append(method, aml_return(addr));
> + aml_append(dev, method);
> +
> + /* the ADDR method returns two 32-bit words representing the lower and
> + * upper halves * of the physical address of the fw_cfg blob
> + * (holding the GUID)
> + */
> + method = aml_method("ADDR", 0, AML_NOTSERIALIZED);
> +
> + addr = aml_local(0);
> + aml_append(method, aml_store(aml_package(2), addr));
> +
> + aml_append(method, aml_store(aml_add(aml_name("VGIA"),
> + aml_int(VMGENID_GUID_OFFSET), NULL),
> + aml_index(addr, aml_int(0))));
> + aml_append(method, aml_store(aml_int(0), aml_index(addr, aml_int(1))));
> + aml_append(method, aml_return(addr));
> +
> + aml_append(dev, method);
> + aml_append(scope, dev);
> + aml_append(ssdt, scope);
> +
> + /* attach an ACPI notify */
> + method = aml_method("\\_GPE._E05", 0, AML_NOTSERIALIZED);
> + aml_append(method, aml_notify(aml_name("\\_SB.VGEN"), aml_int(0x80)));
> + aml_append(ssdt, method);
> +
> + g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
> +
> + /* Allocate guest memory for the Data fw_cfg blob */
> + bios_linker_loader_alloc(linker, VMGENID_GUID_FW_CFG_FILE, guid, 4096,
> + false /* page boundary, high memory */);
> +
> + /* Patch address of GUID fw_cfg blob into the ADDR fw_cfg blob
> + * so QEMU can write the GUID there. The address is expected to be
> + * < 4GB, but write 64 bits anyway.
> + */
> + bios_linker_loader_write_pointer(linker,
> + VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint64_t),
> + VMGENID_GUID_FW_CFG_FILE, VMGENID_GUID_OFFSET);
> +
> + /* Patch address of GUID fw_cfg blob into the AML so OSPM can retrieve
> + * and read it. Note that while we provide storage for 64 bits, only
> + * the least-signficant 32 get patched into AML.
> + */
> + bios_linker_loader_add_pointer(linker,
> + ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t),
> + VMGENID_GUID_FW_CFG_FILE, 0);
> +
> + build_header(linker, table_data,
> + (void *)(table_data->data + table_data->len - ssdt->buf->len),
> + "SSDT", ssdt->buf->len, 1, NULL, "VMGENID");
> + free_aml_allocator();
> +}
> +
> +void vmgenid_add_fw_cfg(VmGenIdState *vms, FWCfgState *s, GArray *guid)
> +{
> + /* Create a read-only fw_cfg file for GUID */
> + fw_cfg_add_file(s, VMGENID_GUID_FW_CFG_FILE, guid->data,
> + VMGENID_FW_CFG_SIZE);
> + /* Create a read-write fw_cfg file for Address */
> + fw_cfg_add_file_callback(s, VMGENID_ADDR_FW_CFG_FILE, NULL, NULL,
> + vms->vmgenid_addr_le,
> + ARRAY_SIZE(vms->vmgenid_addr_le), false);
> +}
> +
> +static void vmgenid_update_guest(VmGenIdState *vms)
> +{
> + Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL);
> + uint32_t vmgenid_addr;
> + QemuUUID guid_le;
> +
> + if (obj) {
> + /* Write the GUID to guest memory */
> + memcpy(&vmgenid_addr, vms->vmgenid_addr_le, sizeof(vmgenid_addr));
> + vmgenid_addr = le32_to_cpu(vmgenid_addr);
> + /* A zero value in vmgenid_addr means that BIOS has not yet written
> + * the address
> + */
> + if (vmgenid_addr) {
> + /* QemuUUID has the first three words as big-endian, and expect
> + * that any GUIDs passed in will always be BE. The guest,
> + * however, will expect the fields to be little-endian.
> + * Perform a byte swap immediately before writing.
> + */
> + guid_le = vms->guid;
> + qemu_uuid_bswap(&guid_le);
> + /* The GUID is written at a fixed offset into the fw_cfg file
> + * in order to implement the "OVMF SDT Header probe suppressor"
> + * see docs/specs/vmgenid.txt for more details
> + */
> + cpu_physical_memory_write(vmgenid_addr, guid_le.data,
> + sizeof(guid_le.data));
> + /* Send _GPE.E05 event */
> + acpi_send_event(DEVICE(obj), ACPI_VMGENID_CHANGE_STATUS);
> + }
> + }
> +}
> +
> +static void vmgenid_set_guid(Object *obj, const char *value, Error **errp)
> +{
> + VmGenIdState *vms = VMGENID(obj);
> +
> + if (!strcmp(value, "auto")) {
> + qemu_uuid_generate(&vms->guid);
> + } else if (qemu_uuid_parse(value, &vms->guid) < 0) {
> + error_setg(errp, "'%s. %s': Failed to parse GUID string: %s",
> + object_get_typename(OBJECT(vms)), VMGENID_GUID, value);
> + return;
> + }
> +
> + vmgenid_update_guest(vms);
> +}
> +
> +/* After restoring an image, we need to update the guest memory and notify
> + * it of a potential change to VM Generation ID
> + */
> +static int vmgenid_post_load(void *opaque, int version_id)
> +{
> + VmGenIdState *vms = opaque;
> + vmgenid_update_guest(vms);
> + return 0;
> +}
> +
> +static const VMStateDescription vmstate_vmgenid = {
> + .name = "vmgenid",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .post_load = vmgenid_post_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8_ARRAY(vmgenid_addr_le, VmGenIdState, sizeof(uint64_t)),
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> +static void vmgenid_handle_reset(void *opaque)
> +{
> + VmGenIdState *vms = VMGENID(opaque);
> + /* Clear the guest-allocated GUID address when the VM resets */
> + memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le));
> +}
> +
> +static void vmgenid_realize(DeviceState *dev, Error **errp)
> +{
> + VmGenIdState *vms = VMGENID(dev);
> + qemu_register_reset(vmgenid_handle_reset, vms);
> +}
> +
> +static void vmgenid_device_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->vmsd = &vmstate_vmgenid;
> + dc->realize = vmgenid_realize;
> + dc->hotpluggable = false;
> +
> + object_class_property_add_str(klass, VMGENID_GUID, NULL,
> + vmgenid_set_guid, NULL);
> + object_class_property_set_description(klass, VMGENID_GUID,
> + "Set Global Unique Identifier "
> + "(big-endian) or auto for random value",
> + NULL);
> +}
> +
> +static const TypeInfo vmgenid_device_info = {
> + .name = VMGENID_DEVICE,
> + .parent = TYPE_DEVICE,
> + .instance_size = sizeof(VmGenIdState),
> + .class_init = vmgenid_device_class_init,
> +};
> +
> +static void vmgenid_register_types(void)
> +{
> + type_register_static(&vmgenid_device_info);
> +}
> +
> +type_init(vmgenid_register_types)
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 1c928ab..db04cf5 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -42,6 +42,7 @@
> #include "hw/acpi/memory_hotplug.h"
> #include "sysemu/tpm.h"
> #include "hw/acpi/tpm.h"
> +#include "hw/acpi/vmgenid.h"
> #include "sysemu/tpm_backend.h"
> #include "hw/timer/mc146818rtc_regs.h"
> #include "sysemu/numa.h"
> @@ -2610,6 +2611,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> size_t aml_len = 0;
> GArray *tables_blob = tables->table_data;
> AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL };
> + Object *vmgenid_dev;
>
> acpi_get_pm_info(&pm);
> acpi_get_misc_info(&misc);
> @@ -2653,6 +2655,13 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> acpi_add_table(table_offsets, tables_blob);
> build_madt(tables_blob, tables->linker, pcms);
>
> + vmgenid_dev = find_vmgenid_dev();
> + if (vmgenid_dev) {
> + acpi_add_table(table_offsets, tables_blob);
> + vmgenid_build_acpi(VMGENID(vmgenid_dev), tables_blob,
> + tables->vmgenid, tables->linker);
> + }
> +
> if (misc.has_hpet) {
> acpi_add_table(table_offsets, tables_blob);
> build_hpet(tables_blob, tables->linker);
> @@ -2823,6 +2832,7 @@ void acpi_setup(void)
> PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> AcpiBuildTables tables;
> AcpiBuildState *build_state;
> + Object *vmgenid_dev;
>
> if (!pcms->fw_cfg) {
> ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> @@ -2859,6 +2869,12 @@ void acpi_setup(void)
> fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
> tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>
> + vmgenid_dev = find_vmgenid_dev();
> + if (vmgenid_dev) {
> + vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
> + tables.vmgenid);
> + }
> +
> if (!pcmc->rsdp_in_ram) {
> /*
> * Keep for compatibility with old machine types.
> diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
> index 71d3c48..3c2e4e9 100644
> --- a/include/hw/acpi/acpi_dev_interface.h
> +++ b/include/hw/acpi/acpi_dev_interface.h
> @@ -11,6 +11,7 @@ typedef enum {
> ACPI_CPU_HOTPLUG_STATUS = 4,
> ACPI_MEMORY_HOTPLUG_STATUS = 8,
> ACPI_NVDIMM_HOTPLUG_STATUS = 16,
> + ACPI_VMGENID_CHANGE_STATUS = 32,
> } AcpiEventStatusBits;
>
> #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
> diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h
> new file mode 100644
> index 0000000..db7fa0e
> --- /dev/null
> +++ b/include/hw/acpi/vmgenid.h
> @@ -0,0 +1,35 @@
> +#ifndef ACPI_VMGENID_H
> +#define ACPI_VMGENID_H
> +
> +#include "hw/acpi/bios-linker-loader.h"
> +#include "hw/qdev.h"
> +#include "qemu/uuid.h"
> +
> +#define VMGENID_DEVICE "vmgenid"
> +#define VMGENID_GUID "guid"
> +#define VMGENID_GUID_FW_CFG_FILE "etc/vmgenid_guid"
> +#define VMGENID_ADDR_FW_CFG_FILE "etc/vmgenid_addr"
> +
> +#define VMGENID_FW_CFG_SIZE 4096 /* Occupy a page of memory */
> +#define VMGENID_GUID_OFFSET 40 /* allow space for
> + * OVMF SDT Header Probe Supressor
> + */
> +
> +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE)
> +
> +typedef struct VmGenIdState {
> + DeviceClass parent_obj;
> + QemuUUID guid; /* The 128-bit GUID seen by the guest */
> + uint8_t vmgenid_addr_le[8]; /* Address of the GUID (little-endian) */
> +} VmGenIdState;
> +
> +static inline Object *find_vmgenid_dev(void)
> +{
> + return object_resolve_path_type("", VMGENID_DEVICE, NULL);
> +}
> +
> +void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid,
> + BIOSLinker *linker);
> +void vmgenid_add_fw_cfg(VmGenIdState *vms, FWCfgState *s, GArray *guid);
> +
> +#endif
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 7/8] tests: Add unit tests for the VM Generation ID feature
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 7/8] tests: Add unit tests for the VM Generation ID feature ben
@ 2017-02-16 10:36 ` Igor Mammedov
2017-02-16 17:05 ` Ben Warren
0 siblings, 1 reply; 27+ messages in thread
From: Igor Mammedov @ 2017-02-16 10:36 UTC (permalink / raw)
To: ben; +Cc: qemu-devel, lersek, mst
On Wed, 15 Feb 2017 22:18:17 -0800
ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
>
> The following tests are implemented:
> * test that a GUID passed in by command line is propagated to the guest.
> Read the GUID both from guest memory and from the monitor
> * test that the "auto" argument to the GUID generates a valid GUID, as
> seen by the guest.
>
> This patch is loosely based on a previous patch from:
> Gal Hammer <ghammer@redhat.com> and Igor Mammedov <imammedo@redhat.com>
>
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> ---
> tests/Makefile.include | 2 +
> tests/vmgenid-test.c | 174 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 176 insertions(+)
> create mode 100644 tests/vmgenid-test.c
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 143507e..8d36341 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -241,6 +241,7 @@ check-qtest-i386-y += tests/usb-hcd-xhci-test$(EXESUF)
> gcov-files-i386-y += hw/usb/hcd-xhci.c
> check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
> check-qtest-i386-y += tests/q35-test$(EXESUF)
> +check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
> gcov-files-i386-y += hw/pci-host/q35.c
> check-qtest-i386-$(CONFIG_VHOST_NET_TEST_i386) += tests/vhost-user-test$(EXESUF)
> ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
> @@ -726,6 +727,7 @@ tests/ivshmem-test$(EXESUF): tests/ivshmem-test.o contrib/ivshmem-server/ivshmem
> tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o contrib/libvhost-user/libvhost-user.o $(test-util-obj-y)
> tests/test-uuid$(EXESUF): tests/test-uuid.o $(test-util-obj-y)
> tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
> +tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/acpi-utils.o
>
> tests/migration/stress$(EXESUF): tests/migration/stress.o
> $(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
> new file mode 100644
> index 0000000..1741455
> --- /dev/null
> +++ b/tests/vmgenid-test.c
> @@ -0,0 +1,174 @@
> +/*
> + * QTest testcase for VM Generation ID
> + *
> + * Copyright (c) 2016 Red Hat, Inc.
> + * Copyright (c) 2017 Skyport Systems
> + *
> + * 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 <glib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include "qemu/osdep.h"
> +#include "qemu/bitmap.h"
> +#include "qemu/uuid.h"
> +#include "hw/acpi/acpi-defs.h"
> +#include "acpi-utils.h"
> +#include "libqtest.h"
> +
> +#define VGID_GUID "324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
> +#define VMGENID_GUID_OFFSET 40 /* allow space for
> + * OVMF SDT Header Probe Supressor
> + */
> +
> +typedef struct {
> + AcpiTableHeader header;
> + gchar name_op;
> + gchar vgia[4];
> + gchar val_op;
> + uint32_t vgia_val;
> +} QEMU_PACKED VgidTable;
> +
> +static uint32_t acpi_find_vgia(void)
> +{
> + uint32_t off;
> + AcpiRsdpDescriptor rsdp_table;
> + uint32_t rsdt;
> + AcpiRsdtDescriptorRev1 rsdt_table;
> + int tables_nr;
> + uint32_t *tables;
> + AcpiTableHeader ssdt_table;
> + VgidTable vgid_table;
> + int i;
> +
> + off = acpi_find_rsdp_address();
> + g_assert_cmphex(off, <, 0x100000);
something's off here,
it fails for me for me with:
QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 tests/vmgenid-test
/x86_64/vmgenid/vmgenid: **
ERROR:/home/imammedo/builds/qemu/tests/vmgenid-test.c:47:acpi_find_vgia: assertion failed (off < 0x100000): (0x00100000 < 0x00100000)
> +
> + acpi_parse_rsdp_table(off, &rsdp_table);
> +
> + rsdt = rsdp_table.rsdt_physical_address;
> + /* read the header */
> + ACPI_READ_TABLE_HEADER(&rsdt_table, rsdt);
> + ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
> +
> + /* compute the table entries in rsdt */
> + tables_nr = (rsdt_table.length - sizeof(AcpiRsdtDescriptorRev1)) /
> + sizeof(uint32_t);
> + g_assert_cmpint(tables_nr, >, 0);
> +
> + /* get the addresses of the tables pointed by rsdt */
> + tables = g_new0(uint32_t, tables_nr);
> + ACPI_READ_ARRAY_PTR(tables, tables_nr, rsdt);
> +
> + for (i = 0; i < tables_nr; i++) {
> + ACPI_READ_TABLE_HEADER(&ssdt_table, tables[i]);
> + if (!strncmp((char *)ssdt_table.oem_table_id, "VMGENID", 7)) {
> + /* the first entry in the table should be VGIA
> + * That's all we need
> + */
> + ACPI_READ_FIELD(vgid_table.name_op, tables[i]);
> + g_assert(vgid_table.name_op == 0x08); /* name */
> + ACPI_READ_ARRAY(vgid_table.vgia, tables[i]);
> + g_assert(memcmp(vgid_table.vgia, "VGIA", 4) == 0);
> + ACPI_READ_FIELD(vgid_table.val_op, tables[i]);
> + g_assert(vgid_table.val_op == 0x0C); /* dword */
> + ACPI_READ_FIELD(vgid_table.vgia_val, tables[i]);
> + /* The GUID is written at a fixed offset into the fw_cfg file
> + * in order to implement the "OVMF SDT Header probe suppressor"
> + * see docs/specs/vmgenid.txt for more details
> + */
> + return vgid_table.vgia_val + VMGENID_GUID_OFFSET;
> + }
> + }
> + return 0;
> +}
> +
> +static void read_guid_from_memory(QemuUUID *guid)
> +{
> + uint32_t vmgenid_addr;
> + int i;
> +
> + vmgenid_addr = acpi_find_vgia();
> + g_assert(vmgenid_addr);
> +
> + /* Read the GUID directly from guest memory */
> + for (i = 0; i < 16; i++) {
> + guid->data[i] = readb(vmgenid_addr + i);
> + }
> + /* The GUID is in little-endian format in the guest, while QEMU
> + * uses big-endian. Swap after reading.
> + */
> + qemu_uuid_bswap(guid);
> +}
> +
> +static void read_guid_from_monitor(QemuUUID *guid)
> +{
> + QDict *rsp, *rsp_ret;
> + const char *guid_str;
> +
> + rsp = qmp("{ 'execute': 'query-vm-generation-id' }");
> + if (qdict_haskey(rsp, "return")) {
> + rsp_ret = qdict_get_qdict(rsp, "return");
> + g_assert(qdict_haskey(rsp_ret, "guid"));
> + guid_str = qdict_get_str(rsp_ret, "guid");
> + g_assert(qemu_uuid_parse(guid_str, guid) == 0);
> + }
> + QDECREF(rsp);
> +}
> +
> +static void vmgenid_test(void)
> +{
> + QemuUUID expected, measured;
> + gchar *cmd;
> +
> + g_assert(qemu_uuid_parse(VGID_GUID, &expected) == 0);
> +
> + cmd = g_strdup_printf("-machine accel=tcg -device vmgenid,id=testvgid,"
> + "guid=%s", VGID_GUID);
> + qtest_start(cmd);
> +
> + /* Read the GUID from accessing guest memory */
> + read_guid_from_memory(&measured);
> + g_assert(memcmp(measured.data, expected.data, sizeof(measured.data)) == 0);
> +
> + /* Read the GUID via the monitor */
> + read_guid_from_monitor(&measured);
> + g_assert(memcmp(measured.data, expected.data, sizeof(measured.data)) == 0);
> +
> + qtest_quit(global_qtest);
> + g_free(cmd);
> +}
> +
> +static void vmgenid_set_guid_auto_test(void)
> +{
> + const char *cmd;
> + QemuUUID measured;
> +
> + cmd = "-machine accel=tcg -device vmgenid,id=testvgid," "guid=auto";
> + qtest_start(cmd);
> +
> + read_guid_from_memory(&measured);
> +
> + /* Just check that the GUID is non-null */
> + g_assert(!qemu_uuid_is_null(&measured));
> +
> + qtest_quit(global_qtest);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + int ret;
> +
> + g_test_init(&argc, &argv, NULL);
> +
> + qtest_add_func("/vmgenid/vmgenid", vmgenid_test);
> + qtest_add_func("/vmgenid/vmgenid/set-guid-auto",
> + vmgenid_set_guid_auto_test);
> + ret = g_test_run();
> +
> + qtest_end();
> +
> + return ret;
> +}
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 8/8] MAINTAINERS: Add VM Generation ID entry
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 8/8] MAINTAINERS: Add VM Generation ID entry ben
@ 2017-02-16 10:44 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2017-02-16 10:44 UTC (permalink / raw)
To: ben, qemu-devel; +Cc: imammedo, mst
On 02/16/17 07:18, ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
>
> Also add BIOS tables entry
>
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> ---
> MAINTAINERS | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fb57d8e..e2e4b4f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -909,6 +909,7 @@ F: hw/acpi/*
> F: hw/smbios/*
> F: hw/i386/acpi-build.[hc]
> F: hw/arm/virt-acpi-build.c
> +F: tests/bios-tables-test.c
(1) Please add:
tests/acpi-utils.c
tests/acpi-utils.h
(2) I think this hunk belongs to
[PATCH v7 6/8] tests: Move reusable ACPI code into a utility file
or else an entirely separate patch.
>
> ppc4xx
> M: Alexander Graf <agraf@suse.de>
> @@ -1123,6 +1124,13 @@ F: hw/nvram/chrp_nvram.c
> F: include/hw/nvram/chrp_nvram.h
> F: tests/prom-env-test.c
>
> +VM Generation ID
> +M: Ben Warren <ben@skyportsystems.com>
> +S: Maintained
> +F: hw/acpi/vmgenid.c
> +F: include/hw/acpi/vmgenid.h
> +F: tests/vmgenid-test.c
> +
(3) Please add:
docs/specs/vmgenid.txt
stubs/vmgenid.c
With those changes, for this patch:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> Subsystems
> ----------
> Audio
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID
2017-02-16 6:18 [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID ben
` (7 preceding siblings ...)
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 8/8] MAINTAINERS: Add VM Generation ID entry ben
@ 2017-02-16 14:29 ` Igor Mammedov
2017-02-16 14:50 ` Ben Warren
2017-02-16 20:55 ` Laszlo Ersek
9 siblings, 1 reply; 27+ messages in thread
From: Igor Mammedov @ 2017-02-16 14:29 UTC (permalink / raw)
To: ben; +Cc: qemu-devel, lersek, mst
On Wed, 15 Feb 2017 22:18:10 -0800
ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
>
> This patch set adds support for passing a GUID to Windows guests. It is a
> re-implementation of previous patch sets written by Igor Mammedov et al, but
> this time passing the GUID data as a fw_cfg blob.
>
> This patch set has dependencies on new guest functionality, in particular the
> support for a new linker-loader command and the ability to write back data
> to QEMU over a DMA link. Work is in flight in both SeaBIOS and OVMF to support this.
>
> v6->v7:
> - Rebased to top of tree.
> - Added 'src_offset' field to "write pointer" command
Ben,
Do you plan to post updated write pointer seabios impl.?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/8] linker-loader: Add new 'write pointer' command
2017-02-16 9:43 ` Igor Mammedov
@ 2017-02-16 14:43 ` Michael S. Tsirkin
2017-02-16 15:48 ` Eric Blake
1 sibling, 0 replies; 27+ messages in thread
From: Michael S. Tsirkin @ 2017-02-16 14:43 UTC (permalink / raw)
To: Igor Mammedov; +Cc: ben, qemu-devel, lersek
On Thu, Feb 16, 2017 at 10:43:10AM +0100, Igor Mammedov wrote:
> On Wed, 15 Feb 2017 22:18:11 -0800
> ben@skyportsystems.com wrote:
>
> > From: Ben Warren <ben@skyportsystems.com>
> >
> > This is similar to the existing 'add pointer' functionality, but instead
> > of instructing the guest (BIOS or UEFI) to patch memory, it instructs
> > the guest to write the pointer back to QEMU via a writeable fw_cfg file.
> >
> > Signed-off-by: Ben Warren <ben@skyportsystems.com>
> > ---
> > hw/acpi/bios-linker-loader.c | 66 ++++++++++++++++++++++++++++++++++--
> > include/hw/acpi/bios-linker-loader.h | 7 ++++
> > 2 files changed, 70 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
> > index d963ebe..d5fb703 100644
> > --- a/hw/acpi/bios-linker-loader.c
> > +++ b/hw/acpi/bios-linker-loader.c
> > @@ -78,6 +78,21 @@ struct BiosLinkerLoaderEntry {
> > uint32_t length;
> > } cksum;
> >
> > + /*
> > + * COMMAND_WRITE_POINTER - write the fw_cfg file (originating from
> > + * @dest_file) at @wr_pointer.offset, by adding a pointer to
> > + * @src_offset within the table originating from @src_file.
> > + * 1,2,4 or 8 byte unsigned addition is used depending on
> > + * @wr_pointer.size.
> > + */
> > + struct {
> > + char dest_file[BIOS_LINKER_LOADER_FILESZ];
> > + char src_file[BIOS_LINKER_LOADER_FILESZ];
> > + uint32_t dst_offset;
> > + uint32_t src_offset;
> > + uint8_t size;
> > + } wr_pointer;
> > +
> > /* padding */
> > char pad[124];
> Shouldn't padding be reduced by 4 bytes to keep
> sizeof(BiosLinkerLoaderEntry) the same as before patch,
> so that old bios would be able to skip this unknown command
> and read the next at the right offset?
IMHO no - because it's a union.
> > };
> > @@ -85,9 +100,10 @@ struct BiosLinkerLoaderEntry {
> > typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry;
> >
> > enum {
> > - BIOS_LINKER_LOADER_COMMAND_ALLOCATE = 0x1,
> > - BIOS_LINKER_LOADER_COMMAND_ADD_POINTER = 0x2,
> > - BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> > + BIOS_LINKER_LOADER_COMMAND_ALLOCATE = 0x1,
> > + BIOS_LINKER_LOADER_COMMAND_ADD_POINTER = 0x2,
> > + BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> > + BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER = 0x4,
> > };
> >
> > enum {
> > @@ -278,3 +294,47 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
> >
> > g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
> > }
> > +
> > +/*
> > + * bios_linker_loader_write_pointer: ask guest to write a pointer to the
> > + * source file into the destination file, and write it back to QEMU via
> > + * fw_cfg DMA.
> > + *
> > + * @linker: linker object instance
> > + * @dest_file: destination file that must be written
> > + * @dst_patched_offset: location within destination file blob to be patched
> > + * with the pointer to @src_file, in bytes
> > + * @dst_patched_offset_size: size of the pointer to be patched
> > + * at @dst_patched_offset in @dest_file blob, in bytes
> > + * @src_file: source file who's address must be taken
> > + * @src_offset: location within source file blob to which
> > + * @dest_file+@dst_patched_offset will point to after
> > + * firmware's executed WRITE_POINTER command
> > + */
> > +void bios_linker_loader_write_pointer(BIOSLinker *linker,
> > + const char *dest_file,
> > + uint32_t dst_patched_offset,
> > + uint8_t dst_patched_size,
> > + const char *src_file,
> > + uint32_t src_offset)
> > +{
> > + BiosLinkerLoaderEntry entry;
> > + const BiosLinkerFileEntry *source_file =
> > + bios_linker_find_file(linker, src_file);
> > +
> > + assert(source_file);
> > + assert(src_offset <= source_file->blob->len);
> off by one, should be '<'
>
> > + memset(&entry, 0, sizeof entry);
> > + strncpy(entry.wr_pointer.dest_file, dest_file,
> > + sizeof entry.wr_pointer.dest_file - 1);
> > + strncpy(entry.wr_pointer.src_file, src_file,
> > + sizeof entry.wr_pointer.src_file - 1);
> > + entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER);
> > + entry.wr_pointer.dst_offset = cpu_to_le32(dst_patched_offset);
> > + entry.wr_pointer.src_offset = cpu_to_le32(dst_patched_offset);
> > + entry.wr_pointer.size = dst_patched_size;
> > + assert(dst_patched_size == 1 || dst_patched_size == 2 ||
> > + dst_patched_size == 4 || dst_patched_size == 8);
> > +
> > + g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
> > +}
> > diff --git a/include/hw/acpi/bios-linker-loader.h b/include/hw/acpi/bios-linker-loader.h
> > index fa1e5d1..efe17b0 100644
> > --- a/include/hw/acpi/bios-linker-loader.h
> > +++ b/include/hw/acpi/bios-linker-loader.h
> > @@ -26,5 +26,12 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
> > const char *src_file,
> > uint32_t src_offset);
> >
> > +void bios_linker_loader_write_pointer(BIOSLinker *linker,
> > + const char *dest_file,
> > + uint32_t dst_patched_offset,
> > + uint8_t dst_patched_size,
> > + const char *src_file,
> > + uint32_t src_offset);
> > +
> > void bios_linker_loader_cleanup(BIOSLinker *linker);
> > #endif
>
> Taking in account comments above are corner cases.
> 1: old bios running on new QEMU with vmgenid device and OLD/not supported bios
> 2: assert check
>
> It's ok fixes for above issues being fixed in follow up patch
> or as fixup while patch is staged in pci tree
>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID
2017-02-16 14:29 ` [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID Igor Mammedov
@ 2017-02-16 14:50 ` Ben Warren
0 siblings, 0 replies; 27+ messages in thread
From: Ben Warren @ 2017-02-16 14:50 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, Laszlo Ersek, mst
[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]
> On Feb 16, 2017, at 6:29 AM, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Wed, 15 Feb 2017 22:18:10 -0800
> ben@skyportsystems.com wrote:
>
>> From: Ben Warren <ben@skyportsystems.com>
>>
>> This patch set adds support for passing a GUID to Windows guests. It is a
>> re-implementation of previous patch sets written by Igor Mammedov et al, but
>> this time passing the GUID data as a fw_cfg blob.
>>
>> This patch set has dependencies on new guest functionality, in particular the
>> support for a new linker-loader command and the ability to write back data
>> to QEMU over a DMA link. Work is in flight in both SeaBIOS and OVMF to support this.
>>
>> v6->v7:
>> - Rebased to top of tree.
>> - Added 'src_offset' field to "write pointer" command
> Ben,
>
> Do you plan to post updated write pointer seabios impl.?
Yes, I will do so this morning (I expect within the next 4 hours). I thought I’d let you guys hash out the ‘src_offset’ issue first.
—Ben
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3583 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/8] linker-loader: Add new 'write pointer' command
2017-02-16 9:43 ` Igor Mammedov
2017-02-16 14:43 ` Michael S. Tsirkin
@ 2017-02-16 15:48 ` Eric Blake
1 sibling, 0 replies; 27+ messages in thread
From: Eric Blake @ 2017-02-16 15:48 UTC (permalink / raw)
To: Igor Mammedov, ben; +Cc: lersek, qemu-devel, mst
[-- Attachment #1: Type: text/plain, Size: 1505 bytes --]
On 02/16/2017 03:43 AM, Igor Mammedov wrote:
>> +++ b/hw/acpi/bios-linker-loader.c
>> @@ -78,6 +78,21 @@ struct BiosLinkerLoaderEntry {
>> uint32_t length;
>> } cksum;
>>
>> + /*
>> + * COMMAND_WRITE_POINTER - write the fw_cfg file (originating from
>> + * @dest_file) at @wr_pointer.offset, by adding a pointer to
>> + * @src_offset within the table originating from @src_file.
>> + * 1,2,4 or 8 byte unsigned addition is used depending on
>> + * @wr_pointer.size.
>> + */
>> + struct {
>> + char dest_file[BIOS_LINKER_LOADER_FILESZ];
>> + char src_file[BIOS_LINKER_LOADER_FILESZ];
>> + uint32_t dst_offset;
>> + uint32_t src_offset;
>> + uint8_t size;
>> + } wr_pointer;
>> +
>> /* padding */
>> char pad[124];
> Shouldn't padding be reduced by 4 bytes to keep
> sizeof(BiosLinkerLoaderEntry) the same as before patch,
> so that old bios would be able to skip this unknown command
> and read the next at the right offset?
No, because you are in the middle of a union rather than a struct (the
outer BiosLinkerLoaderEntry struct size is determined by the largest
member of the union, which is 'char pad[124]'; the new wr_pointer
addition to the union does not change the size of the union).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/8] linker-loader: Add new 'write pointer' command
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 1/8] linker-loader: Add new 'write pointer' command ben
2017-02-16 9:43 ` Igor Mammedov
@ 2017-02-16 17:01 ` Laszlo Ersek
2017-02-16 17:04 ` Ben Warren
1 sibling, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2017-02-16 17:01 UTC (permalink / raw)
To: ben; +Cc: qemu-devel, mst, imammedo
On 02/16/17 07:18, ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
>
> This is similar to the existing 'add pointer' functionality, but instead
> of instructing the guest (BIOS or UEFI) to patch memory, it instructs
> the guest to write the pointer back to QEMU via a writeable fw_cfg file.
>
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> ---
> hw/acpi/bios-linker-loader.c | 66 ++++++++++++++++++++++++++++++++++--
> include/hw/acpi/bios-linker-loader.h | 7 ++++
> 2 files changed, 70 insertions(+), 3 deletions(-)
>
> diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
> index d963ebe..d5fb703 100644
> --- a/hw/acpi/bios-linker-loader.c
> +++ b/hw/acpi/bios-linker-loader.c
> @@ -78,6 +78,21 @@ struct BiosLinkerLoaderEntry {
> uint32_t length;
> } cksum;
>
> + /*
> + * COMMAND_WRITE_POINTER - write the fw_cfg file (originating from
> + * @dest_file) at @wr_pointer.offset, by adding a pointer to
> + * @src_offset within the table originating from @src_file.
> + * 1,2,4 or 8 byte unsigned addition is used depending on
> + * @wr_pointer.size.
> + */
> + struct {
> + char dest_file[BIOS_LINKER_LOADER_FILESZ];
> + char src_file[BIOS_LINKER_LOADER_FILESZ];
> + uint32_t dst_offset;
> + uint32_t src_offset;
> + uint8_t size;
> + } wr_pointer;
> +
> /* padding */
> char pad[124];
> };
> @@ -85,9 +100,10 @@ struct BiosLinkerLoaderEntry {
> typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry;
>
> enum {
> - BIOS_LINKER_LOADER_COMMAND_ALLOCATE = 0x1,
> - BIOS_LINKER_LOADER_COMMAND_ADD_POINTER = 0x2,
> - BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> + BIOS_LINKER_LOADER_COMMAND_ALLOCATE = 0x1,
> + BIOS_LINKER_LOADER_COMMAND_ADD_POINTER = 0x2,
> + BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> + BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER = 0x4,
> };
>
> enum {
> @@ -278,3 +294,47 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
>
> g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
> }
> +
> +/*
> + * bios_linker_loader_write_pointer: ask guest to write a pointer to the
> + * source file into the destination file, and write it back to QEMU via
> + * fw_cfg DMA.
> + *
> + * @linker: linker object instance
> + * @dest_file: destination file that must be written
> + * @dst_patched_offset: location within destination file blob to be patched
> + * with the pointer to @src_file, in bytes
> + * @dst_patched_offset_size: size of the pointer to be patched
> + * at @dst_patched_offset in @dest_file blob, in bytes
> + * @src_file: source file who's address must be taken
> + * @src_offset: location within source file blob to which
> + * @dest_file+@dst_patched_offset will point to after
> + * firmware's executed WRITE_POINTER command
> + */
> +void bios_linker_loader_write_pointer(BIOSLinker *linker,
> + const char *dest_file,
> + uint32_t dst_patched_offset,
> + uint8_t dst_patched_size,
> + const char *src_file,
> + uint32_t src_offset)
> +{
> + BiosLinkerLoaderEntry entry;
> + const BiosLinkerFileEntry *source_file =
> + bios_linker_find_file(linker, src_file);
> +
> + assert(source_file);
> + assert(src_offset <= source_file->blob->len);
(1) Off by one, as pointed out by Igor.
> + memset(&entry, 0, sizeof entry);
> + strncpy(entry.wr_pointer.dest_file, dest_file,
> + sizeof entry.wr_pointer.dest_file - 1);
> + strncpy(entry.wr_pointer.src_file, src_file,
> + sizeof entry.wr_pointer.src_file - 1);
> + entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER);
> + entry.wr_pointer.dst_offset = cpu_to_le32(dst_patched_offset);
> + entry.wr_pointer.src_offset = cpu_to_le32(dst_patched_offset);
(2) copy/paste error, you should be assigning "src_offset", as in:
> diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
> index d5fb7033a0be..d1a9f2f33dcc 100644
> --- a/hw/acpi/bios-linker-loader.c
> +++ b/hw/acpi/bios-linker-loader.c
> @@ -331,7 +331,7 @@ void bios_linker_loader_write_pointer(BIOSLinker *linker,
> sizeof entry.wr_pointer.src_file - 1);
> entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER);
> entry.wr_pointer.dst_offset = cpu_to_le32(dst_patched_offset);
> - entry.wr_pointer.src_offset = cpu_to_le32(dst_patched_offset);
> + entry.wr_pointer.src_offset = cpu_to_le32(src_offset);
> entry.wr_pointer.size = dst_patched_size;
> assert(dst_patched_size == 1 || dst_patched_size == 2 ||
> dst_patched_size == 4 || dst_patched_size == 8);
With these errors fixed:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
I also tested this series (with the assignment under (2) fixed up, of course), as documented earlier in <https://www.mail-archive.com/qemu-devel@nongnu.org/msg430075.html> (msgid <678c203f-3768-7e65-6e48-6729473b6ffa@redhat.com>).
Hence, with (1) and (2) fixed, you can also add
Tested-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> + entry.wr_pointer.size = dst_patched_size;
> + assert(dst_patched_size == 1 || dst_patched_size == 2 ||
> + dst_patched_size == 4 || dst_patched_size == 8);
> +
> + g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
> +}
> diff --git a/include/hw/acpi/bios-linker-loader.h b/include/hw/acpi/bios-linker-loader.h
> index fa1e5d1..efe17b0 100644
> --- a/include/hw/acpi/bios-linker-loader.h
> +++ b/include/hw/acpi/bios-linker-loader.h
> @@ -26,5 +26,12 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
> const char *src_file,
> uint32_t src_offset);
>
> +void bios_linker_loader_write_pointer(BIOSLinker *linker,
> + const char *dest_file,
> + uint32_t dst_patched_offset,
> + uint8_t dst_patched_size,
> + const char *src_file,
> + uint32_t src_offset);
> +
> void bios_linker_loader_cleanup(BIOSLinker *linker);
> #endif
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/8] linker-loader: Add new 'write pointer' command
2017-02-16 17:01 ` Laszlo Ersek
@ 2017-02-16 17:04 ` Ben Warren
0 siblings, 0 replies; 27+ messages in thread
From: Ben Warren @ 2017-02-16 17:04 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel, Michael S. Tsirkin, imammedo
[-- Attachment #1: Type: text/plain, Size: 7338 bytes --]
> On Feb 16, 2017, at 9:01 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 02/16/17 07:18, ben@skyportsystems.com <mailto:ben@skyportsystems.com> wrote:
>> From: Ben Warren <ben@skyportsystems.com>
>>
>> This is similar to the existing 'add pointer' functionality, but instead
>> of instructing the guest (BIOS or UEFI) to patch memory, it instructs
>> the guest to write the pointer back to QEMU via a writeable fw_cfg file.
>>
>> Signed-off-by: Ben Warren <ben@skyportsystems.com>
>> ---
>> hw/acpi/bios-linker-loader.c | 66 ++++++++++++++++++++++++++++++++++--
>> include/hw/acpi/bios-linker-loader.h | 7 ++++
>> 2 files changed, 70 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
>> index d963ebe..d5fb703 100644
>> --- a/hw/acpi/bios-linker-loader.c
>> +++ b/hw/acpi/bios-linker-loader.c
>> @@ -78,6 +78,21 @@ struct BiosLinkerLoaderEntry {
>> uint32_t length;
>> } cksum;
>>
>> + /*
>> + * COMMAND_WRITE_POINTER - write the fw_cfg file (originating from
>> + * @dest_file) at @wr_pointer.offset, by adding a pointer to
>> + * @src_offset within the table originating from @src_file.
>> + * 1,2,4 or 8 byte unsigned addition is used depending on
>> + * @wr_pointer.size.
>> + */
>> + struct {
>> + char dest_file[BIOS_LINKER_LOADER_FILESZ];
>> + char src_file[BIOS_LINKER_LOADER_FILESZ];
>> + uint32_t dst_offset;
>> + uint32_t src_offset;
>> + uint8_t size;
>> + } wr_pointer;
>> +
>> /* padding */
>> char pad[124];
>> };
>> @@ -85,9 +100,10 @@ struct BiosLinkerLoaderEntry {
>> typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry;
>>
>> enum {
>> - BIOS_LINKER_LOADER_COMMAND_ALLOCATE = 0x1,
>> - BIOS_LINKER_LOADER_COMMAND_ADD_POINTER = 0x2,
>> - BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
>> + BIOS_LINKER_LOADER_COMMAND_ALLOCATE = 0x1,
>> + BIOS_LINKER_LOADER_COMMAND_ADD_POINTER = 0x2,
>> + BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
>> + BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER = 0x4,
>> };
>>
>> enum {
>> @@ -278,3 +294,47 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
>>
>> g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
>> }
>> +
>> +/*
>> + * bios_linker_loader_write_pointer: ask guest to write a pointer to the
>> + * source file into the destination file, and write it back to QEMU via
>> + * fw_cfg DMA.
>> + *
>> + * @linker: linker object instance
>> + * @dest_file: destination file that must be written
>> + * @dst_patched_offset: location within destination file blob to be patched
>> + * with the pointer to @src_file, in bytes
>> + * @dst_patched_offset_size: size of the pointer to be patched
>> + * at @dst_patched_offset in @dest_file blob, in bytes
>> + * @src_file: source file who's address must be taken
>> + * @src_offset: location within source file blob to which
>> + * @dest_file+@dst_patched_offset will point to after
>> + * firmware's executed WRITE_POINTER command
>> + */
>> +void bios_linker_loader_write_pointer(BIOSLinker *linker,
>> + const char *dest_file,
>> + uint32_t dst_patched_offset,
>> + uint8_t dst_patched_size,
>> + const char *src_file,
>> + uint32_t src_offset)
>> +{
>> + BiosLinkerLoaderEntry entry;
>> + const BiosLinkerFileEntry *source_file =
>> + bios_linker_find_file(linker, src_file);
>> +
>> + assert(source_file);
>> + assert(src_offset <= source_file->blob->len);
>
> (1) Off by one, as pointed out by Igor.
Oh, “off-by-one” errors. One of the three biggest sources of bugs in programming. The other being recursion. Sorry, couldn’t resist :)
>
>> + memset(&entry, 0, sizeof entry);
>> + strncpy(entry.wr_pointer.dest_file, dest_file,
>> + sizeof entry.wr_pointer.dest_file - 1);
>> + strncpy(entry.wr_pointer.src_file, src_file,
>> + sizeof entry.wr_pointer.src_file - 1);
>> + entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER);
>> + entry.wr_pointer.dst_offset = cpu_to_le32(dst_patched_offset);
>> + entry.wr_pointer.src_offset = cpu_to_le32(dst_patched_offset);
>
> (2) copy/paste error, you should be assigning "src_offset", as in:
>
Oops.
>> diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
>> index d5fb7033a0be..d1a9f2f33dcc 100644
>> --- a/hw/acpi/bios-linker-loader.c
>> +++ b/hw/acpi/bios-linker-loader.c
>> @@ -331,7 +331,7 @@ void bios_linker_loader_write_pointer(BIOSLinker *linker,
>> sizeof entry.wr_pointer.src_file - 1);
>> entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER);
>> entry.wr_pointer.dst_offset = cpu_to_le32(dst_patched_offset);
>> - entry.wr_pointer.src_offset = cpu_to_le32(dst_patched_offset);
>> + entry.wr_pointer.src_offset = cpu_to_le32(src_offset);
>> entry.wr_pointer.size = dst_patched_size;
>> assert(dst_patched_size == 1 || dst_patched_size == 2 ||
>> dst_patched_size == 4 || dst_patched_size == 8);
>
> With these errors fixed:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
>
> I also tested this series (with the assignment under (2) fixed up, of course), as documented earlier in <https://www.mail-archive.com/qemu-devel@nongnu.org/msg430075.html <https://www.mail-archive.com/qemu-devel@nongnu.org/msg430075.html>> (msgid <678c203f-3768-7e65-6e48-6729473b6ffa@redhat.com <mailto:678c203f-3768-7e65-6e48-6729473b6ffa@redhat.com>>).
>
> Hence, with (1) and (2) fixed, you can also add
>
> Tested-by: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
>
> Thanks
> Laszlo
>
Thanks a lot!
—Ben
>> + entry.wr_pointer.size = dst_patched_size;
>> + assert(dst_patched_size == 1 || dst_patched_size == 2 ||
>> + dst_patched_size == 4 || dst_patched_size == 8);
>> +
>> + g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
>> +}
>> diff --git a/include/hw/acpi/bios-linker-loader.h b/include/hw/acpi/bios-linker-loader.h
>> index fa1e5d1..efe17b0 100644
>> --- a/include/hw/acpi/bios-linker-loader.h
>> +++ b/include/hw/acpi/bios-linker-loader.h
>> @@ -26,5 +26,12 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
>> const char *src_file,
>> uint32_t src_offset);
>>
>> +void bios_linker_loader_write_pointer(BIOSLinker *linker,
>> + const char *dest_file,
>> + uint32_t dst_patched_offset,
>> + uint8_t dst_patched_size,
>> + const char *src_file,
>> + uint32_t src_offset);
>> +
>> void bios_linker_loader_cleanup(BIOSLinker *linker);
>> #endif
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3583 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 7/8] tests: Add unit tests for the VM Generation ID feature
2017-02-16 10:36 ` Igor Mammedov
@ 2017-02-16 17:05 ` Ben Warren
0 siblings, 0 replies; 27+ messages in thread
From: Ben Warren @ 2017-02-16 17:05 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, lersek, mst
[-- Attachment #1: Type: text/plain, Size: 8537 bytes --]
> On Feb 16, 2017, at 2:36 AM, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Wed, 15 Feb 2017 22:18:17 -0800
> ben@skyportsystems.com <mailto:ben@skyportsystems.com> wrote:
>
>> From: Ben Warren <ben@skyportsystems.com>
>>
>> The following tests are implemented:
>> * test that a GUID passed in by command line is propagated to the guest.
>> Read the GUID both from guest memory and from the monitor
>> * test that the "auto" argument to the GUID generates a valid GUID, as
>> seen by the guest.
>>
>> This patch is loosely based on a previous patch from:
>> Gal Hammer <ghammer@redhat.com> and Igor Mammedov <imammedo@redhat.com>
>>
>> Signed-off-by: Ben Warren <ben@skyportsystems.com>
>> ---
>> tests/Makefile.include | 2 +
>> tests/vmgenid-test.c | 174 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 176 insertions(+)
>> create mode 100644 tests/vmgenid-test.c
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index 143507e..8d36341 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -241,6 +241,7 @@ check-qtest-i386-y += tests/usb-hcd-xhci-test$(EXESUF)
>> gcov-files-i386-y += hw/usb/hcd-xhci.c
>> check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
>> check-qtest-i386-y += tests/q35-test$(EXESUF)
>> +check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
>> gcov-files-i386-y += hw/pci-host/q35.c
>> check-qtest-i386-$(CONFIG_VHOST_NET_TEST_i386) += tests/vhost-user-test$(EXESUF)
>> ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
>> @@ -726,6 +727,7 @@ tests/ivshmem-test$(EXESUF): tests/ivshmem-test.o contrib/ivshmem-server/ivshmem
>> tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o contrib/libvhost-user/libvhost-user.o $(test-util-obj-y)
>> tests/test-uuid$(EXESUF): tests/test-uuid.o $(test-util-obj-y)
>> tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
>> +tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/acpi-utils.o
>>
>> tests/migration/stress$(EXESUF): tests/migration/stress.o
>> $(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
>> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
>> new file mode 100644
>> index 0000000..1741455
>> --- /dev/null
>> +++ b/tests/vmgenid-test.c
>> @@ -0,0 +1,174 @@
>> +/*
>> + * QTest testcase for VM Generation ID
>> + *
>> + * Copyright (c) 2016 Red Hat, Inc.
>> + * Copyright (c) 2017 Skyport Systems
>> + *
>> + * 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 <glib.h>
>> +#include <string.h>
>> +#include <unistd.h>
>> +#include "qemu/osdep.h"
>> +#include "qemu/bitmap.h"
>> +#include "qemu/uuid.h"
>> +#include "hw/acpi/acpi-defs.h"
>> +#include "acpi-utils.h"
>> +#include "libqtest.h"
>> +
>> +#define VGID_GUID "324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
>> +#define VMGENID_GUID_OFFSET 40 /* allow space for
>> + * OVMF SDT Header Probe Supressor
>> + */
>> +
>> +typedef struct {
>> + AcpiTableHeader header;
>> + gchar name_op;
>> + gchar vgia[4];
>> + gchar val_op;
>> + uint32_t vgia_val;
>> +} QEMU_PACKED VgidTable;
>> +
>> +static uint32_t acpi_find_vgia(void)
>> +{
>> + uint32_t off;
>> + AcpiRsdpDescriptor rsdp_table;
>> + uint32_t rsdt;
>> + AcpiRsdtDescriptorRev1 rsdt_table;
>> + int tables_nr;
>> + uint32_t *tables;
>> + AcpiTableHeader ssdt_table;
>> + VgidTable vgid_table;
>> + int i;
>> +
>> + off = acpi_find_rsdp_address();
>> + g_assert_cmphex(off, <, 0x100000);
> something's off here,
> it fails for me for me with:
>
> QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 tests/vmgenid-test
> /x86_64/vmgenid/vmgenid: **
> ERROR:/home/imammedo/builds/qemu/tests/vmgenid-test.c:47:acpi_find_vgia: assertion failed (off < 0x100000): (0x00100000 < 0x00100000)
>
Probably a timing issue. I’ll get to the bottom of it ASAP
>> +
>> + acpi_parse_rsdp_table(off, &rsdp_table);
>> +
>> + rsdt = rsdp_table.rsdt_physical_address;
>> + /* read the header */
>> + ACPI_READ_TABLE_HEADER(&rsdt_table, rsdt);
>> + ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
>> +
>> + /* compute the table entries in rsdt */
>> + tables_nr = (rsdt_table.length - sizeof(AcpiRsdtDescriptorRev1)) /
>> + sizeof(uint32_t);
>> + g_assert_cmpint(tables_nr, >, 0);
>> +
>> + /* get the addresses of the tables pointed by rsdt */
>> + tables = g_new0(uint32_t, tables_nr);
>> + ACPI_READ_ARRAY_PTR(tables, tables_nr, rsdt);
>> +
>> + for (i = 0; i < tables_nr; i++) {
>> + ACPI_READ_TABLE_HEADER(&ssdt_table, tables[i]);
>> + if (!strncmp((char *)ssdt_table.oem_table_id, "VMGENID", 7)) {
>> + /* the first entry in the table should be VGIA
>> + * That's all we need
>> + */
>> + ACPI_READ_FIELD(vgid_table.name_op, tables[i]);
>> + g_assert(vgid_table.name_op == 0x08); /* name */
>> + ACPI_READ_ARRAY(vgid_table.vgia, tables[i]);
>> + g_assert(memcmp(vgid_table.vgia, "VGIA", 4) == 0);
>> + ACPI_READ_FIELD(vgid_table.val_op, tables[i]);
>> + g_assert(vgid_table.val_op == 0x0C); /* dword */
>> + ACPI_READ_FIELD(vgid_table.vgia_val, tables[i]);
>> + /* The GUID is written at a fixed offset into the fw_cfg file
>> + * in order to implement the "OVMF SDT Header probe suppressor"
>> + * see docs/specs/vmgenid.txt for more details
>> + */
>> + return vgid_table.vgia_val + VMGENID_GUID_OFFSET;
>> + }
>> + }
>> + return 0;
>> +}
>> +
>> +static void read_guid_from_memory(QemuUUID *guid)
>> +{
>> + uint32_t vmgenid_addr;
>> + int i;
>> +
>> + vmgenid_addr = acpi_find_vgia();
>> + g_assert(vmgenid_addr);
>> +
>> + /* Read the GUID directly from guest memory */
>> + for (i = 0; i < 16; i++) {
>> + guid->data[i] = readb(vmgenid_addr + i);
>> + }
>> + /* The GUID is in little-endian format in the guest, while QEMU
>> + * uses big-endian. Swap after reading.
>> + */
>> + qemu_uuid_bswap(guid);
>> +}
>> +
>> +static void read_guid_from_monitor(QemuUUID *guid)
>> +{
>> + QDict *rsp, *rsp_ret;
>> + const char *guid_str;
>> +
>> + rsp = qmp("{ 'execute': 'query-vm-generation-id' }");
>> + if (qdict_haskey(rsp, "return")) {
>> + rsp_ret = qdict_get_qdict(rsp, "return");
>> + g_assert(qdict_haskey(rsp_ret, "guid"));
>> + guid_str = qdict_get_str(rsp_ret, "guid");
>> + g_assert(qemu_uuid_parse(guid_str, guid) == 0);
>> + }
>> + QDECREF(rsp);
>> +}
>> +
>> +static void vmgenid_test(void)
>> +{
>> + QemuUUID expected, measured;
>> + gchar *cmd;
>> +
>> + g_assert(qemu_uuid_parse(VGID_GUID, &expected) == 0);
>> +
>> + cmd = g_strdup_printf("-machine accel=tcg -device vmgenid,id=testvgid,"
>> + "guid=%s", VGID_GUID);
>> + qtest_start(cmd);
>> +
>> + /* Read the GUID from accessing guest memory */
>> + read_guid_from_memory(&measured);
>> + g_assert(memcmp(measured.data, expected.data, sizeof(measured.data)) == 0);
>> +
>> + /* Read the GUID via the monitor */
>> + read_guid_from_monitor(&measured);
>> + g_assert(memcmp(measured.data, expected.data, sizeof(measured.data)) == 0);
>> +
>> + qtest_quit(global_qtest);
>> + g_free(cmd);
>> +}
>> +
>> +static void vmgenid_set_guid_auto_test(void)
>> +{
>> + const char *cmd;
>> + QemuUUID measured;
>> +
>> + cmd = "-machine accel=tcg -device vmgenid,id=testvgid," "guid=auto";
>> + qtest_start(cmd);
>> +
>> + read_guid_from_memory(&measured);
>> +
>> + /* Just check that the GUID is non-null */
>> + g_assert(!qemu_uuid_is_null(&measured));
>> +
>> + qtest_quit(global_qtest);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> + int ret;
>> +
>> + g_test_init(&argc, &argv, NULL);
>> +
>> + qtest_add_func("/vmgenid/vmgenid", vmgenid_test);
>> + qtest_add_func("/vmgenid/vmgenid/set-guid-auto",
>> + vmgenid_set_guid_auto_test);
>> + ret = g_test_run();
>> +
>> + qtest_end();
>> +
>> + return ret;
>> +}
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3583 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/8] ACPI: Add vmgenid blob storage to the build tables
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 3/8] ACPI: Add vmgenid blob storage to the build tables ben
@ 2017-02-16 17:05 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2017-02-16 17:05 UTC (permalink / raw)
To: ben, qemu-devel; +Cc: mst, imammedo
On 02/16/17 07:18, ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
>
> This allows them to be centrally initialized and destroyed
>
> The "AcpiBuildTables.vmgenid" array will be used to construct the
> "etc/vmgenid_guid" fw_cfg blob.
>
> Its contents will be linked into fw_cfg after being built on the
> pc_machine_done() -> acpi_setup() -> acpi_build() call path, and dropped
> without use on the subsequent, guest triggered, acpi_build_update() ->
> acpi_build() call path.
>
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/acpi/aml-build.c | 2 ++
> include/hw/acpi/aml-build.h | 1 +
> 2 files changed, 3 insertions(+)
Tested-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support ben
2017-02-16 9:56 ` Igor Mammedov
@ 2017-02-16 17:11 ` Laszlo Ersek
1 sibling, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2017-02-16 17:11 UTC (permalink / raw)
To: ben, qemu-devel; +Cc: mst, imammedo
On 02/16/17 07:18, 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 | 239 +++++++++++++++++++++++++++++++++++
> hw/i386/acpi-build.c | 16 +++
> include/hw/acpi/acpi_dev_interface.h | 1 +
> include/hw/acpi/vmgenid.h | 35 +++++
> 7 files changed, 294 insertions(+)
> create mode 100644 hw/acpi/vmgenid.c
> create mode 100644 include/hw/acpi/vmgenid.h
I compared this version against v6. It looks good. I also tested it
(with the fixup I mentioned under v7 1/8, and without live migration).
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 5/8] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 5/8] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands ben
@ 2017-02-16 17:13 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2017-02-16 17:13 UTC (permalink / raw)
To: ben, qemu-devel; +Cc: mst, imammedo
On 02/16/17 07:18, ben@skyportsystems.com wrote:
> From: Igor Mammedov <imammedo@redhat.com>
>
> Add commands to query Virtual Machine Generation ID counter.
>
> QMP command example:
> { "execute": "query-vm-generation-id" }
>
> HMP command example:
> info vm-generation-id
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
> hmp-commands-info.hx | 14 ++++++++++++++
> hmp.c | 9 +++++++++
> hmp.h | 1 +
> hw/acpi/vmgenid.c | 16 ++++++++++++++++
> qapi-schema.json | 20 ++++++++++++++++++++
> stubs/Makefile.objs | 1 +
> stubs/vmgenid.c | 9 +++++++++
> 7 files changed, 70 insertions(+)
> create mode 100644 stubs/vmgenid.c
I tested the "info vm-generation-id" command via HMP.
Tested-by: Laszlo Ersek <lersek@redhat.com>
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support
2017-02-16 9:56 ` Igor Mammedov
@ 2017-02-16 18:32 ` Ben Warren
2017-02-16 19:03 ` Laszlo Ersek
0 siblings, 1 reply; 27+ messages in thread
From: Ben Warren @ 2017-02-16 18:32 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, lersek, mst
[-- Attachment #1: Type: text/plain, Size: 2002 bytes --]
> On Feb 16, 2017, at 1:56 AM, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Wed, 15 Feb 2017 22:18:14 -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>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>
> Thing to get fixed on top is to check
> that there isn't vmgenid device present already
> into realizefn() and fail gracefully is there is.
>
I made the change as follows:
===
diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
index c8465df..fa3de6d 100644
--- a/hw/acpi/vmgenid.c
+++ b/hw/acpi/vmgenid.c
@@ -207,8 +207,15 @@ static void vmgenid_handle_reset(void *opaque)
static void vmgenid_realize(DeviceState *dev, Error **errp)
{
+ static bool is_realized = false;
VmGenIdState *vms = VMGENID(dev);
+
+ if (is_realized) {
+ error_setg(errp, "Device %s may only be specified once",
+ VMGENID_DEVICE);
+ }
qemu_register_reset(vmgenid_handle_reset, vms);
+ is_realized = true;
}
===
Sample output:
$ ../workspace/qemu/x86_64-softmmu/qemu-system-x86_64 -nographic -bios ../workspace/seabios/out/bios.bin -machine type=q35 -hda beiwe-with-vmgenid-driver.qcow2 -qmp unix:./qmp-sock,server,nowait -device vmgenid,guid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb66" -device vmgenid,guid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb67"
qemu-system-x86_64: -device vmgenid,guid=324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb67: Device vmgenid may only be specified once
Good enough? If so, I’ll roll it in. Otherwise, it’ll be a supplemental patch.
—Ben
<snip>
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3583 bytes --]
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support
2017-02-16 18:32 ` Ben Warren
@ 2017-02-16 19:03 ` Laszlo Ersek
2017-02-16 19:05 ` Ben Warren
0 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2017-02-16 19:03 UTC (permalink / raw)
To: Ben Warren, Igor Mammedov; +Cc: qemu-devel, mst
On 02/16/17 19:32, Ben Warren wrote:
>
>> On Feb 16, 2017, at 1:56 AM, Igor Mammedov <imammedo@redhat.com> wrote:
>>
>> On Wed, 15 Feb 2017 22:18:14 -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>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>
>> Thing to get fixed on top is to check
>> that there isn't vmgenid device present already
>> into realizefn() and fail gracefully is there is.
>>
> I made the change as follows:
> ===
>
> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> index c8465df..fa3de6d 100644
> --- a/hw/acpi/vmgenid.c
> +++ b/hw/acpi/vmgenid.c
> @@ -207,8 +207,15 @@ static void vmgenid_handle_reset(void *opaque)
>
> static void vmgenid_realize(DeviceState *dev, Error **errp)
> {
> + static bool is_realized = false;
> VmGenIdState *vms = VMGENID(dev);
> +
> + if (is_realized) {
> + error_setg(errp, "Device %s may only be specified once",
> + VMGENID_DEVICE);
> + }
> qemu_register_reset(vmgenid_handle_reset, vms);
> + is_realized = true;
> }
>
> ===
> Sample output:
> $ ../workspace/qemu/x86_64-softmmu/qemu-system-x86_64 -nographic -bios ../workspace/seabios/out/bios.bin -machine type=q35 -hda beiwe-with-vmgenid-driver.qcow2 -qmp unix:./qmp-sock,server,nowait -device vmgenid,guid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb66" -device vmgenid,guid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb67"
> qemu-system-x86_64: -device vmgenid,guid=324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb67: Device vmgenid may only be specified once
>
>
> Good enough? If so, I’ll roll it in. Otherwise, it’ll be a supplemental patch.
Ehm, I'd say let's postpone that patch. Function (or even file) scope
static variables that plaster over QOM shortcomings (or, well,
"non-trivial QOM patterns") are strongly frowned upon. I've done it
myself before (totally as a last resort, really -- not kidding!), and it
was real hard to get into the tree.
This deserves more discussion, so please delay it.
(While at it, please remember my other recommendation for realize():
enforcing that the running machine type / version provides the guest
with the DMA interface for fw_cfg. For that, based on prior consensus,
you'll need another device property, to be set in parallel to
fw_cfg_mem's and fw_cfg_io's "dma-enabled" property. So I think it makes
sense to address all of these things in a separate, followup series.)
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support
2017-02-16 19:03 ` Laszlo Ersek
@ 2017-02-16 19:05 ` Ben Warren
0 siblings, 0 replies; 27+ messages in thread
From: Ben Warren @ 2017-02-16 19:05 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Igor Mammedov, qemu-devel, mst
[-- Attachment #1: Type: text/plain, Size: 3108 bytes --]
> On Feb 16, 2017, at 11:03 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 02/16/17 19:32, Ben Warren wrote:
>>
>>> On Feb 16, 2017, at 1:56 AM, Igor Mammedov <imammedo@redhat.com> wrote:
>>>
>>> On Wed, 15 Feb 2017 22:18:14 -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>
>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>>
>>> Thing to get fixed on top is to check
>>> that there isn't vmgenid device present already
>>> into realizefn() and fail gracefully is there is.
>>>
>> I made the change as follows:
>> ===
>>
>> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
>> index c8465df..fa3de6d 100644
>> --- a/hw/acpi/vmgenid.c
>> +++ b/hw/acpi/vmgenid.c
>> @@ -207,8 +207,15 @@ static void vmgenid_handle_reset(void *opaque)
>>
>> static void vmgenid_realize(DeviceState *dev, Error **errp)
>> {
>> + static bool is_realized = false;
>> VmGenIdState *vms = VMGENID(dev);
>> +
>> + if (is_realized) {
>> + error_setg(errp, "Device %s may only be specified once",
>> + VMGENID_DEVICE);
>> + }
>> qemu_register_reset(vmgenid_handle_reset, vms);
>> + is_realized = true;
>> }
>>
>> ===
>> Sample output:
>> $ ../workspace/qemu/x86_64-softmmu/qemu-system-x86_64 -nographic -bios ../workspace/seabios/out/bios.bin -machine type=q35 -hda beiwe-with-vmgenid-driver.qcow2 -qmp unix:./qmp-sock,server,nowait -device vmgenid,guid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb66" -device vmgenid,guid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb67"
>> qemu-system-x86_64: -device vmgenid,guid=324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb67: Device vmgenid may only be specified once
>>
>>
>> Good enough? If so, I’ll roll it in. Otherwise, it’ll be a supplemental patch.
>
> Ehm, I'd say let's postpone that patch. Function (or even file) scope
> static variables that plaster over QOM shortcomings (or, well,
> "non-trivial QOM patterns") are strongly frowned upon. I've done it
> myself before (totally as a last resort, really -- not kidding!), and it
> was real hard to get into the tree.
>
> This deserves more discussion, so please delay it.
>
> (While at it, please remember my other recommendation for realize():
> enforcing that the running machine type / version provides the guest
> with the DMA interface for fw_cfg. For that, based on prior consensus,
> you'll need another device property, to be set in parallel to
> fw_cfg_mem's and fw_cfg_io's "dma-enabled" property. So I think it makes
> sense to address all of these things in a separate, followup series.)
>
OK, makes sense.
> Thanks!
> Laszlo
—Ben
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3583 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID
2017-02-16 6:18 [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID ben
` (8 preceding siblings ...)
2017-02-16 14:29 ` [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID Igor Mammedov
@ 2017-02-16 20:55 ` Laszlo Ersek
9 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2017-02-16 20:55 UTC (permalink / raw)
To: ben, qemu-devel; +Cc: imammedo, mst
On 02/16/17 07:18, ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
>
> This patch set adds support for passing a GUID to Windows guests. It is a
> re-implementation of previous patch sets written by Igor Mammedov et al, but
> this time passing the GUID data as a fw_cfg blob.
>
> This patch set has dependencies on new guest functionality, in particular the
> support for a new linker-loader command and the ability to write back data
> to QEMU over a DMA link. Work is in flight in both SeaBIOS and OVMF to support this.
>
> v6->v7:
> - Rebased to top of tree.
> - Added 'src_offset' field to "write pointer" command
> - Reworked unit tests based on feedback
> - various minor changes based on feedback
> - Added entries to MAINTAINERS file
I posted the OVMF series:
[edk2] [PATCH 0/5] OvmfPkg: support QEMU_LOADER_WRITE_POINTER
Message-Id: <20170216204137.30221-1-lersek@redhat.com>
https://lists.01.org/pipermail/edk2-devel/2017-February/007454.html
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2017-02-16 20:55 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-16 6:18 [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID ben
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 1/8] linker-loader: Add new 'write pointer' command ben
2017-02-16 9:43 ` Igor Mammedov
2017-02-16 14:43 ` Michael S. Tsirkin
2017-02-16 15:48 ` Eric Blake
2017-02-16 17:01 ` Laszlo Ersek
2017-02-16 17:04 ` Ben Warren
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 2/8] docs: VM Generation ID device description ben
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 3/8] ACPI: Add vmgenid blob storage to the build tables ben
2017-02-16 17:05 ` Laszlo Ersek
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 4/8] ACPI: Add Virtual Machine Generation ID support ben
2017-02-16 9:56 ` Igor Mammedov
2017-02-16 18:32 ` Ben Warren
2017-02-16 19:03 ` Laszlo Ersek
2017-02-16 19:05 ` Ben Warren
2017-02-16 17:11 ` Laszlo Ersek
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 5/8] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands ben
2017-02-16 17:13 ` Laszlo Ersek
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 6/8] tests: Move reusable ACPI code into a utility file ben
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 7/8] tests: Add unit tests for the VM Generation ID feature ben
2017-02-16 10:36 ` Igor Mammedov
2017-02-16 17:05 ` Ben Warren
2017-02-16 6:18 ` [Qemu-devel] [PATCH v7 8/8] MAINTAINERS: Add VM Generation ID entry ben
2017-02-16 10:44 ` Laszlo Ersek
2017-02-16 14:29 ` [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID Igor Mammedov
2017-02-16 14:50 ` Ben Warren
2017-02-16 20:55 ` Laszlo Ersek
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.