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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

Jan