From: "Marcin Wojtas" <mw@semihalf.com>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>,
edk2-devel-groups-io <devel@edk2.groups.io>,
gaoliming@byosoft.com.cn, "Gao, Liming" <liming.gao@intel.com>,
Jon Nettleton <jon@solid-run.com>,
Leif Lindholm <leif@nuviainc.com>
Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/VariableRuntimeDxe: avoid double VA conversion of FVB protocol
Date: Wed, 16 Mar 2022 02:01:19 +0100 [thread overview]
Message-ID: <CAPv3WKdduWWrGfytk9ti4s9ho3nLs-6hkyvsNCXd7n-DQZr=3w@mail.gmail.com> (raw)
In-Reply-To: <00cb01d71adc$84805850$8d8108f0$@byosoft.com.cn>
Hi Ard,
śr., 17 mar 2021 o 04:20 gaoliming <gaoliming@byosoft.com.cn> napisał(a):
>
> Ard:
>
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Laszlo Ersek
> > 发送时间: 2021年3月16日 23:58
> > 收件人: Ard Biesheuvel <ardb@kernel.org>; devel@edk2.groups.io
> > 抄送: liming.gao@intel.com; jon@solid-run.com; leif@nuviainc.com
> > 主题: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/VariableRuntimeDxe:
> > avoid double VA conversion of FVB protocol
> >
> > On 03/13/21 00:05, Ard Biesheuvel wrote:
> > > For historical reasons, the VariableRuntimeDxe performs virtual address
> > > conversion on the FVB protocol member pointers of the protocol instance
> > > that backs the EFI variable store. However, the driver that produces the
> > > actual instance should be doing this, as it is the owner, and provides
> > > the actual implementation of those methods.
> > >
> > > Unfortunately, we cannot simply change this: existing drivers may rely
> > > on this behavior, and so the variable driver should take care to only
> > > convert the pointers when necessary, but leave them alone when the owner
> > > is taking care of this.
>
> To answer Laszlo question, I want to know the impact for the simple fix.
>
> > >
> > > The virtual address switch event can be delivered in arbitrary order, and
> > > so we should take care of this in a way that does not rely on whether this
> > > driver converts its pointers either before or after the owner of the FVB
> > > protocol receives the event.
> > >
> > > So let's not convert the pointers at all when the event is delivered, but
> > > record the converted addresses in a shadow FVB protocol. This way, we can
> > > check when the variable driver is invoked at runtime whether the switch
> > > has occurred or not, and perform the switch if it hasn't.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > > Build tested only.
> > >
> > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 50
> > +++++++++++++++++---
> > > 1 file changed, 43 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> > > index 0fca0bb2a9b5..3d83ac4452ee 100644
> > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> > > @@ -37,6 +37,8 @@ EDKII_VAR_CHECK_PROTOCOL
> > mVarCheck = { VarCheckRegis
> > >
> > VarCheckVariablePropertySet,
> > >
> > VarCheckVariablePropertyGet };
> > >
> > > +STATIC EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL
> > mFvbProtocolShadow;
> > > +
> > > /**
> > > Some Secure Boot Policy Variable may update following other variable
> > changes(SecureBoot follows PK change, etc).
> > > Record their initial State when variable write service is ready.
> > > @@ -105,8 +107,26 @@ AcquireLockOnlyAtBootTime (
> > > IN EFI_LOCK *Lock
> > > )
> > > {
> >
> > (1) Why is AcquireLockOnlyAtBootTime() the best place to add this?
> >
> > (Considering especially the leading comment, "This is a temperary
> > function that will be removed when EfiAcquireLock() in UefiLib can
> > handle the call in UEFI Runtimer driver in RT phase".)
> >
> > Obviously, I don't want to create more work for you! I just feel that,
> > if this is not the best place, we shouldn't overload this function with
> > a new responsibility.
> >
> > > + EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *FvbInstance;
> > > +
> > > if (!AtRuntime ()) {
> > > EfiAcquireLock (Lock);
> > > + } else {
> > > + //
> > > + // Check whether we need to swap in the converted pointer values
> > for the
> > > + // FVB protocol methods
> > > + //
> > > + FvbInstance = mVariableModuleGlobal->FvbInstance;
> > > + if (FvbInstance != NULL &&
> > > + FvbInstance->GetBlockSize !=
> > mFvbProtocolShadow.GetBlockSize) {
> >
> > (2) It occurs to me that the following pattern (in a single-threaded
> > environment):
> >
> > if (a != b) {
> > a = b;
> > }
> >
> > is just:
> >
> > a = b;
> >
> > (the small CPU cost notwithstanding).
> >
> > And that puts this patch in a bit different light: it's not that we may
> > or may not convert. Instead, we *always* convert, the question is *when*
> > we apply the result of the conversion.
> >
> > Functionally, there is no difference, but to me mentally, there'd be a
> > difference, if we said "delay applying the runtime conversion until
> > first call".
> >
> > Anyway... just wanted to highlight this: we could drop the GetBlockSize
> > funcptr comparison. But, we don't have to.
> >
> > Given the reviews from Liming and Hao -- and thank you in the first
> > place for writing the patch! --, I won't really ask for a v2. I'd just
> > somehow prefer the compat logic to be elsewhere, rather than in
> > AcquireLockOnlyAtBootTime(). In the first place, I'm not clear what we
> > currently use AcquireLockOnlyAtBootTime() for.
> >
> > Up to the maintainers to decide whether this justifies v2. If not:
> >
> > Acked-by: Laszlo Ersek <lersek@redhat.com>
> >
> > Thanks
> > Laszlo
> >
> > > + FvbInstance->GetBlockSize =
> > mFvbProtocolShadow.GetBlockSize;
> > > + FvbInstance->GetPhysicalAddress =
> > mFvbProtocolShadow.GetPhysicalAddress;
> > > + FvbInstance->GetAttributes =
> > mFvbProtocolShadow.GetAttributes;
> > > + FvbInstance->SetAttributes =
> > mFvbProtocolShadow.SetAttributes;
> > > + FvbInstance->Read =
> > mFvbProtocolShadow.Read;
> > > + FvbInstance->Write = mFvbProtocolShadow.Write;
> > > + FvbInstance->EraseBlocks =
> > mFvbProtocolShadow.EraseBlocks;
> > > + }
> > > }
> > > }
> > >
> > > @@ -247,13 +267,22 @@ VariableClassAddressChangeEvent (
> > > UINTN Index;
> > >
> > > if (mVariableModuleGlobal->FvbInstance != NULL) {
> > > - EfiConvertPointer (0x0, (VOID **)
> > &mVariableModuleGlobal->FvbInstance->GetBlockSize);
> > > - EfiConvertPointer (0x0, (VOID **)
> > &mVariableModuleGlobal->FvbInstance->GetPhysicalAddress);
> > > - EfiConvertPointer (0x0, (VOID **)
> > &mVariableModuleGlobal->FvbInstance->GetAttributes);
> > > - EfiConvertPointer (0x0, (VOID **)
> > &mVariableModuleGlobal->FvbInstance->SetAttributes);
> > > - EfiConvertPointer (0x0, (VOID **)
> > &mVariableModuleGlobal->FvbInstance->Read);
> > > - EfiConvertPointer (0x0, (VOID **)
> > &mVariableModuleGlobal->FvbInstance->Write);
> > > - EfiConvertPointer (0x0, (VOID **)
> > &mVariableModuleGlobal->FvbInstance->EraseBlocks);
> > > + //
> > > + // This module did not produce the FVB protocol instance that
> > provides the
> > > + // variable store, so it should not be the one performing the pointer
> > > + // conversion on its members. However, some FVB protocol
> > implementations
> > > + // may rely on this behavior, which was present in older versions of
> > this
> > > + // driver, so we need to perform the conversion if the protocol
> > producer
> > > + // fails to do so. So let's record the converted values now, and swap
> > them
> > > + // in later if they haven't changed values.
> > > + //
> > > + EfiConvertPointer (0x0, (VOID
> > **)&mFvbProtocolShadow.GetBlockSize);
> > > + EfiConvertPointer (0x0, (VOID
> > **)&mFvbProtocolShadow.GetPhysicalAddress);
> > > + EfiConvertPointer (0x0, (VOID
> > **)&mFvbProtocolShadow.GetAttributes);
> > > + EfiConvertPointer (0x0, (VOID
> > **)&mFvbProtocolShadow.SetAttributes);
> > > + EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.Read);
> > > + EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.Write);
> > > + EfiConvertPointer (0x0, (VOID
> > **)&mFvbProtocolShadow.EraseBlocks);
> > > EfiConvertPointer (0x0, (VOID **)
> > &mVariableModuleGlobal->FvbInstance);
> > > }
> > > EfiConvertPointer (0x0, (VOID **)
> > &mVariableModuleGlobal->PlatformLangCodes);
> > > @@ -454,6 +483,13 @@ FtwNotificationEvent (
> > > }
> > > mVariableModuleGlobal->FvbInstance = FvbProtocol;
> > >
> > > + //
> > > + // Store the boot time values of the function pointers so we can
> > compare
> > > + // them later. This is needed to avoid double conversion during the call
> > > + // to SetVirtualAddressMap.
> > > + //
> > > + CopyMem (&mFvbProtocolShadow, FvbProtocol, sizeof
> > mFvbProtocolShadow);
> > > +
> I think the simply change is to directly update mVariableModuleGlobal->FvbInstance.
>
> mVariableModuleGlobal->FvbInstance = &mFvbProtocolShadow;
>
I'm digging up the patch from abyss. Do you think it's possible to
answer Liming's questions and merge it upstream somehow?
Best regards,
Marcin
prev parent reply other threads:[~2022-03-16 1:01 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-12 23:05 [PATCH 1/1] MdeModulePkg/VariableRuntimeDxe: avoid double VA conversion of FVB protocol Ard Biesheuvel
2021-03-13 11:32 ` Jon Nettleton
2021-03-15 2:40 ` 回复: [edk2-devel] " gaoliming
2021-03-15 5:56 ` Wu, Hao A
2021-03-16 15:58 ` Laszlo Ersek
2021-03-16 19:19 ` [edk2-devel] " Samer El-Haj-Mahmoud
2021-03-17 3:20 ` 回复: " gaoliming
2022-03-16 1:01 ` Marcin Wojtas [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPv3WKdduWWrGfytk9ti4s9ho3nLs-6hkyvsNCXd7n-DQZr=3w@mail.gmail.com' \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox