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(-)
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. (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. 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(-) -- 2.34.1
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(-) >
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. >
© 2016 - 2024 Red Hat, Inc.