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

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

[Bin Meng]

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

-- 
2.34.1

[PATCH 1/3] hw/arm: Refactor struct arm_boot_info::get_dtb()

[Bin Meng]

At present we expect struct arm_boot_info::get_dtb() to return the
device tree pointer as well as the device tree size. However, this
is not necessary as we can get the device tree size via the device
tree header directly. Change get_dtb() signature to drop the *size
argument, and get the size by ourselves.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 include/hw/arm/boot.h     | 8 ++++----
 hw/arm/boot.c             | 3 ++-
 hw/arm/sbsa-ref.c         | 3 +--
 hw/arm/virt.c             | 4 +---
 hw/arm/xlnx-versal-virt.c | 4 +---
 5 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/include/hw/arm/boot.h b/include/hw/arm/boot.h
index 80c492d742..37fd1b520e 100644
--- a/include/hw/arm/boot.h
+++ b/include/hw/arm/boot.h
@@ -82,11 +82,11 @@ struct arm_boot_info {
                                      const struct arm_boot_info *info);
     /* if a board is able to create a dtb without a dtb file then it
      * sets get_dtb. This will only be used if no dtb file is provided
-     * by the user. On success, sets *size to the length of the created
-     * dtb, and returns a pointer to it. (The caller must free this memory
-     * with g_free() when it has finished with it.) On failure, returns NULL.
+     * by the user. On success, returns a pointer to it. (The caller must
+     * free this memory with g_free() when it has finished with it.)
+     * On failure, returns NULL.
      */
-    void *(*get_dtb)(const struct arm_boot_info *info, int *size);
+    void *(*get_dtb)(const struct arm_boot_info *info);
     /* if a board needs to be able to modify a device tree provided by
      * the user it should implement this hook.
      */
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 84ea6a807a..ff1173299f 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -538,11 +538,12 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
         }
         g_free(filename);
     } else {
-        fdt = binfo->get_dtb(binfo, &size);
+        fdt = binfo->get_dtb(binfo);
         if (!fdt) {
             fprintf(stderr, "Board was unable to create a dtb blob\n");
             goto fail;
         }
+        size = fdt_totalsize(fdt);
     }
 
     if (addr_limit > addr && size > (addr_limit - addr)) {
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 477dca0637..c5023871a7 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -681,12 +681,11 @@ static void create_pcie(SBSAMachineState *sms)
     create_smmu(sms, pci->bus);
 }
 
-static void *sbsa_ref_dtb(const struct arm_boot_info *binfo, int *fdt_size)
+static void *sbsa_ref_dtb(const struct arm_boot_info *binfo)
 {
     const SBSAMachineState *board = container_of(binfo, SBSAMachineState,
                                                  bootinfo);
 
-    *fdt_size = board->fdt_size;
     return board->fdt;
 }
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 2793121cb4..1996fffa99 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1577,14 +1577,12 @@ static void create_secure_ram(VirtMachineState *vms,
     g_free(nodename);
 }
 
-static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
+static void *machvirt_dtb(const struct arm_boot_info *binfo)
 {
     const VirtMachineState *board = container_of(binfo, VirtMachineState,
                                                  bootinfo);
     MachineState *ms = MACHINE(board);
 
-
-    *fdt_size = board->fdt_size;
     return ms->fdt;
 }
 
diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
index 537118224f..1e043c813e 100644
--- a/hw/arm/xlnx-versal-virt.c
+++ b/hw/arm/xlnx-versal-virt.c
@@ -551,12 +551,10 @@ static void versal_virt_modify_dtb(const struct arm_boot_info *binfo,
     fdt_add_memory_nodes(s, fdt, binfo->ram_size);
 }
 
-static void *versal_virt_get_dtb(const struct arm_boot_info *binfo,
-                                  int *fdt_size)
+static void *versal_virt_get_dtb(const struct arm_boot_info *binfo)
 {
     const VersalVirt *board = container_of(binfo, VersalVirt, binfo);
 
-    *fdt_size = board->fdt_size;
     return board->fdt;
 }
 
-- 
2.34.1

Re: [PATCH 1/3] hw/arm: Refactor struct arm_boot_info::get_dtb()

[Philippe Mathieu-Daudé]

On 15/1/24 05:34, Bin Meng wrote:
> At present we expect struct arm_boot_info::get_dtb() to return the
> device tree pointer as well as the device tree size. However, this
> is not necessary as we can get the device tree size via the device
> tree header directly. Change get_dtb() signature to drop the *size
> argument, and get the size by ourselves.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>   include/hw/arm/boot.h     | 8 ++++----
>   hw/arm/boot.c             | 3 ++-
>   hw/arm/sbsa-ref.c         | 3 +--
>   hw/arm/virt.c             | 4 +---
>   hw/arm/xlnx-versal-virt.c | 4 +---
>   5 files changed, 9 insertions(+), 13 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/3] hw/arm: Refactor struct arm_boot_info::get_dtb()

[Alistair Francis]

On Mon, Jan 15, 2024 at 2:36 PM Bin Meng <bin.meng@windriver.com> wrote:
>
> At present we expect struct arm_boot_info::get_dtb() to return the
> device tree pointer as well as the device tree size. However, this
> is not necessary as we can get the device tree size via the device
> tree header directly. Change get_dtb() signature to drop the *size
> argument, and get the size by ourselves.
>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>
>  include/hw/arm/boot.h     | 8 ++++----
>  hw/arm/boot.c             | 3 ++-
>  hw/arm/sbsa-ref.c         | 3 +--
>  hw/arm/virt.c             | 4 +---
>  hw/arm/xlnx-versal-virt.c | 4 +---
>  5 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/include/hw/arm/boot.h b/include/hw/arm/boot.h
> index 80c492d742..37fd1b520e 100644
> --- a/include/hw/arm/boot.h
> +++ b/include/hw/arm/boot.h
> @@ -82,11 +82,11 @@ struct arm_boot_info {
>                                       const struct arm_boot_info *info);
>      /* if a board is able to create a dtb without a dtb file then it
>       * sets get_dtb. This will only be used if no dtb file is provided
> -     * by the user. On success, sets *size to the length of the created
> -     * dtb, and returns a pointer to it. (The caller must free this memory
> -     * with g_free() when it has finished with it.) On failure, returns NULL.
> +     * by the user. On success, returns a pointer to it. (The caller must
> +     * free this memory with g_free() when it has finished with it.)
> +     * On failure, returns NULL.
>       */
> -    void *(*get_dtb)(const struct arm_boot_info *info, int *size);
> +    void *(*get_dtb)(const struct arm_boot_info *info);
>      /* if a board needs to be able to modify a device tree provided by
>       * the user it should implement this hook.
>       */
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 84ea6a807a..ff1173299f 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -538,11 +538,12 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>          }
>          g_free(filename);
>      } else {
> -        fdt = binfo->get_dtb(binfo, &size);
> +        fdt = binfo->get_dtb(binfo);
>          if (!fdt) {
>              fprintf(stderr, "Board was unable to create a dtb blob\n");
>              goto fail;
>          }
> +        size = fdt_totalsize(fdt);
>      }
>
>      if (addr_limit > addr && size > (addr_limit - addr)) {
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 477dca0637..c5023871a7 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -681,12 +681,11 @@ static void create_pcie(SBSAMachineState *sms)
>      create_smmu(sms, pci->bus);
>  }
>
> -static void *sbsa_ref_dtb(const struct arm_boot_info *binfo, int *fdt_size)
> +static void *sbsa_ref_dtb(const struct arm_boot_info *binfo)
>  {
>      const SBSAMachineState *board = container_of(binfo, SBSAMachineState,
>                                                   bootinfo);
>
> -    *fdt_size = board->fdt_size;
>      return board->fdt;
>  }
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 2793121cb4..1996fffa99 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1577,14 +1577,12 @@ static void create_secure_ram(VirtMachineState *vms,
>      g_free(nodename);
>  }
>
> -static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
> +static void *machvirt_dtb(const struct arm_boot_info *binfo)
>  {
>      const VirtMachineState *board = container_of(binfo, VirtMachineState,
>                                                   bootinfo);
>      MachineState *ms = MACHINE(board);
>
> -
> -    *fdt_size = board->fdt_size;
>      return ms->fdt;
>  }
>
> diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> index 537118224f..1e043c813e 100644
> --- a/hw/arm/xlnx-versal-virt.c
> +++ b/hw/arm/xlnx-versal-virt.c
> @@ -551,12 +551,10 @@ static void versal_virt_modify_dtb(const struct arm_boot_info *binfo,
>      fdt_add_memory_nodes(s, fdt, binfo->ram_size);
>  }
>
> -static void *versal_virt_get_dtb(const struct arm_boot_info *binfo,
> -                                  int *fdt_size)
> +static void *versal_virt_get_dtb(const struct arm_boot_info *binfo)
>  {
>      const VersalVirt *board = container_of(binfo, VersalVirt, binfo);
>
> -    *fdt_size = board->fdt_size;
>      return board->fdt;
>  }
>
> --
> 2.34.1
>
>

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

[Bin Meng]

By default QEMU generates a 1 MiB sized device tree. Let's pack it
to save some room.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 hw/arm/boot.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index ff1173299f..511ec10ed0 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -662,6 +662,17 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
         binfo->modify_dtb(binfo, fdt);
     }
 
+    /*
+     * By default QEMU generates a 1 MiB sized device tree.
+     * Let's pack it to save some room.
+     */
+    if (binfo->get_dtb) {
+        rc = fdt_pack(fdt);
+        /* Should only fail if we've built a corrupted tree */
+        g_assert(rc == 0);
+        size = fdt_totalsize(fdt);
+    }
+
     qemu_fdt_dumpdtb(fdt, size);
 
     /* Put the DTB into the memory map as a ROM image: this will ensure
-- 
2.34.1

Re: [PATCH 2/3] hw/arm: Pack the QEMU generated device tree

[Peter Maydell]

On Mon, 15 Jan 2024 at 04:34, Bin Meng <bin.meng@windriver.com> wrote:
>
> By default QEMU generates a 1 MiB sized device tree. Let's pack it
> to save some room.
>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
>
>  hw/arm/boot.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index ff1173299f..511ec10ed0 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -662,6 +662,17 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>          binfo->modify_dtb(binfo, fdt);
>      }
>
> +    /*
> +     * By default QEMU generates a 1 MiB sized device tree.
> +     * Let's pack it to save some room.
> +     */
> +    if (binfo->get_dtb) {
> +        rc = fdt_pack(fdt);
> +        /* Should only fail if we've built a corrupted tree */
> +        g_assert(rc == 0);

We generally use plain old assert(), not g_assert().

> +        size = fdt_totalsize(fdt);
> +    }
> +
>      qemu_fdt_dumpdtb(fdt, size);

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Bin Meng]

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>

---

 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-

-- 
2.34.1

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

[Alex Bennée]

Bin Meng <bin.meng@windriver.com> 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 <bin.meng@windriver.com>
>
> ---
>
>  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-

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Bin Meng]

On Mon, Jan 15, 2024 at 7:40 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Bin Meng <bin.meng@windriver.com> 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

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

[Laszlo Ersek]

On 1/15/24 15:46, Bin Meng wrote:
> On Mon, Jan 15, 2024 at 7:40 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Bin Meng <bin.meng@windriver.com> 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

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

[Peter Maydell]

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

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

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

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

[Peter Maydell]

On Fri, 19 Jan 2024 at 17:19, Laszlo Ersek <lersek@redhat.com> wrote:
>
> 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.

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