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

[Laszlo Ersek]

On 07/07/17 15:13, Laszlo Ersek wrote:
> 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",

also replace decimal "44", the offset of "info contents", with "40"...
you get the idea.

Thanks
Laszlo

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