Re: [edk2-devel] [edk2][RFC] OvmfPkg/AcpiPlatformDxe: patch FADT PSCI bits if FDT advertises it

["Ard Biesheuvel" <ardb@kernel.org>]

On Thu, 7 Sept 2023 at 16:51, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 9/7/23 16:27, Ard Biesheuvel wrote:
> > On Mon, 16 Jan 2023 at 12:39, Evgeny Iakovlev
> > <eiakovlev@linux.microsoft.com> wrote:
> >>
> >> EL3 firmware may implement PSCI interface on aarch64 platforms,
> >> including qemu-tcg-aarch64. However, EL3 firmware does not usually own
> >> pulling and deploying ACPI tables from qemu fw_cfg. Thus the only way
> >> EL3 can advertise PSCI on qemu is in FDT. One such EL3 fw is ARM trusted
> >> firmware. Qemu itself also won't advertise PSCI in either FDT or ACPI if
> >> EL3 firmware is present.
> >>
> >> PSCI can be advertised in both FDT and ACPI, and Hyper-V/NT kernel
> >> expect to see all information published in ACPI. To better support
> >> running Hyper-V/NT on qemu-tcg-aarch64 with EDK2 as UEFI implementation
> >> and ARM trusted firmware as EL3 PSCI implementation we can patch in PSCI
> >> bits in ACPI FADT when pulling tables from fw_cfg if PSCI node is
> >> advertised in FDT. EDK2 owns ACPI table publishing and is also aware of
> >> FDT on arm, so it is ideally poised to handle this.
> >>
> >> This change illustrates how it could potentially be done. I am looking
> >> for comments on overall validity of the idea to patch FADT and whether
> >> or not this particular approach of handling it in AcpiPlatformDxe is the
> >> way to do it or maybe it is better to handle it via
> >> gQemuAcpiTableNotifyProtocolGuid somehow.
> >>
> >> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
> >
> > Thanks for the patch, and apologies for the lack of response.
> >
> > First of all, I suspect this patch breaks non-ARM users of this
> > driver, so the patch is problematic as is. (It makes
> > gFdtClientProtocolGuid mandatory, right?)
> >
> > Then, I'd like to hear from other folks on cc what they think about
> > this. Perhaps it is simply a matter of tweaking QEMU so it exposes the
> > correct PSCI setting in the FADT when it emulates secure world.
> > Patching it like this feels like a last resort to me, rather than a
> > well designed interface.
>
> Thanks for the CC; both of your concerns are valid.
>
> The FDT client proto GUID has no reason to exist in (e.g.) an X64 OVMF
> build.
>
> Second, and more importantly, this is a total layering violation for
> AcpiPlatformDxe. QEMU is the single source of truth for AcpiPlatformDxe,
> and AcpiPlatformDxe must remain as blind as possible to the actual ACPI
> content.
>
> In the situation described by the commit message, the ACPI content
> exposed by QEMU is simply invalid. That's what should be fixed in QEMU
> (and not papered over in edk2). Something somewhere is responsible for
> setting the property value in question to "hvc"; that something
> precisely is responsible (directly or indirectly) for making QEMU expose
> the proper FADT.
>
> I've now grepped the QEMU source tree for '"hvc"'; the relevant hit
> seems to be in "hw/arm/boot.c", function fdt_add_psci_node(), under case
> label QEMU_PSCI_CONDUIT_HVC. So, whatever sets psci-conduit to
> QEMU_PSCI_CONDUIT_HVC should also make sure the FADT matches it.
>
> Taking one step back, in "hw/arm/virt.c" we have:
>
>     if (vms->secure && firmware_loaded) {
>         vms->psci_conduit = QEMU_PSCI_CONDUIT_DISABLED;
>     } else if (vms->virt) {
>         vms->psci_conduit = QEMU_PSCI_CONDUIT_SMC;
>     } else {
>         vms->psci_conduit = QEMU_PSCI_CONDUIT_HVC;
>     }
>

The problem here is that QEMU does not know whether the EL3 firmware
running in the guest implements PSCI or not.

> So I figure the ACPI generator should be steered off the same information.
>
> BTW... I see the following in "hw/arm/virt-acpi-build.c", function
> build_fadt_rev6():
>
>     case QEMU_PSCI_CONDUIT_HVC:
>         fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT |
>                              ACPI_FADT_ARM_PSCI_USE_HVC;
>         break;
>
> That dates back minimally as far as commit 79e993a0a804
> ("hw/arm/virt-acpi-build: use SMC if booting in EL2", 2017-01-20).
>
> So why is it not taking effect? Patching edk2 should not be necessary at
> all, QEMU should already be doing the right thing.
>
> The commit message states, "Qemu itself also won't advertise PSCI in
> [...] ACPI if EL3 firmware is present"; if that's correct (I can't
> tell), then it may be the problem.
>

Exactly.

When not emulating EL2 or EL3 (which is equivalent to the KVM case),
PSCI calls are made using HVC instructions, which are handled by QEMU
directly.

When EL2 emulation is enabled, PSCI calls are made using SMC
instructions but using the same handling in QEMU.

When EL3 emulation is enabled, QEMU can no longer 'overrule' the side
effects of SMC instructions but has to deliver them to the firmware
that occupies EL3. Whether or not that firmware implements PSCI is not
known to QEMU, and so it assumes it is not, and populates the FADT
fields accordingly.

IIRC QEMU has some patching logic for ACPI tables. Could we make use
of that here?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108401): https://edk2.groups.io/g/devel/message/108401
Mute This Topic: https://groups.io/mt/101215483/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-