Re: [PATCH v3] hvmloader: indicate ACPI tables with "ACPI data" type in e820

From: Jan Beulich <jbeulich@suse.com>
To: Igor Druzhinin <igor.druzhinin@citrix.com>
Cc: xen-devel@lists.xenproject.org, andrew.cooper3@citrix.com,
	roger.pau@citrix.com, wl@xen.org, iwj@xenproject.org
Subject: Re: [PATCH v3] hvmloader: indicate ACPI tables with "ACPI data" type in e820
Date: Tue, 8 Sep 2020 11:15:18 +0200	[thread overview]
Message-ID: <376039c8-41d7-2216-36e8-1a226b0af499@suse.com> (raw)
In-Reply-To: <1599522163-21992-1-git-send-email-igor.druzhinin@citrix.com>

On 08.09.2020 01:42, Igor Druzhinin wrote:
> Changes in v3:
> - switched from NVS to regular "ACPI data" type by separating ACPI allocations
>   into their own region
> - gave more information on particular kexec usecase that requires this change

Thanks a lot for doing both of these.

> --- 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 acpi_mem_end =
> +        ACPI_MEMORY_DYNAMIC_START + (acpi_pages_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 tables as ACPI data.
> +     * That should help the guest to treat it correctly later: e.g. pass to
> +     * the next kernel on kexec or reclaim if necessary.
> +     */
> +
> +    e820[nr].addr = RESERVED_MEMBASE;
> +    e820[nr].size = acpi_mem_end - RESERVED_MEMBASE;
> +    e820[nr].type = E820_ACPI;
> +    nr++;

This region may be empty (afaict), when !acpi_enabled. Imo we'd better
avoid adding empty ranges.

> @@ -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 = acpi_mem_end;
> +        e820[nr].size = igd_opregion_base - acpi_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 = acpi_mem_end;
>          e820[nr].size = (uint32_t)-e820[nr].addr;
>          e820[nr].type = E820_RESERVED;
>          nr++;

In both cases - why not RESERVED_MEMORY_DYNAMIC_START? I.e. why
mark reserved space that isn't in use for anything?

Jan