* [Qemu-devel] [PULL 01/15] linker-loader: Add new 'write pointer' command
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
@ 2017-03-02 6:20 ` Michael S. Tsirkin
2017-03-02 6:20 ` [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description Michael S. Tsirkin
` (16 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 6:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Ben Warren, Laszlo Ersek, Igor Mammedov
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/acpi/bios-linker-loader.h | 7 ++++
hw/acpi/bios-linker-loader.c | 66 ++++++++++++++++++++++++++++++++++--
2 files changed, 70 insertions(+), 3 deletions(-)
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
diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
index d963ebe..046183a 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(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);
+
+ g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
+}
--
MST
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
2017-03-02 6:20 ` [Qemu-devel] [PULL 01/15] linker-loader: Add new 'write pointer' command Michael S. Tsirkin
@ 2017-03-02 6:20 ` Michael S. Tsirkin
2017-04-12 20:06 ` Marc-André Lureau
2017-03-02 6:20 ` [Qemu-devel] [PULL 03/15] ACPI: Add vmgenid blob storage to the build tables Michael S. Tsirkin
` (15 subsequent siblings)
17 siblings, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 6:20 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Ben Warren, Gal Hammer, Laszlo Ersek, Igor Mammedov
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>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@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.
--
MST
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description
2017-03-02 6:20 ` [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description Michael S. Tsirkin
@ 2017-04-12 20:06 ` Marc-André Lureau
2017-04-12 20:17 ` Laszlo Ersek
` (2 more replies)
0 siblings, 3 replies; 41+ messages in thread
From: Marc-André Lureau @ 2017-04-12 20:06 UTC (permalink / raw)
To: Michael S. Tsirkin, qemu-devel
Cc: Gal Hammer, Peter Maydell, Laszlo Ersek, Ben Warren, Igor Mammedov
Hi
On Thu, Mar 2, 2017 at 10:22 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> 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>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@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 default will keep uuid to null, should it be documented? Wouldn't it
make sense to default to 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.
>
Is this supposed to be not permitted?
{ "execute": "qom-set", "arguments": { "path":
"/machine/peripheral-anon/device[1]", "property": "guid", "value": "auto" }
}
Is there any linux kernel support being worked on?
thanks
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description
2017-04-12 20:06 ` Marc-André Lureau
@ 2017-04-12 20:17 ` Laszlo Ersek
2017-04-12 20:17 ` Ben Warren
2017-04-12 20:20 ` Michael S. Tsirkin
2 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2017-04-12 20:17 UTC (permalink / raw)
To: Marc-André Lureau, Michael S. Tsirkin, qemu-devel
Cc: Gal Hammer, Peter Maydell, Ben Warren, Igor Mammedov
On 04/12/17 22:06, Marc-André Lureau wrote:
> On Thu, Mar 2, 2017 at 10:22 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>> +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 default will keep uuid to null, should it be documented? Wouldn't it
> make sense to default to auto?
I guess it might.
>
>
>> +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.
>>
>
> Is this supposed to be not permitted?
>
> { "execute": "qom-set", "arguments": { "path":
> "/machine/peripheral-anon/device[1]", "property": "guid", "value": "auto" }
> }
I don't know if qom-set can be disabled for individual devices / device
properties. Either way, setting a new GUID for VMGENID will definitely
take custom code.
> Is there any linux kernel support being worked on?
There's an RHBZ for it (alias "vmgenid-kernel"):
https://bugzilla.redhat.com/show_bug.cgi?id=1159983
(It is private because RHBZs for the kernel component are private by
default.)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description
2017-04-12 20:06 ` Marc-André Lureau
2017-04-12 20:17 ` Laszlo Ersek
@ 2017-04-12 20:17 ` Ben Warren
2017-04-12 20:22 ` Marc-André Lureau
2017-04-12 20:20 ` Michael S. Tsirkin
2 siblings, 1 reply; 41+ messages in thread
From: Ben Warren @ 2017-04-12 20:17 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Michael S. Tsirkin, qemu-devel, Gal Hammer, Peter Maydell,
Laszlo Ersek, Igor Mammedov
> On Apr 12, 2017, at 1:06 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, Mar 2, 2017 at 10:22 AM Michael S. Tsirkin <mst@redhat.com <mailto:mst@redhat.com>> wrote:
> From: Ben Warren <ben@skyportsystems.com <mailto:ben@skyportsystems.com>>
>
> This patch is based off an earlier version by
> Gal Hammer (ghammer@redhat.com <mailto:ghammer@redhat.com>)
>
> Requirements section, ASCII diagrams and overall help
> provided by Laszlo Ersek (lersek@redhat.com <mailto:lersek@redhat.com>)
>
> Signed-off-by: Gal Hammer <ghammer@redhat.com <mailto:ghammer@redhat.com>>
> Signed-off-by: Ben Warren <ben@skyportsystems.com <mailto:ben@skyportsystems.com>>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com <mailto:imammedo@redhat.com>>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com <mailto:mst@redhat.com>>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com <mailto:mst@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 <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 default will keep uuid to null, should it be documented? Wouldn't it make sense to default to auto?
There is no default - you have to supply a value. It’s up to whatever software is managing VM lifecycle to decide what value to pass in. Always setting to ‘auto’ will cause a lot of churn within Windows that may or may not be acceptable to your use case.
>
> +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.
>
> Is this supposed to be not permitted?
>
> { "execute": "qom-set", "arguments": { "path": "/machine/peripheral-anon/device[1]", "property": "guid", "value": "auto" } }
>
> Is there any linux kernel support being worked on?
This isn’t really relevant to the Linux kernel, at least in any way I can think of. What did you have in mind?
>
> thanks
> --
> Marc-André Lureau
—Ben
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description
2017-04-12 20:17 ` Ben Warren
@ 2017-04-12 20:22 ` Marc-André Lureau
2017-04-12 20:25 ` Ben Warren
0 siblings, 1 reply; 41+ messages in thread
From: Marc-André Lureau @ 2017-04-12 20:22 UTC (permalink / raw)
To: Ben Warren
Cc: Michael S. Tsirkin, qemu-devel, Gal Hammer, Peter Maydell,
Laszlo Ersek, Igor Mammedov
Hi
On Thu, Apr 13, 2017 at 12:17 AM Ben Warren <ben@skyportsystems.com> wrote:
> On Apr 12, 2017, at 1:06 PM, Marc-André Lureau <marcandre.lureau@gmail.com>
> wrote:
>
> +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 default will keep uuid to null, should it be documented? Wouldn't it
> make sense to default to auto?
>
> There is no default - you have to supply a value. It’s up to whatever
> software is managing VM lifecycle to decide what value to pass in. Always
> setting to ‘auto’ will cause a lot of churn within Windows that may or may
> not be acceptable to your use case.
>
>
Why would you have a vmgenid device if it's always null? Does that please
some windows use-cases as well?
>
>
> +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.
>
>
> Is this supposed to be not permitted?
>
> { "execute": "qom-set", "arguments": { "path":
> "/machine/peripheral-anon/device[1]", "property": "guid", "value": "auto" }
> }
>
> Is there any linux kernel support being worked on?
>
> This isn’t really relevant to the Linux kernel, at least in any way I can
> think of. What did you have in mind?
>
Testing, but apparently we do have RFE for RHEL as Laszlo pointed out.
Thanks
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description
2017-04-12 20:22 ` Marc-André Lureau
@ 2017-04-12 20:25 ` Ben Warren
2017-04-12 20:47 ` Marc-André Lureau
0 siblings, 1 reply; 41+ messages in thread
From: Ben Warren @ 2017-04-12 20:25 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Michael S. Tsirkin, qemu-devel, Gal Hammer, Peter Maydell,
Laszlo Ersek, Igor Mammedov
> On Apr 12, 2017, at 1:22 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, Apr 13, 2017 at 12:17 AM Ben Warren <ben@skyportsystems.com <mailto:ben@skyportsystems.com>> wrote:
>> On Apr 12, 2017, at 1:06 PM, Marc-André Lureau <marcandre.lureau@gmail.com <mailto:marcandre.lureau@gmail.com>> wrote:
>>
>> +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 default will keep uuid to null, should it be documented? Wouldn't it make sense to default to auto?
>
> There is no default - you have to supply a value. It’s up to whatever software is managing VM lifecycle to decide what value to pass in. Always setting to ‘auto’ will cause a lot of churn within Windows that may or may not be acceptable to your use case.
>
>
> Why would you have a vmgenid device if it's always null? Does that please some windows use-cases as well?
>
I don’t get what you mean by this. What device is always null? Either the device is instantiated or it isn’t. If not there, Windows will not find a device and I don’t know how derived objects (Invocation ID, etc.) are handled.
>>
>> +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.
>>
>> Is this supposed to be not permitted?
>>
>> { "execute": "qom-set", "arguments": { "path": "/machine/peripheral-anon/device[1]", "property": "guid", "value": "auto" } }
>>
>> Is there any linux kernel support being worked on?
>
> This isn’t really relevant to the Linux kernel, at least in any way I can think of. What did you have in mind?
>
> Testing, but apparently we do have RFE for RHEL as Laszlo pointed out.
OK, so you mean a guest driver. I do have one that needs work to go upstream, but has been helpful to me in testing.
https://github.com/ben-skyportsystems/vmgenid-test <https://github.com/ben-skyportsystems/vmgenid-test>
>
> Thanks
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description
2017-04-12 20:25 ` Ben Warren
@ 2017-04-12 20:47 ` Marc-André Lureau
2017-04-12 21:03 ` Ben Warren
0 siblings, 1 reply; 41+ messages in thread
From: Marc-André Lureau @ 2017-04-12 20:47 UTC (permalink / raw)
To: Ben Warren
Cc: Michael S. Tsirkin, qemu-devel, Gal Hammer, Peter Maydell,
Laszlo Ersek, Igor Mammedov
Hi
On Thu, Apr 13, 2017 at 12:25 AM Ben Warren <ben@skyportsystems.com> wrote:
> On Apr 12, 2017, at 1:22 PM, Marc-André Lureau <marcandre.lureau@gmail.com>
> wrote:
>
> Hi
>
> On Thu, Apr 13, 2017 at 12:17 AM Ben Warren <ben@skyportsystems.com>
> wrote:
>
> On Apr 12, 2017, at 1:06 PM, Marc-André Lureau <marcandre.lureau@gmail.com>
> wrote:
>
> +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 default will keep uuid to null, should it be documented? Wouldn't it
> make sense to default to auto?
>
> There is no default - you have to supply a value. It’s up to whatever
> software is managing VM lifecycle to decide what value to pass in. Always
> setting to ‘auto’ will cause a lot of churn within Windows that may or may
> not be acceptable to your use case.
>
>
> Why would you have a vmgenid device if it's always null? Does that please
> some windows use-cases as well?
>
>
> I don’t get what you mean by this. What device is always null? Either
> the device is instantiated or it isn’t. If not there, Windows will not
> find a device and I don’t know how derived objects (Invocation ID, etc.)
> are handled.
>
If you start a VM without specifying guid argument, you'll always have a
genid null uuid, event after a migration (this could have been handled by
qemu without requiring management layer, no?). I don't understand why auto
would create more churn than what the management layer would do by setting
new uuid for each VM started. Could you explain?
>
>
> +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.
>
>
> Is this supposed to be not permitted?
>
> { "execute": "qom-set", "arguments": { "path":
> "/machine/peripheral-anon/device[1]", "property": "guid", "value": "auto" }
> }
>
> Is there any linux kernel support being worked on?
>
> This isn’t really relevant to the Linux kernel, at least in any way I can
> think of. What did you have in mind?
>
>
> Testing, but apparently we do have RFE for RHEL as Laszlo pointed out.
>
> OK, so you mean a guest driver. I do have one that needs work to go
> upstream, but has been helpful to me in testing.
> https://github.com/ben-skyportsystems/vmgenid-test
>
Thanks, that's exactly what I was looking for :)
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description
2017-04-12 20:47 ` Marc-André Lureau
@ 2017-04-12 21:03 ` Ben Warren
2017-04-12 21:17 ` Marc-André Lureau
0 siblings, 1 reply; 41+ messages in thread
From: Ben Warren @ 2017-04-12 21:03 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Michael S. Tsirkin, qemu-devel, Gal Hammer, Peter Maydell,
Laszlo Ersek, Igor Mammedov
> On Apr 12, 2017, at 1:47 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, Apr 13, 2017 at 12:25 AM Ben Warren <ben@skyportsystems.com <mailto:ben@skyportsystems.com>> wrote:
>> On Apr 12, 2017, at 1:22 PM, Marc-André Lureau <marcandre.lureau@gmail.com <mailto:marcandre.lureau@gmail.com>> wrote:
>>
>> Hi
>>
>> On Thu, Apr 13, 2017 at 12:17 AM Ben Warren <ben@skyportsystems.com <mailto:ben@skyportsystems.com>> wrote:
>>> On Apr 12, 2017, at 1:06 PM, Marc-André Lureau <marcandre.lureau@gmail.com <mailto:marcandre.lureau@gmail.com>> wrote:
>>>
>>> +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 default will keep uuid to null, should it be documented? Wouldn't it make sense to default to auto?
>>
>> There is no default - you have to supply a value. It’s up to whatever software is managing VM lifecycle to decide what value to pass in. Always setting to ‘auto’ will cause a lot of churn within Windows that may or may not be acceptable to your use case.
>>
>>
>> Why would you have a vmgenid device if it's always null? Does that please some windows use-cases as well?
>>
>
> I don’t get what you mean by this. What device is always null? Either the device is instantiated or it isn’t. If not there, Windows will not find a device and I don’t know how derived objects (Invocation ID, etc.) are handled.
>
> If you start a VM without specifying guid argument, you'll always have a genid null uuid, event after a migration (this could have been handled by qemu without requiring management layer, no?). I don't understand why auto would create more churn than what the management layer would do by setting new uuid for each VM started. Could you explain?
>
Looks like there’s a bug. GUID should be a mandatory parameter. As for the churn, I’ll give you one example. If an Active Directory Domain Controller (ADDC) detects a change in VM Generation ID, it takes this to mean that the VM has been rolled back in time, and so its replication sequence numbers are “dirty”. This has the effect of causing the Domain controller to perform a full “pull replication” with other ADDCs. In large deployments this can be costly. VM Generation ID is used by other applications besides AD.
>
>>>
>>> +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.
>>>
>>> Is this supposed to be not permitted?
>>>
>>> { "execute": "qom-set", "arguments": { "path": "/machine/peripheral-anon/device[1]", "property": "guid", "value": "auto" } }
>>>
>>> Is there any linux kernel support being worked on?
>>
>> This isn’t really relevant to the Linux kernel, at least in any way I can think of. What did you have in mind?
>>
>> Testing, but apparently we do have RFE for RHEL as Laszlo pointed out.
>
> OK, so you mean a guest driver. I do have one that needs work to go upstream, but has been helpful to me in testing.
> https://github.com/ben-skyportsystems/vmgenid-test <https://github.com/ben-skyportsystems/vmgenid-test>
>
> Thanks, that's exactly what I was looking for :)
Good. I wish I had the time to integrate this upstream, but it’s one of those things that is good enough, and so will have to wait for another time.
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description
2017-04-12 21:03 ` Ben Warren
@ 2017-04-12 21:17 ` Marc-André Lureau
2017-04-12 21:25 ` Michael S. Tsirkin
2017-04-13 10:18 ` Marc-André Lureau
0 siblings, 2 replies; 41+ messages in thread
From: Marc-André Lureau @ 2017-04-12 21:17 UTC (permalink / raw)
To: Ben Warren
Cc: Michael S. Tsirkin, qemu-devel, Gal Hammer, Peter Maydell,
Laszlo Ersek, Igor Mammedov
Hi
On Thu, Apr 13, 2017 at 1:03 AM Ben Warren <ben@skyportsystems.com> wrote:
> On Apr 12, 2017, at 1:47 PM, Marc-André Lureau <marcandre.lureau@gmail.com>
> wrote:
>
> Hi
>
> On Thu, Apr 13, 2017 at 12:25 AM Ben Warren <ben@skyportsystems.com>
> wrote:
>
> On Apr 12, 2017, at 1:22 PM, Marc-André Lureau <marcandre.lureau@gmail.com>
> wrote:
>
> Hi
>
> On Thu, Apr 13, 2017 at 12:17 AM Ben Warren <ben@skyportsystems.com>
> wrote:
>
> On Apr 12, 2017, at 1:06 PM, Marc-André Lureau <marcandre.lureau@gmail.com>
> wrote:
>
> +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 default will keep uuid to null, should it be documented? Wouldn't it
> make sense to default to auto?
>
> There is no default - you have to supply a value. It’s up to whatever
> software is managing VM lifecycle to decide what value to pass in. Always
> setting to ‘auto’ will cause a lot of churn within Windows that may or may
> not be acceptable to your use case.
>
>
> Why would you have a vmgenid device if it's always null? Does that please
> some windows use-cases as well?
>
>
> I don’t get what you mean by this. What device is always null? Either
> the device is instantiated or it isn’t. If not there, Windows will not
> find a device and I don’t know how derived objects (Invocation ID, etc.)
> are handled.
>
>
> If you start a VM without specifying guid argument, you'll always have a
> genid null uuid, event after a migration (this could have been handled by
> qemu without requiring management layer, no?). I don't understand why auto
> would create more churn than what the management layer would do by setting
> new uuid for each VM started. Could you explain?
>
> Looks like there’s a bug. GUID should be a mandatory parameter.
>
Not necessarily a bug, if the guid can be changed when starting a "new" VM,
which I think should work.
However, I didn't manage to get your driver noticing the acpi event. I
tried to migrate/save & restore, and no vmgenid_notify kernel messages came
out, nor notices got incremented. How did you test it?
> As for the churn, I’ll give you one example. If an Active Directory
> Domain Controller (ADDC) detects a change in VM Generation ID, it takes
> this to mean that the VM has been rolled back in time, and so its
> replication sequence numbers are “dirty”. This has the effect of causing
> the Domain controller to perform a full “pull replication” with other
> ADDCs. In large deployments this can be costly. VM Generation ID is used
> by other applications besides AD.
>
>
I start to understand better the use case and how the device should be used.
thanks again
>
>
>
> +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.
>
>
> Is this supposed to be not permitted?
>
> { "execute": "qom-set", "arguments": { "path":
> "/machine/peripheral-anon/device[1]", "property": "guid", "value": "auto" }
> }
>
> Is there any linux kernel support being worked on?
>
> This isn’t really relevant to the Linux kernel, at least in any way I can
> think of. What did you have in mind?
>
>
> Testing, but apparently we do have RFE for RHEL as Laszlo pointed out.
>
> OK, so you mean a guest driver. I do have one that needs work to go
> upstream, but has been helpful to me in testing.
> https://github.com/ben-skyportsystems/vmgenid-test
>
>
> Thanks, that's exactly what I was looking for :)
>
>
> Good. I wish I had the time to integrate this upstream, but it’s one of
> those things that is good enough, and so will have to wait for another time.
>
> --
> Marc-André Lureau
>
> --
Marc-André Lureau
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description
2017-04-12 21:17 ` Marc-André Lureau
@ 2017-04-12 21:25 ` Michael S. Tsirkin
2017-04-13 10:18 ` Marc-André Lureau
1 sibling, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-04-12 21:25 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Ben Warren, qemu-devel, Gal Hammer, Peter Maydell, Laszlo Ersek,
Igor Mammedov
On Wed, Apr 12, 2017 at 09:17:12PM +0000, Marc-André Lureau wrote:
> Hi
>
> On Thu, Apr 13, 2017 at 1:03 AM Ben Warren <ben@skyportsystems.com> wrote:
>
> On Apr 12, 2017, at 1:47 PM, Marc-André Lureau <
> marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, Apr 13, 2017 at 12:25 AM Ben Warren <ben@skyportsystems.com>
> wrote:
>
> On Apr 12, 2017, at 1:22 PM, Marc-André Lureau <
> marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, Apr 13, 2017 at 12:17 AM Ben Warren <
> ben@skyportsystems.com> wrote:
>
> On Apr 12, 2017, at 1:06 PM, Marc-André Lureau <
> marcandre.lureau@gmail.com> wrote:
>
>
> +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 default will keep uuid to null, should it be
> documented? Wouldn't it make sense to default to auto?
>
> There is no default - you have to supply a value. It’s up
> to whatever software is managing VM lifecycle to decide
> what value to pass in. Always setting to ‘auto’ will cause
> a lot of churn within Windows that may or may not be
> acceptable to your use case.
>
>
>
> Why would you have a vmgenid device if it's always null? Does
> that please some windows use-cases as well?
>
>
> I don’t get what you mean by this. What device is always null?
> Either the device is instantiated or it isn’t. If not there,
> Windows will not find a device and I don’t know how derived objects
> (Invocation ID, etc.) are handled.
>
>
> If you start a VM without specifying guid argument, you'll always have
> a genid null uuid, event after a migration (this could have been
> handled by qemu without requiring management layer, no?). I don't
> understand why auto would create more churn than what the management
> layer would do by setting new uuid for each VM started. Could you
> explain?
>
>
> Looks like there’s a bug. GUID should be a mandatory parameter.
>
>
> Not necessarily a bug, if the guid can be changed when starting a "new" VM,
> which I think should work.
I think spec does not allow for a special "invalid" guid value ATM.
> However, I didn't manage to get your driver noticing the acpi event. I tried to
> migrate/save & restore, and no vmgenid_notify kernel messages came out, nor
> notices got incremented. How did you test it?
>
>
> As for the churn, I’ll give you one example. If an Active Directory Domain
> Controller (ADDC) detects a change in VM Generation ID, it takes this to
> mean that the VM has been rolled back in time, and so its replication
> sequence numbers are “dirty”. This has the effect of causing the Domain
> controller to perform a full “pull replication” with other ADDCs. In large
> deployments this can be costly. VM Generation ID is used by other
> applications besides AD.
>
>
>
>
> I start to understand better the use case and how the device should be used.
>
> thanks again
>
>
>
>
>
> +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.
>
>
> Is this supposed to be not permitted?
>
> { "execute": "qom-set", "arguments": { "path": "/
> machine/peripheral-anon/device[1]", "property": "guid",
> "value": "auto" } }
>
> Is there any linux kernel support being worked on?
>
> This isn’t really relevant to the Linux kernel, at least in
> any way I can think of. What did you have in mind?
>
>
> Testing, but apparently we do have RFE for RHEL as Laszlo
> pointed out.
>
> OK, so you mean a guest driver. I do have one that needs work to
> go upstream, but has been helpful to me in testing.
> https://github.com/ben-skyportsystems/vmgenid-test
>
>
> Thanks, that's exactly what I was looking for :)
>
>
> Good. I wish I had the time to integrate this upstream, but it’s one of
> those things that is good enough, and so will have to wait for another
> time.
>
>
> --
> Marc-André Lureau
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description
2017-04-12 21:17 ` Marc-André Lureau
2017-04-12 21:25 ` Michael S. Tsirkin
@ 2017-04-13 10:18 ` Marc-André Lureau
1 sibling, 0 replies; 41+ messages in thread
From: Marc-André Lureau @ 2017-04-13 10:18 UTC (permalink / raw)
To: Ben Warren
Cc: Michael S. Tsirkin, qemu-devel, Gal Hammer, Peter Maydell,
Laszlo Ersek, Igor Mammedov
Hi
On Thu, Apr 13, 2017 at 1:17 AM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Thu, Apr 13, 2017 at 1:03 AM Ben Warren <ben@skyportsystems.com> wrote:
>
> On Apr 12, 2017, at 1:47 PM, Marc-André Lureau <marcandre.lureau@gmail.com>
> wrote:
>
> Hi
>
> On Thu, Apr 13, 2017 at 12:25 AM Ben Warren <ben@skyportsystems.com>
> wrote:
>
> On Apr 12, 2017, at 1:22 PM, Marc-André Lureau <marcandre.lureau@gmail.com>
> wrote:
>
> Hi
>
> On Thu, Apr 13, 2017 at 12:17 AM Ben Warren <ben@skyportsystems.com>
> wrote:
>
> On Apr 12, 2017, at 1:06 PM, Marc-André Lureau <marcandre.lureau@gmail.com>
> wrote:
>
> +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 default will keep uuid to null, should it be documented? Wouldn't it
> make sense to default to auto?
>
> There is no default - you have to supply a value. It’s up to whatever
> software is managing VM lifecycle to decide what value to pass in. Always
> setting to ‘auto’ will cause a lot of churn within Windows that may or may
> not be acceptable to your use case.
>
>
> Why would you have a vmgenid device if it's always null? Does that please
> some windows use-cases as well?
>
>
> I don’t get what you mean by this. What device is always null? Either
> the device is instantiated or it isn’t. If not there, Windows will not
> find a device and I don’t know how derived objects (Invocation ID, etc.)
> are handled.
>
>
> If you start a VM without specifying guid argument, you'll always have a
> genid null uuid, event after a migration (this could have been handled by
> qemu without requiring management layer, no?). I don't understand why auto
> would create more churn than what the management layer would do by setting
> new uuid for each VM started. Could you explain?
>
> Looks like there’s a bug. GUID should be a mandatory parameter.
>
>
> Not necessarily a bug, if the guid can be changed when starting a "new"
> VM, which I think should work.
>
> However, I didn't manage to get your driver noticing the acpi event. I
> tried to migrate/save & restore, and no vmgenid_notify kernel messages came
> out, nor notices got incremented. How did you test it?
>
>
Actually I was using an old bios, now I can change the guid and the guest
notices the change.
{ "execute": "qom-set", "arguments": { "path":
"/machine/peripheral-anon/device[1]", "property": "guid", "value": "..." } }
works, the guest noticed the change and the new value can be read. I don't
know what else would be required (except a better API) to allow QMP to
modify it.
thanks
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description
2017-04-12 20:06 ` Marc-André Lureau
2017-04-12 20:17 ` Laszlo Ersek
2017-04-12 20:17 ` Ben Warren
@ 2017-04-12 20:20 ` Michael S. Tsirkin
2 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-04-12 20:20 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Gal Hammer, Peter Maydell, Laszlo Ersek, Ben Warren,
Igor Mammedov
On Wed, Apr 12, 2017 at 08:06:32PM +0000, Marc-André Lureau wrote:
> Hi
>
> On Thu, Mar 2, 2017 at 10:22 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> 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>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@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 default will keep uuid to null, should it be documented? Wouldn't it make
> sense to default to auto?
Interesting. I'd say default should fail init.
>
> +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.
>
>
> Is this supposed to be not permitted?
>
> { "execute": "qom-set", "arguments": { "path": "/machine/peripheral-anon/device
> [1]", "property": "guid", "value": "auto" } }
anon means in particular no stability.
Also we are yet to tie this to generate
an interrupt and update guest memory properly.
Patches welcome.
> Is there any linux kernel support being worked on?
I vaguely rememeber some patches but couldn't find them.
An ACPI driver would be easy to implement, one thing to
be careful about is to make sure all memory maps are cached
as spec requires this.
> thanks
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PULL 03/15] ACPI: Add vmgenid blob storage to the build tables
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
2017-03-02 6:20 ` [Qemu-devel] [PULL 01/15] linker-loader: Add new 'write pointer' command Michael S. Tsirkin
2017-03-02 6:20 ` [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description Michael S. Tsirkin
@ 2017-03-02 6:20 ` Michael S. Tsirkin
2017-03-02 6:20 ` [Qemu-devel] [PULL 04/15] ACPI: Add Virtual Machine Generation ID support Michael S. Tsirkin
` (14 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 6:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Ben Warren, Laszlo Ersek, Igor Mammedov
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>
Tested-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/acpi/aml-build.h | 1 +
hw/acpi/aml-build.c | 2 ++
2 files changed, 3 insertions(+)
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;
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 */
--
MST
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PULL 04/15] ACPI: Add Virtual Machine Generation ID support
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
` (2 preceding siblings ...)
2017-03-02 6:20 ` [Qemu-devel] [PULL 03/15] ACPI: Add vmgenid blob storage to the build tables Michael S. Tsirkin
@ 2017-03-02 6:20 ` Michael S. Tsirkin
2017-03-02 6:20 ` [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands Michael S. Tsirkin
` (13 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 6:20 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Ben Warren, Igor Mammedov, Laszlo Ersek,
Paolo Bonzini, Richard Henderson, Eduardo Habkost
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
default-configs/i386-softmmu.mak | 1 +
default-configs/x86_64-softmmu.mak | 1 +
include/hw/acpi/acpi_dev_interface.h | 1 +
include/hw/acpi/vmgenid.h | 35 +++++
hw/acpi/vmgenid.c | 242 +++++++++++++++++++++++++++++++++++
hw/i386/acpi-build.c | 16 +++
hw/acpi/Makefile.objs | 1 +
7 files changed, 297 insertions(+)
create mode 100644 include/hw/acpi/vmgenid.h
create mode 100644 hw/acpi/vmgenid.c
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/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
diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
new file mode 100644
index 0000000..c8465df
--- /dev/null
+++ b/hw/acpi/vmgenid.c
@@ -0,0 +1,242 @@
+/*
+ * 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.
+ * The address that is patched in is offset in order to implement
+ * the "OVMF SDT Header probe suppressor"
+ * see docs/specs/vmgenid.txt for more details.
+ */
+ 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/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
--
MST
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
` (3 preceding siblings ...)
2017-03-02 6:20 ` [Qemu-devel] [PULL 04/15] ACPI: Add Virtual Machine Generation ID support Michael S. Tsirkin
@ 2017-03-02 6:20 ` Michael S. Tsirkin
2017-03-02 8:11 ` Markus Armbruster
2017-03-02 6:20 ` [Qemu-devel] [PULL 06/15] tests: Move reusable ACPI code into a utility file Michael S. Tsirkin
` (12 subsequent siblings)
17 siblings, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 6:20 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Igor Mammedov, Eric Blake, Ben Warren,
Laszlo Ersek, Dr. David Alan Gilbert, Markus Armbruster,
Paolo Bonzini
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>
Tested-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
qapi-schema.json | 20 ++++++++++++++++++++
hmp.h | 1 +
hmp.c | 9 +++++++++
hw/acpi/vmgenid.c | 16 ++++++++++++++++
stubs/vmgenid.c | 9 +++++++++
hmp-commands-info.hx | 14 ++++++++++++++
stubs/Makefile.objs | 1 +
7 files changed, 70 insertions(+)
create mode 100644 stubs/vmgenid.c
diff --git a/qapi-schema.json b/qapi-schema.json
index d6186d4..84692da 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -6188,3 +6188,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/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/hmp.c b/hmp.c
index 83e287e..aadbcf5 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2576,3 +2576,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/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
index c8465df..744f284 100644
--- a/hw/acpi/vmgenid.c
+++ b/hw/acpi/vmgenid.c
@@ -240,3 +240,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/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;
+}
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/stubs/Makefile.objs b/stubs/Makefile.objs
index aa6050f..224f04b 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -36,3 +36,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
--
MST
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands
2017-03-02 6:20 ` [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands Michael S. Tsirkin
@ 2017-03-02 8:11 ` Markus Armbruster
2017-03-02 8:25 ` Laszlo Ersek
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2017-03-02 8:11 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Peter Maydell, Ben Warren, Laszlo Ersek,
Dr. David Alan Gilbert, Paolo Bonzini, Igor Mammedov
Crash bug, I'm afraid...
"Michael S. Tsirkin" <mst@redhat.com> writes:
> 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>
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> qapi-schema.json | 20 ++++++++++++++++++++
> hmp.h | 1 +
> hmp.c | 9 +++++++++
> hw/acpi/vmgenid.c | 16 ++++++++++++++++
> stubs/vmgenid.c | 9 +++++++++
> hmp-commands-info.hx | 14 ++++++++++++++
> stubs/Makefile.objs | 1 +
> 7 files changed, 70 insertions(+)
> create mode 100644 stubs/vmgenid.c
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index d6186d4..84692da 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -6188,3 +6188,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/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> index c8465df..744f284 100644
> --- a/hw/acpi/vmgenid.c
> +++ b/hw/acpi/vmgenid.c
> @@ -240,3 +240,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;
> +}
This either returns a GuidInfo or NULL.
If it returns NULL, the generated qmp_marshal_output_GuidInfo() will
crash like this:
#0 0x00007fffdb4c7765 in raise () at /lib64/libc.so.6
#1 0x00007fffdb4c936a in abort () at /lib64/libc.so.6
#2 0x00007fffdb4bff97 in __assert_fail_base () at /lib64/libc.so.6
#3 0x00007fffdb4c0042 in () at /lib64/libc.so.6
#4 0x0000555555c800a0 in visit_start_struct (v=
0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, size=8, errp=0x7fffffffc790) at /work/armbru/qemu/qapi/qapi-visit-core.c:49
#5 0x0000555555c5d398 in visit_type_GuidInfo (v=
0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, errp=0x7fffffffc7d8) at qapi-visit.c:6038
#6 0x0000555555922fc0 in qmp_marshal_output_GuidInfo (ret_in=0x0, ret_out=0x7fffffffc870, errp=0x7fffffffc820) at qmp-marshal.c:5148
#7 0x0000555555923123 in qmp_marshal_query_vm_generation_id (args=0x555557a67500, ret=0x7fffffffc870, errp=0x7fffffffc868) at qmp-marshal.c:5186
#8 0x0000555555c83a73 in do_qmp_dispatch (request=0x555556ab3200, errp=0x7fffffffc8c0) at /work/armbru/qemu/qapi/qmp-dispatch.c:97
#9 0x0000555555c83ba3 in qmp_dispatch (request=0x555556ab3200)
at /work/armbru/qemu/qapi/qmp-dispatch.c:124
#10 0x00005555557bb671 in handle_qmp_command (parser=
0x5555568776b0, tokens=0x555556858520) at /work/armbru/qemu/monitor.c:3789
#11 0x0000555555c8af2f in json_message_process_token (lexer=0x5555568776b8, input=0x5555568582e0, type=JSON_RCURLY, x=48, y=1)
at /work/armbru/qemu/qobject/json-streamer.c:105
Sorry for not having reviewed this earlier. On the other hand, how come
this survived testing?
[...]
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands
2017-03-02 8:11 ` Markus Armbruster
@ 2017-03-02 8:25 ` Laszlo Ersek
2017-03-02 8:37 ` Laszlo Ersek
2017-03-02 8:42 ` Markus Armbruster
0 siblings, 2 replies; 41+ messages in thread
From: Laszlo Ersek @ 2017-03-02 8:25 UTC (permalink / raw)
To: Markus Armbruster, Michael S. Tsirkin
Cc: Peter Maydell, Ben Warren, qemu-devel, Dr. David Alan Gilbert,
Igor Mammedov, Paolo Bonzini
On 03/02/17 09:11, Markus Armbruster wrote:
> Crash bug, I'm afraid...
>
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
>> 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>
>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> qapi-schema.json | 20 ++++++++++++++++++++
>> hmp.h | 1 +
>> hmp.c | 9 +++++++++
>> hw/acpi/vmgenid.c | 16 ++++++++++++++++
>> stubs/vmgenid.c | 9 +++++++++
>> hmp-commands-info.hx | 14 ++++++++++++++
>> stubs/Makefile.objs | 1 +
>> 7 files changed, 70 insertions(+)
>> create mode 100644 stubs/vmgenid.c
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index d6186d4..84692da 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -6188,3 +6188,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/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
>> index c8465df..744f284 100644
>> --- a/hw/acpi/vmgenid.c
>> +++ b/hw/acpi/vmgenid.c
>> @@ -240,3 +240,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;
>> +}
>
> This either returns a GuidInfo or NULL.
>
> If it returns NULL, the generated qmp_marshal_output_GuidInfo() will
> crash like this:
>
> #0 0x00007fffdb4c7765 in raise () at /lib64/libc.so.6
> #1 0x00007fffdb4c936a in abort () at /lib64/libc.so.6
> #2 0x00007fffdb4bff97 in __assert_fail_base () at /lib64/libc.so.6
> #3 0x00007fffdb4c0042 in () at /lib64/libc.so.6
> #4 0x0000555555c800a0 in visit_start_struct (v=
> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, size=8, errp=0x7fffffffc790) at /work/armbru/qemu/qapi/qapi-visit-core.c:49
> #5 0x0000555555c5d398 in visit_type_GuidInfo (v=
> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, errp=0x7fffffffc7d8) at qapi-visit.c:6038
> #6 0x0000555555922fc0 in qmp_marshal_output_GuidInfo (ret_in=0x0, ret_out=0x7fffffffc870, errp=0x7fffffffc820) at qmp-marshal.c:5148
> #7 0x0000555555923123 in qmp_marshal_query_vm_generation_id (args=0x555557a67500, ret=0x7fffffffc870, errp=0x7fffffffc868) at qmp-marshal.c:5186
> #8 0x0000555555c83a73 in do_qmp_dispatch (request=0x555556ab3200, errp=0x7fffffffc8c0) at /work/armbru/qemu/qapi/qmp-dispatch.c:97
> #9 0x0000555555c83ba3 in qmp_dispatch (request=0x555556ab3200)
> at /work/armbru/qemu/qapi/qmp-dispatch.c:124
> #10 0x00005555557bb671 in handle_qmp_command (parser=
> 0x5555568776b0, tokens=0x555556858520) at /work/armbru/qemu/monitor.c:3789
> #11 0x0000555555c8af2f in json_message_process_token (lexer=0x5555568776b8, input=0x5555568582e0, type=JSON_RCURLY, x=48, y=1)
> at /work/armbru/qemu/qobject/json-streamer.c:105
>
> Sorry for not having reviewed this earlier. On the other hand, how come
> this survived testing?
We primarily focused on testing the functionality, not the failure /
corner cases, I think. (There are at least two other known startup
scenarios that are not handled correctly just yet, although they don't
crash -- multiple vmgenid devices, and vmgenid on machtypes without
writeable fw_cfg.)
Also, in my testing I only called the monitor command via HMP (from
virsh), and AFAICS that one doesn't crash even if the device is missing.
Nonetheless, qmp_query_vm_generation_id() should set an error when it
returns NULL, yes.
I think Ben wants to post a followup series with those fixes mentioned
above, in time for 2.9, perhaps this crash can be addressed in there
too. Ben?
Thanks
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands
2017-03-02 8:25 ` Laszlo Ersek
@ 2017-03-02 8:37 ` Laszlo Ersek
2017-03-02 9:54 ` Markus Armbruster
2017-03-02 8:42 ` Markus Armbruster
1 sibling, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2017-03-02 8:37 UTC (permalink / raw)
To: Markus Armbruster, Michael S. Tsirkin
Cc: Peter Maydell, Ben Warren, qemu-devel, Dr. David Alan Gilbert,
Igor Mammedov, Paolo Bonzini
On 03/02/17 09:25, Laszlo Ersek wrote:
> On 03/02/17 09:11, Markus Armbruster wrote:
>> Crash bug, I'm afraid...
>>
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>
>>> 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>
>>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>> qapi-schema.json | 20 ++++++++++++++++++++
>>> hmp.h | 1 +
>>> hmp.c | 9 +++++++++
>>> hw/acpi/vmgenid.c | 16 ++++++++++++++++
>>> stubs/vmgenid.c | 9 +++++++++
>>> hmp-commands-info.hx | 14 ++++++++++++++
>>> stubs/Makefile.objs | 1 +
>>> 7 files changed, 70 insertions(+)
>>> create mode 100644 stubs/vmgenid.c
>>>
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index d6186d4..84692da 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -6188,3 +6188,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/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
>>> index c8465df..744f284 100644
>>> --- a/hw/acpi/vmgenid.c
>>> +++ b/hw/acpi/vmgenid.c
>>> @@ -240,3 +240,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;
>>> +}
>>
>> This either returns a GuidInfo or NULL.
>>
>> If it returns NULL, the generated qmp_marshal_output_GuidInfo() will
>> crash like this:
>>
>> #0 0x00007fffdb4c7765 in raise () at /lib64/libc.so.6
>> #1 0x00007fffdb4c936a in abort () at /lib64/libc.so.6
>> #2 0x00007fffdb4bff97 in __assert_fail_base () at /lib64/libc.so.6
>> #3 0x00007fffdb4c0042 in () at /lib64/libc.so.6
>> #4 0x0000555555c800a0 in visit_start_struct (v=
>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, size=8, errp=0x7fffffffc790) at /work/armbru/qemu/qapi/qapi-visit-core.c:49
>> #5 0x0000555555c5d398 in visit_type_GuidInfo (v=
>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, errp=0x7fffffffc7d8) at qapi-visit.c:6038
>> #6 0x0000555555922fc0 in qmp_marshal_output_GuidInfo (ret_in=0x0, ret_out=0x7fffffffc870, errp=0x7fffffffc820) at qmp-marshal.c:5148
>> #7 0x0000555555923123 in qmp_marshal_query_vm_generation_id (args=0x555557a67500, ret=0x7fffffffc870, errp=0x7fffffffc868) at qmp-marshal.c:5186
>> #8 0x0000555555c83a73 in do_qmp_dispatch (request=0x555556ab3200, errp=0x7fffffffc8c0) at /work/armbru/qemu/qapi/qmp-dispatch.c:97
>> #9 0x0000555555c83ba3 in qmp_dispatch (request=0x555556ab3200)
>> at /work/armbru/qemu/qapi/qmp-dispatch.c:124
>> #10 0x00005555557bb671 in handle_qmp_command (parser=
>> 0x5555568776b0, tokens=0x555556858520) at /work/armbru/qemu/monitor.c:3789
>> #11 0x0000555555c8af2f in json_message_process_token (lexer=0x5555568776b8, input=0x5555568582e0, type=JSON_RCURLY, x=48, y=1)
>> at /work/armbru/qemu/qobject/json-streamer.c:105
>>
>> Sorry for not having reviewed this earlier. On the other hand, how come
>> this survived testing?
>
> We primarily focused on testing the functionality, not the failure /
> corner cases, I think. (There are at least two other known startup
> scenarios that are not handled correctly just yet, although they don't
> crash -- multiple vmgenid devices, and vmgenid on machtypes without
> writeable fw_cfg.)
>
> Also, in my testing I only called the monitor command via HMP (from
> virsh), and AFAICS that one doesn't crash even if the device is missing.
> Nonetheless, qmp_query_vm_generation_id() should set an error when it
> returns NULL, yes.
>
> I think Ben wants to post a followup series with those fixes mentioned
> above, in time for 2.9, perhaps this crash can be addressed in there
> too. Ben?
Regarding your other email ("New QMP command without a test -> automatic
NAK"), Ben did write a small test suite for this feature:
[Qemu-devel] [PATCH v8 7/8] tests: Add unit tests for the VM Generation
ID feature
and it contains a test called "/vmgenid/vmgenid/query-monitor".
That patch is not part of the pull request because the functional tests
in the same suite only pass if you have an updated SeaBIOS blob in the
tree as well. While the SeaBIOS patches have been committed, the update
for the blob bundled with QEMU is still in progress. Thus the patch with
the tests is being delayed.
So, it wasn't negligence, we simply missed this failure case because we
were so focused on the long-awaited functionality. We didn't miss other
failure cases that were a bit closer to the functionality.
BTW, now that I look at said "/vmgenid/vmgenid/query-monitor" test case,
it also doesn't seem to catch this error -- the monitor command is
apparently not called without the device present. So yeah, review
focused specifically on QMP / QAPI bits is always welcome.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands
2017-03-02 8:37 ` Laszlo Ersek
@ 2017-03-02 9:54 ` Markus Armbruster
0 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2017-03-02 9:54 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Michael S. Tsirkin, Peter Maydell, Ben Warren, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini, Igor Mammedov
Laszlo Ersek <lersek@redhat.com> writes:
> On 03/02/17 09:25, Laszlo Ersek wrote:
>
> Regarding your other email ("New QMP command without a test -> automatic
> NAK"), Ben did write a small test suite for this feature:
>
> [Qemu-devel] [PATCH v8 7/8] tests: Add unit tests for the VM Generation
> ID feature
>
> and it contains a test called "/vmgenid/vmgenid/query-monitor".
>
> That patch is not part of the pull request because the functional tests
> in the same suite only pass if you have an updated SeaBIOS blob in the
> tree as well. While the SeaBIOS patches have been committed, the update
> for the blob bundled with QEMU is still in progress. Thus the patch with
> the tests is being delayed.
Posting the testing part that depends on in-progress work only as RFC
sounds like a perfectly acceptable case of the "not practical" exception
mentioned "New QMP command without a test -> automatic NAK".
> So, it wasn't negligence, we simply missed this failure case because we
> were so focused on the long-awaited functionality. We didn't miss other
> failure cases that were a bit closer to the functionality.
>
> BTW, now that I look at said "/vmgenid/vmgenid/query-monitor" test case,
> it also doesn't seem to catch this error -- the monitor command is
> apparently not called without the device present.
I expect you to try to cover all QMP command error cases with automated
tests. I don't expect you to always succeed. We're all human :)
> So yeah, review
> focused specifically on QMP / QAPI bits is always welcome.
It's a struggle, to be honest. Bits of QMP and QAPI are often buried
deep in patches dealing with other stuff. QMP can usually be separated
into its own patch, but QAPI is just infrastructure, it *wants* to be
used.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands
2017-03-02 8:25 ` Laszlo Ersek
2017-03-02 8:37 ` Laszlo Ersek
@ 2017-03-02 8:42 ` Markus Armbruster
2017-03-02 13:15 ` Laszlo Ersek
1 sibling, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2017-03-02 8:42 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Michael S. Tsirkin, Peter Maydell, Ben Warren, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini, Igor Mammedov
Laszlo Ersek <lersek@redhat.com> writes:
> On 03/02/17 09:11, Markus Armbruster wrote:
>> Crash bug, I'm afraid...
>>
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>
>>> 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>
>>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>> qapi-schema.json | 20 ++++++++++++++++++++
>>> hmp.h | 1 +
>>> hmp.c | 9 +++++++++
>>> hw/acpi/vmgenid.c | 16 ++++++++++++++++
>>> stubs/vmgenid.c | 9 +++++++++
>>> hmp-commands-info.hx | 14 ++++++++++++++
>>> stubs/Makefile.objs | 1 +
>>> 7 files changed, 70 insertions(+)
>>> create mode 100644 stubs/vmgenid.c
>>>
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index d6186d4..84692da 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -6188,3 +6188,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/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
>>> index c8465df..744f284 100644
>>> --- a/hw/acpi/vmgenid.c
>>> +++ b/hw/acpi/vmgenid.c
>>> @@ -240,3 +240,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;
>>> +}
>>
>> This either returns a GuidInfo or NULL.
>>
>> If it returns NULL, the generated qmp_marshal_output_GuidInfo() will
>> crash like this:
>>
>> #0 0x00007fffdb4c7765 in raise () at /lib64/libc.so.6
>> #1 0x00007fffdb4c936a in abort () at /lib64/libc.so.6
>> #2 0x00007fffdb4bff97 in __assert_fail_base () at /lib64/libc.so.6
>> #3 0x00007fffdb4c0042 in () at /lib64/libc.so.6
>> #4 0x0000555555c800a0 in visit_start_struct (v=
>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, size=8, errp=0x7fffffffc790) at /work/armbru/qemu/qapi/qapi-visit-core.c:49
>> #5 0x0000555555c5d398 in visit_type_GuidInfo (v=
>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, errp=0x7fffffffc7d8) at qapi-visit.c:6038
>> #6 0x0000555555922fc0 in qmp_marshal_output_GuidInfo (ret_in=0x0, ret_out=0x7fffffffc870, errp=0x7fffffffc820) at qmp-marshal.c:5148
>> #7 0x0000555555923123 in qmp_marshal_query_vm_generation_id (args=0x555557a67500, ret=0x7fffffffc870, errp=0x7fffffffc868) at qmp-marshal.c:5186
>> #8 0x0000555555c83a73 in do_qmp_dispatch (request=0x555556ab3200, errp=0x7fffffffc8c0) at /work/armbru/qemu/qapi/qmp-dispatch.c:97
>> #9 0x0000555555c83ba3 in qmp_dispatch (request=0x555556ab3200)
>> at /work/armbru/qemu/qapi/qmp-dispatch.c:124
>> #10 0x00005555557bb671 in handle_qmp_command (parser=
>> 0x5555568776b0, tokens=0x555556858520) at /work/armbru/qemu/monitor.c:3789
>> #11 0x0000555555c8af2f in json_message_process_token (lexer=0x5555568776b8, input=0x5555568582e0, type=JSON_RCURLY, x=48, y=1)
>> at /work/armbru/qemu/qobject/json-streamer.c:105
>>
>> Sorry for not having reviewed this earlier. On the other hand, how come
>> this survived testing?
>
> We primarily focused on testing the functionality, not the failure /
> corner cases, I think.
That's as natural as it is wrong :)
I'm a lackluster tester myself. The only way I can bring myself to test
systematically is write automated tests. Fortunately, our
infrastructure for that is much better than it used to be. Our habits
still seem to be trailing the infrastructure, though.
See also "New QMP command without a test -> automatic NAK", Message-ID:
<871sugkrw5.fsf@dusky.pond.sub.org>.
> (There are at least two other known startup
> scenarios that are not handled correctly just yet, although they don't
> crash -- multiple vmgenid devices, and vmgenid on machtypes without
> writeable fw_cfg.)
>
> Also, in my testing I only called the monitor command via HMP (from
> virsh), and AFAICS that one doesn't crash even if the device is missing.
Correct.
> Nonetheless, qmp_query_vm_generation_id() should set an error when it
> returns NULL, yes.
That's the obvious fix. It's a one-liner.
> I think Ben wants to post a followup series with those fixes mentioned
> above, in time for 2.9, perhaps this crash can be addressed in there
> too. Ben?
Since the fix is a one-liner, I'd like you guys to consider respinning
this pull request with the fix.
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands
2017-03-02 8:42 ` Markus Armbruster
@ 2017-03-02 13:15 ` Laszlo Ersek
2017-03-02 14:50 ` Ben Warren
0 siblings, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2017-03-02 13:15 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael S. Tsirkin, Peter Maydell, Ben Warren, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini, Igor Mammedov
On 03/02/17 09:42, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
>
>> On 03/02/17 09:11, Markus Armbruster wrote:
>>> Crash bug, I'm afraid...
>>>
>>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>>
>>>> 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>
>>>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>> qapi-schema.json | 20 ++++++++++++++++++++
>>>> hmp.h | 1 +
>>>> hmp.c | 9 +++++++++
>>>> hw/acpi/vmgenid.c | 16 ++++++++++++++++
>>>> stubs/vmgenid.c | 9 +++++++++
>>>> hmp-commands-info.hx | 14 ++++++++++++++
>>>> stubs/Makefile.objs | 1 +
>>>> 7 files changed, 70 insertions(+)
>>>> create mode 100644 stubs/vmgenid.c
>>>>
>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>> index d6186d4..84692da 100644
>>>> --- a/qapi-schema.json
>>>> +++ b/qapi-schema.json
>>>> @@ -6188,3 +6188,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/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
>>>> index c8465df..744f284 100644
>>>> --- a/hw/acpi/vmgenid.c
>>>> +++ b/hw/acpi/vmgenid.c
>>>> @@ -240,3 +240,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;
>>>> +}
>>>
>>> This either returns a GuidInfo or NULL.
>>>
>>> If it returns NULL, the generated qmp_marshal_output_GuidInfo() will
>>> crash like this:
>>>
>>> #0 0x00007fffdb4c7765 in raise () at /lib64/libc.so.6
>>> #1 0x00007fffdb4c936a in abort () at /lib64/libc.so.6
>>> #2 0x00007fffdb4bff97 in __assert_fail_base () at /lib64/libc.so.6
>>> #3 0x00007fffdb4c0042 in () at /lib64/libc.so.6
>>> #4 0x0000555555c800a0 in visit_start_struct (v=
>>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, size=8, errp=0x7fffffffc790) at /work/armbru/qemu/qapi/qapi-visit-core.c:49
>>> #5 0x0000555555c5d398 in visit_type_GuidInfo (v=
>>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, errp=0x7fffffffc7d8) at qapi-visit.c:6038
>>> #6 0x0000555555922fc0 in qmp_marshal_output_GuidInfo (ret_in=0x0, ret_out=0x7fffffffc870, errp=0x7fffffffc820) at qmp-marshal.c:5148
>>> #7 0x0000555555923123 in qmp_marshal_query_vm_generation_id (args=0x555557a67500, ret=0x7fffffffc870, errp=0x7fffffffc868) at qmp-marshal.c:5186
>>> #8 0x0000555555c83a73 in do_qmp_dispatch (request=0x555556ab3200, errp=0x7fffffffc8c0) at /work/armbru/qemu/qapi/qmp-dispatch.c:97
>>> #9 0x0000555555c83ba3 in qmp_dispatch (request=0x555556ab3200)
>>> at /work/armbru/qemu/qapi/qmp-dispatch.c:124
>>> #10 0x00005555557bb671 in handle_qmp_command (parser=
>>> 0x5555568776b0, tokens=0x555556858520) at /work/armbru/qemu/monitor.c:3789
>>> #11 0x0000555555c8af2f in json_message_process_token (lexer=0x5555568776b8, input=0x5555568582e0, type=JSON_RCURLY, x=48, y=1)
>>> at /work/armbru/qemu/qobject/json-streamer.c:105
>>>
>>> Sorry for not having reviewed this earlier. On the other hand, how come
>>> this survived testing?
>>
>> We primarily focused on testing the functionality, not the failure /
>> corner cases, I think.
>
> That's as natural as it is wrong :)
>
> I'm a lackluster tester myself. The only way I can bring myself to test
> systematically is write automated tests. Fortunately, our
> infrastructure for that is much better than it used to be. Our habits
> still seem to be trailing the infrastructure, though.
>
> See also "New QMP command without a test -> automatic NAK", Message-ID:
> <871sugkrw5.fsf@dusky.pond.sub.org>.
>
>> (There are at least two other known startup
>> scenarios that are not handled correctly just yet, although they don't
>> crash -- multiple vmgenid devices, and vmgenid on machtypes without
>> writeable fw_cfg.)
>>
>> Also, in my testing I only called the monitor command via HMP (from
>> virsh), and AFAICS that one doesn't crash even if the device is missing.
>
> Correct.
>
>> Nonetheless, qmp_query_vm_generation_id() should set an error when it
>> returns NULL, yes.
>
> That's the obvious fix. It's a one-liner.
>
>> I think Ben wants to post a followup series with those fixes mentioned
>> above, in time for 2.9, perhaps this crash can be addressed in there
>> too. Ben?
>
> Since the fix is a one-liner, I'd like you guys to consider respinning
> this pull request with the fix.
If Ben & Michael send out a new pull with the above fixed, I'd like to
point out that the updated SeaBIOS blob is now in the tree (see commit
8779fccbef0c, "seabios: update to 1.10.2 release", 2017-02-28), so the
unit test patch can be included as well.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands
2017-03-02 13:15 ` Laszlo Ersek
@ 2017-03-02 14:50 ` Ben Warren
2017-03-02 15:32 ` Michael S. Tsirkin
0 siblings, 1 reply; 41+ messages in thread
From: Ben Warren @ 2017-03-02 14:50 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Markus Armbruster, Michael S. Tsirkin, Peter Maydell, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini, Igor Mammedov
[-- Attachment #1: Type: text/plain, Size: 6647 bytes --]
I
> On Mar 2, 2017, at 5:15 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 03/02/17 09:42, Markus Armbruster wrote:
>> Laszlo Ersek <lersek@redhat.com> writes:
>>
>>> On 03/02/17 09:11, Markus Armbruster wrote:
>>>> Crash bug, I'm afraid...
>>>>
>>>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>>>
>>>>> 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>
>>>>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> ---
>>>>> qapi-schema.json | 20 ++++++++++++++++++++
>>>>> hmp.h | 1 +
>>>>> hmp.c | 9 +++++++++
>>>>> hw/acpi/vmgenid.c | 16 ++++++++++++++++
>>>>> stubs/vmgenid.c | 9 +++++++++
>>>>> hmp-commands-info.hx | 14 ++++++++++++++
>>>>> stubs/Makefile.objs | 1 +
>>>>> 7 files changed, 70 insertions(+)
>>>>> create mode 100644 stubs/vmgenid.c
>>>>>
>>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>>> index d6186d4..84692da 100644
>>>>> --- a/qapi-schema.json
>>>>> +++ b/qapi-schema.json
>>>>> @@ -6188,3 +6188,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/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
>>>>> index c8465df..744f284 100644
>>>>> --- a/hw/acpi/vmgenid.c
>>>>> +++ b/hw/acpi/vmgenid.c
>>>>> @@ -240,3 +240,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;
>>>>> +}
>>>>
>>>> This either returns a GuidInfo or NULL.
>>>>
>>>> If it returns NULL, the generated qmp_marshal_output_GuidInfo() will
>>>> crash like this:
>>>>
>>>> #0 0x00007fffdb4c7765 in raise () at /lib64/libc.so.6
>>>> #1 0x00007fffdb4c936a in abort () at /lib64/libc.so.6
>>>> #2 0x00007fffdb4bff97 in __assert_fail_base () at /lib64/libc.so.6
>>>> #3 0x00007fffdb4c0042 in () at /lib64/libc.so.6
>>>> #4 0x0000555555c800a0 in visit_start_struct (v=
>>>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, size=8, errp=0x7fffffffc790) at /work/armbru/qemu/qapi/qapi-visit-core.c:49
>>>> #5 0x0000555555c5d398 in visit_type_GuidInfo (v=
>>>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, errp=0x7fffffffc7d8) at qapi-visit.c:6038
>>>> #6 0x0000555555922fc0 in qmp_marshal_output_GuidInfo (ret_in=0x0, ret_out=0x7fffffffc870, errp=0x7fffffffc820) at qmp-marshal.c:5148
>>>> #7 0x0000555555923123 in qmp_marshal_query_vm_generation_id (args=0x555557a67500, ret=0x7fffffffc870, errp=0x7fffffffc868) at qmp-marshal.c:5186
>>>> #8 0x0000555555c83a73 in do_qmp_dispatch (request=0x555556ab3200, errp=0x7fffffffc8c0) at /work/armbru/qemu/qapi/qmp-dispatch.c:97
>>>> #9 0x0000555555c83ba3 in qmp_dispatch (request=0x555556ab3200)
>>>> at /work/armbru/qemu/qapi/qmp-dispatch.c:124
>>>> #10 0x00005555557bb671 in handle_qmp_command (parser=
>>>> 0x5555568776b0, tokens=0x555556858520) at /work/armbru/qemu/monitor.c:3789
>>>> #11 0x0000555555c8af2f in json_message_process_token (lexer=0x5555568776b8, input=0x5555568582e0, type=JSON_RCURLY, x=48, y=1)
>>>> at /work/armbru/qemu/qobject/json-streamer.c:105
>>>>
>>>> Sorry for not having reviewed this earlier. On the other hand, how come
>>>> this survived testing?
>>>
>>> We primarily focused on testing the functionality, not the failure /
>>> corner cases, I think.
>>
>> That's as natural as it is wrong :)
>>
>> I'm a lackluster tester myself. The only way I can bring myself to test
>> systematically is write automated tests. Fortunately, our
>> infrastructure for that is much better than it used to be. Our habits
>> still seem to be trailing the infrastructure, though.
>>
>> See also "New QMP command without a test -> automatic NAK", Message-ID:
>> <871sugkrw5.fsf@dusky.pond.sub.org>.
>>
>>> (There are at least two other known startup
>>> scenarios that are not handled correctly just yet, although they don't
>>> crash -- multiple vmgenid devices, and vmgenid on machtypes without
>>> writeable fw_cfg.)
>>>
>>> Also, in my testing I only called the monitor command via HMP (from
>>> virsh), and AFAICS that one doesn't crash even if the device is missing.
>>
>> Correct.
>>
>>> Nonetheless, qmp_query_vm_generation_id() should set an error when it
>>> returns NULL, yes.
>>
>> That's the obvious fix. It's a one-liner.
>>
>>> I think Ben wants to post a followup series with those fixes mentioned
>>> above, in time for 2.9, perhaps this crash can be addressed in there
>>> too. Ben?
>>
>> Since the fix is a one-liner, I'd like you guys to consider respinning
>> this pull request with the fix.
>
> If Ben & Michael send out a new pull with the above fixed, I'd like to
> point out that the updated SeaBIOS blob is now in the tree (see commit
> 8779fccbef0c, "seabios: update to 1.10.2 release", 2017-02-28), so the
> unit test patch can be included as well.
>
Yes, the SeaBIOS image is now pulled in, so it’d be great to pull in the unit tests too. What do I do here - send a patch with the fix, or a re-spin of the single original patch without the bug, or a re-spin of the patch series?
> Thanks
> Laszlo
>
>
—Ben
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3583 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands
2017-03-02 14:50 ` Ben Warren
@ 2017-03-02 15:32 ` Michael S. Tsirkin
2017-03-02 16:24 ` Laszlo Ersek
0 siblings, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 15:32 UTC (permalink / raw)
To: Ben Warren
Cc: Laszlo Ersek, Markus Armbruster, Peter Maydell, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini, Igor Mammedov
On Thu, Mar 02, 2017 at 06:50:45AM -0800, Ben Warren wrote:
> I
> > On Mar 2, 2017, at 5:15 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > On 03/02/17 09:42, Markus Armbruster wrote:
> >> Laszlo Ersek <lersek@redhat.com> writes:
> >>
> >>> On 03/02/17 09:11, Markus Armbruster wrote:
> >>>> Crash bug, I'm afraid...
> >>>>
> >>>> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >>>>
> >>>>> 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>
> >>>>> Tested-by: Laszlo Ersek <lersek@redhat.com>
> >>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>>> ---
> >>>>> qapi-schema.json | 20 ++++++++++++++++++++
> >>>>> hmp.h | 1 +
> >>>>> hmp.c | 9 +++++++++
> >>>>> hw/acpi/vmgenid.c | 16 ++++++++++++++++
> >>>>> stubs/vmgenid.c | 9 +++++++++
> >>>>> hmp-commands-info.hx | 14 ++++++++++++++
> >>>>> stubs/Makefile.objs | 1 +
> >>>>> 7 files changed, 70 insertions(+)
> >>>>> create mode 100644 stubs/vmgenid.c
> >>>>>
> >>>>> diff --git a/qapi-schema.json b/qapi-schema.json
> >>>>> index d6186d4..84692da 100644
> >>>>> --- a/qapi-schema.json
> >>>>> +++ b/qapi-schema.json
> >>>>> @@ -6188,3 +6188,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/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> >>>>> index c8465df..744f284 100644
> >>>>> --- a/hw/acpi/vmgenid.c
> >>>>> +++ b/hw/acpi/vmgenid.c
> >>>>> @@ -240,3 +240,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;
> >>>>> +}
> >>>>
> >>>> This either returns a GuidInfo or NULL.
> >>>>
> >>>> If it returns NULL, the generated qmp_marshal_output_GuidInfo() will
> >>>> crash like this:
> >>>>
> >>>> #0 0x00007fffdb4c7765 in raise () at /lib64/libc.so.6
> >>>> #1 0x00007fffdb4c936a in abort () at /lib64/libc.so.6
> >>>> #2 0x00007fffdb4bff97 in __assert_fail_base () at /lib64/libc.so.6
> >>>> #3 0x00007fffdb4c0042 in () at /lib64/libc.so.6
> >>>> #4 0x0000555555c800a0 in visit_start_struct (v=
> >>>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, size=8, errp=0x7fffffffc790) at /work/armbru/qemu/qapi/qapi-visit-core.c:49
> >>>> #5 0x0000555555c5d398 in visit_type_GuidInfo (v=
> >>>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, errp=0x7fffffffc7d8) at qapi-visit.c:6038
> >>>> #6 0x0000555555922fc0 in qmp_marshal_output_GuidInfo (ret_in=0x0, ret_out=0x7fffffffc870, errp=0x7fffffffc820) at qmp-marshal.c:5148
> >>>> #7 0x0000555555923123 in qmp_marshal_query_vm_generation_id (args=0x555557a67500, ret=0x7fffffffc870, errp=0x7fffffffc868) at qmp-marshal.c:5186
> >>>> #8 0x0000555555c83a73 in do_qmp_dispatch (request=0x555556ab3200, errp=0x7fffffffc8c0) at /work/armbru/qemu/qapi/qmp-dispatch.c:97
> >>>> #9 0x0000555555c83ba3 in qmp_dispatch (request=0x555556ab3200)
> >>>> at /work/armbru/qemu/qapi/qmp-dispatch.c:124
> >>>> #10 0x00005555557bb671 in handle_qmp_command (parser=
> >>>> 0x5555568776b0, tokens=0x555556858520) at /work/armbru/qemu/monitor.c:3789
> >>>> #11 0x0000555555c8af2f in json_message_process_token (lexer=0x5555568776b8, input=0x5555568582e0, type=JSON_RCURLY, x=48, y=1)
> >>>> at /work/armbru/qemu/qobject/json-streamer.c:105
> >>>>
> >>>> Sorry for not having reviewed this earlier. On the other hand, how come
> >>>> this survived testing?
> >>>
> >>> We primarily focused on testing the functionality, not the failure /
> >>> corner cases, I think.
> >>
> >> That's as natural as it is wrong :)
> >>
> >> I'm a lackluster tester myself. The only way I can bring myself to test
> >> systematically is write automated tests. Fortunately, our
> >> infrastructure for that is much better than it used to be. Our habits
> >> still seem to be trailing the infrastructure, though.
> >>
> >> See also "New QMP command without a test -> automatic NAK", Message-ID:
> >> <871sugkrw5.fsf@dusky.pond.sub.org>.
> >>
> >>> (There are at least two other known startup
> >>> scenarios that are not handled correctly just yet, although they don't
> >>> crash -- multiple vmgenid devices, and vmgenid on machtypes without
> >>> writeable fw_cfg.)
> >>>
> >>> Also, in my testing I only called the monitor command via HMP (from
> >>> virsh), and AFAICS that one doesn't crash even if the device is missing.
> >>
> >> Correct.
> >>
> >>> Nonetheless, qmp_query_vm_generation_id() should set an error when it
> >>> returns NULL, yes.
> >>
> >> That's the obvious fix. It's a one-liner.
> >>
> >>> I think Ben wants to post a followup series with those fixes mentioned
> >>> above, in time for 2.9, perhaps this crash can be addressed in there
> >>> too. Ben?
> >>
> >> Since the fix is a one-liner, I'd like you guys to consider respinning
> >> this pull request with the fix.
> >
> > If Ben & Michael send out a new pull with the above fixed, I'd like to
> > point out that the updated SeaBIOS blob is now in the tree (see commit
> > 8779fccbef0c, "seabios: update to 1.10.2 release", 2017-02-28), so the
> > unit test patch can be included as well.
> >
> Yes, the SeaBIOS image is now pulled in, so it’d be great to pull in the unit tests too. What do I do here - send a patch with the fix, or a re-spin of the single original patch without the bug, or a re-spin of the patch series?
> > Thanks
> > Laszlo
> >
> >
> —Ben
>
Send the fix first of all pls.
Can you send pointer to the test patches pls? I have some version on a
branch but I'd rather not guess.
--
MST
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands
2017-03-02 15:32 ` Michael S. Tsirkin
@ 2017-03-02 16:24 ` Laszlo Ersek
0 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2017-03-02 16:24 UTC (permalink / raw)
To: Michael S. Tsirkin, Ben Warren
Cc: Markus Armbruster, Peter Maydell, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini, Igor Mammedov
On 03/02/17 16:32, Michael S. Tsirkin wrote:
> On Thu, Mar 02, 2017 at 06:50:45AM -0800, Ben Warren wrote:
>> I
>>> On Mar 2, 2017, at 5:15 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>> On 03/02/17 09:42, Markus Armbruster wrote:
>>>> Laszlo Ersek <lersek@redhat.com> writes:
>>>>
>>>>> On 03/02/17 09:11, Markus Armbruster wrote:
>>>>>> Crash bug, I'm afraid...
>>>>>>
>>>>>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>>>>>
>>>>>>> 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>
>>>>>>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>>>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>>> ---
>>>>>>> qapi-schema.json | 20 ++++++++++++++++++++
>>>>>>> hmp.h | 1 +
>>>>>>> hmp.c | 9 +++++++++
>>>>>>> hw/acpi/vmgenid.c | 16 ++++++++++++++++
>>>>>>> stubs/vmgenid.c | 9 +++++++++
>>>>>>> hmp-commands-info.hx | 14 ++++++++++++++
>>>>>>> stubs/Makefile.objs | 1 +
>>>>>>> 7 files changed, 70 insertions(+)
>>>>>>> create mode 100644 stubs/vmgenid.c
>>>>>>>
>>>>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>>>>> index d6186d4..84692da 100644
>>>>>>> --- a/qapi-schema.json
>>>>>>> +++ b/qapi-schema.json
>>>>>>> @@ -6188,3 +6188,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/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
>>>>>>> index c8465df..744f284 100644
>>>>>>> --- a/hw/acpi/vmgenid.c
>>>>>>> +++ b/hw/acpi/vmgenid.c
>>>>>>> @@ -240,3 +240,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;
>>>>>>> +}
>>>>>>
>>>>>> This either returns a GuidInfo or NULL.
>>>>>>
>>>>>> If it returns NULL, the generated qmp_marshal_output_GuidInfo() will
>>>>>> crash like this:
>>>>>>
>>>>>> #0 0x00007fffdb4c7765 in raise () at /lib64/libc.so.6
>>>>>> #1 0x00007fffdb4c936a in abort () at /lib64/libc.so.6
>>>>>> #2 0x00007fffdb4bff97 in __assert_fail_base () at /lib64/libc.so.6
>>>>>> #3 0x00007fffdb4c0042 in () at /lib64/libc.so.6
>>>>>> #4 0x0000555555c800a0 in visit_start_struct (v=
>>>>>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, size=8, errp=0x7fffffffc790) at /work/armbru/qemu/qapi/qapi-visit-core.c:49
>>>>>> #5 0x0000555555c5d398 in visit_type_GuidInfo (v=
>>>>>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, errp=0x7fffffffc7d8) at qapi-visit.c:6038
>>>>>> #6 0x0000555555922fc0 in qmp_marshal_output_GuidInfo (ret_in=0x0, ret_out=0x7fffffffc870, errp=0x7fffffffc820) at qmp-marshal.c:5148
>>>>>> #7 0x0000555555923123 in qmp_marshal_query_vm_generation_id (args=0x555557a67500, ret=0x7fffffffc870, errp=0x7fffffffc868) at qmp-marshal.c:5186
>>>>>> #8 0x0000555555c83a73 in do_qmp_dispatch (request=0x555556ab3200, errp=0x7fffffffc8c0) at /work/armbru/qemu/qapi/qmp-dispatch.c:97
>>>>>> #9 0x0000555555c83ba3 in qmp_dispatch (request=0x555556ab3200)
>>>>>> at /work/armbru/qemu/qapi/qmp-dispatch.c:124
>>>>>> #10 0x00005555557bb671 in handle_qmp_command (parser=
>>>>>> 0x5555568776b0, tokens=0x555556858520) at /work/armbru/qemu/monitor.c:3789
>>>>>> #11 0x0000555555c8af2f in json_message_process_token (lexer=0x5555568776b8, input=0x5555568582e0, type=JSON_RCURLY, x=48, y=1)
>>>>>> at /work/armbru/qemu/qobject/json-streamer.c:105
>>>>>>
>>>>>> Sorry for not having reviewed this earlier. On the other hand, how come
>>>>>> this survived testing?
>>>>>
>>>>> We primarily focused on testing the functionality, not the failure /
>>>>> corner cases, I think.
>>>>
>>>> That's as natural as it is wrong :)
>>>>
>>>> I'm a lackluster tester myself. The only way I can bring myself to test
>>>> systematically is write automated tests. Fortunately, our
>>>> infrastructure for that is much better than it used to be. Our habits
>>>> still seem to be trailing the infrastructure, though.
>>>>
>>>> See also "New QMP command without a test -> automatic NAK", Message-ID:
>>>> <871sugkrw5.fsf@dusky.pond.sub.org>.
>>>>
>>>>> (There are at least two other known startup
>>>>> scenarios that are not handled correctly just yet, although they don't
>>>>> crash -- multiple vmgenid devices, and vmgenid on machtypes without
>>>>> writeable fw_cfg.)
>>>>>
>>>>> Also, in my testing I only called the monitor command via HMP (from
>>>>> virsh), and AFAICS that one doesn't crash even if the device is missing.
>>>>
>>>> Correct.
>>>>
>>>>> Nonetheless, qmp_query_vm_generation_id() should set an error when it
>>>>> returns NULL, yes.
>>>>
>>>> That's the obvious fix. It's a one-liner.
>>>>
>>>>> I think Ben wants to post a followup series with those fixes mentioned
>>>>> above, in time for 2.9, perhaps this crash can be addressed in there
>>>>> too. Ben?
>>>>
>>>> Since the fix is a one-liner, I'd like you guys to consider respinning
>>>> this pull request with the fix.
>>>
>>> If Ben & Michael send out a new pull with the above fixed, I'd like to
>>> point out that the updated SeaBIOS blob is now in the tree (see commit
>>> 8779fccbef0c, "seabios: update to 1.10.2 release", 2017-02-28), so the
>>> unit test patch can be included as well.
>>>
>> Yes, the SeaBIOS image is now pulled in, so it’d be great to pull in the unit tests too. What do I do here - send a patch with the fix, or a re-spin of the single original patch without the bug, or a re-spin of the patch series?
>>> Thanks
>>> Laszlo
>>>
>>>
>> —Ben
>>
>
> Send the fix first of all pls.
Ben, once you add error_setg() to the "return NULL" branch in
qmp_query_vm_generation_id(), please remember to print and also release
that error object in hmp_info_vm_generation_id() as well. Sth like (not
even build tested):
diff --git a/hmp.c b/hmp.c
index 0ed1157c2c3a..c057be49a5ce 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2580,9 +2580,11 @@ void hmp_hotpluggable_cpus(Monitor *mon, const
QDict *qdict)
void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict)
{
- GuidInfo *info = qmp_query_vm_generation_id(NULL);
+ Error *err = NULL;
+ GuidInfo *info = qmp_query_vm_generation_id(&err);
if (info) {
monitor_printf(mon, "%s\n", info->guid);
}
+ hmp_handle_error(mon, &err);
qapi_free_GuidInfo(info);
}
>
> Can you send pointer to the test patches pls? I have some version on a
> branch but I'd rather not guess.
>
subject: [Qemu-devel] [PATCH v8 7/8] tests: Add unit tests for the VM
Generation ID feature
message-id:
<5353279f68be14c63f049bc34902675290e45b1d.1487286467.git.ben@skyportsystems.com>
URLs:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg430491.html
http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03781.html
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PULL 06/15] tests: Move reusable ACPI code into a utility file
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
` (4 preceding siblings ...)
2017-03-02 6:20 ` [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands Michael S. Tsirkin
@ 2017-03-02 6:20 ` Michael S. Tsirkin
2017-03-02 6:20 ` [Qemu-devel] [PULL 07/15] MAINTAINERS: Add VM Generation ID entries Michael S. Tsirkin
` (11 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 6:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Ben Warren, Igor Mammedov
From: Ben Warren <ben@skyportsystems.com>
Also usable by upcoming VM Generation ID tests
Signed-off-by: Ben Warren <ben@skyportsystems.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
tests/acpi-utils.h | 94 +++++++++++++++++++++++++++++++++
tests/acpi-utils.c | 65 +++++++++++++++++++++++
tests/bios-tables-test.c | 132 ++++++-----------------------------------------
MAINTAINERS | 2 +
tests/Makefile.include | 2 +-
5 files changed, 177 insertions(+), 118 deletions(-)
create mode 100644 tests/acpi-utils.h
create mode 100644 tests/acpi-utils.c
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/acpi-utils.c b/tests/acpi-utils.c
new file mode 100644
index 0000000..41dc1ea
--- /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/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;
diff --git a/MAINTAINERS b/MAINTAINERS
index be79f68..8b54935 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -908,6 +908,8 @@ 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
+F: tests/acpi-utils.[hc]
ppc4xx
M: Alexander Graf <agraf@suse.de>
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3310c17..364ef1b 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -669,7 +669,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)
--
MST
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PULL 07/15] MAINTAINERS: Add VM Generation ID entries
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
` (5 preceding siblings ...)
2017-03-02 6:20 ` [Qemu-devel] [PULL 06/15] tests: Move reusable ACPI code into a utility file Michael S. Tsirkin
@ 2017-03-02 6:20 ` Michael S. Tsirkin
2017-03-02 6:20 ` [Qemu-devel] [PULL 08/15] virtio: check for vring setup in virtio_queue_empty Michael S. Tsirkin
` (10 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 6:20 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Ben Warren, Laszlo Ersek, Igor Mammedov,
Thomas Huth, Michael Tokarev, Stefan Hajnoczi
From: Ben Warren <ben@skyportsystems.com>
Signed-off-by: Ben Warren <ben@skyportsystems.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 8b54935..b9a2171 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1124,6 +1124,15 @@ 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: docs/specs/vmgenid.txt
+F: tests/vmgenid-test.c
+F: stubs/vmgenid.c
+
Subsystems
----------
Audio
--
MST
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PULL 08/15] virtio: check for vring setup in virtio_queue_empty
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
` (6 preceding siblings ...)
2017-03-02 6:20 ` [Qemu-devel] [PULL 07/15] MAINTAINERS: Add VM Generation ID entries Michael S. Tsirkin
@ 2017-03-02 6:20 ` Michael S. Tsirkin
2017-03-02 6:20 ` [Qemu-devel] [PULL 09/15] virtio: guard vring access when setting notification Michael S. Tsirkin
` (9 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 6:20 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Paolo Bonzini, Gerd Hoffmann, Alex Williamson,
Laszlo Ersek, Cornelia Huck
From: Paolo Bonzini <pbonzini@redhat.com>
If the vring has not been set up, there is nothing in the virtqueue.
virtio_queue_host_notifier_aio_poll calls virtio_queue_empty even in
this case; we have to filter it out just like virtio_queue_notify_aio_vq.
Reported-by: Gerd Hoffmann <kraxel@redhat.com>
Tested-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
hw/virtio/virtio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 23483c7..e487e36 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2291,7 +2291,7 @@ static bool virtio_queue_host_notifier_aio_poll(void *opaque)
VirtQueue *vq = container_of(n, VirtQueue, host_notifier);
bool progress;
- if (virtio_queue_empty(vq)) {
+ if (!vq->vring.desc || virtio_queue_empty(vq)) {
return false;
}
--
MST
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PULL 09/15] virtio: guard vring access when setting notification
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
` (7 preceding siblings ...)
2017-03-02 6:20 ` [Qemu-devel] [PULL 08/15] virtio: check for vring setup in virtio_queue_empty Michael S. Tsirkin
@ 2017-03-02 6:20 ` Michael S. Tsirkin
2017-03-02 6:20 ` [Qemu-devel] [PULL 10/15] virtio: invalidate memory in vring_set_avail_event() Michael S. Tsirkin
` (8 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 6:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Cornelia Huck
From: Cornelia Huck <cornelia.huck@de.ibm.com>
Switching to vring caches exposed an existing bug in
virtio_queue_set_notification(): We can't access vring structures
if they have not been set up yet. This may happen, for example,
for virtio-blk devices with multiple queues: The code will try to
switch notifiers for every queue, but the guest may have only set up
a subset of them.
Fix this by guarding access to the vring memory by checking for
vring.desc. The first aio poll will iron out any remaining
inconsistencies for later-configured queues (buggy legacy drivers).
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/virtio/virtio.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index e487e36..bf8a644 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -288,6 +288,10 @@ void virtio_queue_set_notification(VirtQueue *vq, int enable)
{
vq->notification = enable;
+ if (!vq->vring.desc) {
+ return;
+ }
+
rcu_read_lock();
if (virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
vring_set_avail_event(vq, vring_avail_idx(vq));
--
MST
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PULL 10/15] virtio: invalidate memory in vring_set_avail_event()
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
` (8 preceding siblings ...)
2017-03-02 6:20 ` [Qemu-devel] [PULL 09/15] virtio: guard vring access when setting notification Michael S. Tsirkin
@ 2017-03-02 6:20 ` Michael S. Tsirkin
2017-03-02 6:20 ` [Qemu-devel] [PULL 11/15] virtio: add missing region cache init in virtio_load() Michael S. Tsirkin
` (7 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 6:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Stefan Hajnoczi, Paolo Bonzini, Eric Auger
From: Stefan Hajnoczi <stefanha@redhat.com>
Remember to invalidate the avail event field so the memory pages are
marked dirty.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
---
hw/virtio/virtio.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index bf8a644..294c909 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -282,6 +282,7 @@ static inline void vring_set_avail_event(VirtQueue *vq, uint16_t val)
caches = atomic_rcu_read(&vq->vring.caches);
pa = offsetof(VRingUsed, ring[vq->vring.num]);
virtio_stw_phys_cached(vq->vdev, &caches->used, pa, val);
+ address_space_cache_invalidate(&caches->used, pa, sizeof(val));
}
void virtio_queue_set_notification(VirtQueue *vq, int enable)
--
MST
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PULL 11/15] virtio: add missing region cache init in virtio_load()
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
` (9 preceding siblings ...)
2017-03-02 6:20 ` [Qemu-devel] [PULL 10/15] virtio: invalidate memory in vring_set_avail_event() Michael S. Tsirkin
@ 2017-03-02 6:20 ` Michael S. Tsirkin
2017-03-02 6:20 ` [Qemu-devel] [PULL 12/15] virtio: unbreak virtio-pci with IOMMU after caching ring translations Michael S. Tsirkin
` (6 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 6:20 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Stefan Hajnoczi, Dr . David Alan Gilbert,
Paolo Bonzini, Cornelia Huck, Eric Auger
From: Stefan Hajnoczi <stefanha@redhat.com>
Commit 97cd965c070152bc626c7507df9fb356bbe1cd81 ("virtio: use
VRingMemoryRegionCaches for avail and used rings") switched to a memory
region cache to avoid repeated map/unmap operations.
The virtio_load() process is a little tricky because vring addresses are
serialized in two separate places. VIRTIO 1.0 devices serialize desc
and then a subsection with used and avail. Legacy devices only
serialize desc.
Live migration of VIRTIO 1.0 devices fails on the destination host with:
VQ 0 size 0x80 < last_avail_idx 0x12f8 - used_idx 0x0
Failed to load virtio-blk:virtio
error while loading state for instance 0x0 of device '0000:00:04.0/virtio-blk'
This happens because the memory region cache is only initialized after
desc is loaded and not after the used and avail subsection is loaded.
If the guest chose memory addresses that don't match the legacy ring
layout then the wrong guest memory location is accessed.
Wait until all ring addresses are known before trying to initialize the
region cache. Also clarify the incomplete comment about VIRTIO-1 ring
address subsection.
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
---
hw/virtio/virtio.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 294c909..efce4b3 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1857,7 +1857,10 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
if (k->has_variable_vring_alignment) {
qemu_put_be32(f, vdev->vq[i].vring.align);
}
- /* XXX virtio-1 devices */
+ /*
+ * Save desc now, the rest of the ring addresses are saved in
+ * subsections for VIRTIO-1 devices.
+ */
qemu_put_be64(f, vdev->vq[i].vring.desc);
qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
if (k->save_queue) {
@@ -1998,14 +2001,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
vdev->vq[i].signalled_used_valid = false;
vdev->vq[i].notification = true;
- if (vdev->vq[i].vring.desc) {
- /* XXX virtio-1 devices */
- virtio_queue_update_rings(vdev, i);
- } else if (vdev->vq[i].last_avail_idx) {
+ if (!vdev->vq[i].vring.desc && vdev->vq[i].last_avail_idx) {
error_report("VQ %d address 0x0 "
"inconsistent with Host index 0x%x",
i, vdev->vq[i].last_avail_idx);
- return -1;
+ return -1;
}
if (k->load_queue) {
ret = k->load_queue(qbus->parent, i, f);
@@ -2066,6 +2066,19 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
for (i = 0; i < num; i++) {
if (vdev->vq[i].vring.desc) {
uint16_t nheads;
+
+ /*
+ * VIRTIO-1 devices migrate desc, used, and avail ring addresses so
+ * only the region cache needs to be set up. Legacy devices need
+ * to calculate used and avail ring addresses based on the desc
+ * address.
+ */
+ if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+ virtio_init_region_cache(vdev, i);
+ } else {
+ virtio_queue_update_rings(vdev, i);
+ }
+
nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
/* Check it isn't doing strange things with descriptor numbers. */
if (nheads > vdev->vq[i].vring.num) {
--
MST
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PULL 12/15] virtio: unbreak virtio-pci with IOMMU after caching ring translations
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
` (10 preceding siblings ...)
2017-03-02 6:20 ` [Qemu-devel] [PULL 11/15] virtio: add missing region cache init in virtio_load() Michael S. Tsirkin
@ 2017-03-02 6:20 ` Michael S. Tsirkin
2017-03-02 6:20 ` [Qemu-devel] [PULL 13/15] acpi: simplify _OSC Michael S. Tsirkin
` (5 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 6:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang, Paolo Bonzini
From: Jason Wang <jasowang@redhat.com>
Commit c611c76417f5 ("virtio: add MemoryListener to cache ring
translations") registers a memory listener to dma_as. This may not
work when IOMMU is enabled: dma_as(bus_master_as) were initialized in
pcibus_machine_done() after virtio_realize(). This will cause a
segfault. Fixing this by using pci_device_iommu_address_space()
instead to make sure address space were initialized at this time.
With this fix, IOMMU device were required to be initialized before any
virtio-pci devices.
Fixes: c611c76417f5 ("virtio: add MemoryListener to cache ring translations")
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/virtio/virtio-pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 5ce42af..b76f3f6 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1153,7 +1153,7 @@ static AddressSpace *virtio_pci_get_dma_as(DeviceState *d)
VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
PCIDevice *dev = &proxy->pci_dev;
- return pci_get_address_space(dev);
+ return pci_device_iommu_address_space(dev);
}
static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
--
MST
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PULL 13/15] acpi: simplify _OSC
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
` (11 preceding siblings ...)
2017-03-02 6:20 ` [Qemu-devel] [PULL 12/15] virtio: unbreak virtio-pci with IOMMU after caching ring translations Michael S. Tsirkin
@ 2017-03-02 6:20 ` Michael S. Tsirkin
2017-03-02 6:20 ` [Qemu-devel] [PULL 14/15] tests/acpi: update DSDT after last patch Michael S. Tsirkin
` (4 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 6:20 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Igor Mammedov, Paolo Bonzini, Richard Henderson,
Eduardo Habkost
Our _OSC method has a bunch of unused code loading data
into external CTRL and SUPP fields which are then never
used. Drop this in favor of a single local variable.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
hw/i386/acpi-build.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index db04cf5..efbbfcb 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1804,7 +1804,7 @@ static Aml *build_q35_osc_method(void)
Aml *else_ctx;
Aml *method;
Aml *a_cwd1 = aml_name("CDW1");
- Aml *a_ctrl = aml_name("CTRL");
+ Aml *a_ctrl = aml_local(0);
method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
@@ -1814,7 +1814,6 @@ static Aml *build_q35_osc_method(void)
aml_append(if_ctx, aml_create_dword_field(aml_arg(3), aml_int(4), "CDW2"));
aml_append(if_ctx, aml_create_dword_field(aml_arg(3), aml_int(8), "CDW3"));
- aml_append(if_ctx, aml_store(aml_name("CDW2"), aml_name("SUPP")));
aml_append(if_ctx, aml_store(aml_name("CDW3"), a_ctrl));
/*
@@ -1899,8 +1898,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_int(1)));
- aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
- aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
aml_append(dev, build_q35_osc_method());
aml_append(sb_scope, dev);
aml_append(dsdt, sb_scope);
--
MST
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PULL 14/15] tests/acpi: update DSDT after last patch
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
` (12 preceding siblings ...)
2017-03-02 6:20 ` [Qemu-devel] [PULL 13/15] acpi: simplify _OSC Michael S. Tsirkin
@ 2017-03-02 6:20 ` Michael S. Tsirkin
2017-03-02 6:20 ` [Qemu-devel] [PULL 15/15] hw/pxb-pcie: fix PCI Express hotplug support Michael S. Tsirkin
` (3 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 6:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
tests/acpi-test-data/q35/DSDT | Bin 7860 -> 7824 bytes
tests/acpi-test-data/q35/DSDT.bridge | Bin 7877 -> 7841 bytes
tests/acpi-test-data/q35/DSDT.cphp | Bin 8323 -> 8287 bytes
tests/acpi-test-data/q35/DSDT.ipmibt | Bin 7935 -> 7899 bytes
tests/acpi-test-data/q35/DSDT.memhp | Bin 9225 -> 9189 bytes
5 files changed, 0 insertions(+), 0 deletions(-)
diff --git a/tests/acpi-test-data/q35/DSDT b/tests/acpi-test-data/q35/DSDT
index d11567c3dc220894911166681ee7303fadfe04e8..0dccad439b8e8e00b403c8d290a89630c4329d45 100644
GIT binary patch
delta 106
zcmdmDJHeLACD<iof*b<_W9>w)elBmW_+Y2_=q6{biHlW)M10ue{ezuZy0RIZUBV3)
y__0oWs-<U~P@TXfo3KD&(&S9WT95=62UuEo(qtfQoB$CM0ErbOYz}3NmjwWRx*t3M
delta 142
zcmbPWyTz8vCD<ioiyQ+3<Bf@2{apS$@xe~<(M|3=6Bnz<aRi431Tb(ohXnaBh`4aZ
z`v*I-bY(L*yM!AqaATi%Q_IZ=q@n;QU<^`J4I;Q?LF58~Ns}`fYe7o6IKUdjCQXK^
Sg9r<NgbP55Hybj>%K`v}Ln$x-
diff --git a/tests/acpi-test-data/q35/DSDT.bridge b/tests/acpi-test-data/q35/DSDT.bridge
index 412a6e910404dca9b5d6e6c2d027b9e10ccc2a84..8cd66c3b3143297a5262480e46be9ee811a6291f 100644
GIT binary patch
delta 106
zcmX?VyU>=)CD<iop&SDPqwGYkelBmW_+Y2_=q6{biHlW)M10ue{ezuZy0RIZUBV3)
y__0oWs-<U~P@TXfo3KD&(&S9WT95=62UuEo(qtfQoB$CM0ErbOYz}2CkOcs6zaJm~
delta 142
zcmZ2zd(@W8CD<k8s2l?WW5q<SelCBW_+Y2_=q7ibiHlX_ID$h10vI@)LxOx5L|i!I
z{ezuZy0RIZUBV3)xUo;XspVz_Qc(aDFa{~A1`%AcAaa4gq{*3#wIHQj9AFJ%lP1H|
RL4*ZB!UZ73n++KYWC3vWDHH$z
diff --git a/tests/acpi-test-data/q35/DSDT.cphp b/tests/acpi-test-data/q35/DSDT.cphp
index 79902d0d30f02d74594cb87b14460ea02764d460..3c28a17a69db15dc92c825c1502a7f86ab975c0d 100644
GIT binary patch
delta 106
zcmZp6yzju}66_KZufV{-_-!IrKbJRGe6Uk|bdxjJ#KkH?B0lW#{=v>HUD*uIF5!j?
y{8%SG)zULgs7~OLO;{i>X>ulGEl7fk11v2(X)=&DPJoCBfW!(CHit6m$pZkUtRH*;
delta 142
zcmccb(Co<N66_MvtiZs)7&wuupUa;oKG-Qfy2+hq;$js!j^NOM00s`{kRTrh5f{#Q
z|6pg9u51QpmvF-cZtN3pYPlJKR1^ROj6sU3K?Ij9h+H5rX>ulGEl4RB2UvsHq{%RK
R5Mcq3Z~;j1W<y3jc>q1nC~yD(
diff --git a/tests/acpi-test-data/q35/DSDT.ipmibt b/tests/acpi-test-data/q35/DSDT.ipmibt
index b658329c5b8e7eba009ad96f7cd26674b5a2c861..3ceb876127725f199e3df903bc71e4023e2fa225 100644
GIT binary patch
delta 106
zcmexwd)t=FCD<k8wj2Wk<D!XN{aoH$@xe~<(M`@=6BnxpiTJR``v*I-bY(L*yM!Aq
y@ME3$R7=k|p*n#}HerFlq{*3#wIB&D4zRTFq{%?qH~}Ii01_)m*c{5ZN)`a-jUbHx
delta 142
zcmca@``?z!CD<k8zZ?St<KKx~{apS$@xe~<(M|3=6Bnz<aRi431Tb(ohXnaBh`4aZ
z`v*I-bY(L*yM!AqaATi%Q_IZ=q@n;QU<^`J4I;Q?LF58~Ns}`fYe7o6IKUdjCQXK^
Sg9r<NgbP55Hybjpk_7+}{VIU~
diff --git a/tests/acpi-test-data/q35/DSDT.memhp b/tests/acpi-test-data/q35/DSDT.memhp
index e46c1fb5a21b8b45a4db5955989941875169a46d..bdbefd47a5398ed96498b77bfb6a74f7ea638db1 100644
GIT binary patch
delta 106
zcmeD5c<RpO66_N4RGEQ+v2h|-KbJRGe6Uk|bdxjJ#KkH?B0lW#{=v>HUD*uIF5!j?
y{8%SG)zULgs7~OLO;{i>X>ulGEl7fk11v2(X)=&DPJoCBfW!(CHit59Q33$GF(5Ml
delta 142
zcmaFr-s!>R66_Mfslvd(czYsOKbJpGe6Uk|bdx*J#KkId9KoRh0Sp|@AwfP2A}*Zq
z{=v>HUD*uIF5!j?+}J1H)N(TdsVD#n7=sj5g9t8J5V=5L(&S9WT98sM4zLEXNt0pf
RAi@G5;R2B2&4!FylmJxFDJK8`
--
MST
^ permalink raw reply [flat|nested] 41+ messages in thread* [Qemu-devel] [PULL 15/15] hw/pxb-pcie: fix PCI Express hotplug support
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
` (13 preceding siblings ...)
2017-03-02 6:20 ` [Qemu-devel] [PULL 14/15] tests/acpi: update DSDT after last patch Michael S. Tsirkin
@ 2017-03-02 6:20 ` Michael S. Tsirkin
2017-03-02 8:34 ` [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Peter Maydell
` (2 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 6:20 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Marcel Apfelbaum, Igor Mammedov, Paolo Bonzini,
Richard Henderson, Eduardo Habkost
From: Marcel Apfelbaum <marcel@redhat.com>
Add the missing osc method for pxb-pcie devices as APCI spec recommends,
see 6.2.9.1 OSC Implementation Example for PCI Host Bridge Devices, ACPI 3.0a:
It is recommended that a machine with multiple host bridge devices
should report the same capabilities for all host bridges, and also
negotiate control of the features described in the Control Field in
the same way for all host bridges.
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/i386/acpi-build.c | 3 +++
dtc | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index efbbfcb..8018f05 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1962,6 +1962,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
aml_append(dev, aml_name_decl("_UID", aml_int(bus_num)));
aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
aml_append(dev, aml_name_decl("_BBN", aml_int(bus_num)));
+ if (pci_bus_is_express(bus)) {
+ aml_append(dev, build_q35_osc_method());
+ }
if (numa_node != NUMA_NODE_UNASSIGNED) {
aml_append(dev, aml_name_decl("_PXM", aml_int(numa_node)));
diff --git a/dtc b/dtc
index ec02b34..65cc4d2 160000
--- a/dtc
+++ b/dtc
@@ -1 +1 @@
-Subproject commit ec02b34c05be04f249ffaaca4b666f5246877dea
+Subproject commit 65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf
--
MST
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
` (14 preceding siblings ...)
2017-03-02 6:20 ` [Qemu-devel] [PULL 15/15] hw/pxb-pcie: fix PCI Express hotplug support Michael S. Tsirkin
@ 2017-03-02 8:34 ` Peter Maydell
2017-03-02 15:29 ` Michael S. Tsirkin
2017-03-03 12:42 ` Peter Maydell
2017-03-03 12:44 ` Peter Maydell
17 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2017-03-02 8:34 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: QEMU Developers
On 2 March 2017 at 06:20, Michael S. Tsirkin <mst@redhat.com> wrote:
> The following changes since commit 1e0addb682c3c552fd97480037d4f8ff18e2b87e:
>
> Merge remote-tracking branch 'remotes/sstabellini/tags/xen-20170228-tag' into staging (2017-03-01 20:33:47 +0000)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to 077dd74239a99f6c1e77c5c1aa24cfc7f58cd20c:
>
> hw/pxb-pcie: fix PCI Express hotplug support (2017-03-02 07:31:26 +0200)
>
> ----------------------------------------------------------------
> virtio, pc: fixes, features
>
> virtio support for region caches broke a bunch of stuff - fixing most of
> it though it's not ideal. Still pondering the right way to fix it.
> New: VM gen ID and hotplug for PXB.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
This is now well past the softfreeze deadline; do you have
a justification for putting new features in during freeze?
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features
2017-03-02 8:34 ` [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Peter Maydell
@ 2017-03-02 15:29 ` Michael S. Tsirkin
0 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-02 15:29 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers
On Thu, Mar 02, 2017 at 08:34:42AM +0000, Peter Maydell wrote:
> On 2 March 2017 at 06:20, Michael S. Tsirkin <mst@redhat.com> wrote:
> > The following changes since commit 1e0addb682c3c552fd97480037d4f8ff18e2b87e:
> >
> > Merge remote-tracking branch 'remotes/sstabellini/tags/xen-20170228-tag' into staging (2017-03-01 20:33:47 +0000)
> >
> > are available in the git repository at:
> >
> > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> >
> > for you to fetch changes up to 077dd74239a99f6c1e77c5c1aa24cfc7f58cd20c:
> >
> > hw/pxb-pcie: fix PCI Express hotplug support (2017-03-02 07:31:26 +0200)
> >
> > ----------------------------------------------------------------
> > virtio, pc: fixes, features
> >
> > virtio support for region caches broke a bunch of stuff - fixing most of
> > it though it's not ideal. Still pondering the right way to fix it.
> > New: VM gen ID and hotplug for PXB.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> This is now well past the softfreeze deadline; do you have
> a justification for putting new features in during freeze?
>
> thanks
> -- PMM
pxb hotplug support is more a bugfix than a feature - we just forgot to
include the acpi bits, and the patches are very small. vm gen id has
been in my tree for a long time and bios bits have been merged. If we
keep it out of this release there's more of a chance they will bitrot
which would be a mess.
--
MST
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
` (15 preceding siblings ...)
2017-03-02 8:34 ` [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Peter Maydell
@ 2017-03-03 12:42 ` Peter Maydell
2017-03-03 12:44 ` Peter Maydell
17 siblings, 0 replies; 41+ messages in thread
From: Peter Maydell @ 2017-03-03 12:42 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: QEMU Developers
On 2 March 2017 at 06:20, Michael S. Tsirkin <mst@redhat.com> wrote:
> The following changes since commit 1e0addb682c3c552fd97480037d4f8ff18e2b87e:
>
> Merge remote-tracking branch 'remotes/sstabellini/tags/xen-20170228-tag' into staging (2017-03-01 20:33:47 +0000)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to 077dd74239a99f6c1e77c5c1aa24cfc7f58cd20c:
>
> hw/pxb-pcie: fix PCI Express hotplug support (2017-03-02 07:31:26 +0200)
>
> ----------------------------------------------------------------
> virtio, pc: fixes, features
>
> virtio support for region caches broke a bunch of stuff - fixing most of
> it though it's not ideal. Still pondering the right way to fix it.
> New: VM gen ID and hotplug for PXB.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
Applied, thanks.
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features
2017-03-02 6:20 [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features Michael S. Tsirkin
` (16 preceding siblings ...)
2017-03-03 12:42 ` Peter Maydell
@ 2017-03-03 12:44 ` Peter Maydell
2017-03-03 19:10 ` Michael S. Tsirkin
17 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2017-03-03 12:44 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: QEMU Developers
On 2 March 2017 at 06:20, Michael S. Tsirkin <mst@redhat.com> wrote:
> dtc | 2 +-
I just noticed this erroneous submodule update in here, unfortunately
after I pushed it to master :-(
I'll fix up the tree...
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features
2017-03-03 12:44 ` Peter Maydell
@ 2017-03-03 19:10 ` Michael S. Tsirkin
0 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2017-03-03 19:10 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers
On Fri, Mar 03, 2017 at 12:44:59PM +0000, Peter Maydell wrote:
> On 2 March 2017 at 06:20, Michael S. Tsirkin <mst@redhat.com> wrote:
> > dtc | 2 +-
>
> I just noticed this erroneous submodule update in here, unfortunately
> after I pushed it to master :-(
>
> I'll fix up the tree...
>
> thanks
> -- PMM
Ouch. submodules are a pain :(
Let me know if you need me to rebase. Sorry.
--
MST
^ permalink raw reply [flat|nested] 41+ messages in thread