From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lj1-f170.google.com (mail-lj1-f170.google.com [209.85.208.170]) by mx.groups.io with SMTP id smtpd.web11.19670.1647392491182867856 for ; Tue, 15 Mar 2022 18:01:31 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@semihalf-com.20210112.gappssmtp.com header.s=20210112 header.b=PVOfF78j; spf=none, err=SPF record not found (domain: semihalf.com, ip: 209.85.208.170, mailfrom: mw@semihalf.com) Received: by mail-lj1-f170.google.com with SMTP id q5so1141995ljb.11 for ; Tue, 15 Mar 2022 18:01:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=qysfABeeCpq1W4rDXZ3auwGMVZZRh498P0SCCNkO+Yk=; b=PVOfF78jNNsajv+BgJy06PC+DPn5fMYD19/qPan4cLEqe8l7TieIpv9q7v6ZPmusr5 V4gPe/4f789IRBonA2dk1/AOfBce2jw3lsCb58Xa4S8jJgWaHBqLpCIvdxw6Lroxs+O+ smoF65rGSRGhqhCrEFSd9xYE7SF4pwMUfOq6xBTTD9Kzu/oGJ28ZyeTzoHIThvBcPOLA EZJNErK31uXlcg4NKMeF9FbXQd933ZIZ2WVztkiLNKvUcCO1JRWhrOnc1bV9d0IL2p+a uuT6F9djgG4sGbfNdKEH8cIKKQ+yt0/wjr1nmabJ2AwspjV5ta5fPm39UgdE4SyI6/no 0ovw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=qysfABeeCpq1W4rDXZ3auwGMVZZRh498P0SCCNkO+Yk=; b=uuhPG2pRnsE43igygG3k+RfpVTofk6LcFWLFWcimpxY7zNRyjxH3Cfwa90HIeHFm+7 83RdD+aLy2KE46TZvEF8bYQw+NUOqJyF1YThqB+MGctTNHZfrqVaB/lDV1JSiQihqV6+ EK/Kb6eUilTApzIQQWTXTaLXu0FbmkME4OGF5ggmJ0R0Th5LduwRXpSSWZoVsUU4RQUj HzD0+hvDMtZBGxp1UlWqxXD5RSJsVblWvCHTs58xJBLdRHsM7loj2saLyBC3oui3qm2K xOO2/RHdAvL7WP5/DRHpX1TTj4iDnCGEYeMRDJfuK3NriCKrKv+27yinC9mWnftBLuR4 xpSA== X-Gm-Message-State: AOAM530v4SJpan1IoWMqKEK51b4uV8a3NiNRd3oO7Ko/q2vz3lFaZH47 N9IkMVTtbBA9F6wJaUMyHbMLmyxNi62qhtyJHpqRHw== X-Google-Smtp-Source: ABdhPJzuEK6Hh9xX/I1bGY5nXs6K9hUkJYazDDMUKyYXrOHem8aYLHX9yr3okIathOotQMZtGQWefZQXz7X8V4Vgpi4= X-Received: by 2002:a2e:bf1d:0:b0:247:dea7:f657 with SMTP id c29-20020a2ebf1d000000b00247dea7f657mr18847119ljr.454.1647392489344; Tue, 15 Mar 2022 18:01:29 -0700 (PDT) MIME-Version: 1.0 References: <20210312230554.292964-1-ardb@kernel.org> <20a15ffd-20bb-8170-e96b-c6151083883f@redhat.com> <00cb01d71adc$84805850$8d8108f0$@byosoft.com.cn> In-Reply-To: <00cb01d71adc$84805850$8d8108f0$@byosoft.com.cn> From: "Marcin Wojtas" Date: Wed, 16 Mar 2022 02:01:19 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/VariableRuntimeDxe: avoid double VA conversion of FVB protocol To: Ard Biesheuvel Cc: Laszlo Ersek , edk2-devel-groups-io , gaoliming@byosoft.com.cn, "Gao, Liming" , Jon Nettleton , Leif Lindholm Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Ard, =C5=9Br., 17 mar 2021 o 04:20 gaoliming napisa= =C5=82(a): > > Ard: > > > -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- > > =E5=8F=91=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io =E4=BB=A3=E8=A1=A8 Laszlo Ersek > > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2021=E5=B9=B43=E6=9C=8816=E6=97= =A5 23:58 > > =E6=94=B6=E4=BB=B6=E4=BA=BA: Ard Biesheuvel ; devel@ed= k2.groups.io > > =E6=8A=84=E9=80=81: liming.gao@intel.com; jon@solid-run.com; leif@nuvia= inc.com > > =E4=B8=BB=E9=A2=98: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/VariableR= untimeDxe: > > avoid double VA conversion of FVB protocol > > > > On 03/13/21 00:05, Ard Biesheuvel wrote: > > > For historical reasons, the VariableRuntimeDxe performs virtual addre= ss > > > conversion on the FVB protocol member pointers of the protocol instan= ce > > > that backs the EFI variable store. However, the driver that produces = the > > > actual instance should be doing this, as it is the owner, and provide= s > > > the actual implementation of those methods. > > > > > > Unfortunately, we cannot simply change this: existing drivers may rel= y > > > on this behavior, and so the variable driver should take care to only > > > convert the pointers when necessary, but leave them alone when the ow= ner > > > 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 swit= ch > > > has occurred or not, and perform the switch if it hasn't. > > > > > > Signed-off-by: Ard Biesheuvel > > > --- > > > 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 =3D { VarCheckRegis > > > > > VarCheckVariablePropertySet, > > > > > VarCheckVariablePropertyGet }; > > > > > > +STATIC EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL > > mFvbProtocolShadow; > > > + > > > /** > > > Some Secure Boot Policy Variable may update following other variab= le > > 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 =3D mVariableModuleGlobal->FvbInstance; > > > + if (FvbInstance !=3D NULL && > > > + FvbInstance->GetBlockSize !=3D > > mFvbProtocolShadow.GetBlockSize) { > > > > (2) It occurs to me that the following pattern (in a single-threaded > > environment): > > > > if (a !=3D b) { > > a =3D b; > > } > > > > is just: > > > > a =3D 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 > > > > Thanks > > Laszlo > > > > > + FvbInstance->GetBlockSize =3D > > mFvbProtocolShadow.GetBlockSize; > > > + FvbInstance->GetPhysicalAddress =3D > > mFvbProtocolShadow.GetPhysicalAddress; > > > + FvbInstance->GetAttributes =3D > > mFvbProtocolShadow.GetAttributes; > > > + FvbInstance->SetAttributes =3D > > mFvbProtocolShadow.SetAttributes; > > > + FvbInstance->Read =3D > > mFvbProtocolShadow.Read; > > > + FvbInstance->Write =3D mFvbProtocolShadow.Write; > > > + FvbInstance->EraseBlocks =3D > > mFvbProtocolShadow.EraseBlocks; > > > + } > > > } > > > } > > > > > > @@ -247,13 +267,22 @@ VariableClassAddressChangeEvent ( > > > UINTN Index; > > > > > > if (mVariableModuleGlobal->FvbInstance !=3D 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 po= inter > > > + // conversion on its members. However, some FVB protocol > > implementations > > > + // may rely on this behavior, which was present in older version= s 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 =3D FvbProtocol; > > > > > > + // > > > + // Store the boot time values of the function pointers so we can > > compare > > > + // them later. This is needed to avoid double conversion during th= e call > > > + // to SetVirtualAddressMap. > > > + // > > > + CopyMem (&mFvbProtocolShadow, FvbProtocol, sizeof > > mFvbProtocolShadow); > > > + > I think the simply change is to directly update mVariableModuleGlobal->Fv= bInstance. > > mVariableModuleGlobal->FvbInstance =3D &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