* Re: [edk2-devel] [edk2][RFC] OvmfPkg/AcpiPlatformDxe: patch FADT PSCI bits if FDT advertises it [not found] <20230116113931.1221-1-eiakovlev@linux.microsoft.com> @ 2023-09-07 14:27 ` Ard Biesheuvel 2023-09-07 14:50 ` Laszlo Ersek 0 siblings, 1 reply; 4+ messages in thread From: Ard Biesheuvel @ 2023-09-07 14:27 UTC (permalink / raw) To: Evgeny Iakovlev, kraxel, Laszlo Ersek, edk2-devel-groups-io Cc: rfc, jiewen.yao 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. > --- > Resending the message because i wasn't subscribed to the rfc group the > last time, sorry. > --- > OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 4 +- > OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 108 ++++++++++++++++++++ > 2 files changed, 111 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf > index 8939dde425..a011a43743 100644 > --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf > +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf > @@ -30,6 +30,7 @@ > QemuFwCfgAcpi.c > > [Packages] > + EmbeddedPkg/EmbeddedPkg.dec > MdeModulePkg/MdeModulePkg.dec > MdePkg/MdePkg.dec > OvmfPkg/OvmfPkg.dec > @@ -50,6 +51,7 @@ > [Protocols] > gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED > gEfiPciIoProtocolGuid # PROTOCOL SOMETIMES_CONSUMED > + gFdtClientProtocolGuid # PROTOCOL SOMETIMES_CONSUMED > gQemuAcpiTableNotifyProtocolGuid # PROTOCOL PRODUCES > > [Guids] > @@ -64,4 +66,4 @@ > gEfiMdePkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr > > [Depex] > - gEfiAcpiTableProtocolGuid > + gEfiAcpiTableProtocolGuid AND gFdtClientProtocolGuid > diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c > index c8dee17c13..31d8665d2d 100644 > --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c > +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c > @@ -9,6 +9,7 @@ > **/ > > #include <IndustryStandard/Acpi.h> // EFI_ACPI_DESCRIPTION_HEADER > +#include <IndustryStandard/Acpi51.h> // EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE > #include <IndustryStandard/QemuLoader.h> // QEMU_LOADER_FNAME_SIZE > #include <Library/BaseLib.h> // AsciiStrCmp() > #include <Library/BaseMemoryLib.h> // CopyMem() > @@ -20,6 +21,7 @@ > #include <Library/UefiBootServicesTableLib.h> // gBS > > #include <Protocol/QemuAcpiTableNotify.h> > +#include <Protocol/FdtClient.h> > #include "AcpiPlatform.h" > EFI_HANDLE mQemuAcpiHandle = NULL; > QEMU_ACPI_TABLE_NOTIFY_PROTOCOL mAcpiNotifyProtocol; > @@ -815,6 +817,92 @@ UndoCmdWritePointer ( > )); > } > > +/** > + Locate a PSCI node in DTB published by EL3 firmware that implemented it > + and use information in it to patch ACPI FADT PSCI bits. > + > + The reason for it is that EL3 PSCI implementation, which is not EDK2, > + doesn't own ACPI tables and cannot advertise it there itself, it is using DTB instead. > + > + Qemu also won't advertise PSCI if EL3 firmware is present. > + A real example of that is running ARM trusted firmware in EL3 with PSCI enabled. > + > + @param[in] PonterValue FADT base address > + > + @param[in] TableSize Size of table in bytes > + > + @return EFI_SUCCESS if FAST was patched or if patching was not needed, > + error status returned by FDT client protocol otherwise. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +PatchFadtPsciBits ( > + IN UINT64 PointerValue, > + IN UINTN TableSize > + ) > +{ > + EFI_STATUS Status; > + FDT_CLIENT_PROTOCOL *FdtClient; > + CONST VOID *Prop; > + EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE *Fadt; > + UINT8 MajorMinorRev; > + > + Status = gBS->LocateProtocol ( > + &gFdtClientProtocolGuid, > + NULL, > + (VOID **)&FdtClient > + ); > + if (Status == EFI_NOT_FOUND) { > + goto NoPatchingNeeded; > + } else if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status = FdtClient->FindCompatibleNodeProperty ( > + FdtClient, > + "arm,psci-0.2", > + "method", > + &Prop, > + NULL > + ); > + if (Status == EFI_NOT_FOUND) { > + goto NoPatchingNeeded; > + } else if (EFI_ERROR (Status)) { > + return Status; > + } > + > + // > + // Make sure FADT is large enough for 5.1 > + // > + if (TableSize < sizeof (*Fadt)) { > + goto NoPatchingNeeded; > + } > + > + Fadt = (EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE *)(UINTN)PointerValue; > + > + // > + // Check that FADT revision is at least 5.1 now that it's safe to read fields > + // > + MajorMinorRev = (Fadt->Header.Revision << 4) | (Fadt->MinorVersion & 0x0F); > + if (MajorMinorRev < 0x51) { > + goto NoPatchingNeeded; > + } > + > + // > + // Patch FADT PSCI bits > + // > + Fadt->ArmBootArch = EFI_ACPI_5_1_ARM_PSCI_COMPLIANT; > + if (AsciiStrnCmp (Prop, "hvc", 3) == 0) { > + Fadt->ArmBootArch |= EFI_ACPI_5_1_ARM_PSCI_USE_HVC; > + } > + > + return EFI_SUCCESS; > + > +NoPatchingNeeded: > + return EFI_SUCCESS; > +} > + > // > // We'll be saving the keys of installed tables so that we can roll them back > // in case of failure. 128 tables should be enough for anyone (TM). > @@ -901,6 +989,7 @@ Process2ndPassCmdAddPointer ( > UINT64 PointerValue; > UINTN Blob2Remaining; > UINTN TableSize; > + UINT32 Signature; > CONST EFI_ACPI_1_0_FIRMWARE_ACPI_CONTROL_STRUCTURE *Facs; > CONST EFI_ACPI_DESCRIPTION_HEADER *Header; > EFI_STATUS Status; > @@ -960,6 +1049,7 @@ Process2ndPassCmdAddPointer ( > )); > > TableSize = 0; > + Signature = 0; > > // > // To make our job simple, the FACS has a custom header. Sigh. > @@ -979,6 +1069,7 @@ Process2ndPassCmdAddPointer ( > Facs->Length > )); > TableSize = Facs->Length; > + Signature = Facs->Signature; > } > } > > @@ -1004,6 +1095,7 @@ Process2ndPassCmdAddPointer ( > Header->Length > )); > TableSize = Header->Length; > + Signature = Header->Signature; > > // > // Skip RSDT and XSDT because those are handled by > @@ -1035,6 +1127,22 @@ Process2ndPassCmdAddPointer ( > goto RollbackSeenPointer; > } > > + // > + // For FADT also patch PSCI flag if DTB exposes it. > + // > + if (Signature == EFI_ACPI_1_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) { > + Status = PatchFadtPsciBits(PointerValue, TableSize); > + if (EFI_ERROR (Status)) { > + DEBUG (( > + DEBUG_ERROR, > + "%a: PatchFadtPsciBits(): %r\n", > + __FUNCTION__, > + Status > + )); > + goto RollbackSeenPointer; > + } > + } > + > Status = AcpiProtocol->InstallAcpiTable ( > AcpiProtocol, > (VOID *)(UINTN)PointerValue, > -- > 2.38.1.vfs.0.0 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108395): https://edk2.groups.io/g/devel/message/108395 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [edk2][RFC] OvmfPkg/AcpiPlatformDxe: patch FADT PSCI bits if FDT advertises it 2023-09-07 14:27 ` [edk2-devel] [edk2][RFC] OvmfPkg/AcpiPlatformDxe: patch FADT PSCI bits if FDT advertises it Ard Biesheuvel @ 2023-09-07 14:50 ` Laszlo Ersek 2023-09-07 15:17 ` Ard Biesheuvel 0 siblings, 1 reply; 4+ messages in thread From: Laszlo Ersek @ 2023-09-07 14:50 UTC (permalink / raw) To: Ard Biesheuvel, Evgeny Iakovlev, kraxel, edk2-devel-groups-io Cc: rfc, jiewen.yao 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; } 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. Thanks Laszlo > > >> --- >> Resending the message because i wasn't subscribed to the rfc group the >> last time, sorry. >> --- >> OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 4 +- >> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 108 ++++++++++++++++++++ >> 2 files changed, 111 insertions(+), 1 deletion(-) >> >> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf >> index 8939dde425..a011a43743 100644 >> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf >> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf >> @@ -30,6 +30,7 @@ >> QemuFwCfgAcpi.c >> >> [Packages] >> + EmbeddedPkg/EmbeddedPkg.dec >> MdeModulePkg/MdeModulePkg.dec >> MdePkg/MdePkg.dec >> OvmfPkg/OvmfPkg.dec >> @@ -50,6 +51,7 @@ >> [Protocols] >> gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED >> gEfiPciIoProtocolGuid # PROTOCOL SOMETIMES_CONSUMED >> + gFdtClientProtocolGuid # PROTOCOL SOMETIMES_CONSUMED >> gQemuAcpiTableNotifyProtocolGuid # PROTOCOL PRODUCES >> >> [Guids] >> @@ -64,4 +66,4 @@ >> gEfiMdePkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr >> >> [Depex] >> - gEfiAcpiTableProtocolGuid >> + gEfiAcpiTableProtocolGuid AND gFdtClientProtocolGuid >> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c >> index c8dee17c13..31d8665d2d 100644 >> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c >> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c >> @@ -9,6 +9,7 @@ >> **/ >> >> #include <IndustryStandard/Acpi.h> // EFI_ACPI_DESCRIPTION_HEADER >> +#include <IndustryStandard/Acpi51.h> // EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE >> #include <IndustryStandard/QemuLoader.h> // QEMU_LOADER_FNAME_SIZE >> #include <Library/BaseLib.h> // AsciiStrCmp() >> #include <Library/BaseMemoryLib.h> // CopyMem() >> @@ -20,6 +21,7 @@ >> #include <Library/UefiBootServicesTableLib.h> // gBS >> >> #include <Protocol/QemuAcpiTableNotify.h> >> +#include <Protocol/FdtClient.h> >> #include "AcpiPlatform.h" >> EFI_HANDLE mQemuAcpiHandle = NULL; >> QEMU_ACPI_TABLE_NOTIFY_PROTOCOL mAcpiNotifyProtocol; >> @@ -815,6 +817,92 @@ UndoCmdWritePointer ( >> )); >> } >> >> +/** >> + Locate a PSCI node in DTB published by EL3 firmware that implemented it >> + and use information in it to patch ACPI FADT PSCI bits. >> + >> + The reason for it is that EL3 PSCI implementation, which is not EDK2, >> + doesn't own ACPI tables and cannot advertise it there itself, it is using DTB instead. >> + >> + Qemu also won't advertise PSCI if EL3 firmware is present. >> + A real example of that is running ARM trusted firmware in EL3 with PSCI enabled. >> + >> + @param[in] PonterValue FADT base address >> + >> + @param[in] TableSize Size of table in bytes >> + >> + @return EFI_SUCCESS if FAST was patched or if patching was not needed, >> + error status returned by FDT client protocol otherwise. >> +**/ >> +STATIC >> +EFI_STATUS >> +EFIAPI >> +PatchFadtPsciBits ( >> + IN UINT64 PointerValue, >> + IN UINTN TableSize >> + ) >> +{ >> + EFI_STATUS Status; >> + FDT_CLIENT_PROTOCOL *FdtClient; >> + CONST VOID *Prop; >> + EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE *Fadt; >> + UINT8 MajorMinorRev; >> + >> + Status = gBS->LocateProtocol ( >> + &gFdtClientProtocolGuid, >> + NULL, >> + (VOID **)&FdtClient >> + ); >> + if (Status == EFI_NOT_FOUND) { >> + goto NoPatchingNeeded; >> + } else if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + Status = FdtClient->FindCompatibleNodeProperty ( >> + FdtClient, >> + "arm,psci-0.2", >> + "method", >> + &Prop, >> + NULL >> + ); >> + if (Status == EFI_NOT_FOUND) { >> + goto NoPatchingNeeded; >> + } else if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + // >> + // Make sure FADT is large enough for 5.1 >> + // >> + if (TableSize < sizeof (*Fadt)) { >> + goto NoPatchingNeeded; >> + } >> + >> + Fadt = (EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE *)(UINTN)PointerValue; >> + >> + // >> + // Check that FADT revision is at least 5.1 now that it's safe to read fields >> + // >> + MajorMinorRev = (Fadt->Header.Revision << 4) | (Fadt->MinorVersion & 0x0F); >> + if (MajorMinorRev < 0x51) { >> + goto NoPatchingNeeded; >> + } >> + >> + // >> + // Patch FADT PSCI bits >> + // >> + Fadt->ArmBootArch = EFI_ACPI_5_1_ARM_PSCI_COMPLIANT; >> + if (AsciiStrnCmp (Prop, "hvc", 3) == 0) { >> + Fadt->ArmBootArch |= EFI_ACPI_5_1_ARM_PSCI_USE_HVC; >> + } >> + >> + return EFI_SUCCESS; >> + >> +NoPatchingNeeded: >> + return EFI_SUCCESS; >> +} >> + >> // >> // We'll be saving the keys of installed tables so that we can roll them back >> // in case of failure. 128 tables should be enough for anyone (TM). >> @@ -901,6 +989,7 @@ Process2ndPassCmdAddPointer ( >> UINT64 PointerValue; >> UINTN Blob2Remaining; >> UINTN TableSize; >> + UINT32 Signature; >> CONST EFI_ACPI_1_0_FIRMWARE_ACPI_CONTROL_STRUCTURE *Facs; >> CONST EFI_ACPI_DESCRIPTION_HEADER *Header; >> EFI_STATUS Status; >> @@ -960,6 +1049,7 @@ Process2ndPassCmdAddPointer ( >> )); >> >> TableSize = 0; >> + Signature = 0; >> >> // >> // To make our job simple, the FACS has a custom header. Sigh. >> @@ -979,6 +1069,7 @@ Process2ndPassCmdAddPointer ( >> Facs->Length >> )); >> TableSize = Facs->Length; >> + Signature = Facs->Signature; >> } >> } >> >> @@ -1004,6 +1095,7 @@ Process2ndPassCmdAddPointer ( >> Header->Length >> )); >> TableSize = Header->Length; >> + Signature = Header->Signature; >> >> // >> // Skip RSDT and XSDT because those are handled by >> @@ -1035,6 +1127,22 @@ Process2ndPassCmdAddPointer ( >> goto RollbackSeenPointer; >> } >> >> + // >> + // For FADT also patch PSCI flag if DTB exposes it. >> + // >> + if (Signature == EFI_ACPI_1_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) { >> + Status = PatchFadtPsciBits(PointerValue, TableSize); >> + if (EFI_ERROR (Status)) { >> + DEBUG (( >> + DEBUG_ERROR, >> + "%a: PatchFadtPsciBits(): %r\n", >> + __FUNCTION__, >> + Status >> + )); >> + goto RollbackSeenPointer; >> + } >> + } >> + >> Status = AcpiProtocol->InstallAcpiTable ( >> AcpiProtocol, >> (VOID *)(UINTN)PointerValue, >> -- >> 2.38.1.vfs.0.0 >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108396): https://edk2.groups.io/g/devel/message/108396 Mute This Topic: https://groups.io/mt/101215483/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [edk2][RFC] OvmfPkg/AcpiPlatformDxe: patch FADT PSCI bits if FDT advertises it 2023-09-07 14:50 ` Laszlo Ersek @ 2023-09-07 15:17 ` Ard Biesheuvel 2023-09-07 20:07 ` Laszlo Ersek 0 siblings, 1 reply; 4+ messages in thread From: Ard Biesheuvel @ 2023-09-07 15:17 UTC (permalink / raw) To: devel, lersek; +Cc: Evgeny Iakovlev, kraxel, rfc, jiewen.yao 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [edk2][RFC] OvmfPkg/AcpiPlatformDxe: patch FADT PSCI bits if FDT advertises it 2023-09-07 15:17 ` Ard Biesheuvel @ 2023-09-07 20:07 ` Laszlo Ersek 0 siblings, 0 replies; 4+ messages in thread From: Laszlo Ersek @ 2023-09-07 20:07 UTC (permalink / raw) To: Ard Biesheuvel, devel; +Cc: Evgeny Iakovlev, kraxel, rfc, jiewen.yao On 9/7/23 17:17, Ard Biesheuvel wrote: > 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. Whether the EL3 firmware implements PSCI or not is presumably known at QEMU launch time. Is that right? I mean, not inherently known to QEMU, but known to the user, or minimally to the *provider* of the EL3 firmware binary. That meta-datum should be exposed to QEMU via a dedicated command line switch. (It could be a device property, a machine type propery, a PCI host bridge vendor capability, a custom fw_cfg file in the edk2 or tianocore namespace, or some other means that's related to the EL3 firmware -- I reckon the EL3 firmware binary pathname is ultimately specified by the user!) Then QEMU can rely on that information to populate the FADT. This "need" is very-very similar to the necessity that had brought about the firmware descriptor JSON schema and files. When configuring a UEFI firmware binary for a domain, libvirt needs to know various pieces of metadata about the different firmware binaries installed on the host system. Because those properties are not "introspectable" / detectable from the firmware binaries themselves, we expect the providers / packagers of those fw binaries to ship additional firmware descriptor files alongside them. Then libvirt can build (and has built) elaborate selection / filtering logic on the metadata. > IIRC QEMU has some patching logic for ACPI tables. Could we make use > of that here? The ACPI linker/loader commands are in "OvmfPkg/Include/IndustryStandard/QemuLoader.h"; what you're likely referring to is QemuLoaderCmdWritePointer. But that command is for letting QEMU know a guest-side allocation address. No, I really believe that, if QEMU cannot detect something about a particular piece of guest payload, we need to tell it explicitly. And this is not something to be forced upon the end-user -- it should come as metadata together with the EL3 firmware. A similar example is libosinfo. It's different in three regards: - it concerns guest OS-es, not guest firmware - the kinds of information it carries are different (like what devices are supported etc) - its database is about two orders of magnitude larger But the usage pattern is exactly the same. Tell me what you want to run in the guest, I'll give you the optimal domain config in response. In fact recent virt-install refuses to install any new domain by default unless the user tells it the guest OS type / release (or unless virt-install can detect it somehow from the installer media). This patch would set us on a very slippery slope; very soon we'd have a whole lot of patching logic in AcpiPlatformDxe, which would defeat the purpose of the ACPI linker/loader. BTW, I don't understand the FDT references in the commit message. My understanding is that the FDT is placed at GPA 0 by QEMU. The commit message claims the FDT does reflect whether the EL3 firmware implements PSCI. So there seem to be only two possible explanations for that: #1 QEMU does know this property after all, because it places the information in the DTB at GPA 0 #2 the DTB at GPA 0 does not in fact come from QEMU, but from the EL3 firmware -- either whole-sale (i.e., QEMU doesn't expose anything, it all comes from the EL3 firmware), or the EL3 firmware *patches* QEMU's DTB (terrible). Option #2 is quite scary; it's effectively a recipe for the DTB and the ACPI payload to be out of sync -- they no longer come from a common source. Covering up such desynchronization after the fact, in edk2, is a doomed approach IMO. The machine description (capabilities etc) is owned by QEMU; if that's influenced by EL3 fw properties, those should be made explicit to QEMU (or to some other layered management application) via metadata. In my opinion :) Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108415): https://edk2.groups.io/g/devel/message/108415 Mute This Topic: https://groups.io/mt/101215483/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-07 20:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230116113931.1221-1-eiakovlev@linux.microsoft.com>
2023-09-07 14:27 ` [edk2-devel] [edk2][RFC] OvmfPkg/AcpiPlatformDxe: patch FADT PSCI bits if FDT advertises it Ard Biesheuvel
2023-09-07 14:50 ` Laszlo Ersek
2023-09-07 15:17 ` Ard Biesheuvel
2023-09-07 20:07 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox