Message ID | 20230919101240.2569334-1-peter.maydell@linaro.org |
---|---|
Headers | show |
Series | virt: wire up NS EL2 virtual timer IRQ | expand |
Hi Peter, On Tue, 19 Sept 2023 at 12:12, Peter Maydell <peter.maydell@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.juszkiewicz@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. >
On 2023-09-19 11:12, Peter Maydell 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.juszkiewicz@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. Ard already provided the answer for this. > (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. And kind of this one too. Although I might expand somewhat: As I understand it, this only breaks an existing setup where someone is - Running a DEBUG build of edk2 - With virtualization=on I don't see that (currently) happening outside development/ci, and the RELEASE builds should save anyone not actually working on edk2 but only running it to test other things. Ard has given me an r-b on the fix, so that can be merged today. It will then land in edk2-stable202311 around the end of November, but will be on the main branch as soon as it passes ci. I have one comment on my own, which is this clearly has a merge conflict with my PPI rework patches. Regards, Leif > I'm putting this patchset out on the list to get opinions and > review on those two points. > > thanks > -- PMM > > Peter Maydell (3): > tests/qtest/bios-tables-test: Allow changes to virt GTDT > hw/arm/virt: Wire up non-secure EL2 virtual timer IRQ > tests/qtest/bios-tables-test: Update virt/GTDT golden reference > > include/hw/arm/virt.h | 2 ++ > hw/arm/virt-acpi-build.c | 16 ++++++++++++---- > hw/arm/virt.c | 29 ++++++++++++++++++++++++++++- > tests/data/acpi/virt/GTDT | Bin 96 -> 104 bytes > 4 files changed, 42 insertions(+), 5 deletions(-) >