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

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Laszlo Ersek [mailto:address@hidden
> Sent: 22 February 2019 19:11
> To: Auger Eric <address@hidden>; Shameerali Kolothum Thodi
> <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
> 
> 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(-)
> >>