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

[Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>]

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.

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.

In fact while writing this response I found that document which confirms some 
of my
suspicions:
http://www.baldwin.cx/~phoenix/reference/docs/acpi_impguide.pdf

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

>> @@ -210,8 +223,8 @@ int build_e820_table(struct e820entry *e820,
>>      {
>>          uint32_t igd_opregion_base = igd_opregion_pgbase << PAGE_SHIFT;
>>  
>> -        e820[nr].addr = RESERVED_MEMBASE;
>> -        e820[nr].size = (uint32_t) igd_opregion_base - RESERVED_MEMBASE;
>> +        e820[nr].addr = firmware_mem_end;
>> +        e820[nr].size = (uint32_t) igd_opregion_base - firmware_mem_end;
> 
> Any chance I could talk you into also dropping the pointless cast
> at this occasion?

Sure.

Igor