Re: [PATCH v2.1] hvmloader: indicate dynamically allocated memory as ACPI NVS in e820

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Sep 01, 2020 at 03:50:34AM +0100, 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").

Can you add a note that this is a Linux commit? Albeit there's a
reference to kexec above I don't it's entirely clear it's a Linux
commit.

> 
> 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) in contrast with ACPI reclaim (ACPI table) memory would avoid
> potential reuse of this memory by the guest taking into account this region
> may contain runtime structures like VM86 TSS, etc. If necessary, those
> can be moved away later and the region marked as reclaimable.

By looking at domain_construct_memmap from libxl I think the same
problem is not present on that case as regions are properly marked as
RESERVED or ACPI as required?

> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

Just one question below and one nit.

> ---
> Changes in v2.1:
> - fixed previously missed uint32_t occurence
> 
> Changes in v2:
> - gave more information on NVS type selection and potential alternatives
>   in the description
> - minor type fixes suggested
> 
> ---
>  tools/firmware/hvmloader/e820.c | 21 +++++++++++++++++----
>  tools/firmware/hvmloader/util.c |  6 ++++++
>  tools/firmware/hvmloader/util.h |  3 +++
>  3 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
> index 4d1c955..0ad2f05 100644
> --- 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;
> +    unsigned long firmware_mem_end =
> +        RESERVED_MEMORY_DYNAMIC_START + (mem_mfns_allocated() << PAGE_SHIFT);
>  
>      if ( !lowmem_reserved_base )
>              lowmem_reserved_base = 0xA0000;
> @@ -199,8 +201,19 @@ int build_e820_table(struct e820entry *e820,
>      nr++;
>  
>      /*
> +     * Mark populated reserved memory that contains ACPI and other tables as
> +     * ACPI NVS (non-reclaimable) space - that should help the guest to treat
> +     * it correctly later (e.g. pass to the next kernel on kexec).
> +     */
> +
> +    e820[nr].addr = RESERVED_MEMBASE;
> +    e820[nr].size = firmware_mem_end - RESERVED_MEMBASE;
> +    e820[nr].type = E820_NVS;
> +    nr++;
> +
> +    /*
>       * Explicitly reserve space for special pages.
> -     * This space starts at RESERVED_MEMBASE an extends to cover various
> +     * This space starts after ACPI region and extends to cover various
>       * fixed hardware mappings (e.g., LAPIC, IOAPIC, default SVGA framebuffer).
>       *
>       * If igd_opregion_pgbase we need to split the RESERVED region in two.
> @@ -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 = igd_opregion_base - firmware_mem_end;

Is there anything between firmware_mem_end and igd_opregion_base now?
You already account for RESERVED_MEMBASE to firmware_mem_end.

>          e820[nr].type = E820_RESERVED;
>          nr++;
>  
> @@ -227,7 +240,7 @@ int build_e820_table(struct e820entry *e820,
>      }
>      else
>      {
> -        e820[nr].addr = RESERVED_MEMBASE;
> +        e820[nr].addr = firmware_mem_end;
>          e820[nr].size = (uint32_t)-e820[nr].addr;
>          e820[nr].type = E820_RESERVED;
>          nr++;
> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
> index 0c3f2d2..59cde4a 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -444,6 +444,12 @@ void mem_hole_populate_ram(xen_pfn_t mfn, uint32_t nr_mfns)
>  static uint32_t alloc_up = RESERVED_MEMORY_DYNAMIC_START - 1;
>  static uint32_t alloc_down = RESERVED_MEMORY_DYNAMIC_END;
>  
> +unsigned long mem_mfns_allocated(void)

mem_pages_allocated might be better.

Thanks, Roger.