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

From: Igor Druzhinin <igor.druzhinin@citrix.com>
To: Jan Beulich <jbeulich@suse.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:44:45 +0100	[thread overview]
Message-ID: <01346ec8-ece4-200a-d108-bd65aab23d47@citrix.com> (raw)
In-Reply-To: <376039c8-41d7-2216-36e8-1a226b0af499@suse.com>

On 08/09/2020 10:15, Jan Beulich wrote:
> 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.

Thanks, will gate creation of this region on acpi_enabled.

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

I think it's better to reserve space that a) isn't suppose to be in use for anything - 
we don't really want some MMIO being accidentally mapped there and confusing whatever in
our fragile model, b) that wasn't a hole before so it'd be dangerous to make it that
way here. Overall, I think it's better to keep this space as reserved as possible as
before.

Igor