Hi Peter, On Tue, 19 Sept 2023 at 12:12, Peter Maydell <peter.mayd...@linaro.org> wrote: > > This patchset is an RFC that wires up the NS EL2 virtual timer IRQ on > the virt board, similarly to what > https://patchew.org/QEMU/20230913140610.214893-1-marcin.juszkiew...@linaro.org/ > does for the sbsa-ref board. > > Patches 1 and 3 are the usual dance to keep the ACPI unit tests happy > with the change to the ACPI table contents; patch 2 is the meat. > > This is an RFC for two reasons: > > (1) I'm not very familiar with ACPI, and patch 2 needs to update the > ACPI GTDT table to report the interrupt number. In particular, this > means we need the rev 3 of this table (present in ACPI spec 6.3 and > later), not the rev 2 we currently report. I'm not sure if it's > permitted to rev just this table, or if we would need to upgrade all > our ACPI tables to the newer spec first. >
Using a newer revision of a table is fine as long as the FADT major/minor fields match the spec version that introduced it. No need to update all the other tables. > (2) The change causes EDK2 (UEFI) to assert on startup: > ASSERT [ArmTimerDxe] > /home/kraxel/projects/qemu/roms/edk2/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c(72): > PropSize == 36 || PropSize == 48 > This is because the EDK2 code that consumes the QEMU generated device > tree blob is incorrectly insisting that the architectural-timer > interrupts property has only 3 or 4 entries, so it falls over if > given a dtb with the 5th entry for the EL2 virtual timer irq. In > particular this breaks the avocado test: > machine_aarch64_virt.py:Aarch64VirtMachine.test_alpine_virt_tcg_gic_max > I'm not entirely sure what to do about this -- we can get EDK2 fixed > and update our own test case, but there's a lot of binaries out there > in the wild that won't run if we just update the virt board the way > this patchset does. We could perhaps make the virt board change be > dependent on machine type version, as a way to let users fall back to > the old behaviour. > ASSERT()s only fire in DEBUG builds so the impact should be limited. > I'm putting this patchset out on the list to get opinions and > review on those two points. >
