Re: [PATCH] hvmloader: indicate firmware tables as ACPI NVS in e820

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.08.2020 16:45, Igor Druzhinin wrote:
> On 28/08/2020 08:51, Jan Beulich wrote:
>> On 28.08.2020 02:13, Igor Druzhinin wrote:
>>> Guest kernel does need to know in some cases where the tables are located
>>> to treat these regions properly. One example is kexec process where
>>> the first kernel needs to pass firmware region locations to the second
>>> kernel which is now a requirement after 02a3e3cdb7f12 ("x86/boot: Parse SRAT
>>> table and count immovable memory regions").
>>>
>>> The memory that hvmloader allocates in the reserved region mostly contains
>>> these useful tables and could be safely indicated as ACPI without the need
>>> to designate a sub-region specially for that. Making it non-reclaimable
>>> (ACPI NVS) would avoid potential reuse of this memory by the guest.
>>> Swtiching from Reserved to ACPI NVS type for this memory would also mean
>>> its content is preserved across S4 (which is true for any ACPI type 
>>> according
>>> to the spec).
>>
>> Marking the range as ACPI is certainly fine, but why did you choose NVS?
>> There's nothing non-volatile there afaict. And the part of the last
>> sentence in parentheses suggests to me that the "normal" ACPI type is as
>> suitable for the purpose you're after.
> 
> The problem with normal ACPI type is that according to the spec it's 
> reclaimable
> by the OS:
> "This range is available RAM usable by the OS after it reads the ACPI tables."
> while NVS type is specifically designated as non-reclaimable. The spec is also
> denotes that both normal and NVS types should be preserved across S4 - that 
> sounds
> ambiguous to me. But it might mean that non-NVS preservation is not OS
> responsibility as it's assumed to be static.

I've taken a random system and found that all of its "real" ACPI
tables, in particular the DSDT, live in "ACPI data", not "ACPI NVS".
The DSDT includes AML, which I understand is needed at runtime. So
"after it reads the ACPI tables" is even more ambiguous than just
wrt S4 as you say.

> Our region also contains VM86 TSS which we certainly need at runtime 
> (although that
> could be allocated from the reserved region above if desirable).
> 
> To stay on the safe side and do not rely on perceived sensible OS behavior 
> NVS type
> was chosen which OS shouldn't touch under any circumstances.

Can you at the very least emphasize this in the description?

>>> --- a/tools/firmware/hvmloader/e820.c
>>> +++ b/tools/firmware/hvmloader/e820.c
>>> @@ -155,6 +155,8 @@ int build_e820_table(struct e820entry *e820,
>>>  {
>>>      unsigned int nr = 0, i, j;
>>>      uint32_t low_mem_end = hvm_info->low_mem_pgend << PAGE_SHIFT;
>>> +    uint32_t firmware_mem_end =
>>> +        RESERVED_MEMORY_DYNAMIC_START + (mem_mfns_allocated() << 
>>> PAGE_SHIFT);
>>
>> Here and elsewhere - please avoid uint32_t and friends when a plain
>> C type (unsigned int or unsigned long in this case) will do.
> 
> Ok, should I also take a chance and convert some of the surroundings?

In general I'd recommend only adjusting code which gets touched
anyway; in a few cases adjacent code better also gets adjusted
so everything together looks reasonably consistent afterwards.
But I didn't think this was necessary here. IOW - I'd suggest
you don't, but in the end it's up to you (at the risk of needing
to undo parts if you end up going too far).

Jan