[RFC,0/4] ARM virt: ACPI memory hotplug support

Message

Shameerali Kolothum Thodi Jan. 28, 2019, 11:05 a.m. UTC

This series is an attempt to provide hotplug support to both
pc-dimm and nvdimm device memory on ARM virt platform. This is
based on Eric's recent works to support PCDIMM/NVDIMM device memory[1].
The kernel support for arm64 memory hot add was added only
recently by Robin[2] and hence the guest kernel should be => 5.0-rc1.

This makes use of PL061 GPIO controller to sent related ACPI events
to the Guest. The only reference I could find with respect to the GPIO
pins usage is here[3] which says, "use PIN 3 for system_powerdown,
reserving PIN 0, 1, 2 for PCI hotplug, CPU hotplug and memory hotplug".
Hence Pin 2 is used for PCDIMM and pin 4 for NVDIMM.

This is sanity tested on a HiSilicon ARM64 platform and appreciate
any further testing.

This series can be applied on top of Eric's branch here[4]

Test:
------
Please use a Guest kernel image >5.0-rc1 with all the mem/nvdimm
hotplug related CONFIGs enabled.

./qemu-system-aarch64 \
-machine virt,gic-version=3,nvdimm \
-m 1G,maxmem=4G,slots=4 \
-cpu host \
-kernel Image \
-initrd rootfs-iperf.cpio \
-bios QEMU_EFI.fd \
-numa node,nodeid=0 \
-net none \
-nographic -enable-kvm \
-append "console=ttyAMA0 acpi=force earlycon=pl011,0x9000000"

Enter Qemu monitor,
Add pc-dimm:
object_add memory-backend-ram,id=mem1,size=1G
device_add pc-dimm,id=dimm1,memdev=mem1

Add nvdimm:
object_add memory-backend-ram,id=mem2,size=1G
device_add nvdimm,id=dimm2,memdev=mem2

Known Issue:

It is observed that hot adding nvdimm will results in guest reboot
failure. EDK2 fails to build the ACPI tables on reboot. Please find
below EDK2 log on Guest reboot after nvdimm hot-add,

ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
OnRootBridgesConnected: InstallAcpiTables: Protocol Error

The root cause seems to be EDK2 ACPI table checksum failure
as NFIT table is getting updated on hot-add. This needs further
investigation.

Thanks,
Shameer

[1]https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05740.html
[2]https://patchwork.kernel.org/patch/10724455/
[3]https://lists.gnu.org/archive/html/qemu-arm/2015-12/msg00095.html
[4]https://github.com/eauger/qemu/tree/v3.1.0-dimm-v5

Shameer Kolothum (4):
  hw:acpi: Make ACPI IO address space configurable
  hw/arm/virt: Add GPIO based pcdimm hotplug ACPI event support
  hw/arm/virt: Enable pc-dimm hotplug support
  hw/arm/virt: Add nvdimm hotplug support

 default-configs/arm-softmmu.mak  |   1 +
 hw/acpi/memory_hotplug.c         |  13 +++--
 hw/arm/virt-acpi-build.c         |  45 +++++++++++++++--
 hw/arm/virt.c                    | 105 ++++++++++++++++++++++++++++++++++++---
 hw/i386/acpi-build.c             |   3 +-
 include/hw/acpi/memory_hotplug.h |   6 ++-
 include/hw/arm/virt.h            |  15 ++++++
 7 files changed, 168 insertions(+), 20 deletions(-)

Comments

Eric Auger Feb. 22, 2019, 4:03 p.m. UTC | #1

Hi Shameer,

On 1/28/19 12:05 PM, Shameer Kolothum wrote:
> This series is an attempt to provide hotplug support to both
> pc-dimm and nvdimm device memory on ARM virt platform. This is
> based on Eric's recent works to support PCDIMM/NVDIMM device memory[1].
> The kernel support for arm64 memory hot add was added only
> recently by Robin[2] and hence the guest kernel should be => 5.0-rc1.
> 
> This makes use of PL061 GPIO controller to sent related ACPI events
s/sent/send
> to the Guest. The only reference I could find with respect to the GPIO
> pins usage is here[3] which says, "use PIN 3 for system_powerdown,
> reserving PIN 0, 1, 2 for PCI hotplug, CPU hotplug and memory hotplug".
> Hence Pin 2 is used for PCDIMM and pin 4 for NVDIMM.
> 
> This is sanity tested on a HiSilicon ARM64 platform and appreciate
> any further testing.

I did some testing on another platform and I got the exactly the same
results as yours: PCDIMM hot plug works fine. Also after system_reset I
still can see the slots.
Hot-unplug is not supported though.
For NVDIMM, hot-add works fine and and I can see the slots using ndctl
on guest. But after system_reset, the guest does not boot properly.

> 
> This series can be applied on top of Eric's branch here[4]
> 
> Test:
> ------
> Please use a Guest kernel image >5.0-rc1 with all the mem/nvdimm
> hotplug related CONFIGs enabled.
> 
> ./qemu-system-aarch64 \
> -machine virt,gic-version=3,nvdimm \
> -m 1G,maxmem=4G,slots=4 \
> -cpu host \
> -kernel Image \
> -initrd rootfs-iperf.cpio \
> -bios QEMU_EFI.fd \
> -numa node,nodeid=0 \
> -net none \
> -nographic -enable-kvm \
> -append "console=ttyAMA0 acpi=force earlycon=pl011,0x9000000"
> 
> Enter Qemu monitor,
> Add pc-dimm:
> object_add memory-backend-ram,id=mem1,size=1G
> device_add pc-dimm,id=dimm1,memdev=mem1
> 
> Add nvdimm:
> object_add memory-backend-ram,id=mem2,size=1G
> device_add nvdimm,id=dimm2,memdev=mem2
> 
> Known Issue:
> 
> It is observed that hot adding nvdimm will results in guest reboot
> failure. EDK2 fails to build the ACPI tables on reboot. Please find
> below EDK2 log on Guest reboot after nvdimm hot-add,
> 
> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
> 
> The root cause seems to be EDK2 ACPI table checksum failure
> as NFIT table is getting updated on hot-add. This needs further
> investigation.
+ Ard, Leif, Laszlo if they have any idea of what is missing/wrong.

Thanks

Eric
> 
> Thanks,
> Shameer
> 
> [1]https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05740.html
> [2]https://patchwork.kernel.org/patch/10724455/
> [3]https://lists.gnu.org/archive/html/qemu-arm/2015-12/msg00095.html
> [4]https://github.com/eauger/qemu/tree/v3.1.0-dimm-v5
> 
> Shameer Kolothum (4):
>   hw:acpi: Make ACPI IO address space configurable
>   hw/arm/virt: Add GPIO based pcdimm hotplug ACPI event support
>   hw/arm/virt: Enable pc-dimm hotplug support
>   hw/arm/virt: Add nvdimm hotplug support
> 
>  default-configs/arm-softmmu.mak  |   1 +
>  hw/acpi/memory_hotplug.c         |  13 +++--
>  hw/arm/virt-acpi-build.c         |  45 +++++++++++++++--
>  hw/arm/virt.c                    | 105 ++++++++++++++++++++++++++++++++++++---
>  hw/i386/acpi-build.c             |   3 +-
>  include/hw/acpi/memory_hotplug.h |   6 ++-
>  include/hw/arm/virt.h            |  15 ++++++
>  7 files changed, 168 insertions(+), 20 deletions(-)
>

Laszlo Ersek Feb. 22, 2019, 7:11 p.m. UTC | #2

On 02/22/19 17:03, Auger Eric wrote:
> Hi Shameer,
> 
> On 1/28/19 12:05 PM, Shameer Kolothum wrote:
>> This series is an attempt to provide hotplug support to both
>> pc-dimm and nvdimm device memory on ARM virt platform. This is
>> based on Eric's recent works to support PCDIMM/NVDIMM device memory[1].
>> The kernel support for arm64 memory hot add was added only
>> recently by Robin[2] and hence the guest kernel should be => 5.0-rc1.
>>
>> This makes use of PL061 GPIO controller to sent related ACPI events
> s/sent/send
>> to the Guest. The only reference I could find with respect to the GPIO
>> pins usage is here[3] which says, "use PIN 3 for system_powerdown,
>> reserving PIN 0, 1, 2 for PCI hotplug, CPU hotplug and memory hotplug".
>> Hence Pin 2 is used for PCDIMM and pin 4 for NVDIMM.
>>
>> This is sanity tested on a HiSilicon ARM64 platform and appreciate
>> any further testing.
> 
> I did some testing on another platform and I got the exactly the same
> results as yours: PCDIMM hot plug works fine. Also after system_reset I
> still can see the slots.
> Hot-unplug is not supported though.
> For NVDIMM, hot-add works fine and and I can see the slots using ndctl
> on guest. But after system_reset, the guest does not boot properly.
> 
>>
>> This series can be applied on top of Eric's branch here[4]
>>
>> Test:
>> ------
>> Please use a Guest kernel image >5.0-rc1 with all the mem/nvdimm
>> hotplug related CONFIGs enabled.
>>
>> ./qemu-system-aarch64 \
>> -machine virt,gic-version=3,nvdimm \
>> -m 1G,maxmem=4G,slots=4 \
>> -cpu host \
>> -kernel Image \
>> -initrd rootfs-iperf.cpio \
>> -bios QEMU_EFI.fd \
>> -numa node,nodeid=0 \
>> -net none \
>> -nographic -enable-kvm \
>> -append "console=ttyAMA0 acpi=force earlycon=pl011,0x9000000"
>>
>> Enter Qemu monitor,
>> Add pc-dimm:
>> object_add memory-backend-ram,id=mem1,size=1G
>> device_add pc-dimm,id=dimm1,memdev=mem1
>>
>> Add nvdimm:
>> object_add memory-backend-ram,id=mem2,size=1G
>> device_add nvdimm,id=dimm2,memdev=mem2
>>
>> Known Issue:
>>
>> It is observed that hot adding nvdimm will results in guest reboot
>> failure. EDK2 fails to build the ACPI tables on reboot. Please find
>> below EDK2 log on Guest reboot after nvdimm hot-add,
>>
>> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
>>
>> The root cause seems to be EDK2 ACPI table checksum failure
>> as NFIT table is getting updated on hot-add. This needs further
>> investigation.
> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.

Huh, very interesting; I usually don't expect my sanity checks to fire
in practice. :)

The message

  ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"

is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
linker/loader script.

Please see the command definition in QEMU's
"hw/acpi/bios-linker-loader.c". In particular, please refer to the
function bios_linker_loader_add_checksum(), which builds the command
structure, and documents the fields.

(You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
"OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
same information.)

The error message is logged if:
- the offset at which the checksum should be stored falls outside of the
size of the fw_cfg blob, or
- the range over which the checksum should be calculated falls outside
(at least in part) of the fw_cfg blob.

To me this suggests that QEMU generates an invalid COMMAND_ADD_CHECKSUM
command for the firmware.

... I've tried to skim the patches briefly. I think there must be an
error in the DSDT building logic that is only active on reboot if an
nvdimm module was hot-added before the reboot.

Thanks,
Laszlo


>> [1]https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05740.html
>> [2]https://patchwork.kernel.org/patch/10724455/
>> [3]https://lists.gnu.org/archive/html/qemu-arm/2015-12/msg00095.html
>> [4]https://github.com/eauger/qemu/tree/v3.1.0-dimm-v5
>>
>> Shameer Kolothum (4):
>>   hw:acpi: Make ACPI IO address space configurable
>>   hw/arm/virt: Add GPIO based pcdimm hotplug ACPI event support
>>   hw/arm/virt: Enable pc-dimm hotplug support
>>   hw/arm/virt: Add nvdimm hotplug support
>>
>>  default-configs/arm-softmmu.mak  |   1 +
>>  hw/acpi/memory_hotplug.c         |  13 +++--
>>  hw/arm/virt-acpi-build.c         |  45 +++++++++++++++--
>>  hw/arm/virt.c                    | 105 ++++++++++++++++++++++++++++++++++++---
>>  hw/i386/acpi-build.c             |   3 +-
>>  include/hw/acpi/memory_hotplug.h |   6 ++-
>>  include/hw/arm/virt.h            |  15 ++++++
>>  7 files changed, 168 insertions(+), 20 deletions(-)
>>

Shameerali Kolothum Thodi Feb. 25, 2019, 9:47 a.m. UTC | #3

Hi Eric,

> -----Original Message-----
> From: Auger Eric [mailto:[e-mail]]
> Sent: 22 February 2019 16:03
> To: Shameerali Kolothum Thodi <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Cc: xuwei (O) <[e-mail]>; Linuxarm <[e-mail]>; Ard
> Biesheuvel <[e-mail]>; Leif Lindholm (Linaro address)
> <[e-mail]>; Laszlo Ersek <[e-mail]>
> Subject: Re: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
> 
> Hi Shameer,
> 
> On 1/28/19 12:05 PM, Shameer Kolothum wrote:
> > This series is an attempt to provide hotplug support to both
> > pc-dimm and nvdimm device memory on ARM virt platform. This is
> > based on Eric's recent works to support PCDIMM/NVDIMM device
> memory[1].
> > The kernel support for arm64 memory hot add was added only
> > recently by Robin[2] and hence the guest kernel should be => 5.0-rc1.
> >
> > This makes use of PL061 GPIO controller to sent related ACPI events
> s/sent/send
> > to the Guest. The only reference I could find with respect to the GPIO
> > pins usage is here[3] which says, "use PIN 3 for system_powerdown,
> > reserving PIN 0, 1, 2 for PCI hotplug, CPU hotplug and memory hotplug".
> > Hence Pin 2 is used for PCDIMM and pin 4 for NVDIMM.
> >
> > This is sanity tested on a HiSilicon ARM64 platform and appreciate
> > any further testing.
> 
> I did some testing on another platform and I got the exactly the same
> results as yours: PCDIMM hot plug works fine. Also after system_reset I
> still can see the slots.
> Hot-unplug is not supported though.

Thanks for giving it a spin. Hot unplug is disabled for now as kernel doesn’t
have support for it yet.

> For NVDIMM, hot-add works fine and and I can see the slots using ndctl
> on guest. But after system_reset, the guest does not boot properly.

Right. And I assume you are seeing the same error message as below.
Thanks for adding relevant people to the discussion.

Cheers,
Shameer

> >
> > This series can be applied on top of Eric's branch here[4]
> >
> > Test:
> > ------
> > Please use a Guest kernel image >5.0-rc1 with all the mem/nvdimm
> > hotplug related CONFIGs enabled.
> >
> > ./qemu-system-aarch64 \
> > -machine virt,gic-version=3,nvdimm \
> > -m 1G,maxmem=4G,slots=4 \
> > -cpu host \
> > -kernel Image \
> > -initrd rootfs-iperf.cpio \
> > -bios QEMU_EFI.fd \
> > -numa node,nodeid=0 \
> > -net none \
> > -nographic -enable-kvm \
> > -append "console=ttyAMA0 acpi=force earlycon=pl011,0x9000000"
> >
> > Enter Qemu monitor,
> > Add pc-dimm:
> > object_add memory-backend-ram,id=mem1,size=1G
> > device_add pc-dimm,id=dimm1,memdev=mem1
> >
> > Add nvdimm:
> > object_add memory-backend-ram,id=mem2,size=1G
> > device_add nvdimm,id=dimm2,memdev=mem2
> >
> > Known Issue:
> >
> > It is observed that hot adding nvdimm will results in guest reboot
> > failure. EDK2 fails to build the ACPI tables on reboot. Please find
> > below EDK2 log on Guest reboot after nvdimm hot-add,
> >
> > ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> > OnRootBridgesConnected: InstallAcpiTables: Protocol Error
> >
> > The root cause seems to be EDK2 ACPI table checksum failure
> > as NFIT table is getting updated on hot-add. This needs further
> > investigation.
> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
> 
> Thanks
> 
> Eric
> >
> > Thanks,
> > Shameer
> >
> > [1]https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05740.html
> > [2]https://patchwork.kernel.org/patch/10724455/
> > [3]https://lists.gnu.org/archive/html/qemu-arm/2015-12/msg00095.html
> > [4]https://github.com/eauger/qemu/tree/v3.1.0-dimm-v5
> >
> > Shameer Kolothum (4):
> >   hw:acpi: Make ACPI IO address space configurable
> >   hw/arm/virt: Add GPIO based pcdimm hotplug ACPI event support
> >   hw/arm/virt: Enable pc-dimm hotplug support
> >   hw/arm/virt: Add nvdimm hotplug support
> >
> >  default-configs/arm-softmmu.mak  |   1 +
> >  hw/acpi/memory_hotplug.c         |  13 +++--
> >  hw/arm/virt-acpi-build.c         |  45 +++++++++++++++--
> >  hw/arm/virt.c                    | 105
> ++++++++++++++++++++++++++++++++++++---
> >  hw/i386/acpi-build.c             |   3 +-
> >  include/hw/acpi/memory_hotplug.h |   6 ++-
> >  include/hw/arm/virt.h            |  15 ++++++
> >  7 files changed, 168 insertions(+), 20 deletions(-)
> >

Shameerali Kolothum Thodi Feb. 25, 2019, 9:54 a.m. UTC | #4

> -----Original Message-----
> From: Laszlo Ersek [mailto:[e-mail]]
> Sent: 22 February 2019 19:11
> To: Auger Eric <[e-mail]>; Shameerali Kolothum Thodi
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]
> Cc: xuwei (O) <[e-mail]>; Linuxarm <[e-mail]>; Ard
> Biesheuvel <[e-mail]>; Leif Lindholm (Linaro address)
> <[e-mail]>
> Subject: Re: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
> 
> On 02/22/19 17:03, Auger Eric wrote:
> > Hi Shameer,
> >
> > On 1/28/19 12:05 PM, Shameer Kolothum wrote:
> >> This series is an attempt to provide hotplug support to both
> >> pc-dimm and nvdimm device memory on ARM virt platform. This is
> >> based on Eric's recent works to support PCDIMM/NVDIMM device
> memory[1].
> >> The kernel support for arm64 memory hot add was added only
> >> recently by Robin[2] and hence the guest kernel should be => 5.0-rc1.
> >>
> >> This makes use of PL061 GPIO controller to sent related ACPI events
> > s/sent/send
> >> to the Guest. The only reference I could find with respect to the GPIO
> >> pins usage is here[3] which says, "use PIN 3 for system_powerdown,
> >> reserving PIN 0, 1, 2 for PCI hotplug, CPU hotplug and memory hotplug".
> >> Hence Pin 2 is used for PCDIMM and pin 4 for NVDIMM.
> >>
> >> This is sanity tested on a HiSilicon ARM64 platform and appreciate
> >> any further testing.
> >
> > I did some testing on another platform and I got the exactly the same
> > results as yours: PCDIMM hot plug works fine. Also after system_reset I
> > still can see the slots.
> > Hot-unplug is not supported though.
> > For NVDIMM, hot-add works fine and and I can see the slots using ndctl
> > on guest. But after system_reset, the guest does not boot properly.
> >
> >>
> >> This series can be applied on top of Eric's branch here[4]
> >>
> >> Test:
> >> ------
> >> Please use a Guest kernel image >5.0-rc1 with all the mem/nvdimm
> >> hotplug related CONFIGs enabled.
> >>
> >> ./qemu-system-aarch64 \
> >> -machine virt,gic-version=3,nvdimm \
> >> -m 1G,maxmem=4G,slots=4 \
> >> -cpu host \
> >> -kernel Image \
> >> -initrd rootfs-iperf.cpio \
> >> -bios QEMU_EFI.fd \
> >> -numa node,nodeid=0 \
> >> -net none \
> >> -nographic -enable-kvm \
> >> -append "console=ttyAMA0 acpi=force earlycon=pl011,0x9000000"
> >>
> >> Enter Qemu monitor,
> >> Add pc-dimm:
> >> object_add memory-backend-ram,id=mem1,size=1G
> >> device_add pc-dimm,id=dimm1,memdev=mem1
> >>
> >> Add nvdimm:
> >> object_add memory-backend-ram,id=mem2,size=1G
> >> device_add nvdimm,id=dimm2,memdev=mem2
> >>
> >> Known Issue:
> >>
> >> It is observed that hot adding nvdimm will results in guest reboot
> >> failure. EDK2 fails to build the ACPI tables on reboot. Please find
> >> below EDK2 log on Guest reboot after nvdimm hot-add,
> >>
> >> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> >> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
> >>
> >> The root cause seems to be EDK2 ACPI table checksum failure
> >> as NFIT table is getting updated on hot-add. This needs further
> >> investigation.
> > + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
> 
> Huh, very interesting; I usually don't expect my sanity checks to fire
> in practice. :)
> 
> The message
> 
>   ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> 
> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
> linker/loader script.
> 
> Please see the command definition in QEMU's
> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
> function bios_linker_loader_add_checksum(), which builds the command
> structure, and documents the fields.
> 
> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
> same information.)
> 
> The error message is logged if:
> - the offset at which the checksum should be stored falls outside of the
> size of the fw_cfg blob, or
> - the range over which the checksum should be calculated falls outside
> (at least in part) of the fw_cfg blob.
> 
> To me this suggests that QEMU generates an invalid
> COMMAND_ADD_CHECKSUM
> command for the firmware.
> 
> ... I've tried to skim the patches briefly. I think there must be an
> error in the DSDT building logic that is only active on reboot if an
> nvdimm module was hot-added before the reboot.

Thanks for taking a look and the pointers. I will debug this further
and get back.

Thanks,
Shameer

> Thanks,
> Laszlo
> 
> 
> >> [1]https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05740.html
> >> [2]https://patchwork.kernel.org/patch/10724455/
> >> [3]https://lists.gnu.org/archive/html/qemu-arm/2015-12/msg00095.html
> >> [4]https://github.com/eauger/qemu/tree/v3.1.0-dimm-v5
> >>
> >> Shameer Kolothum (4):
> >>   hw:acpi: Make ACPI IO address space configurable
> >>   hw/arm/virt: Add GPIO based pcdimm hotplug ACPI event support
> >>   hw/arm/virt: Enable pc-dimm hotplug support
> >>   hw/arm/virt: Add nvdimm hotplug support
> >>
> >>  default-configs/arm-softmmu.mak  |   1 +
> >>  hw/acpi/memory_hotplug.c         |  13 +++--
> >>  hw/arm/virt-acpi-build.c         |  45 +++++++++++++++--
> >>  hw/arm/virt.c                    | 105
> ++++++++++++++++++++++++++++++++++++---
> >>  hw/i386/acpi-build.c             |   3 +-
> >>  include/hw/acpi/memory_hotplug.h |   6 ++-
> >>  include/hw/arm/virt.h            |  15 ++++++
> >>  7 files changed, 168 insertions(+), 20 deletions(-)
> >>

Shameerali Kolothum Thodi Feb. 27, 2019, 12:55 p.m. UTC | #5

Hi Laszlo,

> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 25 February 2019 09:54
> To: 'Laszlo Ersek' <[e-mail]>; Auger Eric <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Cc: xuwei (O) <[e-mail]>; Linuxarm <[e-mail]>; Ard
> Biesheuvel <[e-mail]>; Leif Lindholm (Linaro address)
> <[e-mail]>
> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support

[...]
 
> > >> The root cause seems to be EDK2 ACPI table checksum failure
> > >> as NFIT table is getting updated on hot-add. This needs further
> > >> investigation.
> > > + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
> >
> > Huh, very interesting; I usually don't expect my sanity checks to fire
> > in practice. :)
> >
> > The message
> >
> >   ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> >
> > is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
> > finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
> > linker/loader script.
> >
> > Please see the command definition in QEMU's
> > "hw/acpi/bios-linker-loader.c". In particular, please refer to the
> > function bios_linker_loader_add_checksum(), which builds the command
> > structure, and documents the fields.
> >
> > (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
> > "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
> > same information.)
> >
> > The error message is logged if:
> > - the offset at which the checksum should be stored falls outside of the
> > size of the fw_cfg blob, or
> > - the range over which the checksum should be calculated falls outside
> > (at least in part) of the fw_cfg blob.
> >
> > To me this suggests that QEMU generates an invalid
> > COMMAND_ADD_CHECKSUM
> > command for the firmware.
> >
> > ... I've tried to skim the patches briefly. I think there must be an
> > error in the DSDT building logic that is only active on reboot if an
> > nvdimm module was hot-added before the reboot.
> 
> Thanks for taking a look and the pointers. I will debug this further
> and get back.

The root cause of the issue seems to be UEFI not seeing the updated acpi
table blob size on reboot once a new NFIT table is added(nvdimm hot added).

Please see the debug logs below,

Initial Guest boot
---------------------------

Debug logs from Qemu:

build_header: acpi sig DSDT len 0x5127
build_header: acpi sig FACP len 0x10c
build_header: acpi sig APIC len 0xa8
build_header: acpi sig GTDT len 0x60
build_header: acpi sig MCFG len 0x3c
build_header: acpi sig SPCR len 0x50
build_header: acpi sig SRAT len 0x92
build_header: acpi sig SSDT len 0x38f
build_header: acpi sig XSDT len 0x5c
virt_acpi_build: acpi table_blob len 0x5844

Debug logs from UEFI:

ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0x5C Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 Length=0x14 Blob->Size=0x24
ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20 Start=0x0 Length=0x24 Blob->Size=0x24
InstallQemuFwCfgTables: installed 8 tables

Guest Reboot after ndimm hot added
------------------------------------

Debug logs from Qemu:

build_header: acpi sig DSDT len 0x5127
build_header: acpi sig FACP len 0x10c
build_header: acpi sig APIC len 0xa8
build_header: acpi sig GTDT len 0x60
build_header: acpi sig MCFG len 0x3c
build_header: acpi sig SPCR len 0x50
build_header: acpi sig SRAT len 0x92
build_header: acpi sig SSDT len 0x38f
build_header: acpi sig NFIT len 0xe0  -->New
build_header: acpi sig XSDT len 0x64
virt_acpi_build: acpi table_blob len 0x592c -->blob len updated

Debug logs from UEFI:

ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844  -->Wrong blob size.
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0xE0 Blob->Size=0x5844
ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
OnRootBridgesConnected: InstallAcpiTables: Protocol Error


To me it seems on ARM vit acpi path, the blob len is calculated based
on actual tables and is updated only in virt_acpi_setup() --> acpi_add_rom_blob()
path. I had a look at the x86 code and it looks like, there, the blob len gets updated
with an additional buffer to take care of table resizing[1].

As a hack i added the same to ARM virt and it seems to resolve the issue.
I am not sure this is the best approach to fix this though.

Please let me know your thoughts.

Thanks,
Shameer

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 132414c..4291553 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -50,6 +50,8 @@
 #define ARM_SPI_BASE 32
 #define ACPI_POWER_BUTTON_DEVICE "PWRB"

+#define ACPI_BUILD_TABLE_SIZE    0x20000
+
 static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
 {
     uint16_t i;
@@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
         build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
     }

+    /* Make sure we have a buffer in case we need to resize the tables. */
+    g_array_set_size(tables_blob, ROUND_UP(acpi_data_len(tables_blob),
+                     ACPI_BUILD_TABLE_SIZE));
+
     /* Cleanup memory that's no longer used. */
     g_array_free(table_offsets, true);
 }

[1] https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792

Igor Mammedov Feb. 27, 2019, 4:42 p.m. UTC | #6

On Wed, 27 Feb 2019 12:55:18 +0000
Shameerali Kolothum Thodi <[e-mail]> wrote:

> Hi Laszlo,
> 
> > -----Original Message-----
> > From: Shameerali Kolothum Thodi
> > Sent: 25 February 2019 09:54
> > To: 'Laszlo Ersek' <[e-mail]>; Auger Eric <[e-mail]>;
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]
> > Cc: xuwei (O) <[e-mail]>; Linuxarm <[e-mail]>; Ard
> > Biesheuvel <[e-mail]>; Leif Lindholm (Linaro address)
> > <[e-mail]>
> > Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support  
> 
> [...]
>  
> > > >> The root cause seems to be EDK2 ACPI table checksum failure
> > > >> as NFIT table is getting updated on hot-add. This needs further
> > > >> investigation.  
> > > > + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.  
> > >
> > > Huh, very interesting; I usually don't expect my sanity checks to fire
> > > in practice. :)
> > >
> > > The message
> > >
> > >   ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> > >
> > > is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
> > > finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
> > > linker/loader script.
> > >
> > > Please see the command definition in QEMU's
> > > "hw/acpi/bios-linker-loader.c". In particular, please refer to the
> > > function bios_linker_loader_add_checksum(), which builds the command
> > > structure, and documents the fields.
> > >
> > > (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
> > > "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
> > > same information.)
> > >
> > > The error message is logged if:
> > > - the offset at which the checksum should be stored falls outside of the
> > > size of the fw_cfg blob, or
> > > - the range over which the checksum should be calculated falls outside
> > > (at least in part) of the fw_cfg blob.
> > >
> > > To me this suggests that QEMU generates an invalid
> > > COMMAND_ADD_CHECKSUM
> > > command for the firmware.
> > >
> > > ... I've tried to skim the patches briefly. I think there must be an
> > > error in the DSDT building logic that is only active on reboot if an
> > > nvdimm module was hot-added before the reboot.  
> > 
> > Thanks for taking a look and the pointers. I will debug this further
> > and get back.  
> 
> The root cause of the issue seems to be UEFI not seeing the updated acpi
> table blob size on reboot once a new NFIT table is added(nvdimm hot added).
> 
> Please see the debug logs below,
> 
> Initial Guest boot
> ---------------------------
> 
> Debug logs from Qemu:
> 
> build_header: acpi sig DSDT len 0x5127
> build_header: acpi sig FACP len 0x10c
> build_header: acpi sig APIC len 0xa8
> build_header: acpi sig GTDT len 0x60
> build_header: acpi sig MCFG len 0x3c
> build_header: acpi sig SPCR len 0x50
> build_header: acpi sig SRAT len 0x92
> build_header: acpi sig SSDT len 0x38f
> build_header: acpi sig XSDT len 0x5c
> virt_acpi_build: acpi table_blob len 0x5844
> 
> Debug logs from UEFI:
> 
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0x5C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 Length=0x14 Blob->Size=0x24
> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20 Start=0x0 Length=0x24 Blob->Size=0x24
> InstallQemuFwCfgTables: installed 8 tables
> 
> Guest Reboot after ndimm hot added
> ------------------------------------
> 
> Debug logs from Qemu:
> 
> build_header: acpi sig DSDT len 0x5127
> build_header: acpi sig FACP len 0x10c
> build_header: acpi sig APIC len 0xa8
> build_header: acpi sig GTDT len 0x60
> build_header: acpi sig MCFG len 0x3c
> build_header: acpi sig SPCR len 0x50
> build_header: acpi sig SRAT len 0x92
> build_header: acpi sig SSDT len 0x38f
> build_header: acpi sig NFIT len 0xe0  -->New
> build_header: acpi sig XSDT len 0x64
> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
> 
> Debug logs from UEFI:
> 
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844  -->Wrong blob size.
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0xE0 Blob->Size=0x5844
> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
> 
> 
> To me it seems on ARM vit acpi path, the blob len is calculated based
> on actual tables and is updated only in virt_acpi_setup() --> acpi_add_rom_blob()
> path. I had a look at the x86 code and it looks like, there, the blob len gets updated
> with an additional buffer to take care of table resizing[1].
> 
> As a hack i added the same to ARM virt and it seems to resolve the issue.
> I am not sure this is the best approach to fix this though.
> 
> Please let me know your thoughts.
> 
> Thanks,
> Shameer
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 132414c..4291553 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -50,6 +50,8 @@
>  #define ARM_SPI_BASE 32
>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> 
> +#define ACPI_BUILD_TABLE_SIZE    0x20000
> +
>  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
>  {
>      uint16_t i;
> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>          build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
>      }
> 
> +    /* Make sure we have a buffer in case we need to resize the tables. */
> +    g_array_set_size(tables_blob, ROUND_UP(acpi_data_len(tables_blob),
> +                     ACPI_BUILD_TABLE_SIZE));
not sure fixup is correct approach.

On reset (on QEMU level), it's upto to QEMU to rebuild tables and it's
upto firmware to reread those.
Maybe issue existed before hotplug it's just that hotplug exposes it.
(something is missing compared to x86 or we have the same issue
there too just no one have triggered it yet).
I suggest to find root cause first before we start paper over it. 

> +
>      /* Cleanup memory that's no longer used. */
>      g_array_free(table_offsets, true);
>  }
> 
> [1] https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792
> 
> 
> 
> 
> 
>

Laszlo Ersek Feb. 27, 2019, 8:14 p.m. UTC | #7

On 02/27/19 13:55, Shameerali Kolothum Thodi wrote:
> Hi Laszlo,
> 
>> -----Original Message-----
>> From: Shameerali Kolothum Thodi
>> Sent: 25 February 2019 09:54
>> To: 'Laszlo Ersek' <[e-mail]>; Auger Eric <[e-mail]>;
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]
>> Cc: xuwei (O) <[e-mail]>; Linuxarm <[e-mail]>; Ard
>> Biesheuvel <[e-mail]>; Leif Lindholm (Linaro address)
>> <[e-mail]>
>> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
> 
> [...]
>  
>>>>> The root cause seems to be EDK2 ACPI table checksum failure
>>>>> as NFIT table is getting updated on hot-add. This needs further
>>>>> investigation.
>>>> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
>>>
>>> Huh, very interesting; I usually don't expect my sanity checks to fire
>>> in practice. :)
>>>
>>> The message
>>>
>>>   ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>>>
>>> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
>>> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
>>> linker/loader script.
>>>
>>> Please see the command definition in QEMU's
>>> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
>>> function bios_linker_loader_add_checksum(), which builds the command
>>> structure, and documents the fields.
>>>
>>> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
>>> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
>>> same information.)
>>>
>>> The error message is logged if:
>>> - the offset at which the checksum should be stored falls outside of the
>>> size of the fw_cfg blob, or
>>> - the range over which the checksum should be calculated falls outside
>>> (at least in part) of the fw_cfg blob.
>>>
>>> To me this suggests that QEMU generates an invalid
>>> COMMAND_ADD_CHECKSUM
>>> command for the firmware.
>>>
>>> ... I've tried to skim the patches briefly. I think there must be an
>>> error in the DSDT building logic that is only active on reboot if an
>>> nvdimm module was hot-added before the reboot.
>>
>> Thanks for taking a look and the pointers. I will debug this further
>> and get back.
> 
> The root cause of the issue seems to be UEFI not seeing the updated acpi
> table blob size on reboot once a new NFIT table is added(nvdimm hot added).
> 
> Please see the debug logs below,
> 
> Initial Guest boot
> ---------------------------
> 
> Debug logs from Qemu:
> 
> build_header: acpi sig DSDT len 0x5127
> build_header: acpi sig FACP len 0x10c
> build_header: acpi sig APIC len 0xa8
> build_header: acpi sig GTDT len 0x60
> build_header: acpi sig MCFG len 0x3c
> build_header: acpi sig SPCR len 0x50
> build_header: acpi sig SRAT len 0x92
> build_header: acpi sig SSDT len 0x38f
> build_header: acpi sig XSDT len 0x5c
> virt_acpi_build: acpi table_blob len 0x5844
> 
> Debug logs from UEFI:
> 
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0x5C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 Length=0x14 Blob->Size=0x24
> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20 Start=0x0 Length=0x24 Blob->Size=0x24
> InstallQemuFwCfgTables: installed 8 tables
> 
> Guest Reboot after ndimm hot added
> ------------------------------------
> 
> Debug logs from Qemu:
> 
> build_header: acpi sig DSDT len 0x5127
> build_header: acpi sig FACP len 0x10c
> build_header: acpi sig APIC len 0xa8
> build_header: acpi sig GTDT len 0x60
> build_header: acpi sig MCFG len 0x3c
> build_header: acpi sig SPCR len 0x50
> build_header: acpi sig SRAT len 0x92
> build_header: acpi sig SSDT len 0x38f
> build_header: acpi sig NFIT len 0xe0  -->New
> build_header: acpi sig XSDT len 0x64
> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
> 
> Debug logs from UEFI:
> 
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844  -->Wrong blob size.
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0xE0 Blob->Size=0x5844
> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
> 
> 
> To me it seems on ARM vit acpi path, the blob len is calculated based
> on actual tables and is updated only in virt_acpi_setup() --> acpi_add_rom_blob()
> path. I had a look at the x86 code and it looks like, there, the blob len gets updated
> with an additional buffer to take care of table resizing[1].
> 
> As a hack i added the same to ARM virt and it seems to resolve the issue.
> I am not sure this is the best approach to fix this though.
> 
> Please let me know your thoughts.
> 
> Thanks,
> Shameer
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 132414c..4291553 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -50,6 +50,8 @@
>  #define ARM_SPI_BASE 32
>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> 
> +#define ACPI_BUILD_TABLE_SIZE    0x20000
> +
>  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
>  {
>      uint16_t i;
> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>          build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
>      }
> 
> +    /* Make sure we have a buffer in case we need to resize the tables. */
> +    g_array_set_size(tables_blob, ROUND_UP(acpi_data_len(tables_blob),
> +                     ACPI_BUILD_TABLE_SIZE));
> +
>      /* Cleanup memory that's no longer used. */
>      g_array_free(table_offsets, true);
>  }
> 
> [1] https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792

Nice analysis, thanks.

I think the line that you reference, i.e.

  acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);

in acpi_build() [hw/i386/acpi-build.c] masks this issue for x86 only as
a side effect. To my understanding, the alignment / padding exists there
for migration compatibility. It doesn't exist for updating the size of
the ACPI blobs in fw_cfg across reboots. The issue is masked because the
alignment is large enough (un-changed) to contain the regenerated blobs
as well.

Given that the "virt" machine type is versioned, I think migration
compat is a valid concern there too. This in itself would justify a
similar padding.

I don't know if we want to specifically care about size-changing
ACPI-regen across reboot. I believe measures for that specific use case
don't exist in x86 machine types either.

Another trick that is occasionally used (but might not apply here, I'm
uncertain) is to always generate the relevant ACPI objects, but, in case
they are not justified for the virtual hardware config, invalidate them
by overwriting particular parts of them (for example, one or two bytes
of their names). Hopefully this shouldn't introduce ACPI or AML errors,
just make the ACPI interpreter ignore the affected objects.

Thanks,
Laszlo

Eric Auger Feb. 28, 2019, 10:12 a.m. UTC | #8

Hi Laszlo,

On 2/27/19 9:14 PM, Laszlo Ersek wrote:
> On 02/27/19 13:55, Shameerali Kolothum Thodi wrote:
>> Hi Laszlo,
>>
>>> -----Original Message-----
>>> From: Shameerali Kolothum Thodi
>>> Sent: 25 February 2019 09:54
>>> To: 'Laszlo Ersek' <[e-mail]>; Auger Eric <[e-mail]>;
>>> [e-mail]; [e-mail];
>>> [e-mail]; [e-mail]; [e-mail]
>>> Cc: xuwei (O) <[e-mail]>; Linuxarm <[e-mail]>; Ard
>>> Biesheuvel <[e-mail]>; Leif Lindholm (Linaro address)
>>> <[e-mail]>
>>> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
>>
>> [...]
>>  
>>>>>> The root cause seems to be EDK2 ACPI table checksum failure
>>>>>> as NFIT table is getting updated on hot-add. This needs further
>>>>>> investigation.
>>>>> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
>>>>
>>>> Huh, very interesting; I usually don't expect my sanity checks to fire
>>>> in practice. :)
>>>>
>>>> The message
>>>>
>>>>   ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>>>>
>>>> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
>>>> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
>>>> linker/loader script.
>>>>
>>>> Please see the command definition in QEMU's
>>>> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
>>>> function bios_linker_loader_add_checksum(), which builds the command
>>>> structure, and documents the fields.
>>>>
>>>> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
>>>> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
>>>> same information.)
>>>>
>>>> The error message is logged if:
>>>> - the offset at which the checksum should be stored falls outside of the
>>>> size of the fw_cfg blob, or
>>>> - the range over which the checksum should be calculated falls outside
>>>> (at least in part) of the fw_cfg blob.
>>>>
>>>> To me this suggests that QEMU generates an invalid
>>>> COMMAND_ADD_CHECKSUM
>>>> command for the firmware.
>>>>
>>>> ... I've tried to skim the patches briefly. I think there must be an
>>>> error in the DSDT building logic that is only active on reboot if an
>>>> nvdimm module was hot-added before the reboot.
>>>
>>> Thanks for taking a look and the pointers. I will debug this further
>>> and get back.
>>
>> The root cause of the issue seems to be UEFI not seeing the updated acpi
>> table blob size on reboot once a new NFIT table is added(nvdimm hot added).
>>
>> Please see the debug logs below,
>>
>> Initial Guest boot
>> ---------------------------
>>
>> Debug logs from Qemu:
>>
>> build_header: acpi sig DSDT len 0x5127
>> build_header: acpi sig FACP len 0x10c
>> build_header: acpi sig APIC len 0xa8
>> build_header: acpi sig GTDT len 0x60
>> build_header: acpi sig MCFG len 0x3c
>> build_header: acpi sig SPCR len 0x50
>> build_header: acpi sig SRAT len 0x92
>> build_header: acpi sig SSDT len 0x38f
>> build_header: acpi sig XSDT len 0x5c
>> virt_acpi_build: acpi table_blob len 0x5844
>>
>> Debug logs from UEFI:
>>
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0x5C Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 Length=0x14 Blob->Size=0x24
>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20 Start=0x0 Length=0x24 Blob->Size=0x24
>> InstallQemuFwCfgTables: installed 8 tables
>>
>> Guest Reboot after ndimm hot added
>> ------------------------------------
>>
>> Debug logs from Qemu:
>>
>> build_header: acpi sig DSDT len 0x5127
>> build_header: acpi sig FACP len 0x10c
>> build_header: acpi sig APIC len 0xa8
>> build_header: acpi sig GTDT len 0x60
>> build_header: acpi sig MCFG len 0x3c
>> build_header: acpi sig SPCR len 0x50
>> build_header: acpi sig SRAT len 0x92
>> build_header: acpi sig SSDT len 0x38f
>> build_header: acpi sig NFIT len 0xe0  -->New
>> build_header: acpi sig XSDT len 0x64
>> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
>>
>> Debug logs from UEFI:
>>
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844  -->Wrong blob size.
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0xE0 Blob->Size=0x5844
>> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
>>
>>
>> To me it seems on ARM vit acpi path, the blob len is calculated based
>> on actual tables and is updated only in virt_acpi_setup() --> acpi_add_rom_blob()
>> path. I had a look at the x86 code and it looks like, there, the blob len gets updated
>> with an additional buffer to take care of table resizing[1].
>>
>> As a hack i added the same to ARM virt and it seems to resolve the issue.
>> I am not sure this is the best approach to fix this though.
>>
>> Please let me know your thoughts.
>>
>> Thanks,
>> Shameer
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 132414c..4291553 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -50,6 +50,8 @@
>>  #define ARM_SPI_BASE 32
>>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>>
>> +#define ACPI_BUILD_TABLE_SIZE    0x20000
>> +
>>  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
>>  {
>>      uint16_t i;
>> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>          build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
>>      }
>>
>> +    /* Make sure we have a buffer in case we need to resize the tables. */
>> +    g_array_set_size(tables_blob, ROUND_UP(acpi_data_len(tables_blob),
>> +                     ACPI_BUILD_TABLE_SIZE));
>> +
>>      /* Cleanup memory that's no longer used. */
>>      g_array_free(table_offsets, true);
>>  }
>>
>> [1] https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792
> 
> Nice analysis, thanks.
> 
> I think the line that you reference, i.e.
> 
>   acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
> 
> in acpi_build() [hw/i386/acpi-build.c] masks this issue for x86 only as
> a side effect. To my understanding, the alignment / padding exists there
> for migration compatibility. It doesn't exist for updating the size of
> the ACPI blobs in fw_cfg across reboots. The issue is masked because the
> alignment is large enough (un-changed) to contain the regenerated blobs
> as well.>
> Given that the "virt" machine type is versioned, I think migration
> compat is a valid concern there too. This in itself would justify a
> similar padding.
I don't understand the migration compat issue. Please could you elaborate?
> 
> I don't know if we want to specifically care about size-changing
> ACPI-regen across reboot. I believe measures for that specific use case
> don't exist in x86 machine types either.
The NFIT redimensioning should exit on x86 too?
> 
> Another trick that is occasionally used (but might not apply here, I'm
> uncertain) is to always generate the relevant ACPI objects, but, in case
> they are not justified for the virtual hardware config, invalidate them
> by overwriting particular parts of them (for example, one or two bytes
> of their names). Hopefully this shouldn't introduce ACPI or AML errors,
> just make the ACPI interpreter ignore the affected objects.

Thanks!

Eric
> 
> Thanks,
> Laszlo
>

Shameerali Kolothum Thodi Feb. 28, 2019, 12:04 p.m. UTC | #9

> -----Original Message-----
> From: Auger Eric [mailto:[e-mail]]
> Sent: 28 February 2019 10:12
> To: Laszlo Ersek <[e-mail]>; Shameerali Kolothum Thodi
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]
> Cc: xuwei (O) <[e-mail]>; Linuxarm <[e-mail]>; Ard
> Biesheuvel <[e-mail]>; Leif Lindholm (Linaro address)
> <[e-mail]>
> Subject: Re: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
> 
> Hi Laszlo,
> 
> On 2/27/19 9:14 PM, Laszlo Ersek wrote:
> > On 02/27/19 13:55, Shameerali Kolothum Thodi wrote:
> >> Hi Laszlo,
> >>
> >>> -----Original Message-----
> >>> From: Shameerali Kolothum Thodi
> >>> Sent: 25 February 2019 09:54
> >>> To: 'Laszlo Ersek' <[e-mail]>; Auger Eric
> <[e-mail]>;
> >>> [e-mail]; [e-mail];
> >>> [e-mail]; [e-mail];
> [e-mail]
> >>> Cc: xuwei (O) <[e-mail]>; Linuxarm <[e-mail]>;
> Ard
> >>> Biesheuvel <[e-mail]>; Leif Lindholm (Linaro address)
> >>> <[e-mail]>
> >>> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
> >>
> >> [...]
> >>
> >>>>>> The root cause seems to be EDK2 ACPI table checksum failure
> >>>>>> as NFIT table is getting updated on hot-add. This needs further
> >>>>>> investigation.
> >>>>> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
> >>>>
> >>>> Huh, very interesting; I usually don't expect my sanity checks to fire
> >>>> in practice. :)
> >>>>
> >>>> The message
> >>>>
> >>>>   ProcessCmdAddChecksum: invalid checksum range in
> "etc/acpi/tables"
> >>>>
> >>>> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when
> it
> >>>> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
> >>>> linker/loader script.
> >>>>
> >>>> Please see the command definition in QEMU's
> >>>> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
> >>>> function bios_linker_loader_add_checksum(), which builds the command
> >>>> structure, and documents the fields.
> >>>>
> >>>> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
> >>>> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for
> the
> >>>> same information.)
> >>>>
> >>>> The error message is logged if:
> >>>> - the offset at which the checksum should be stored falls outside of the
> >>>> size of the fw_cfg blob, or
> >>>> - the range over which the checksum should be calculated falls outside
> >>>> (at least in part) of the fw_cfg blob.
> >>>>
> >>>> To me this suggests that QEMU generates an invalid
> >>>> COMMAND_ADD_CHECKSUM
> >>>> command for the firmware.
> >>>>
> >>>> ... I've tried to skim the patches briefly. I think there must be an
> >>>> error in the DSDT building logic that is only active on reboot if an
> >>>> nvdimm module was hot-added before the reboot.
> >>>
> >>> Thanks for taking a look and the pointers. I will debug this further
> >>> and get back.
> >>
> >> The root cause of the issue seems to be UEFI not seeing the updated acpi
> >> table blob size on reboot once a new NFIT table is added(nvdimm hot
> added).
> >>
> >> Please see the debug logs below,
> >>
> >> Initial Guest boot
> >> ---------------------------
> >>
> >> Debug logs from Qemu:
> >>
> >> build_header: acpi sig DSDT len 0x5127
> >> build_header: acpi sig FACP len 0x10c
> >> build_header: acpi sig APIC len 0xa8
> >> build_header: acpi sig GTDT len 0x60
> >> build_header: acpi sig MCFG len 0x3c
> >> build_header: acpi sig SPCR len 0x50
> >> build_header: acpi sig SRAT len 0x92
> >> build_header: acpi sig SSDT len 0x38f
> >> build_header: acpi sig XSDT len 0x5c
> >> virt_acpi_build: acpi table_blob len 0x5844
> >>
> >> Debug logs from UEFI:
> >>
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9
> Start=0x0 Length=0x5127 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130
> Start=0x5127 Length=0x10C Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C
> Start=0x5233 Length=0xA8 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4
> Start=0x52DB Length=0x60 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344
> Start=0x533B Length=0x3C Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380
> Start=0x5377 Length=0x50 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0
> Start=0x53C7 Length=0x92 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462
> Start=0x5459 Length=0x38F Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1
> Start=0x57E8 Length=0x5C Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0
> Length=0x14 Blob->Size=0x24
> >> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20
> Start=0x0 Length=0x24 Blob->Size=0x24
> >> InstallQemuFwCfgTables: installed 8 tables
> >>
> >> Guest Reboot after ndimm hot added
> >> ------------------------------------
> >>
> >> Debug logs from Qemu:
> >>
> >> build_header: acpi sig DSDT len 0x5127
> >> build_header: acpi sig FACP len 0x10c
> >> build_header: acpi sig APIC len 0xa8
> >> build_header: acpi sig GTDT len 0x60
> >> build_header: acpi sig MCFG len 0x3c
> >> build_header: acpi sig SPCR len 0x50
> >> build_header: acpi sig SRAT len 0x92
> >> build_header: acpi sig SSDT len 0x38f
> >> build_header: acpi sig NFIT len 0xe0  -->New
> >> build_header: acpi sig XSDT len 0x64
> >> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
> >>
> >> Debug logs from UEFI:
> >>
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9
> Start=0x0 Length=0x5127 Blob->Size=0x5844  -->Wrong blob size.
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130
> Start=0x5127 Length=0x10C Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C
> Start=0x5233 Length=0xA8 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4
> Start=0x52DB Length=0x60 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344
> Start=0x533B Length=0x3C Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380
> Start=0x5377 Length=0x50 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0
> Start=0x53C7 Length=0x92 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462
> Start=0x5459 Length=0x38F Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1
> Start=0x57E8 Length=0xE0 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> >> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
> >>
> >>
> >> To me it seems on ARM vit acpi path, the blob len is calculated based
> >> on actual tables and is updated only in virt_acpi_setup() -->
> acpi_add_rom_blob()
> >> path. I had a look at the x86 code and it looks like, there, the blob len gets
> updated
> >> with an additional buffer to take care of table resizing[1].
> >>
> >> As a hack i added the same to ARM virt and it seems to resolve the issue.
> >> I am not sure this is the best approach to fix this though.
> >>
> >> Please let me know your thoughts.
> >>
> >> Thanks,
> >> Shameer
> >>
> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >> index 132414c..4291553 100644
> >> --- a/hw/arm/virt-acpi-build.c
> >> +++ b/hw/arm/virt-acpi-build.c
> >> @@ -50,6 +50,8 @@
> >>  #define ARM_SPI_BASE 32
> >>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> >>
> >> +#define ACPI_BUILD_TABLE_SIZE    0x20000
> >> +
> >>  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> >>  {
> >>      uint16_t i;
> >> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms,
> AcpiBuildTables *tables)
> >>          build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> >>      }
> >>
> >> +    /* Make sure we have a buffer in case we need to resize the tables. */
> >> +    g_array_set_size(tables_blob,
> ROUND_UP(acpi_data_len(tables_blob),
> >> +                     ACPI_BUILD_TABLE_SIZE));
> >> +
> >>      /* Cleanup memory that's no longer used. */
> >>      g_array_free(table_offsets, true);
> >>  }
> >>
> >> [1]
> https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792
> >
> > Nice analysis, thanks.
> >
> > I think the line that you reference, i.e.
> >
> >   acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);

Yes.

> > in acpi_build() [hw/i386/acpi-build.c] masks this issue for x86 only as
> > a side effect. To my understanding, the alignment / padding exists there
> > for migration compatibility. It doesn't exist for updating the size of
> > the ACPI blobs in fw_cfg across reboots. The issue is masked because the
> > alignment is large enough (un-changed) to contain the regenerated blobs
> > as well.>
> > Given that the "virt" machine type is versioned, I think migration
> > compat is a valid concern there too. This in itself would justify a
> > similar padding.
> I don't understand the migration compat issue. Please could you elaborate?

Yes. Please elaborate.

> > I don't know if we want to specifically care about size-changing
> > ACPI-regen across reboot. I believe measures for that specific use case
> > don't exist in x86 machine types either.
> The NFIT redimensioning should exit on x86 too?

I had a go with x86 commenting out the padding completely. But interestingly
somewhere in x86 code path the blob length gets aligned to 0x1000 boundary
and it escapes the sanity check.

With padding, UEFI always sees, ACPI_BUILD_TABLE_SIZE (0x20000)

ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49 Start=0x40 Length=0x19E9 Blob->Size=0x20000

With padding removed,

From Qemu,
acpi_build: tables_blob len 0x2c62

UEFI:
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49 Start=0x40 Length=0x1F33 Blob->Size=0x3000

Guest Reboot after adding a number of nvdimms,

Qemu:
acpi_build:  tables_blob len 0x380e

UEFI:
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49 Start=0x40 Length=0x2401 Blob->Size=0x4000

I couldn’t figure out from the code where that round up is happening. But
x86 also has same issue if the padding is removed, I guess.

Thanks,
Shameer

> > Another trick that is occasionally used (but might not apply here, I'm
> > uncertain) is to always generate the relevant ACPI objects, but, in case
> > they are not justified for the virtual hardware config, invalidate them
> > by overwriting particular parts of them (for example, one or two bytes
> > of their names). Hopefully this shouldn't introduce ACPI or AML errors,
> > just make the ACPI interpreter ignore the affected objects.
> 
> Thanks!
> 
> Eric
> >
> > Thanks,
> > Laszlo
> >

Laszlo Ersek Feb. 28, 2019, 12:27 p.m. UTC | #10

On 02/28/19 11:12, Auger Eric wrote:
> Hi Laszlo,
> 
> On 2/27/19 9:14 PM, Laszlo Ersek wrote:
>> On 02/27/19 13:55, Shameerali Kolothum Thodi wrote:
>>> Hi Laszlo,
>>>
>>>> -----Original Message-----
>>>> From: Shameerali Kolothum Thodi
>>>> Sent: 25 February 2019 09:54
>>>> To: 'Laszlo Ersek' <[e-mail]>; Auger Eric <[e-mail]>;
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail]
>>>> Cc: xuwei (O) <[e-mail]>; Linuxarm <[e-mail]>; Ard
>>>> Biesheuvel <[e-mail]>; Leif Lindholm (Linaro address)
>>>> <[e-mail]>
>>>> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
>>>
>>> [...]
>>>  
>>>>>>> The root cause seems to be EDK2 ACPI table checksum failure
>>>>>>> as NFIT table is getting updated on hot-add. This needs further
>>>>>>> investigation.
>>>>>> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
>>>>>
>>>>> Huh, very interesting; I usually don't expect my sanity checks to fire
>>>>> in practice. :)
>>>>>
>>>>> The message
>>>>>
>>>>>   ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>>>>>
>>>>> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
>>>>> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
>>>>> linker/loader script.
>>>>>
>>>>> Please see the command definition in QEMU's
>>>>> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
>>>>> function bios_linker_loader_add_checksum(), which builds the command
>>>>> structure, and documents the fields.
>>>>>
>>>>> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
>>>>> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
>>>>> same information.)
>>>>>
>>>>> The error message is logged if:
>>>>> - the offset at which the checksum should be stored falls outside of the
>>>>> size of the fw_cfg blob, or
>>>>> - the range over which the checksum should be calculated falls outside
>>>>> (at least in part) of the fw_cfg blob.
>>>>>
>>>>> To me this suggests that QEMU generates an invalid
>>>>> COMMAND_ADD_CHECKSUM
>>>>> command for the firmware.
>>>>>
>>>>> ... I've tried to skim the patches briefly. I think there must be an
>>>>> error in the DSDT building logic that is only active on reboot if an
>>>>> nvdimm module was hot-added before the reboot.
>>>>
>>>> Thanks for taking a look and the pointers. I will debug this further
>>>> and get back.
>>>
>>> The root cause of the issue seems to be UEFI not seeing the updated acpi
>>> table blob size on reboot once a new NFIT table is added(nvdimm hot added).
>>>
>>> Please see the debug logs below,
>>>
>>> Initial Guest boot
>>> ---------------------------
>>>
>>> Debug logs from Qemu:
>>>
>>> build_header: acpi sig DSDT len 0x5127
>>> build_header: acpi sig FACP len 0x10c
>>> build_header: acpi sig APIC len 0xa8
>>> build_header: acpi sig GTDT len 0x60
>>> build_header: acpi sig MCFG len 0x3c
>>> build_header: acpi sig SPCR len 0x50
>>> build_header: acpi sig SRAT len 0x92
>>> build_header: acpi sig SSDT len 0x38f
>>> build_header: acpi sig XSDT len 0x5c
>>> virt_acpi_build: acpi table_blob len 0x5844
>>>
>>> Debug logs from UEFI:
>>>
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0x5C Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 Length=0x14 Blob->Size=0x24
>>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20 Start=0x0 Length=0x24 Blob->Size=0x24
>>> InstallQemuFwCfgTables: installed 8 tables
>>>
>>> Guest Reboot after ndimm hot added
>>> ------------------------------------
>>>
>>> Debug logs from Qemu:
>>>
>>> build_header: acpi sig DSDT len 0x5127
>>> build_header: acpi sig FACP len 0x10c
>>> build_header: acpi sig APIC len 0xa8
>>> build_header: acpi sig GTDT len 0x60
>>> build_header: acpi sig MCFG len 0x3c
>>> build_header: acpi sig SPCR len 0x50
>>> build_header: acpi sig SRAT len 0x92
>>> build_header: acpi sig SSDT len 0x38f
>>> build_header: acpi sig NFIT len 0xe0  -->New
>>> build_header: acpi sig XSDT len 0x64
>>> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
>>>
>>> Debug logs from UEFI:
>>>
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844  -->Wrong blob size.
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0xE0 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>>> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
>>>
>>>
>>> To me it seems on ARM vit acpi path, the blob len is calculated based
>>> on actual tables and is updated only in virt_acpi_setup() --> acpi_add_rom_blob()
>>> path. I had a look at the x86 code and it looks like, there, the blob len gets updated
>>> with an additional buffer to take care of table resizing[1].
>>>
>>> As a hack i added the same to ARM virt and it seems to resolve the issue.
>>> I am not sure this is the best approach to fix this though.
>>>
>>> Please let me know your thoughts.
>>>
>>> Thanks,
>>> Shameer
>>>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index 132414c..4291553 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -50,6 +50,8 @@
>>>  #define ARM_SPI_BASE 32
>>>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>>>
>>> +#define ACPI_BUILD_TABLE_SIZE    0x20000
>>> +
>>>  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
>>>  {
>>>      uint16_t i;
>>> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>>          build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
>>>      }
>>>
>>> +    /* Make sure we have a buffer in case we need to resize the tables. */
>>> +    g_array_set_size(tables_blob, ROUND_UP(acpi_data_len(tables_blob),
>>> +                     ACPI_BUILD_TABLE_SIZE));
>>> +
>>>      /* Cleanup memory that's no longer used. */
>>>      g_array_free(table_offsets, true);
>>>  }
>>>
>>> [1] https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792
>>
>> Nice analysis, thanks.
>>
>> I think the line that you reference, i.e.
>>
>>   acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
>>
>> in acpi_build() [hw/i386/acpi-build.c] masks this issue for x86 only as
>> a side effect. To my understanding, the alignment / padding exists there
>> for migration compatibility. It doesn't exist for updating the size of
>> the ACPI blobs in fw_cfg across reboots. The issue is masked because the
>> alignment is large enough (un-changed) to contain the regenerated blobs
>> as well.>
>> Given that the "virt" machine type is versioned, I think migration
>> compat is a valid concern there too. This in itself would justify a
>> similar padding.
> I don't understand the migration compat issue. Please could you elaborate?

git-blame explains it to some extent -- please see commit 07fb61760cde
("pc: hack for migration compatibility from QEMU 2.0", 2014-07-28).

I don't remember any details at this point that the commit does not
state. (I see that I reviewed the patch back then, so perhaps the
mailing list archive has some discussion.)

Interestingly, the commit message refers to "memory hotplug work" too.

... Ahh, wait, I do remember the main issue now. Here's the thing. The
ACPI payload that QEMU generates for the firmware is considered a part
of the firmware itself. Therefore, it is not versioned -- because the
firmware itself is not versioned. (In other words, if you migrate a VM
from one host to another host, and that other host has different
firmware that the VM will pick up after re-launch (from cold boot), then
the firmware will change in the VM.)

By considering ACPI a part of the firmware, QEMU never versioned the
ACPI payload, just like the actual firmware was never versioned. In
other words, if you have machine type Foo on qemu release Bar, and
machine type Foo on qemu release Baz, compat properties and such will
ensure that the virtual hardware looks the same to the guest, but QEMU
will *not* ensure that the ACPI payload generated at QEMU startup (more
precisely, at "machine done") will be identical. Despite the fact that
both QEMU instances use machine type Foo.

Now, combine this with the feature that fw_cfg has been backed by RAM
Blocks, for a quite long time now (this wasn't always the case, but it
has been for multiple years now). The end result is that the RAM
block(s) holding the initial ACPI payload may differ between releases
Bar and Baz, within the same machine type Foo. This means that migration
between them will fail, due to RAMBlock size difference.

Hence the padding -- it tries to cancel out small variances in ACPI
payload size.

>>
>> I don't know if we want to specifically care about size-changing
>> ACPI-regen across reboot. I believe measures for that specific use case
>> don't exist in x86 machine types either.
> The NFIT redimensioning should exit on x86 too?

That's not my point. My point was that the padding, which was originally
supposed to mask variances in ACPI payload size across *QEMU releases*,
for migration compat, ended up masking a variance of different origin:
namely ACPI regeneration at reboot (with different contents). In other
words, we never implemented any specific measures for this
resize-on-reboot issue, instead we allowed the migration compat code
(the padding) to take care of it as well.

In virt, there is no such ACPI padding code (for migration compat) --
for whatever reason --, and so it *also* cannot take care of the
resize-on-reboot problem.

[...]

Thanks
Laszlo

Eric Auger Feb. 28, 2019, 1:32 p.m. UTC | #11

Hi Laszlo,
On 2/28/19 1:27 PM, Laszlo Ersek wrote:
> On 02/28/19 11:12, Auger Eric wrote:
>> Hi Laszlo,
>>
>> On 2/27/19 9:14 PM, Laszlo Ersek wrote:
>>> On 02/27/19 13:55, Shameerali Kolothum Thodi wrote:
>>>> Hi Laszlo,
>>>>
>>>>> -----Original Message-----
>>>>> From: Shameerali Kolothum Thodi
>>>>> Sent: 25 February 2019 09:54
>>>>> To: 'Laszlo Ersek' <[e-mail]>; Auger Eric <[e-mail]>;
>>>>> [e-mail]; [e-mail];
>>>>> [e-mail]; [e-mail]; [e-mail]
>>>>> Cc: xuwei (O) <[e-mail]>; Linuxarm <[e-mail]>; Ard
>>>>> Biesheuvel <[e-mail]>; Leif Lindholm (Linaro address)
>>>>> <[e-mail]>
>>>>> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
>>>>
>>>> [...]
>>>>  
>>>>>>>> The root cause seems to be EDK2 ACPI table checksum failure
>>>>>>>> as NFIT table is getting updated on hot-add. This needs further
>>>>>>>> investigation.
>>>>>>> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
>>>>>>
>>>>>> Huh, very interesting; I usually don't expect my sanity checks to fire
>>>>>> in practice. :)
>>>>>>
>>>>>> The message
>>>>>>
>>>>>>   ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>>>>>>
>>>>>> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
>>>>>> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
>>>>>> linker/loader script.
>>>>>>
>>>>>> Please see the command definition in QEMU's
>>>>>> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
>>>>>> function bios_linker_loader_add_checksum(), which builds the command
>>>>>> structure, and documents the fields.
>>>>>>
>>>>>> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
>>>>>> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
>>>>>> same information.)
>>>>>>
>>>>>> The error message is logged if:
>>>>>> - the offset at which the checksum should be stored falls outside of the
>>>>>> size of the fw_cfg blob, or
>>>>>> - the range over which the checksum should be calculated falls outside
>>>>>> (at least in part) of the fw_cfg blob.
>>>>>>
>>>>>> To me this suggests that QEMU generates an invalid
>>>>>> COMMAND_ADD_CHECKSUM
>>>>>> command for the firmware.
>>>>>>
>>>>>> ... I've tried to skim the patches briefly. I think there must be an
>>>>>> error in the DSDT building logic that is only active on reboot if an
>>>>>> nvdimm module was hot-added before the reboot.
>>>>>
>>>>> Thanks for taking a look and the pointers. I will debug this further
>>>>> and get back.
>>>>
>>>> The root cause of the issue seems to be UEFI not seeing the updated acpi
>>>> table blob size on reboot once a new NFIT table is added(nvdimm hot added).
>>>>
>>>> Please see the debug logs below,
>>>>
>>>> Initial Guest boot
>>>> ---------------------------
>>>>
>>>> Debug logs from Qemu:
>>>>
>>>> build_header: acpi sig DSDT len 0x5127
>>>> build_header: acpi sig FACP len 0x10c
>>>> build_header: acpi sig APIC len 0xa8
>>>> build_header: acpi sig GTDT len 0x60
>>>> build_header: acpi sig MCFG len 0x3c
>>>> build_header: acpi sig SPCR len 0x50
>>>> build_header: acpi sig SRAT len 0x92
>>>> build_header: acpi sig SSDT len 0x38f
>>>> build_header: acpi sig XSDT len 0x5c
>>>> virt_acpi_build: acpi table_blob len 0x5844
>>>>
>>>> Debug logs from UEFI:
>>>>
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0x5C Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 Length=0x14 Blob->Size=0x24
>>>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20 Start=0x0 Length=0x24 Blob->Size=0x24
>>>> InstallQemuFwCfgTables: installed 8 tables
>>>>
>>>> Guest Reboot after ndimm hot added
>>>> ------------------------------------
>>>>
>>>> Debug logs from Qemu:
>>>>
>>>> build_header: acpi sig DSDT len 0x5127
>>>> build_header: acpi sig FACP len 0x10c
>>>> build_header: acpi sig APIC len 0xa8
>>>> build_header: acpi sig GTDT len 0x60
>>>> build_header: acpi sig MCFG len 0x3c
>>>> build_header: acpi sig SPCR len 0x50
>>>> build_header: acpi sig SRAT len 0x92
>>>> build_header: acpi sig SSDT len 0x38f
>>>> build_header: acpi sig NFIT len 0xe0  -->New
>>>> build_header: acpi sig XSDT len 0x64
>>>> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
>>>>
>>>> Debug logs from UEFI:
>>>>
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844  -->Wrong blob size.
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0xE0 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>>>> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
>>>>
>>>>
>>>> To me it seems on ARM vit acpi path, the blob len is calculated based
>>>> on actual tables and is updated only in virt_acpi_setup() --> acpi_add_rom_blob()
>>>> path. I had a look at the x86 code and it looks like, there, the blob len gets updated
>>>> with an additional buffer to take care of table resizing[1].
>>>>
>>>> As a hack i added the same to ARM virt and it seems to resolve the issue.
>>>> I am not sure this is the best approach to fix this though.
>>>>
>>>> Please let me know your thoughts.
>>>>
>>>> Thanks,
>>>> Shameer
>>>>
>>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>>> index 132414c..4291553 100644
>>>> --- a/hw/arm/virt-acpi-build.c
>>>> +++ b/hw/arm/virt-acpi-build.c
>>>> @@ -50,6 +50,8 @@
>>>>  #define ARM_SPI_BASE 32
>>>>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>>>>
>>>> +#define ACPI_BUILD_TABLE_SIZE    0x20000
>>>> +
>>>>  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
>>>>  {
>>>>      uint16_t i;
>>>> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>>>          build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
>>>>      }
>>>>
>>>> +    /* Make sure we have a buffer in case we need to resize the tables. */
>>>> +    g_array_set_size(tables_blob, ROUND_UP(acpi_data_len(tables_blob),
>>>> +                     ACPI_BUILD_TABLE_SIZE));
>>>> +
>>>>      /* Cleanup memory that's no longer used. */
>>>>      g_array_free(table_offsets, true);
>>>>  }
>>>>
>>>> [1] https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792
>>>
>>> Nice analysis, thanks.
>>>
>>> I think the line that you reference, i.e.
>>>
>>>   acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
>>>
>>> in acpi_build() [hw/i386/acpi-build.c] masks this issue for x86 only as
>>> a side effect. To my understanding, the alignment / padding exists there
>>> for migration compatibility. It doesn't exist for updating the size of
>>> the ACPI blobs in fw_cfg across reboots. The issue is masked because the
>>> alignment is large enough (un-changed) to contain the regenerated blobs
>>> as well.>
>>> Given that the "virt" machine type is versioned, I think migration
>>> compat is a valid concern there too. This in itself would justify a
>>> similar padding.
>> I don't understand the migration compat issue. Please could you elaborate?
> 
> git-blame explains it to some extent -- please see commit 07fb61760cde
> ("pc: hack for migration compatibility from QEMU 2.0", 2014-07-28).
> 
> I don't remember any details at this point that the commit does not
> state. (I see that I reviewed the patch back then, so perhaps the
> mailing list archive has some discussion.)
> 
> Interestingly, the commit message refers to "memory hotplug work" too.
> 
> ... Ahh, wait, I do remember the main issue now. Here's the thing. The
> ACPI payload that QEMU generates for the firmware is considered a part
> of the firmware itself. Therefore, it is not versioned -- because the
> firmware itself is not versioned. (In other words, if you migrate a VM
> from one host to another host, and that other host has different
> firmware that the VM will pick up after re-launch (from cold boot), then
> the firmware will change in the VM.)
> 
> By considering ACPI a part of the firmware, QEMU never versioned the
> ACPI payload, just like the actual firmware was never versioned. In
> other words, if you have machine type Foo on qemu release Bar, and
> machine type Foo on qemu release Baz, compat properties and such will
> ensure that the virtual hardware looks the same to the guest, but QEMU
> will *not* ensure that the ACPI payload generated at QEMU startup (more
> precisely, at "machine done") will be identical. Despite the fact that
> both QEMU instances use machine type Foo.
> 
> Now, combine this with the feature that fw_cfg has been backed by RAM
> Blocks, for a quite long time now (this wasn't always the case, but it
> has been for multiple years now). The end result is that the RAM
> block(s) holding the initial ACPI payload may differ between releases
> Bar and Baz, within the same machine type Foo. This means that migration
> between them will fail, due to RAMBlock size difference.
> 
> Hence the padding -- it tries to cancel out small variances in ACPI
> payload size.
> 
>>>
>>> I don't know if we want to specifically care about size-changing
>>> ACPI-regen across reboot. I believe measures for that specific use case
>>> don't exist in x86 machine types either.
>> The NFIT redimensioning should exit on x86 too?
> 
> That's not my point. My point was that the padding, which was originally
> supposed to mask variances in ACPI payload size across *QEMU releases*,
> for migration compat, ended up masking a variance of different origin:
> namely ACPI regeneration at reboot (with different contents). In other
> words, we never implemented any specific measures for this
> resize-on-reboot issue, instead we allowed the migration compat code
> (the padding) to take care of it as well.
> 
> In virt, there is no such ACPI padding code (for migration compat) --
> for whatever reason --, and so it *also* cannot take care of the
> resize-on-reboot problem.

That's clearer now. Thank you for the explanations.

Thanks

Eric
> 
> [...]
> 
> Thanks
> Laszlo
>

Igor Mammedov Feb. 28, 2019, 1:43 p.m. UTC | #12

On Thu, 28 Feb 2019 13:27:54 +0100
Laszlo Ersek <[e-mail]> wrote:

> On 02/28/19 11:12, Auger Eric wrote:
> > Hi Laszlo,
> > 
> > On 2/27/19 9:14 PM, Laszlo Ersek wrote:  
> >> On 02/27/19 13:55, Shameerali Kolothum Thodi wrote:  
> >>> Hi Laszlo,
> >>>  
> >>>> -----Original Message-----
> >>>> From: Shameerali Kolothum Thodi
> >>>> Sent: 25 February 2019 09:54
> >>>> To: 'Laszlo Ersek' <[e-mail]>; Auger Eric <[e-mail]>;
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail]; [e-mail]
> >>>> Cc: xuwei (O) <[e-mail]>; Linuxarm <[e-mail]>; Ard
> >>>> Biesheuvel <[e-mail]>; Leif Lindholm (Linaro address)
> >>>> <[e-mail]>
> >>>> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support  
> >>>
> >>> [...]
> >>>    
> >>>>>>> The root cause seems to be EDK2 ACPI table checksum failure
> >>>>>>> as NFIT table is getting updated on hot-add. This needs further
> >>>>>>> investigation.  
> >>>>>> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.  
> >>>>>
> >>>>> Huh, very interesting; I usually don't expect my sanity checks to fire
> >>>>> in practice. :)
> >>>>>
> >>>>> The message
> >>>>>
> >>>>>   ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> >>>>>
> >>>>> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
> >>>>> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
> >>>>> linker/loader script.
> >>>>>
> >>>>> Please see the command definition in QEMU's
> >>>>> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
> >>>>> function bios_linker_loader_add_checksum(), which builds the command
> >>>>> structure, and documents the fields.
> >>>>>
> >>>>> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
> >>>>> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
> >>>>> same information.)
> >>>>>
> >>>>> The error message is logged if:
> >>>>> - the offset at which the checksum should be stored falls outside of the
> >>>>> size of the fw_cfg blob, or
> >>>>> - the range over which the checksum should be calculated falls outside
> >>>>> (at least in part) of the fw_cfg blob.
> >>>>>
> >>>>> To me this suggests that QEMU generates an invalid
> >>>>> COMMAND_ADD_CHECKSUM
> >>>>> command for the firmware.
> >>>>>
> >>>>> ... I've tried to skim the patches briefly. I think there must be an
> >>>>> error in the DSDT building logic that is only active on reboot if an
> >>>>> nvdimm module was hot-added before the reboot.  
> >>>>
> >>>> Thanks for taking a look and the pointers. I will debug this further
> >>>> and get back.  
> >>>
> >>> The root cause of the issue seems to be UEFI not seeing the updated acpi
> >>> table blob size on reboot once a new NFIT table is added(nvdimm hot added).
> >>>
> >>> Please see the debug logs below,
> >>>
> >>> Initial Guest boot
> >>> ---------------------------
> >>>
> >>> Debug logs from Qemu:
> >>>
> >>> build_header: acpi sig DSDT len 0x5127
> >>> build_header: acpi sig FACP len 0x10c
> >>> build_header: acpi sig APIC len 0xa8
> >>> build_header: acpi sig GTDT len 0x60
> >>> build_header: acpi sig MCFG len 0x3c
> >>> build_header: acpi sig SPCR len 0x50
> >>> build_header: acpi sig SRAT len 0x92
> >>> build_header: acpi sig SSDT len 0x38f
> >>> build_header: acpi sig XSDT len 0x5c
> >>> virt_acpi_build: acpi table_blob len 0x5844
> >>>
> >>> Debug logs from UEFI:
> >>>
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0x5C Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 Length=0x14 Blob->Size=0x24
> >>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20 Start=0x0 Length=0x24 Blob->Size=0x24
> >>> InstallQemuFwCfgTables: installed 8 tables
> >>>
> >>> Guest Reboot after ndimm hot added
> >>> ------------------------------------
> >>>
> >>> Debug logs from Qemu:
> >>>
> >>> build_header: acpi sig DSDT len 0x5127
> >>> build_header: acpi sig FACP len 0x10c
> >>> build_header: acpi sig APIC len 0xa8
> >>> build_header: acpi sig GTDT len 0x60
> >>> build_header: acpi sig MCFG len 0x3c
> >>> build_header: acpi sig SPCR len 0x50
> >>> build_header: acpi sig SRAT len 0x92
> >>> build_header: acpi sig SSDT len 0x38f
> >>> build_header: acpi sig NFIT len 0xe0  -->New
> >>> build_header: acpi sig XSDT len 0x64
> >>> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
> >>>
> >>> Debug logs from UEFI:
> >>>
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844  -->Wrong blob size.
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0xE0 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> >>> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
> >>>
> >>>
> >>> To me it seems on ARM vit acpi path, the blob len is calculated based
> >>> on actual tables and is updated only in virt_acpi_setup() --> acpi_add_rom_blob()
> >>> path. I had a look at the x86 code and it looks like, there, the blob len gets updated
> >>> with an additional buffer to take care of table resizing[1].
> >>>
> >>> As a hack i added the same to ARM virt and it seems to resolve the issue.
> >>> I am not sure this is the best approach to fix this though.
> >>>
> >>> Please let me know your thoughts.
> >>>
> >>> Thanks,
> >>> Shameer
> >>>
> >>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >>> index 132414c..4291553 100644
> >>> --- a/hw/arm/virt-acpi-build.c
> >>> +++ b/hw/arm/virt-acpi-build.c
> >>> @@ -50,6 +50,8 @@
> >>>  #define ARM_SPI_BASE 32
> >>>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> >>>
> >>> +#define ACPI_BUILD_TABLE_SIZE    0x20000
> >>> +
> >>>  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> >>>  {
> >>>      uint16_t i;
> >>> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> >>>          build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> >>>      }
> >>>
> >>> +    /* Make sure we have a buffer in case we need to resize the tables. */
> >>> +    g_array_set_size(tables_blob, ROUND_UP(acpi_data_len(tables_blob),
> >>> +                     ACPI_BUILD_TABLE_SIZE));
> >>> +
> >>>      /* Cleanup memory that's no longer used. */
> >>>      g_array_free(table_offsets, true);
> >>>  }
> >>>
> >>> [1] https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792  
> >>
> >> Nice analysis, thanks.
> >>
> >> I think the line that you reference, i.e.
> >>
> >>   acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
> >>
> >> in acpi_build() [hw/i386/acpi-build.c] masks this issue for x86 only as
> >> a side effect. To my understanding, the alignment / padding exists there
> >> for migration compatibility. It doesn't exist for updating the size of
> >> the ACPI blobs in fw_cfg across reboots. The issue is masked because the
> >> alignment is large enough (un-changed) to contain the regenerated blobs
> >> as well.>
> >> Given that the "virt" machine type is versioned, I think migration
> >> compat is a valid concern there too. This in itself would justify a
> >> similar padding.  
> > I don't understand the migration compat issue. Please could you elaborate?  
> 
> git-blame explains it to some extent -- please see commit 07fb61760cde
> ("pc: hack for migration compatibility from QEMU 2.0", 2014-07-28).
> 
> I don't remember any details at this point that the commit does not
> state. (I see that I reviewed the patch back then, so perhaps the
> mailing list archive has some discussion.)
> 
> Interestingly, the commit message refers to "memory hotplug work" too.
> 
> ... Ahh, wait, I do remember the main issue now. Here's the thing. The
> ACPI payload that QEMU generates for the firmware is considered a part
> of the firmware itself. Therefore, it is not versioned -- because the
> firmware itself is not versioned. (In other words, if you migrate a VM
> from one host to another host, and that other host has different
> firmware that the VM will pick up after re-launch (from cold boot), then
> the firmware will change in the VM.)
> 
> By considering ACPI a part of the firmware, QEMU never versioned the
> ACPI payload, just like the actual firmware was never versioned. In
> other words, if you have machine type Foo on qemu release Bar, and
> machine type Foo on qemu release Baz, compat properties and such will
> ensure that the virtual hardware looks the same to the guest, but QEMU
> will *not* ensure that the ACPI payload generated at QEMU startup (more
> precisely, at "machine done") will be identical. Despite the fact that
> both QEMU instances use machine type Foo.
> 
> Now, combine this with the feature that fw_cfg has been backed by RAM
> Blocks, for a quite long time now (this wasn't always the case, but it
> has been for multiple years now). The end result is that the RAM
> block(s) holding the initial ACPI payload may differ between releases
> Bar and Baz, within the same machine type Foo. This means that migration
> between them will fail, due to RAMBlock size difference.
> 
> Hence the padding -- it tries to cancel out small variances in ACPI
> payload size.
> 
> >>
> >> I don't know if we want to specifically care about size-changing
> >> ACPI-regen across reboot. I believe measures for that specific use case
> >> don't exist in x86 machine types either.  
> > The NFIT redimensioning should exit on x86 too?  
> 
> That's not my point. My point was that the padding, which was originally
> supposed to mask variances in ACPI payload size across *QEMU releases*,
> for migration compat, ended up masking a variance of different origin:
> namely ACPI regeneration at reboot (with different contents). In other
> words, we never implemented any specific measures for this
> resize-on-reboot issue, instead we allowed the migration compat code
> (the padding) to take care of it as well.
> 
> In virt, there is no such ACPI padding code (for migration compat) --
> for whatever reason --, and so it *also* cannot take care of the
> resize-on-reboot problem.
What's describe above is a bit outdate, let me paint a rough current state
in chronological order (I haven't checked how it actually works now).
--
1. how it work(s|ed):
     * tables are generated during machine_done and registered with fw_cfg
     * then oops: we have device_add that could be called later but before
       guest runs and we don't have complete picture wrt CRS resources for
       PCI as that is programmed by firmware, so here goes a trick to
       regenerate tables when firmware accesses its fwcfg entry
       (see how build_state->patched is used), byt that time FW has intialized
       PCI to the extent necessary to build valid tables.
     * then we do hot add ACPI table are not updated until reset
       (acpi_build_reset) and all dance with patching repeats when FW
       gets tables form QEMU.
     * at those versions of QEMU tables size was static
     
2. Later on hotplugging bridges (I think) caused issues on reboot/migration
as it caused DSDT size increase, that made as invent padding race train-wreak.
2.2 then there were cases where tables changed size depending where a device
was cold or hot plugged and may be some other issues I don't recall.

3. With padding race being a lost game (there always were border line configs
where it broke migration), Michael introduced re-sizable blobs 42d859001d for
acpi table blobs, which could be resized on target to accommodate larger
tables blob during migration.

4. Even with resizable memory regions for the sake of backward migration
compatibility we needed to keep padding we used with legacy machine. It's
still broken in some cases but mostly works (we declared padding approach
broken by design and left it as is)
--

Thankfully arm/virt ACPI support materialized after all of the above
so we don't have legacy padding obligations at all. Hence suggest to
find were the bug is instead of trying to start yet another padding race.
 
> [...]
> 
> Thanks
> Laszlo

Shameerali Kolothum Thodi Feb. 28, 2019, 2:02 p.m. UTC | #13

> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 28 February 2019 12:04
> To: 'Auger Eric' <[e-mail]>; Laszlo Ersek <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Cc: xuwei (O) <[e-mail]>; Linuxarm <[e-mail]>; Ard
> Biesheuvel <[e-mail]>; Leif Lindholm (Linaro address)
> <[e-mail]>
> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
> 
> 
> 
> > -----Original Message-----
> > From: Auger Eric [mailto:[e-mail]]
> > Sent: 28 February 2019 10:12
> > To: Laszlo Ersek <[e-mail]>; Shameerali Kolothum Thodi
> > <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail];
> [e-mail];
> > [e-mail]
> > Cc: xuwei (O) <[e-mail]>; Linuxarm <[e-mail]>;
> Ard
> > Biesheuvel <[e-mail]>; Leif Lindholm (Linaro address)
> > <[e-mail]>
> > Subject: Re: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
> >
> > Hi Laszlo,
> >
> > On 2/27/19 9:14 PM, Laszlo Ersek wrote:
> > > On 02/27/19 13:55, Shameerali Kolothum Thodi wrote:
> > >> Hi Laszlo,
> > >>
> > >>> -----Original Message-----
> > >>> From: Shameerali Kolothum Thodi
> > >>> Sent: 25 February 2019 09:54
> > >>> To: 'Laszlo Ersek' <[e-mail]>; Auger Eric
> > <[e-mail]>;
> > >>> [e-mail]; [e-mail];
> > >>> [e-mail]; [e-mail];
> > [e-mail]
> > >>> Cc: xuwei (O) <[e-mail]>; Linuxarm
> <[e-mail]>;
> > Ard
> > >>> Biesheuvel <[e-mail]>; Leif Lindholm (Linaro address)
> > >>> <[e-mail]>
> > >>> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
> > >>
> > >> [...]
> > >>
> > >>>>>> The root cause seems to be EDK2 ACPI table checksum failure
> > >>>>>> as NFIT table is getting updated on hot-add. This needs further
> > >>>>>> investigation.
> > >>>>> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
> > >>>>
> > >>>> Huh, very interesting; I usually don't expect my sanity checks to fire
> > >>>> in practice. :)
> > >>>>
> > >>>> The message
> > >>>>
> > >>>>   ProcessCmdAddChecksum: invalid checksum range in
> > "etc/acpi/tables"
> > >>>>
> > >>>> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when
> > it
> > >>>> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's
> ACPI
> > >>>> linker/loader script.
> > >>>>
> > >>>> Please see the command definition in QEMU's
> > >>>> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
> > >>>> function bios_linker_loader_add_checksum(), which builds the
> command
> > >>>> structure, and documents the fields.
> > >>>>
> > >>>> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
> > >>>> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for
> > the
> > >>>> same information.)
> > >>>>
> > >>>> The error message is logged if:
> > >>>> - the offset at which the checksum should be stored falls outside of the
> > >>>> size of the fw_cfg blob, or
> > >>>> - the range over which the checksum should be calculated falls outside
> > >>>> (at least in part) of the fw_cfg blob.
> > >>>>
> > >>>> To me this suggests that QEMU generates an invalid
> > >>>> COMMAND_ADD_CHECKSUM
> > >>>> command for the firmware.
> > >>>>
> > >>>> ... I've tried to skim the patches briefly. I think there must be an
> > >>>> error in the DSDT building logic that is only active on reboot if an
> > >>>> nvdimm module was hot-added before the reboot.
> > >>>
> > >>> Thanks for taking a look and the pointers. I will debug this further
> > >>> and get back.
> > >>
> > >> The root cause of the issue seems to be UEFI not seeing the updated acpi
> > >> table blob size on reboot once a new NFIT table is added(nvdimm hot
> > added).
> > >>
> > >> Please see the debug logs below,
> > >>
> > >> Initial Guest boot
> > >> ---------------------------
> > >>
> > >> Debug logs from Qemu:
> > >>
> > >> build_header: acpi sig DSDT len 0x5127
> > >> build_header: acpi sig FACP len 0x10c
> > >> build_header: acpi sig APIC len 0xa8
> > >> build_header: acpi sig GTDT len 0x60
> > >> build_header: acpi sig MCFG len 0x3c
> > >> build_header: acpi sig SPCR len 0x50
> > >> build_header: acpi sig SRAT len 0x92
> > >> build_header: acpi sig SSDT len 0x38f
> > >> build_header: acpi sig XSDT len 0x5c
> > >> virt_acpi_build: acpi table_blob len 0x5844
> > >>
> > >> Debug logs from UEFI:
> > >>
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9
> > Start=0x0 Length=0x5127 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130
> > Start=0x5127 Length=0x10C Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C
> > Start=0x5233 Length=0xA8 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4
> > Start=0x52DB Length=0x60 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344
> > Start=0x533B Length=0x3C Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380
> > Start=0x5377 Length=0x50 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0
> > Start=0x53C7 Length=0x92 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462
> > Start=0x5459 Length=0x38F Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1
> > Start=0x57E8 Length=0x5C Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8
> Start=0x0
> > Length=0x14 Blob->Size=0x24
> > >> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20
> > Start=0x0 Length=0x24 Blob->Size=0x24
> > >> InstallQemuFwCfgTables: installed 8 tables
> > >>
> > >> Guest Reboot after ndimm hot added
> > >> ------------------------------------
> > >>
> > >> Debug logs from Qemu:
> > >>
> > >> build_header: acpi sig DSDT len 0x5127
> > >> build_header: acpi sig FACP len 0x10c
> > >> build_header: acpi sig APIC len 0xa8
> > >> build_header: acpi sig GTDT len 0x60
> > >> build_header: acpi sig MCFG len 0x3c
> > >> build_header: acpi sig SPCR len 0x50
> > >> build_header: acpi sig SRAT len 0x92
> > >> build_header: acpi sig SSDT len 0x38f
> > >> build_header: acpi sig NFIT len 0xe0  -->New
> > >> build_header: acpi sig XSDT len 0x64
> > >> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
> > >>
> > >> Debug logs from UEFI:
> > >>
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9
> > Start=0x0 Length=0x5127 Blob->Size=0x5844  -->Wrong blob size.
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130
> > Start=0x5127 Length=0x10C Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C
> > Start=0x5233 Length=0xA8 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4
> > Start=0x52DB Length=0x60 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344
> > Start=0x533B Length=0x3C Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380
> > Start=0x5377 Length=0x50 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0
> > Start=0x53C7 Length=0x92 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462
> > Start=0x5459 Length=0x38F Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1
> > Start=0x57E8 Length=0xE0 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> > >> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
> > >>
> > >>
> > >> To me it seems on ARM vit acpi path, the blob len is calculated based
> > >> on actual tables and is updated only in virt_acpi_setup() -->
> > acpi_add_rom_blob()
> > >> path. I had a look at the x86 code and it looks like, there, the blob len gets
> > updated
> > >> with an additional buffer to take care of table resizing[1].
> > >>
> > >> As a hack i added the same to ARM virt and it seems to resolve the issue.
> > >> I am not sure this is the best approach to fix this though.
> > >>
> > >> Please let me know your thoughts.
> > >>
> > >> Thanks,
> > >> Shameer
> > >>
> > >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > >> index 132414c..4291553 100644
> > >> --- a/hw/arm/virt-acpi-build.c
> > >> +++ b/hw/arm/virt-acpi-build.c
> > >> @@ -50,6 +50,8 @@
> > >>  #define ARM_SPI_BASE 32
> > >>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> > >>
> > >> +#define ACPI_BUILD_TABLE_SIZE    0x20000
> > >> +
> > >>  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> > >>  {
> > >>      uint16_t i;
> > >> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms,
> > AcpiBuildTables *tables)
> > >>          build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> > >>      }
> > >>
> > >> +    /* Make sure we have a buffer in case we need to resize the tables.
> */
> > >> +    g_array_set_size(tables_blob,
> > ROUND_UP(acpi_data_len(tables_blob),
> > >> +                     ACPI_BUILD_TABLE_SIZE));
> > >> +
> > >>      /* Cleanup memory that's no longer used. */
> > >>      g_array_free(table_offsets, true);
> > >>  }
> > >>
> > >> [1]
> > https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792
> > >
> > > Nice analysis, thanks.
> > >
> > > I think the line that you reference, i.e.
> > >
> > >   acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
> 
> Yes.
> 
> > > in acpi_build() [hw/i386/acpi-build.c] masks this issue for x86 only as
> > > a side effect. To my understanding, the alignment / padding exists there
> > > for migration compatibility. It doesn't exist for updating the size of
> > > the ACPI blobs in fw_cfg across reboots. The issue is masked because the
> > > alignment is large enough (un-changed) to contain the regenerated blobs
> > > as well.>
> > > Given that the "virt" machine type is versioned, I think migration
> > > compat is a valid concern there too. This in itself would justify a
> > > similar padding.
> > I don't understand the migration compat issue. Please could you elaborate?
> 
> Yes. Please elaborate.
> 
> > > I don't know if we want to specifically care about size-changing
> > > ACPI-regen across reboot. I believe measures for that specific use case
> > > don't exist in x86 machine types either.
> > The NFIT redimensioning should exit on x86 too?
> 
> I had a go with x86 commenting out the padding completely. But interestingly
> somewhere in x86 code path the blob length gets aligned to 0x1000 boundary
> and it escapes the sanity check.
> 
> With padding, UEFI always sees, ACPI_BUILD_TABLE_SIZE (0x20000)
> 
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49
> Start=0x40 Length=0x19E9 Blob->Size=0x20000
> 
> With padding removed,
> 
> From Qemu,
> acpi_build: tables_blob len 0x2c62
> 
> UEFI:
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49
> Start=0x40 Length=0x1F33 Blob->Size=0x3000
> 
> Guest Reboot after adding a number of nvdimms,
> 
> Qemu:
> acpi_build:  tables_blob len 0x380e
> 
> UEFI:
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49
> Start=0x40 Length=0x2401 Blob->Size=0x4000
> 
> I couldn’t figure out from the code where that round up is happening. But
> x86 also has same issue if the padding is removed, I guess.

Ah..I missed the fact that, firmware indeed sees an update in the blob len here
(rounded or not) after reboot. So don’t think x86 has the same issue and padding
is not the right solution as Igor explained in his reply.

I will try to debug this further. Any pointers welcome.

Cheers,
Shameer

> Thanks,
> Shameer
> 
> > > Another trick that is occasionally used (but might not apply here, I'm
> > > uncertain) is to always generate the relevant ACPI objects, but, in case
> > > they are not justified for the virtual hardware config, invalidate them
> > > by overwriting particular parts of them (for example, one or two bytes
> > > of their names). Hopefully this shouldn't introduce ACPI or AML errors,
> > > just make the ACPI interpreter ignore the affected objects.
> >
> > Thanks!
> >
> > Eric
> > >
> > > Thanks,
> > > Laszlo
> > >

Laszlo Ersek March 1, 2019, 1:49 p.m. UTC | #14

On 02/28/19 15:02, Shameerali Kolothum Thodi wrote:

> Ah..I missed the fact that, firmware indeed sees an update in the blob len here
> (rounded or not) after reboot. So don’t think x86 has the same issue and padding
> is not the right solution as Igor explained in his reply.
> 
> I will try to debug this further. Any pointers welcome.

How about this.

(1) The firmware looks up the fw_cfg file called "etc/table-loader" in
the fw_cfg file directory (identified by constant selector key 0x0019,
FW_CFG_FILE_DIR).

(2) The directory entry, once found, tells the firmware two things
simultaneously. The selector key, and the size of the blob.

(3) The firmware selects the selector key from step (2).

(4) QEMU regenerates the ACPI payload (as a select callback).

(5) The firmware reads the number of bytes from the fw_cfg blob that it
learned in step (2).

Here's the problem. As long as QEMU used to perform step (4) only for
the purpose of refreshing PCI resources in the ACPI payload, step (4)
wouldn't *resize* the blob.

However, if step (4) enlarges the blob, then the byte count that step
(5) uses -- from step (2) -- for reading, is obsolete.

Thanks
Laszlo

Igor Mammedov March 1, 2019, 5:39 p.m. UTC | #15

On Fri, 1 Mar 2019 14:49:45 +0100
Laszlo Ersek <[e-mail]> wrote:

> On 02/28/19 15:02, Shameerali Kolothum Thodi wrote:
> 
> > Ah..I missed the fact that, firmware indeed sees an update in the blob len here
> > (rounded or not) after reboot. So don’t think x86 has the same issue and padding
> > is not the right solution as Igor explained in his reply.
> > 
> > I will try to debug this further. Any pointers welcome.  
> 
> How about this.
> 
> (1) The firmware looks up the fw_cfg file called "etc/table-loader" in
> the fw_cfg file directory (identified by constant selector key 0x0019,
> FW_CFG_FILE_DIR).
> 
> (2) The directory entry, once found, tells the firmware two things
> simultaneously. The selector key, and the size of the blob.
> 
> (3) The firmware selects the selector key from step (2).
> 
> (4) QEMU regenerates the ACPI payload (as a select callback).
> 
> (5) The firmware reads the number of bytes from the fw_cfg blob that it
> learned in step (2).
> 
> Here's the problem. As long as QEMU used to perform step (4) only for
> the purpose of refreshing PCI resources in the ACPI payload, step (4)
> wouldn't *resize* the blob.
> 
> However, if step (4) enlarges the blob, then the byte count that step
> (5) uses -- from step (2) -- for reading, is obsolete.
I've thought that was a problem with IO based fw_cfg, as reading size/content
were separates steps and that it was solved by DMA based fw_cfg file read.


> 
> Thanks
> Laszlo

Laszlo Ersek March 5, 2019, 12:14 p.m. UTC | #16

On 03/01/19 18:39, Igor Mammedov wrote:
> On Fri, 1 Mar 2019 14:49:45 +0100
> Laszlo Ersek <[e-mail]> wrote:
> 
>> On 02/28/19 15:02, Shameerali Kolothum Thodi wrote:
>>
>>> Ah..I missed the fact that, firmware indeed sees an update in the blob len here
>>> (rounded or not) after reboot. So don’t think x86 has the same issue and padding
>>> is not the right solution as Igor explained in his reply.
>>>
>>> I will try to debug this further. Any pointers welcome.  
>>
>> How about this.
>>
>> (1) The firmware looks up the fw_cfg file called "etc/table-loader" in
>> the fw_cfg file directory (identified by constant selector key 0x0019,
>> FW_CFG_FILE_DIR).
>>
>> (2) The directory entry, once found, tells the firmware two things
>> simultaneously. The selector key, and the size of the blob.
>>
>> (3) The firmware selects the selector key from step (2).
>>
>> (4) QEMU regenerates the ACPI payload (as a select callback).
>>
>> (5) The firmware reads the number of bytes from the fw_cfg blob that it
>> learned in step (2).
>>
>> Here's the problem. As long as QEMU used to perform step (4) only for
>> the purpose of refreshing PCI resources in the ACPI payload, step (4)
>> wouldn't *resize* the blob.
>>
>> However, if step (4) enlarges the blob, then the byte count that step
>> (5) uses -- from step (2) -- for reading, is obsolete.

> I've thought that was a problem with IO based fw_cfg, as reading size/content
> were separates steps and that it was solved by DMA based fw_cfg file read.

The DMA backend is not relevant for this question, for two reasons:

(a) The question whether the fw_cfg transfer takes places with port IO
vs. DMA is hidden from the fw_cfg client code; that code goes through an
abstract library API.

(b) While the DMA method indeed lets the firmware specify the details of
the transfer with one action, the issue is with the number of bytes that
the firmware requests (that is, not with *how* the firmware requests the
transfer). The firmware has to know the size of the transfer before it
can initiate the transfer (regardless of port IO vs. DMA).


My question is: assume the firmware item in question is selected, and
the QEMU-side select callback runs (regenerating the ACPI payload). Does
this action update the blob size in the fw_cfg file directory as well?

If it does, then I can work around the problem in the firmware. I can
add a re-lookup to the code after the item selection, in order to get
the fresh blob size from the fw_cfg file directory. Then we can use that
size for the actual transfer.

This won't help old firmware on new QEMU, but at least new firmware on
old QEMU will not be hurt (the re-fetching of the fw_cfg file directory
will come with a small performance penalty, but functionally it will be
a no-op).

Thanks
Laszlo