Re: [PATCH 3/3] tests/acpi: Update virt/SSDT.memhp

[Laszlo Ersek]

On 1/19/24 15:29, Peter Maydell wrote:
> On Mon, 15 Jan 2024 at 04:35, Bin Meng <bin.meng@windriver.com> wrote:
>>
>> The Arm dtb changes caused an address change:
>>
>>  DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000001)
>>  {
>>      [ ... ]
>> -    Name (MEMA, 0x43C80000)
>> +    Name (MEMA, 0x43D80000)
>>  }
>>
>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>>
>> ---
> 
> You should follow up (with Laszlo?) to make sure we understand
> why reducing the size of the generated dtb has caused this
> change in the ACPI tables. In particular, if we made the
> dtb *smaller* why has the allocated address here got *larger*?

As a very roughly stated trait (i.e., I'm not claiming this is an exact,
hard rule), the UEFI memory allocator hands out chunks top-down. An
earlier allocation (such as the DTB's) shrinking is consistent with
further allocations being serviced at higher addresses.

> 
> This particular bit of the ACPI tables does seem to be
> annoyingly unstable, though -- for instance commit 55abfc1ffbe54c0
> we had to change this figure when we updated to a newer EDK2
> version, and similarly commit 5f88dd43d0 for the same reason.
> I wonder if we can or should make our data-check be more
> loose about the address reported here, given what Laszlo
> says about how we're basically looking at the address of some
> memory the guest allocated. (cc'd the bios-tables-test
> maintainers for their opinion.)

Right, the allocation address is generally unpredictable. (That's why
the ACPI linker/loader "language" had to be extended with an extra
command, for the sake of the vmgenid device -- so that the firmware
could send the allocation GPA back to QEMU in an "architected" way.)

> 
> I'm also a little concerned that if the ACPI generated
> tables care about the dtb size then we're now going to
> have a situation where any patch we make to the virt board
> that changes the generated dtb at all will result in the
> ACPI tables changing. That would be annoying.

This is generally inevitable, it's just how the ACPI linker/loader
works. The guest allocator can only work with the memory map it gets
from QEMU. The same effect is triggered BTW if you don't change the DTB
but change (on the QEMU command line) the guest RAM size. The ACPI
tables will be allocated at different addresses than before, and so the
pointer fields in other tables, to those tables, will also change.

> 
> Finally, if we do need to update the reference data in
> tests/data/acpi, there is a multi-stage procedure for
> this, documented in the comment at the top of
> tests/qtest/bios-tables-test.c -- basically you need
> first to have a patch that says "ignore discrepancies in
> these files", then the patch that makes the actual change to
> QEMU (in this case your patch 2 in this series), then the
> patch which updates the reference data and removes the files
> from the ignore-this list. (It is because this is a bit of a
> pain that I definitely don't want "any small change to the dtb"
> to turn into "ACPI tables change"...)

Laszlo