From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mx.groups.io with SMTP id smtpd.web10.2835.1571212518716975967 for ; Wed, 16 Oct 2019 00:55:18 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.43, mailfrom: hao.a.wu@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Oct 2019 00:55:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.67,303,1566889200"; d="scan'208";a="347348206" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga004.jf.intel.com with ESMTP; 16 Oct 2019 00:55:17 -0700 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 16 Oct 2019 00:55:17 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by FMSMSX112.amr.corp.intel.com (10.18.116.6) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 16 Oct 2019 00:55:17 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.166]) by shsmsx102.ccr.corp.intel.com ([169.254.2.176]) with mapi id 14.03.0439.000; Wed, 16 Oct 2019 15:55:14 +0800 From: "Wu, Hao A" To: "Kubacki, Michael A" , "devel@edk2.groups.io" CC: "Bi, Dandan" , Ard Biesheuvel , "Dong, Eric" , Laszlo Ersek , "Gao, Liming" , "Kinney, Michael D" , "Ni, Ray" , "Wang, Jian J" , "Yao, Jiewen" Subject: Re: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add local auth status in VariableParsing Thread-Topic: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add local auth status in VariableParsing Thread-Index: AQHVdZ6zvg1BoZXtVUOkq6ImrvN3bKdCQ9cggAZ9cACAFCFiYA== Date: Wed, 16 Oct 2019 07:55:13 +0000 Message-ID: References: <20190928014717.31372-1-michael.a.kubacki@intel.com> <20190928014717.31372-5-michael.a.kubacki@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Kubacki, Michael A > Sent: Friday, October 04, 2019 2:35 AM > To: Wu, Hao A; devel@edk2.groups.io > Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo Ersek; Gao, Liming; K= inney, > Michael D; Ni, Ray; Wang, Jian J; Yao, Jiewen > Subject: RE: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add loca= l > auth status in VariableParsing >=20 > I will make the following changes in V3: >=20 > > InitVariableParsing() seems an internal function, the 'EFIAPI' keyword= can > be > > dropped. Please help to update the function definition in .C file as w= ell. >=20 > I will remove the EFIAPI keyword. >=20 > > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > index 1a57d7e1ba..53d797152c 100644 > > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > @@ -3326,6 +3326,9 @@ InitNonVolatileVariableStore ( > > > mVariableModuleGlobal->MaxVariableSize =3D PcdGet32 > > > (PcdMaxVariableSize); > > > mVariableModuleGlobal->MaxAuthVariableSize =3D ((PcdGet32 > > > (PcdMaxAuthVariableSize) !=3D 0) ? PcdGet32 (PcdMaxAuthVariableSize)= : > > > mVariableModuleGlobal->MaxVariableSize); > > > > > > + Status =3D InitVariableParsing (mVariableModuleGlobal- > > > >VariableGlobal.AuthFormat); > > > + ASSERT_EFI_ERROR (Status); > > > + > > > > > > After the above initialization, mVariableModuleGlobal- > > >VariableGlobal.AuthFormat > > will be changed temporarily within > > ConvertNormalVarStorageToAuthVarStorage() if normal HOB variable store > > will be converted to the auth format: > > > > VOID * > > ConvertNormalVarStorageToAuthVarStorage ( > > VARIABLE_STORE_HEADER *NormalVarStorage > > ) > > { > > ... > > // > > // Set AuthFormat as FALSE for normal variable storage > > // > > mVariableModuleGlobal->VariableGlobal.AuthFormat =3D FALSE; > > ... > > // > > // Restore AuthFormat > > // > > mVariableModuleGlobal->VariableGlobal.AuthFormat =3D TRUE; > > return AuthVarStorage; > > } > > > > > > I think there will be issues in such converting, since I found that at= least > > GetVariableHeaderSize() and NameSizeOfVariable() get called during the > > execution of ConvertNormalVarStorageToAuthVarStorage(). And they are > > checking 'mAuthFormat' rather than 'mVariableModuleGlobal- > > >VariableGlobal.AuthFormat'. > > > > >=20 > You're right that will be a problem. I missed this temporary change in t= he > value. > I'm going to have all the functions dependent on authentication status i= n > VariableParsing.c take it as a parameter and let the respective drivers = linked > against it maintain their own single copy of the authentication state. I am really sorry for not raising this question until I saw the latest pat= ch series: Is it possible to call the InitVariableParsing() function (maybe a rename = for the function for better understanding) for the temporary changes for 'mVariableModuleGlobal->VariableGlobal.AuthFormat' in function ConvertNormalVarStorageToAuthVarStorage()? In my opinion, doing so can avoid changing many function interfaces. Best Regards, Hao Wu >=20 > > > // > > > // Parse non-volatile variable data and get last variable offset. > > > // > > > @@ -3756,18 +3759,13 @@ VariableCommonInitialize ( > > > > > > // > > > // mVariableModuleGlobal->VariableGlobal.AuthFormat > > > - // has been initialized in InitNonVolatileVariableStore(). > > > + // is initialized in InitNonVolatileVariableStore(). > > > // > > > if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > DEBUG ((EFI_D_INFO, "Variable driver will work with auth variab= le > > > format!\n")); > > > - // > > > - // Set AuthSupport to FALSE first, VariableWriteServiceInitiali= ze() will > > > initialize it. > > > - // > > > - mVariableModuleGlobal->VariableGlobal.AuthSupport =3D FALSE; > > > VariableGuid =3D &gEfiAuthenticatedVariableGuid; > > > } else { > > > DEBUG ((EFI_D_INFO, "Variable driver will work without auth > > > variable support!\n")); > > > - mVariableModuleGlobal->VariableGlobal.AuthSupport =3D FALSE; > > > > > > Not sure why the above changes belong to this patch. > > Could you help to double confirm? >=20 > This was used during testing and is not needed. I will remove it. >=20 > Thanks, > Michael >=20 > > -----Original Message----- > > From: Wu, Hao A > > Sent: Thursday, October 3, 2019 1:04 AM > > To: devel@edk2.groups.io; Kubacki, Michael A > > > > Cc: Bi, Dandan ; Ard Biesheuvel > > ; Dong, Eric ; Laszlo > Ersek > > ; Gao, Liming ; Kinney, > Michael > > D ; Ni, Ray ; Wang, Jian= J > > ; Yao, Jiewen > > Subject: RE: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add > local > > auth status in VariableParsing > > > > Inline comments below: > > > > > > > -----Original Message----- > > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf > Of > > > Kubacki, Michael A > > > Sent: Saturday, September 28, 2019 9:47 AM > > > To: devel@edk2.groups.io > > > Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo Ersek; Gao, Limin= g; > > > Kinney, Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen > > > Subject: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add loca= l > > > auth status in VariableParsing > > > > > > The file VariableParsing.c provides generic functionality related to > > > parsing variable related structures and information. In order to > > > calculate offsets for certain operations, the functions must know if > > > authenticated variables are enabled as this increases the size of > > > variable headers. > > > > > > This change removes linking against a global variable in an external > > > file in favor of a statically scoped variable in VariableParsing.c > > > Because this file is unaware of how the authenticated variable statu= s > > > is determined, the variable is set through a function interface > > > invoked during variable driver initialization. > > > > > > Cc: Dandan Bi > > > Cc: Ard Biesheuvel > > > Cc: Eric Dong > > > Cc: Laszlo Ersek > > > Cc: Liming Gao > > > Cc: Michael D Kinney > > > Cc: Ray Ni > > > Cc: Jian J Wang > > > Cc: Hao A Wu > > > Cc: Jiewen Yao > > > Signed-off-by: Michael Kubacki > > > --- > > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h | 14 > > > +++++++++ > > > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 10 > +++--- > > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c | 33 > > > ++++++++++++++++---- > > > 3 files changed, 45 insertions(+), 12 deletions(-) > > > > > > diff --git > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h > > > index 6f2000f3ee..3eba590634 100644 > > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h > > > +++ > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h > > > @@ -308,4 +308,18 @@ UpdateVariableInfo ( > > > IN OUT VARIABLE_INFO_ENTRY **VariableInfo > > > ); > > > > > > +/** > > > + Initializes context needed for variable parsing functions. > > > + > > > + @param[in] AuthFormat If true then indicates authe= nticated > > > variables are supported > > > + > > > + @retval EFI_SUCCESS Initialized successfully > > > + @retval Others An error occurred during ini= tialization > > > +**/ > > > +EFI_STATUS > > > +EFIAPI > > > +InitVariableParsing ( > > > > > > InitVariableParsing() seems an internal function, the 'EFIAPI' keyword= can > be > > dropped. Please help to update the function definition in .C file as w= ell. > > > > > > > + IN BOOLEAN AuthFormat > > > + ); > > > + > > > #endif > > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > index 1a57d7e1ba..53d797152c 100644 > > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > @@ -3326,6 +3326,9 @@ InitNonVolatileVariableStore ( > > > mVariableModuleGlobal->MaxVariableSize =3D PcdGet32 > > > (PcdMaxVariableSize); > > > mVariableModuleGlobal->MaxAuthVariableSize =3D ((PcdGet32 > > > (PcdMaxAuthVariableSize) !=3D 0) ? PcdGet32 (PcdMaxAuthVariableSize)= : > > > mVariableModuleGlobal->MaxVariableSize); > > > > > > + Status =3D InitVariableParsing (mVariableModuleGlobal- > > > >VariableGlobal.AuthFormat); > > > + ASSERT_EFI_ERROR (Status); > > > + > > > > > > After the above initialization, mVariableModuleGlobal- > > >VariableGlobal.AuthFormat > > will be changed temporarily within > > ConvertNormalVarStorageToAuthVarStorage() if normal HOB variable store > > will be converted to the auth format: > > > > VOID * > > ConvertNormalVarStorageToAuthVarStorage ( > > VARIABLE_STORE_HEADER *NormalVarStorage > > ) > > { > > ... > > // > > // Set AuthFormat as FALSE for normal variable storage > > // > > mVariableModuleGlobal->VariableGlobal.AuthFormat =3D FALSE; > > ... > > // > > // Restore AuthFormat > > // > > mVariableModuleGlobal->VariableGlobal.AuthFormat =3D TRUE; > > return AuthVarStorage; > > } > > > > > > I think there will be issues in such converting, since I found that at= least > > GetVariableHeaderSize() and NameSizeOfVariable() get called during the > > execution of ConvertNormalVarStorageToAuthVarStorage(). And they are > > checking 'mAuthFormat' rather than 'mVariableModuleGlobal- > > >VariableGlobal.AuthFormat'. > > > > > > > // > > > // Parse non-volatile variable data and get last variable offset. > > > // > > > @@ -3756,18 +3759,13 @@ VariableCommonInitialize ( > > > > > > // > > > // mVariableModuleGlobal->VariableGlobal.AuthFormat > > > - // has been initialized in InitNonVolatileVariableStore(). > > > + // is initialized in InitNonVolatileVariableStore(). > > > // > > > if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > DEBUG ((EFI_D_INFO, "Variable driver will work with auth variab= le > > > format!\n")); > > > - // > > > - // Set AuthSupport to FALSE first, VariableWriteServiceInitiali= ze() will > > > initialize it. > > > - // > > > - mVariableModuleGlobal->VariableGlobal.AuthSupport =3D FALSE; > > > VariableGuid =3D &gEfiAuthenticatedVariableGuid; > > > } else { > > > DEBUG ((EFI_D_INFO, "Variable driver will work without auth > > > variable support!\n")); > > > - mVariableModuleGlobal->VariableGlobal.AuthSupport =3D FALSE; > > > > > > Not sure why the above changes belong to this patch. > > Could you help to double confirm? > > > > Best Regards, > > Hao Wu > > > > > > > VariableGuid =3D &gEfiVariableGuid; > > > } > > > > > > diff --git > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c > > > index 394195342d..0a47f6d10d 100644 > > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c > > > +++ > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c > > > @@ -9,6 +9,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > > > > #include "VariableParsing.h" > > > > > > +STATIC BOOLEAN mAuthFormat; > > > + > > > /** > > > > > > This code checks if variable header is valid or not. > > > @@ -88,7 +90,7 @@ GetVariableHeaderSize ( { > > > UINTN Value; > > > > > > - if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > + if (mAuthFormat) { > > > Value =3D sizeof (AUTHENTICATED_VARIABLE_HEADER); > > > } else { > > > Value =3D sizeof (VARIABLE_HEADER); @@ -114,7 +116,7 @@ > > > NameSizeOfVariable ( > > > AUTHENTICATED_VARIABLE_HEADER *AuthVariable; > > > > > > AuthVariable =3D (AUTHENTICATED_VARIABLE_HEADER *) Variable; > > > - if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > + if (mAuthFormat) { > > > if (AuthVariable->State =3D=3D (UINT8) (-1) || > > > AuthVariable->DataSize =3D=3D (UINT32) (-1) || > > > AuthVariable->NameSize =3D=3D (UINT32) (-1) || @@ -149,7 +15= 1,7 @@ > > > SetNameSizeOfVariable ( > > > AUTHENTICATED_VARIABLE_HEADER *AuthVariable; > > > > > > AuthVariable =3D (AUTHENTICATED_VARIABLE_HEADER *) Variable; > > > - if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > + if (mAuthFormat) { > > > AuthVariable->NameSize =3D (UINT32) NameSize; > > > } else { > > > Variable->NameSize =3D (UINT32) NameSize; @@ -173,7 +175,7 @@ > > > DataSizeOfVariable ( > > > AUTHENTICATED_VARIABLE_HEADER *AuthVariable; > > > > > > AuthVariable =3D (AUTHENTICATED_VARIABLE_HEADER *) Variable; > > > - if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > + if (mAuthFormat) { > > > if (AuthVariable->State =3D=3D (UINT8) (-1) || > > > AuthVariable->DataSize =3D=3D (UINT32) (-1) || > > > AuthVariable->NameSize =3D=3D (UINT32) (-1) || @@ -208,7 +21= 0,7 @@ > > > SetDataSizeOfVariable ( > > > AUTHENTICATED_VARIABLE_HEADER *AuthVariable; > > > > > > AuthVariable =3D (AUTHENTICATED_VARIABLE_HEADER *) Variable; > > > - if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > + if (mAuthFormat) { > > > AuthVariable->DataSize =3D (UINT32) DataSize; > > > } else { > > > Variable->DataSize =3D (UINT32) DataSize; @@ -248,7 +250,7 @@ > > > GetVendorGuidPtr ( > > > AUTHENTICATED_VARIABLE_HEADER *AuthVariable; > > > > > > AuthVariable =3D (AUTHENTICATED_VARIABLE_HEADER *) Variable; > > > - if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > + if (mAuthFormat) { > > > return &AuthVariable->VendorGuid; > > > } else { > > > return &Variable->VendorGuid; > > > @@ -746,3 +748,22 @@ UpdateVariableInfo ( > > > } > > > } > > > } > > > + > > > +/** > > > + Initializes context needed for variable parsing functions. > > > + > > > + @param[in] AuthFormat If true then indicates authe= nticated > > > variables are supported > > > + > > > + @retval EFI_SUCCESS Initialized successfully > > > + @retval Others An error occurred during ini= tialization > > > +**/ > > > +EFI_STATUS > > > +EFIAPI > > > +InitVariableParsing ( > > > + IN BOOLEAN AuthFormat > > > + ) > > > +{ > > > + mAuthFormat =3D AuthFormat; > > > + > > > + return EFI_SUCCESS; > > > +} > > > -- > > > 2.16.2.windows.1 > > > > > > > > >=20 > > >=20