Re: [Qemu-devel] [PATCH v4 3/9] docs: VM Generation ID device description

[Laszlo Ersek]

On 01/25/17 02:43, address@hidden wrote:
> From: Ben Warren <address@hidden>
> 
> This patch is based off an earlier version by Gal Hammer (address@hidden)

(1) This commit msg line is too long; please wrap it at 74 chars.

> 
> Signed-off-by: Gal Hammer <address@hidden>
> Signed-off-by: Ben Warren <address@hidden>
> ---
>  docs/specs/vmgenid.txt | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 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..d36ed5b
> --- /dev/null
> +++ b/docs/specs/vmgenid.txt
> @@ -0,0 +1,40 @@
> +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.
> +This allows management applications (e.g. libvirt) to notify the guest
> +operating system when the virtual machine is executed with a different
> +configuration (e.g. snapshot execution or creation from a template).
> +
> +This is specified on the web at: 
> http://go.microsoft.com/fwlink/?LinkId=260709
> +
> +---
> +
> +The vmgenid device is a sysbus device with the ACPI ID "QEMUGVID".

(2) The ID has a typo, it should be QEMUVGID (see the next patch).

> +
> +The device has one properties,

(3) "property", singular

> which can be set using the command line
> +argument or the QMP interface:
> + 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"
> +
> +Or to change guid in runtime use:
> + set-vm-generation-id guid=124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87
> + set-vm-generation-id guid=auto
> +
> +According to the specification, any change to the GUID executes an
> +ACPI notification. The vmgenid device triggers the \_GPE._E05 handler
> +which executes the ACPI Notify operation.
> +
> +Although not specified in Microsoft's document, it is assumed that the
> +device is expected to use the little-endian system.
> 

The above seems okay to me, but it's too terse. Please add an ASCII
diagram (or document in plain text) that describes what object in what
ACPI table points where, what fw_cfg files are used, and so on.
Practically, the entire idea behind patch #4.

I'll try to write more about this in response to patch #4. The goal is
to help QEMU developers understand patch #4 better.

... I have something like this in mind:

https://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg03409.html

(4) I think you could simply steal the "Requirements" section (just
mention in the commit message that that part was originally written up
by me). I think this should be helpful, because we can more easily
validate the implementation against such a bullet list.

(5) For the rest of the document, I suggest a quick skim. Quite a good
chunk will not apply to your implementation (justifiedly!), but it will
explain to you how OVMF behaves in this regard. Plus, it should provide
an example or two for ASCII diagrams.

(If you are interested why that patch series was abandoned ultimately:
it's because Microsoft's ACPI interpreter doesn't support the
DataTableRegion operator, as we found out.
<https://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg03406.html>.)

(6) With regard to OVMF, I'd like to emphasize the "OVMF SDT Header
probe suppressor". (SDT stands for ACPI System Description Table.)

In your implementation, it should be a 40-byte zero-padding at the front
of the fw_cfg blob with the GUID. It is necessary due to how OVMF
processes the ADD_POINTER commands (explained in the linked document).

The gist of it is that OVMF will look for an ACPI table header wherever
an ADD_POINTER command points, therefore 36 bytes -- see the size of
ACPI_TABLE_HEADER_DEF -- should be padded out first, to suppress that
heuristics for the vmgenid GUID. Then 4 more bytes are needed to round
up the start address of the GUID to a multiple of 8.

(7) Another comment related to the fw_cfg file that contains the GUID:
If you check requirement R1e, it follows that the fw_cfg file hosting
the GUID should be placed at a 4KB boundary, and cover a full page.

Therefore the fw_cfg file structure should be like:

- 36 bytes zero padding for suppressing OVMF's SDT Header probe
- 4 bytes zero padding for getting to an 8-byte alignment
- 8 bytes vmgenid GUID
- 4048 bytes zero padding, to ensure that nothing else is allocated on
the same page, either by the firmware or the OS.

The above layout has a consequence for both the ACPI payload you
generate, and for how QEMU acts upon the address written by the
firmware. Namely, the allocation address returned by the firmware will
point to the beginning of the buffer, but both the ADDR method and QEMU
will have to reference the GUID field at offset 40 decimal.

For this, when QEMU writes the new GUID, it will have to add 40 to the
address returned by the firmware.

Plus, the ADDR AML method will have to add 40 to the VGIA field manually.

Illustration (feel free to include it in the document):

+----------------------------------+
| SSDT with OEM Table ID = VMGENID |
+----------------------------------+
| ...                              |       TOP OF PAGE
| VGIA qword object ---------------|-----> +---------------------------+
| ...                              |       | fw-allocated array for    |
| _STA method referring to VGIA    |       | "etc/vmgenid"             |
| ...                              |       +---------------------------+
| ADDR method referring to VGIA    |       |  0: OVMF SDT Header probe |
| ...                              |       |     suppressor            |
+----------------------------------+       | 36: padding for 8-byte    |
                                           |     alignment             |
                                           | 40: GUID                  |
                                           | 48: padding to page size  |
                                           +---------------------------+
                                           END OF PAGE

When you patch the VGIA object with an ADD_POINTER command, it must
point to the OVMF SDT Header probe suppressor, at offset 0. It is fine
if the _STA method compares VGIA against plain zero, to see in OSPM if
VGIA was indeed patched by the firmware.

However, the ADDR method must add 40 (offset of GUID) to VGIA.
Similarly, QEMU must add 40 to the value received in "etc/vmgenid_addr".

(8) I also suggest including a decompiled dump of the new ACPI payload.
It *really* helps understanding patch #4, whereas currently I
practically have to reverse-engineer patch #4.

Again, I'll try to associate all of these notes with actual code in
patch #4. Still, the documentation should explain them separately.

Thanks
Laszlo