Re: [Qemu-devel] [PATCH v2 2/7] acpi: add vmcoreinfo device

[Laszlo Ersek]

Marc-André,

sorry about this, but I have another late comment (I shoudl have pointed
this out in v1). Regarding:

On 07/06/17 12:16, Marc-André Lureau wrote:

> +#define VMCOREINFO_OFFSET           40   /* allow space for
> +                                          * OVMF SDT Header Probe Supressor
> +                                          */

and

> +            Method (ADDR, 0, NotSerialized)
> +            {
> +                Local0 = Package (0x02) {}
> +                Local0 [Zero] = (VCIA + 0x28)
> +                Local0 [One] = Zero
> +                Return (Local0)
> +            }

and

> +In order to implement an OVMF "SDT Header Probe Suppressor", the contents of
> +the vmcoreinfo blob has 40 bytes of padding:
> +
> ++-----------------------------------+
> +| SSDT with OEM Table ID = VMCOREIN |
> ++-----------------------------------+
> +| ...                               |       TOP OF PAGE
> +| VCIA dword object ----------------|-----> +---------------------------+
> +| ...                               |       | fw-allocated array for    |
> +| _STA method referring to VCIA     |       | "etc/vmcoreinfo"          |
> +| ...                               |       +---------------------------+
> +| ADDR method referring to VCIA     |       |  0: OVMF SDT Header probe |
> +| ...                               |       |     suppressor            |
> ++-----------------------------------+       | 40: uint32 version field  |
> +                                            | 44: info contents         |
> +                                            |     ....                  |
> +                                            +---------------------------+
> +                                            END OF PAGE

Please define the VMCOREINFO_OFFSET macro like this:

  #define VMCOREINFO_OFFSET sizeof(AcpiTableHeader)

and then please refresh the documentation as well:
- replace decimal "40" with "36",
- replace 0x28 in the dumped SSDT with 0x24.

Namely, in order to suppress the OVMF ACP Table Header Probe -- "SDT"
simply means "System Description Table" --, it's enough to add
zero-padding that *precisely* covers such a header.

Given that we have a typedef for this header in QEMU, we should use it
in the definition of VMCOREINFO_OFFSET.

Now, the reason why VMGENID uses 40 instead comes from another
requirement: the VMGENID GUID has to be aligned at 8 bytes. (See
requirement "R1a" in "docs/specs/vmgenid.txt".) Therefore the directly
necessary 36 bytes of padding are rounded up to 40. See again
"docs/specs/vmgenid.txt":

----------------------------------+
| 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

At offset 36 of "etc/vmgenid_guid", it says "padding for 8-byte alignment".

For VMCOREINFO, you don't need this extra alignment to 8 bytes, before
the "version" field is listed. So please make the VMCOREINFO_OFFSET
reflect what's actually necessary.

The rest of the code doesn't have to be modified (including your
experimental guest kernel driver); changing VMCOREINFO_OFFSET will
update all necessary locations automatically, including the generated
AML. Only the docs have to be synced manually.

... In fact, there *is* yet another location to update: the test case. I
suggest to include "hw/acpi/vmcoreinfo.h" in "tests/vmcoreinfo-test.c",
rather than open-code VMCOREINFO_OFFSET there. ... Oh wait: in the test
case you don't even use VMCOREINFO_OFFSET for anything. So just delete
the macro definition from that patch.

Thank you (and sorry about the churn),
Laszlo