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
