[0/3] hw/arm: Pack the QEMU generated device tree

Message

Bin Meng Jan. 15, 2024, 4:34 a.m. UTC

By default, QEMU generates a 1 MiB sized device tree. This appears
to be unnecessary, as the actual size is much smaller than what the
DTB header claims. Let's pack it to save some room.


Bin Meng (3):
  hw/arm: Refactor struct arm_boot_info::get_dtb()
  hw/arm: Pack the QEMU generated device tree
  tests/acpi: Update virt/SSDT.memhp

 include/hw/arm/boot.h           |   8 ++++----
 hw/arm/boot.c                   |  14 +++++++++++++-
 hw/arm/sbsa-ref.c               |   3 +--
 hw/arm/virt.c                   |   4 +---
 hw/arm/xlnx-versal-virt.c       |   4 +---
 tests/data/acpi/virt/SSDT.memhp | Bin 1817 -> 1817 bytes
 6 files changed, 20 insertions(+), 13 deletions(-)

Comments

Alex Bennée Jan. 15, 2024, 11:39 a.m. UTC | #1

Bin Meng <[e-mail]> writes:

> The Arm dtb changes caused an address change:
>
>  DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000001)
>  {
>      [ ... ]
> -    Name (MEMA, 0x43C80000)
> +    Name (MEMA, 0x43D80000)
>  }

I'm confused by why this changes. Isn't this declaring the size of a
NVDIMM region of the memory map? Why does a DTB change affect an ACPI
based boot?

>
> Signed-off-by: Bin Meng <[e-mail]>
>
> ---
>
>  tests/data/acpi/virt/SSDT.memhp | Bin 1817 -> 1817 bytes
>  1 file changed, 0 insertions(+), 0 deletions(-)
>
> diff --git a/tests/data/acpi/virt/SSDT.memhp b/tests/data/acpi/virt/SSDT.memhp
> index fb3dcde5a10936667ad75a759b8bd444a7b19fc2..4d3ef733276bf5992da5b0bb967f6d60e243417d 100644
> GIT binary patch
> delta 22
> dcmbQqH<OPmIM^jblAVEpao$EQUUsG%&Hz1D1wsG-
>
> delta 22
> dcmbQqH<OPmIM^jblAVEpaot8PUUsGv&Hz2O1wsG-

Bin Meng Jan. 15, 2024, 2:46 p.m. UTC | #2

On Mon, Jan 15, 2024 at 7:40 PM Alex Bennée <[e-mail]> wrote:
>
> Bin Meng <[e-mail]> writes:
>
> > The Arm dtb changes caused an address change:
> >
> >  DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000001)
> >  {
> >      [ ... ]
> > -    Name (MEMA, 0x43C80000)
> > +    Name (MEMA, 0x43D80000)
> >  }
>
> I'm confused by why this changes. Isn't this declaring the size of a
> NVDIMM region of the memory map? Why does a DTB change affect an ACPI
> based boot?
>

I have no idea too. I suspect that's because the AllocateAlignedPages
call to allocate a 1 MiB aligned address in the BiosTableTest.c is
affected by the shrinked DTB now.

+ Laszlo who might know the root cause.

Regards,
Bin

Laszlo Ersek Jan. 15, 2024, 7:07 p.m. UTC | #3

On 1/15/24 15:46, Bin Meng wrote:
> On Mon, Jan 15, 2024 at 7:40 PM Alex Bennée <[e-mail]> wrote:
>>
>> Bin Meng <[e-mail]> writes:
>>
>>> The Arm dtb changes caused an address change:
>>>
>>>  DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000001)
>>>  {
>>>      [ ... ]
>>> -    Name (MEMA, 0x43C80000)
>>> +    Name (MEMA, 0x43D80000)
>>>  }
>>
>> I'm confused by why this changes. Isn't this declaring the size of a
>> NVDIMM region of the memory map? Why does a DTB change affect an ACPI
>> based boot?
>>
> 
> I have no idea too. I suspect that's because the AllocateAlignedPages
> call to allocate a 1 MiB aligned address in the BiosTableTest.c is
> affected by the shrinked DTB now.
> 
> + Laszlo who might know the root cause.

Just speculating:

from "docs/specs/acpi_nvdimm.rst":

Memory:
   QEMU uses BIOS Linker/loader feature to ask BIOS to allocate a memory
   page and dynamically patch its address into an int32 object named "MEMA"
   in ACPI.

Therefore any QEMU-side change that affects memory allocations in the guest may affect the ACPI contents (captured later).

I don't know what the DTB change at hand was, but if (for example) the DTB has grown significantly, that could lead to this. The guest firmware stashes a dynamically allocated copy of the DTB, early on in the PEI phase. Some growth there may change the initial memory map of the DXE phase, which could affect the ACPI linker/loader's allocation operations.

If you can attach the DTB before-after, and the *verbose* firmware log before-after, we might find out finer details.

Laszlo

Peter Maydell Jan. 19, 2024, 2:29 p.m. UTC | #4

On Mon, 15 Jan 2024 at 04:35, Bin Meng <[e-mail]> 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 <[e-mail]>
>
> ---

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

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.)

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.

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"...)

thanks
-- PMM

Laszlo Ersek Jan. 19, 2024, 5:19 p.m. UTC | #5

On 1/19/24 15:29, Peter Maydell wrote:
> On Mon, 15 Jan 2024 at 04:35, Bin Meng <[e-mail]> 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 <[e-mail]>
>>
>> ---
> 
> 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

Peter Maydell Jan. 19, 2024, 5:46 p.m. UTC | #6

On Fri, 19 Jan 2024 at 17:19, Laszlo Ersek <[e-mail]> wrote:
>
> On 1/19/24 15:29, Peter Maydell wrote:
> > On Mon, 15 Jan 2024 at 04:35, Bin Meng <[e-mail]> 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 <[e-mail]>
> >>
> >> ---
> >
> > 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.

Mmm, but previously we weren't packing the dtb we created,
so it would always be the same 1MB regardless of what and
how much we put into it. After this patchset it will be packed
down to its "real" size, so the size will be much more variable.

thanks
-- PMM