From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 088B8D802DF for ; Thu, 7 Sep 2023 14:27:58 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=mne+t0obnY3lzLjU92Zik0jASzxEtHBs3hzcwQH8r54=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1694096877; v=1; b=kVUPEwp3lHRFw8U8PneekCJYI8s5LRiRwDvYJimv3sWZlu8PCB79HqIDXQc9hB5mPxnsIbyJ HBnEF3qIS6pYxv3ToPnmyYbTZjzPPWaT499WAX1JoJ+2fBu2bZxiHPj65OLwJJ/G8XnrICB06xL nOXGhDGNwi852iZ6DQIPnS1w= X-Received: by 127.0.0.2 with SMTP id raJtYY7687511xswg1C8bd1N; Thu, 07 Sep 2023 07:27:57 -0700 X-Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web10.14220.1694096876620988327 for ; Thu, 07 Sep 2023 07:27:57 -0700 X-Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id C2CF3B81E7F; Thu, 7 Sep 2023 14:27:54 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id 69574C32791; Thu, 7 Sep 2023 14:27:53 +0000 (UTC) X-Received: by mail-lf1-f49.google.com with SMTP id 2adb3069b0e04-5007616b756so1712551e87.3; Thu, 07 Sep 2023 07:27:53 -0700 (PDT) X-Gm-Message-State: 28kS2wIOhhDihblCbWU0H5Mpx7686176AA= X-Google-Smtp-Source: AGHT+IH9Z8qYgxF+vyM5ByzkbjKoani4IL7mO1qteQfqw1O5qITPZd/rDDWjmkLQQDCeF3wW54Ld5hsGP0gtr+g9E7I= X-Received: by 2002:a05:6512:5c6:b0:500:9de4:5968 with SMTP id o6-20020a05651205c600b005009de45968mr4493160lfo.59.1694096871364; Thu, 07 Sep 2023 07:27:51 -0700 (PDT) MIME-Version: 1.0 References: <20230116113931.1221-1-eiakovlev@linux.microsoft.com> In-Reply-To: <20230116113931.1221-1-eiakovlev@linux.microsoft.com> From: "Ard Biesheuvel" Date: Thu, 7 Sep 2023 16:27:40 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [edk2][RFC] OvmfPkg/AcpiPlatformDxe: patch FADT PSCI bits if FDT advertises it To: Evgeny Iakovlev , kraxel@redhat.com, Laszlo Ersek , edk2-devel-groups-io Cc: rfc@edk2.groups.io, jiewen.yao@intel.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=kVUPEwp3; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (policy=none) On Mon, 16 Jan 2023 at 12:39, Evgeny Iakovlev 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 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 // EFI_ACPI_DESCRIPTION_HEADER > +#include // EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE > #include // QEMU_LOADER_FNAME_SIZE > #include // AsciiStrCmp() > #include // CopyMem() > @@ -20,6 +21,7 @@ > #include // gBS > > #include > +#include > #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] -=-=-=-=-=-=-=-=-=-=-=-