Re: [RFC 0/3] virt: wire up NS EL2 virtual timer IRQ

[Leif Lindholm]

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
> 20230913140610.214893-1-marcin.juszkiewicz@linaro.org/">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(-)