Re: [Qemu-arm] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support

[Shameerali Kolothum Thodi]

Hi Laszlo,

> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 25 February 2019 09:54
> To: 'Laszlo Ersek' <address@hidden>; Auger Eric <address@hidden>;
> address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden
> Cc: xuwei (O) <address@hidden>; Linuxarm <address@hidden>; Ard
> Biesheuvel <address@hidden>; Leif Lindholm (Linaro address)
> <address@hidden>
> 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