Re: [edk2-devel] [PATCH V1 1/1] BaseTools: Fix GenMake multi-workspace failure

[Bob Feng]

Pushed at SHA-1: 61af5f249495b18f45ca164376c871081448c0e4

Thanks,
Bob

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Bob Feng
Sent: Friday, October 4, 2019 11:04 AM
To: devel@edk2.groups.io; Kinney, Michael D ; 
Kubacki, Michael A 
Cc: Gao, Liming 
Subject: Re: [edk2-devel] [PATCH V1 1/1] BaseTools: Fix GenMake multi-workspace 
failure

Thanks for the fix.

I agree. 
Reviewed-by:  Bob Feng 


-Original Message-
From: devel@edk2.groups.io  On Behalf Of Michael D Kinney
Sent: Friday, October 4, 2019 5:14 AM
To: devel@edk2.groups.io; Kubacki, Michael A ; 
Kinney, Michael D 
Cc: Feng, Bob C ; Gao, Liming 
Subject: Re: [edk2-devel] [PATCH V1 1/1] BaseTools: Fix GenMake multi-workspace 
failure

Hi Michael,

Reviewed-by: Michael D Kinney 

This look really important.  Do you want me to push today?

Mike

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of 
> Kubacki, Michael A
> Sent: Tuesday, October 1, 2019 3:58 PM
> To: devel@edk2.groups.io
> Cc: Feng, Bob C ; Gao, Liming 
> ; Kinney, Michael D 
> Subject: [edk2-devel] [PATCH V1 1/1] BaseTools: Fix GenMake 
> multi-workspace failure
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2232
> 
> Commit 0075ab2cec introduced an issue that causes an exception when 
> multiple workspace packages paths are specified. For example, if 
> edk2-platforms is used, the root directory will contain an edk and 
> edk2-platforms directory representing the respective repositories.
> 
> In GenMake, the path to the package DEC file for a module is 
> discovered by getting the relative path of the INF to the workspace 
> root directory. Each directory in the relative path is incrementally 
> joined to the WORKSPACE directory. The file list in the joined path is 
> searched for a DEC file.
> 
> As an example, if the build command is used on a package outside the
> edk2 repository, the INF file path is relative to the edk2-platforms 
> directory not edk2. This causes directory paths to be built that do 
> not exist.
> Commit 0075ab2cec replaced the
> os.path.exists() call with a try except block that always fails when
> os.listdir() is invoked to enumerate the list of files in the built 
> directory path on packages outside edk2.
> 
> This commit restores the original conditional statement which avoids 
> calling os.listdir() with an invalid directory path.
> 
> Cc: Bob Feng 
> Cc: Liming Gao 
> Cc: Michael D Kinney 
> Signed-off-by: Michael Kubacki
> 
> ---
>  BaseTools/Source/Python/AutoGen/GenMake.py | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py
> b/BaseTools/Source/Python/AutoGen/GenMake.py
> index 584156dab9..97ba158ff2 100755
> --- a/BaseTools/Source/Python/AutoGen/GenMake.py
> +++ b/BaseTools/Source/Python/AutoGen/GenMake.py
> @@ -637,13 +637,11 @@ cleanlib:
>  while not found and os.sep in package_rel_dir:
>  index = package_rel_dir.index(os.sep)
>  current_dir = mws.join(current_dir,
> package_rel_dir[:index])
> -try:
> +if os.path.exists(current_dir):
>  for fl in os.listdir(current_dir):
>  if fl.endswith('.dec'):
>  found = True
>  break
> -except:
> -EdkLogger.error('build',
> FILE_NOT_FOUND, "WORKSPACE does not exist.")
>  package_rel_dir = package_rel_dir[index + 1:]
> 
>  MakefileTemplateDict = {
> --
> 2.16.2.windows.1
> 
> 
> 








-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48468): https://edk2.groups.io/g/devel/message/48468
Mute This Topic: https://groups.io/mt/34364042/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] BaseTools: Fix GenMake multi-workspace failure

[Bob Feng]

Thanks for the fix.

I agree. 
Reviewed-by:  Bob Feng 


-Original Message-
From: devel@edk2.groups.io  On Behalf Of Michael D Kinney
Sent: Friday, October 4, 2019 5:14 AM
To: devel@edk2.groups.io; Kubacki, Michael A ; 
Kinney, Michael D 
Cc: Feng, Bob C ; Gao, Liming 
Subject: Re: [edk2-devel] [PATCH V1 1/1] BaseTools: Fix GenMake multi-workspace 
failure

Hi Michael,

Reviewed-by: Michael D Kinney 

This look really important.  Do you want me to push today?

Mike

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of 
> Kubacki, Michael A
> Sent: Tuesday, October 1, 2019 3:58 PM
> To: devel@edk2.groups.io
> Cc: Feng, Bob C ; Gao, Liming 
> ; Kinney, Michael D 
> Subject: [edk2-devel] [PATCH V1 1/1] BaseTools: Fix GenMake 
> multi-workspace failure
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2232
> 
> Commit 0075ab2cec introduced an issue that causes an exception when 
> multiple workspace packages paths are specified. For example, if 
> edk2-platforms is used, the root directory will contain an edk and 
> edk2-platforms directory representing the respective repositories.
> 
> In GenMake, the path to the package DEC file for a module is 
> discovered by getting the relative path of the INF to the workspace 
> root directory. Each directory in the relative path is incrementally 
> joined to the WORKSPACE directory. The file list in the joined path is 
> searched for a DEC file.
> 
> As an example, if the build command is used on a package outside the 
> edk2 repository, the INF file path is relative to the edk2-platforms 
> directory not edk2. This causes directory paths to be built that do 
> not exist.
> Commit 0075ab2cec replaced the
> os.path.exists() call with a try except block that always fails when 
> os.listdir() is invoked to enumerate the list of files in the built 
> directory path on packages outside edk2.
> 
> This commit restores the original conditional statement which avoids 
> calling os.listdir() with an invalid directory path.
> 
> Cc: Bob Feng 
> Cc: Liming Gao 
> Cc: Michael D Kinney 
> Signed-off-by: Michael Kubacki
> 
> ---
>  BaseTools/Source/Python/AutoGen/GenMake.py | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py
> b/BaseTools/Source/Python/AutoGen/GenMake.py
> index 584156dab9..97ba158ff2 100755
> --- a/BaseTools/Source/Python/AutoGen/GenMake.py
> +++ b/BaseTools/Source/Python/AutoGen/GenMake.py
> @@ -637,13 +637,11 @@ cleanlib:
>  while not found and os.sep in package_rel_dir:
>  index = package_rel_dir.index(os.sep)
>  current_dir = mws.join(current_dir,
> package_rel_dir[:index])
> -try:
> +if os.path.exists(current_dir):
>  for fl in os.listdir(current_dir):
>  if fl.endswith('.dec'):
>  found = True
>  break
> -except:
> -EdkLogger.error('build',
> FILE_NOT_FOUND, "WORKSPACE does not exist.")
>  package_rel_dir = package_rel_dir[index + 1:]
> 
>  MakefileTemplateDict = {
> --
> 2.16.2.windows.1
> 
> 
> 





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48467): https://edk2.groups.io/g/devel/message/48467
Mute This Topic: https://groups.io/mt/34364042/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 27/35] SecurityPkg: stop abusing EFI_EVENT for protocol notify registration

[Yao, Jiewen]

Good catch!

Reviewed by: Jiewen Yao 

> -Original Message-
> From: Laszlo Ersek 
> Sent: Thursday, October 3, 2019 7:07 PM
> To: Zhang, Chao B ; Wang, Jian J
> ; Yao, Jiewen 
> Cc: edk2-devel-groups-io 
> Subject: Re: [edk2-devel] [PATCH 27/35] SecurityPkg: stop abusing EFI_EVENT
> for protocol notify registration
> 
> Pinging SecurityPkg maintainers again, for reviewing this patch.
> 
> Thanks
> Laszlo
> 
> On 09/26/19 14:46, Laszlo Ersek wrote:
> > Chao, Jian, Jiewen,
> >
> > can you please review this patch?
> >
> > Thanks,
> > Laszlo
> >
> >
> > On 09/17/19 21:49, Laszlo Ersek wrote:
> >> EfiCreateProtocolNotifyEvent() takes a (VOID**) for "Registration",
> >> similarly to gBS->RegisterProtocolNotify(). We should pass the address of
> >> an actual pointer-to-VOID, and not the address of an EFI_EVENT. EFI_EVENT
> >> just happens to be specified as (VOID*), and has nothing to do with the
> >> registration.
> >>
> >> This change is a no-op in practice; it's a semantic improvement.
> >>
> >> Cc: Chao Zhang 
> >> Cc: Jian Wang 
> >> Cc: Jiewen Yao 
> >> Signed-off-by: Laszlo Ersek 
> >> ---
> >>
> >> Notes:
> >> build-tested only
> >>
> >>  SecurityPkg/HddPassword/HddPasswordDxe.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/SecurityPkg/HddPassword/HddPasswordDxe.c
> b/SecurityPkg/HddPassword/HddPasswordDxe.c
> >> index b0d795b6597f..051e64091d7f 100644
> >> --- a/SecurityPkg/HddPassword/HddPasswordDxe.c
> >> +++ b/SecurityPkg/HddPassword/HddPasswordDxe.c
> >> @@ -2770,7 +2770,7 @@ HddPasswordDxeInit (
> >>  {
> >>EFI_STATUS Status;
> >>HDD_PASSWORD_DXE_PRIVATE_DATA  *Private;
> >> -  EFI_EVENT  Registration;
> >> +  VOID   *Registration;
> >>EFI_EVENT  EndOfDxeEvent;
> >>EDKII_VARIABLE_LOCK_PROTOCOL   *VariableLock;
> >>
> >>
> >
> >
> > 
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48464): https://edk2.groups.io/g/devel/message/48464
Mute This Topic: https://groups.io/mt/34180229/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache support

[Kubacki, Michael A]

I agree, I will make the default to enable the runtime cache.

> -Original Message-
> From: Kinney, Michael D 
> Sent: Thursday, October 3, 2019 3:01 PM
> To: Kubacki, Michael A ; Wu, Hao A
> ; devel@edk2.groups.io; Kinney, Michael D
> 
> Cc: Bi, Dandan ; Ard Biesheuvel
> ; Dong, Eric ; Laszlo Ersek
> ; Gao, Liming ; Ni, Ray
> ; Wang, Jian J ; Yao, Jiewen
> 
> Subject: RE: [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable()
> cache support
> 
> Michael,
> 
> Perhaps the FeaturePCD for #2 should be enabled by default
> so the platform DSC only needs to set this PCD for some
> validation tests.
> 
> Mike
> 
> 
> > -Original Message-
> > From: Kubacki, Michael A 
> > Sent: Thursday, October 3, 2019 2:54 PM
> > To: Wu, Hao 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: [PATCH V2 7/9] MdeModulePkg/Variable: Add
> > RT GetVariable() cache support
> >
> > #1 - The plan is to remove the polling entirely in V3.
> >
> > #2 - I'd prefer to take a definitive direction and
> > reduce validation and maintenance
> > effort but you and Laszlo both requested this so
> > I'll add a FeaturePCD to control
> > activation of the runtime cache in this patch
> > series. Perhaps this can be removed
> > in the future.
> >
> > #3 - Will be done in V3.
> >
> > Other replies are inline.
> >
> > > -Original Message-
> > > From: Wu, Hao A 
> > > Sent: Thursday, October 3, 2019 1:05 AM
> > > 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: [PATCH V2 7/9] MdeModulePkg/Variable: Add
> > RT GetVariable()
> > > cache support
> > >
> > > Before any comment on the patch, since I am not
> > experienced in the
> > > Variable
> > > driver, I would like to ask for help from other
> > reviewers to look into this
> > > patch and provide feedbacks as well. Thanks in
> > advance.
> > >
> > > With the above fact, some comments provided below
> > maybe wrong. So
> > > please help
> > > to kindly correct me.
> > >
> > >
> > > Some general comments:
> > > 1. I am not sure if bringing the TimerLib dependency
> > (delay in acquiring the
> > >runtime cache read lock) to variable driver (a
> > software driver for the most
> > >part) is a good idea.
> > >
> > >Hope other reviewers can provide some feedbacks for
> > this. Thanks in
> > > advance.
> > >
> > > 2. In my opinion, I prefer a switch can be provided
> > for platform owners to
> > >choose between using the runtime cache and going
> > through SMM for
> > > GetVariable
> > >(and for GetNextVariableName in the next patch as
> > well).
> > >
> > >If platform owners feel uncomfortable with using
> > the runtime cache with
> > >regard to the security perspective, they can switch
> > to the origin solution.
> > >
> > > 3. Please help to remove the 'EFIAPI' keyword for new
> > driver internal
> > > functions;
> > >
> > >
> > > Inline comments below:
> > >
> > >
> > > > -Original Message-
> > > > From: 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, Liming;
> > > Kinney,
> > > > Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao,
> > Jiewen
> > > > Subject: [PATCH V2 7/9] MdeModulePkg/Variable: Add
> > RT GetVariable()
> > > > cache support
> > > >
> > > >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2220
> > > >
> > > > This change reduces SMIs for GetVariable () by
> > maintaining a
> > > > UEFI variable cache in Runtime DXE in addition to
> > the pre-
> > > > existing cache in SMRAM. When the Runtime Service
> > GetVariable()
> > > > is invoked, a Runtime DXE cache is used instead of
> > triggering an
> > > > SMI to VariableSmm. This can improve overall system
> > performance
> > > > by servicing variable read requests without
> > rendezvousing all
> > > > cores into SMM.
> > > >
> > > > The following are important points regarding this
> > change.
> > > >
> > > > 1. All of the non-volatile storage contents are
> > loaded into the
> > > >cache upon driver load. This one time load
> > operation from storage
> > > >is preferred as opposed to building the cache on
> > demand. An on-
> > > >demand cache would require a fallback SMI to load
> > data into the
> > > >cache as variables are requested.
> > > >
> > > > 2. SetVariable () requests will continue to always
> > trigger an SMI.
> > > >This occurs regardless of whether the variable is
> > volatile or
> > > >non-volatile.
> > > >
> > > > 3. Both volatile and non-volatile variables are
> > cached in a runtime
> > > >buffer. As is the case in the 

Re: [edk2-devel] [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache support

[Michael D Kinney]

Michael,

Perhaps the FeaturePCD for #2 should be enabled by default
so the platform DSC only needs to set this PCD for some
validation tests.

Mike


> -Original Message-
> From: Kubacki, Michael A 
> Sent: Thursday, October 3, 2019 2:54 PM
> To: Wu, Hao 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: [PATCH V2 7/9] MdeModulePkg/Variable: Add
> RT GetVariable() cache support
> 
> #1 - The plan is to remove the polling entirely in V3.
> 
> #2 - I'd prefer to take a definitive direction and
> reduce validation and maintenance
> effort but you and Laszlo both requested this so
> I'll add a FeaturePCD to control
> activation of the runtime cache in this patch
> series. Perhaps this can be removed
> in the future.
> 
> #3 - Will be done in V3.
> 
> Other replies are inline.
> 
> > -Original Message-
> > From: Wu, Hao A 
> > Sent: Thursday, October 3, 2019 1:05 AM
> > 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: [PATCH V2 7/9] MdeModulePkg/Variable: Add
> RT GetVariable()
> > cache support
> >
> > Before any comment on the patch, since I am not
> experienced in the
> > Variable
> > driver, I would like to ask for help from other
> reviewers to look into this
> > patch and provide feedbacks as well. Thanks in
> advance.
> >
> > With the above fact, some comments provided below
> maybe wrong. So
> > please help
> > to kindly correct me.
> >
> >
> > Some general comments:
> > 1. I am not sure if bringing the TimerLib dependency
> (delay in acquiring the
> >runtime cache read lock) to variable driver (a
> software driver for the most
> >part) is a good idea.
> >
> >Hope other reviewers can provide some feedbacks for
> this. Thanks in
> > advance.
> >
> > 2. In my opinion, I prefer a switch can be provided
> for platform owners to
> >choose between using the runtime cache and going
> through SMM for
> > GetVariable
> >(and for GetNextVariableName in the next patch as
> well).
> >
> >If platform owners feel uncomfortable with using
> the runtime cache with
> >regard to the security perspective, they can switch
> to the origin solution.
> >
> > 3. Please help to remove the 'EFIAPI' keyword for new
> driver internal
> > functions;
> >
> >
> > Inline comments below:
> >
> >
> > > -Original Message-
> > > From: 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, Liming;
> > Kinney,
> > > Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao,
> Jiewen
> > > Subject: [PATCH V2 7/9] MdeModulePkg/Variable: Add
> RT GetVariable()
> > > cache support
> > >
> > >
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2220
> > >
> > > This change reduces SMIs for GetVariable () by
> maintaining a
> > > UEFI variable cache in Runtime DXE in addition to
> the pre-
> > > existing cache in SMRAM. When the Runtime Service
> GetVariable()
> > > is invoked, a Runtime DXE cache is used instead of
> triggering an
> > > SMI to VariableSmm. This can improve overall system
> performance
> > > by servicing variable read requests without
> rendezvousing all
> > > cores into SMM.
> > >
> > > The following are important points regarding this
> change.
> > >
> > > 1. All of the non-volatile storage contents are
> loaded into the
> > >cache upon driver load. This one time load
> operation from storage
> > >is preferred as opposed to building the cache on
> demand. An on-
> > >demand cache would require a fallback SMI to load
> data into the
> > >cache as variables are requested.
> > >
> > > 2. SetVariable () requests will continue to always
> trigger an SMI.
> > >This occurs regardless of whether the variable is
> volatile or
> > >non-volatile.
> > >
> > > 3. Both volatile and non-volatile variables are
> cached in a runtime
> > >buffer. As is the case in the current EDK II
> variable driver, they
> > >continue to be cached in separate buffers.
> > >
> > > 4. The cache in Runtime DXE and SMM are intended to
> be exact copies
> > >of one another. All SMM variable accesses only
> return data from the
> > >SMM cache. The runtime caches are only updated
> after the variable I/O
> > >operation is successful in SMM. The runtime
> caches are only updated
> > >from SMM.
> > >
> > > 5. Synchronization mechanisms are in place to ensure
> the runtime cache
> > >content integrity with the SMM cache. These may
> result in updates to
> > >runtime cache that are the same in content but
> different in offset and
> > >size from updates to the SMM cache.
> > >
> > > When using SMM variables, two 

Re: [edk2-devel] [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache support

[Kubacki, Michael A]

#1 - The plan is to remove the polling entirely in V3.

#2 - I'd prefer to take a definitive direction and reduce validation and 
maintenance
effort but you and Laszlo both requested this so I'll add a FeaturePCD 
to control
activation of the runtime cache in this patch series. Perhaps this can 
be removed
in the future.

#3 - Will be done in V3. 

Other replies are inline.

> -Original Message-
> From: Wu, Hao A 
> Sent: Thursday, October 3, 2019 1:05 AM
> 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: [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable()
> cache support
> 
> Before any comment on the patch, since I am not experienced in the
> Variable
> driver, I would like to ask for help from other reviewers to look into this
> patch and provide feedbacks as well. Thanks in advance.
> 
> With the above fact, some comments provided below maybe wrong. So
> please help
> to kindly correct me.
> 
> 
> Some general comments:
> 1. I am not sure if bringing the TimerLib dependency (delay in acquiring the
>runtime cache read lock) to variable driver (a software driver for the most
>part) is a good idea.
> 
>Hope other reviewers can provide some feedbacks for this. Thanks in
> advance.
> 
> 2. In my opinion, I prefer a switch can be provided for platform owners to
>choose between using the runtime cache and going through SMM for
> GetVariable
>(and for GetNextVariableName in the next patch as well).
> 
>If platform owners feel uncomfortable with using the runtime cache with
>regard to the security perspective, they can switch to the origin solution.
> 
> 3. Please help to remove the 'EFIAPI' keyword for new driver internal
> functions;
> 
> 
> Inline comments below:
> 
> 
> > -Original Message-
> > From: 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, Liming;
> Kinney,
> > Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> > Subject: [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable()
> > cache support
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2220
> >
> > This change reduces SMIs for GetVariable () by maintaining a
> > UEFI variable cache in Runtime DXE in addition to the pre-
> > existing cache in SMRAM. When the Runtime Service GetVariable()
> > is invoked, a Runtime DXE cache is used instead of triggering an
> > SMI to VariableSmm. This can improve overall system performance
> > by servicing variable read requests without rendezvousing all
> > cores into SMM.
> >
> > The following are important points regarding this change.
> >
> > 1. All of the non-volatile storage contents are loaded into the
> >cache upon driver load. This one time load operation from storage
> >is preferred as opposed to building the cache on demand. An on-
> >demand cache would require a fallback SMI to load data into the
> >cache as variables are requested.
> >
> > 2. SetVariable () requests will continue to always trigger an SMI.
> >This occurs regardless of whether the variable is volatile or
> >non-volatile.
> >
> > 3. Both volatile and non-volatile variables are cached in a runtime
> >buffer. As is the case in the current EDK II variable driver, they
> >continue to be cached in separate buffers.
> >
> > 4. The cache in Runtime DXE and SMM are intended to be exact copies
> >of one another. All SMM variable accesses only return data from the
> >SMM cache. The runtime caches are only updated after the variable I/O
> >operation is successful in SMM. The runtime caches are only updated
> >from SMM.
> >
> > 5. Synchronization mechanisms are in place to ensure the runtime cache
> >content integrity with the SMM cache. These may result in updates to
> >runtime cache that are the same in content but different in offset and
> >size from updates to the SMM cache.
> >
> > When using SMM variables, two caches will now be present.
> > 1. "Runtime Cache" - Maintained in VariableSmmRuntimeDxe. Used to
> > service
> >Runtime Services GetVariable () and GetNextVariableName () callers.
> > 2. "SMM Cache" - Maintained in VariableSmm to service SMM GetVariable
> ()
> >and GetNextVariableName () callers.
> >a. This cache is retained so SMM modules do not operate on data outside
> >   SMRAM.
> >
> > It is possible to view UEFI variable read and write statistics by setting
> > the gEfiMdeModulePkgTokenSpaceGuid.PcdVariableCollectStatistics
> > FeaturePcd
> > to TRUE and using the VariableInfo UEFI application in MdeModulePkg to
> > dump
> > variable statistics to the console. By doing so, a user can view the number
> > of GetVariable () hits from the Runtime DXE variable driver (Runtime Cache
> > hits) and 

Re: [edk2-devel] [PATCH V1 1/1] BaseTools: Fix GenMake multi-workspace failure

[Michael D Kinney]

Hi Michael,

Reviewed-by: Michael D Kinney 

This look really important.  Do you want me to push today?

Mike

> -Original Message-
> From: devel@edk2.groups.io  On
> Behalf Of Kubacki, Michael A
> Sent: Tuesday, October 1, 2019 3:58 PM
> To: devel@edk2.groups.io
> Cc: Feng, Bob C ; Gao, Liming
> ; Kinney, Michael D
> 
> Subject: [edk2-devel] [PATCH V1 1/1] BaseTools: Fix
> GenMake multi-workspace failure
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2232
> 
> Commit 0075ab2cec introduced an issue that causes an
> exception when multiple workspace packages paths are
> specified. For example, if edk2-platforms is used, the
> root directory will contain an edk and edk2-platforms
> directory representing the respective repositories.
> 
> In GenMake, the path to the package DEC file for a
> module is discovered by getting the relative path of the
> INF to the workspace root directory. Each directory in
> the relative path is incrementally joined to the
> WORKSPACE directory. The file list in the joined path is
> searched for a DEC file.
> 
> As an example, if the build command is used on a package
> outside the edk2 repository, the INF file path is
> relative to the edk2-platforms directory not edk2. This
> causes directory paths to be built that do not exist.
> Commit 0075ab2cec replaced the
> os.path.exists() call with a try except block that
> always fails when os.listdir() is invoked to enumerate
> the list of files in the built directory path on
> packages outside edk2.
> 
> This commit restores the original conditional statement
> which avoids calling os.listdir() with an invalid
> directory path.
> 
> Cc: Bob Feng 
> Cc: Liming Gao 
> Cc: Michael D Kinney 
> Signed-off-by: Michael Kubacki
> 
> ---
>  BaseTools/Source/Python/AutoGen/GenMake.py | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py
> b/BaseTools/Source/Python/AutoGen/GenMake.py
> index 584156dab9..97ba158ff2 100755
> --- a/BaseTools/Source/Python/AutoGen/GenMake.py
> +++ b/BaseTools/Source/Python/AutoGen/GenMake.py
> @@ -637,13 +637,11 @@ cleanlib:
>  while not found and os.sep in package_rel_dir:
>  index = package_rel_dir.index(os.sep)
>  current_dir = mws.join(current_dir,
> package_rel_dir[:index])
> -try:
> +if os.path.exists(current_dir):
>  for fl in os.listdir(current_dir):
>  if fl.endswith('.dec'):
>  found = True
>  break
> -except:
> -EdkLogger.error('build',
> FILE_NOT_FOUND, "WORKSPACE does not exist.")
>  package_rel_dir = package_rel_dir[index +
> 1:]
> 
>  MakefileTemplateDict = {
> --
> 2.16.2.windows.1
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48460): https://edk2.groups.io/g/devel/message/48460
Mute This Topic: https://groups.io/mt/34364042/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms] [PATCH 2/5] KabylakeOpenBoardPkg/KabylakeRvp3: Add BIOS Info PEIM

[Agyeman, Prince]

Added BIOS Info PEIM to KabylakeRvp3 to publish
the BIOS info HOB. This PEIM currently publishs
the board's microcode region info.

Cc: Ankit Sinha 
Cc: Nate DeSimone 
Cc: Kubacki Michael A 

Signed-off-by: Prince Agyeman 
---
 .../KabylakeRvp3/BiosInfo/BiosInfo.c  | 93 +++
 .../KabylakeRvp3/BiosInfo/BiosInfo.inf| 48 ++
 .../KabylakeRvp3/OpenBoardPkg.dsc |  2 +
 .../KabylakeRvp3/OpenBoardPkg.fdf |  1 +
 4 files changed, 144 insertions(+)
 create mode 100644 
Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/BiosInfo/BiosInfo.c
 create mode 100644 
Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/BiosInfo/BiosInfo.inf

diff --git 
a/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/BiosInfo/BiosInfo.c 
b/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/BiosInfo/BiosInfo.c
new file mode 100644
index 00..6a058a0fc2
--- /dev/null
+++ b/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/BiosInfo/BiosInfo.c
@@ -0,0 +1,93 @@
+/** @file
+  Driver for BIOS Info support.
+
+  Copyright (c) 2019, Intel Corporation. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define INDEXPORT_TO_ADDRESS(x) (x)
+#define DATAPORT_TO_ADDRESS(x)  ((x) << 16)
+#define PORTWIDTH_TO_ADDRESS(x) ((x) << 32)
+#define PORTBITNUMBER_TO_ADDRESS(x) ((x) << 40)
+#define PORTINDEXNUMBER_TO_ADDRESS(x)   ((x) << 48)
+
+//
+// Internal
+//
+#pragma pack (1)
+
+typedef struct {
+  BIOS_INFO_HEADER  Header;
+  BIOS_INFO_STRUCT  Entry[1];
+} BIOS_INFO;
+#pragma pack ()
+
+GLOBAL_REMOVE_IF_UNREFERENCED BIOS_INFO  mBiosInfo = {
+  {
+BIOS_INFO_SIGNATURE,
+1,
+0,
+  },
+  {
+{
+  FIT_TYPE_01_MICROCODE,
+  BIOS_INFO_STRUCT_ATTRIBUTE_MICROCODE_WHOLE_REGION,
+  0x0100,
+  FixedPcdGet32 (PcdFlashMicrocodeFvSize),
+  FixedPcdGet32 (PcdFlashMicrocodeFvBase)
+}
+  }
+};
+
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_PPI_DESCRIPTOR  mBiosInfoPpiList = {
+  EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
+  ,
+  
+};
+
+/**
+  Installs BiosInfo Ppi.
+
+  @param  FileHandle  Handle of the file being invoked.
+  @param  PeiServices Describes the list of possible PEI Services.
+
+  @retval EFI_SUCCESS   Install the BiosInfo Ppi successfully.
+
+**/
+EFI_STATUS
+EFIAPI
+BiosInfoEntryPoint (
+  IN   EFI_PEI_FILE_HANDLE  FileHandle,
+  IN CONST EFI_PEI_SERVICES **PeiServices
+  )
+{
+  EFI_STATUS  Status;
+  VOID*HobData;
+
+  //
+  // Install PPI, so that other PEI module can add dependency.
+  //
+  Status = PeiServicesInstallPpi ();
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // Build hob, so that DXE module can also get the data.
+  //
+  HobData = BuildGuidHob (, sizeof (mBiosInfo));
+  ASSERT (HobData != NULL);
+  if (HobData == NULL) {
+return EFI_OUT_OF_RESOURCES;
+  }
+  CopyMem (HobData, , sizeof (mBiosInfo));
+
+  return EFI_SUCCESS;
+}
diff --git 
a/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/BiosInfo/BiosInfo.inf 
b/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/BiosInfo/BiosInfo.inf
new file mode 100644
index 00..94543408b1
--- /dev/null
+++ b/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/BiosInfo/BiosInfo.inf
@@ -0,0 +1,48 @@
+### @file
+#  Module Information description file for BIOS Info Driver
+#
+#  Copyright (c) 2019, Intel Corporation. All rights reserved.
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+###
+
+[Defines]
+  INF_VERSION= 0x00010017
+  BASE_NAME  = BiosInfo
+  FILE_GUID  = C83BCE0E-6F16-4D3C-8D9F-4D6F5A032929
+  VERSION_STRING = 1.0
+  MODULE_TYPE= PEIM
+  ENTRY_POINT= BiosInfoEntryPoint
+#
+# The following information is for reference only and not required by the 
build tools.
+#
+# VALID_ARCHITECTURES IA32 X64
+#
+
+[LibraryClasses]
+  PeimEntryPoint
+  PeiServicesLib
+  HobLib
+  BaseMemoryLib
+  DebugLib
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  IntelSiliconPkg/IntelSiliconPkg.dec
+  KabylakeSiliconPkg/SiPkg.dec
+  KabylakeFspBinPkg/KabylakeFspBinPkg.dec
+  BoardModulePkg/BoardModulePkg.dec
+  MinPlatformPkg/MinPlatformPkg.dec
+
+[Pcd]
+  gSiPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase## CONSUMES
+  gSiPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize## CONSUMES
+
+[Sources]
+  BiosInfo.c
+
+[Guids]
+  gBiosInfoGuid ## PRODUCES
+
+[Depex]
+  TRUE
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenBoardPkg.dsc 
b/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenBoardPkg.dsc
index 7090852192..ef69a19aa4 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenBoardPkg.dsc
+++ b/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenBoardPkg.dsc
@@ -324,6 +324,8 @@
   }
 !endif
 
+  

[edk2-devel] [edk2-platforms] [PATCH 5/5] Platform/Intel: Add FIT generation tool

[Agyeman, Prince]

Added fit generation tool to the build
process.

What was done:

Added BIOS_INFO_GUID to the build.cfg

Added BIOS_INFO_GUID to GalagoPro3,
KabylakeRvp3 and WhiskeylakeURvp build_config.cfg
This allows the boards to specify the GUID
associated with the its Bios Info PEIM

BIOS_INFO_GUID is passed as an argument to
FitGen in the FIT table generation process

Cc: Ankit Sinha 
Cc: Nate DeSimone 
Cc: Michael Kubacki 

Signed-off-by: Prince Agyeman 
---
 .../GalagoPro3/build_config.cfg   |  1 +
 .../KabylakeRvp3/build_config.cfg |  1 +
 .../WhiskeylakeURvp/build_config.cfg  |  1 +
 Platform/Intel/build.cfg  |  1 +
 Platform/Intel/build_bios.py  | 57 +++
 5 files changed, 61 insertions(+)

diff --git a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/build_config.cfg 
b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/build_config.cfg
index 8c6c51abb4..3f64239a29 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/build_config.cfg
+++ b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/build_config.cfg
@@ -31,3 +31,4 @@ FSP_PKG_NAME = KabylakeFspPkg
 FSP_BINARY_BUILD = FALSE
 FSP_TEST_RELEASE = FALSE
 SECURE_BOOT_ENABLE = FALSE
+BIOS_INFO_GUID = 3132E669-D16B-4AA7-B09B-BC0EB5F40E1F
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/build_config.cfg 
b/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/build_config.cfg
index 78f808bfaf..f6ae4b342a 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/build_config.cfg
+++ b/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/build_config.cfg
@@ -33,3 +33,4 @@ FSP_PKG_NAME = AmberLakeFspPkg
 FSP_BINARY_BUILD = FALSE
 FSP_TEST_RELEASE = FALSE
 SECURE_BOOT_ENABLE = FALSE
+BIOS_INFO_GUID = C83BCE0E-6F16-4D3C-8D9F-4D6F5A032929
diff --git 
a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/build_config.cfg 
b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/build_config.cfg
index 1b0619bc1c..1dfe5ffd10 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/build_config.cfg
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/build_config.cfg
@@ -31,3 +31,4 @@ FSP_PKG_NAME = CoffeelakeSiliconPkg
 FSP_BINARY_BUILD = FALSE
 FSP_TEST_RELEASE = FALSE
 SECURE_BOOT_ENABLE = FALSE
+BIOS_INFO_GUID = A842B2D2-5C88-44E9-A9E2-4830F26662B7
diff --git a/Platform/Intel/build.cfg b/Platform/Intel/build.cfg
index 2040774d1b..6aee96694c 100644
--- a/Platform/Intel/build.cfg
+++ b/Platform/Intel/build.cfg
@@ -48,6 +48,7 @@ SECURE_BOOT_ENABLE = FALSE
 REBUILD_MODE =
 BUILD_ROM_ONLY =
 NUMBER_OF_PROCESSORS = 0
+BIOS_INFO_GUID =
 
 
 [PLATFORMS]
diff --git a/Platform/Intel/build_bios.py b/Platform/Intel/build_bios.py
index 46285df19a..ea098de705 100644
--- a/Platform/Intel/build_bios.py
+++ b/Platform/Intel/build_bios.py
@@ -196,6 +196,31 @@ def pre_build(build_config, build_type="DEBUG", 
silent=False, toolchain=None):
 if return_code != 0:
 build_failed(config)
 
+#
+# build platform silicon tools
+#
+# save the current workspace
+saved_work_directory = config["WORKSPACE"]
+# change the workspace to silicon tools directory
+config["WORKSPACE"] = os.path.join(config["WORKSPACE_SILICON"], "Tools")
+
+command = ["nmake"]
+if os.name == "posix":  # linux
+command = ["make"]
+# add path to generated FitGen binary to
+# environment path variable
+config["PATH"] += os.pathsep + \
+  os.path.join(config["BASE_TOOLS_PATH"],
+   "Source", "C", "bin")
+
+# build the silicon tools
+_, _, result, return_code = execute_script(command, config, shell=shell)
+if return_code != 0:
+build_failed(config)
+
+# restore WORKSPACE environment variable
+config["WORKSPACE"] = saved_work_directory
+
 config["SILENT_MODE"] = 'TRUE' if silent else 'FALSE'
 
 print("==")
@@ -404,6 +429,35 @@ def post_build(config):
 :returns: nothing
 """
 print("Running post_build to complete the build process.")
+board_fd = config["BOARD"].upper()
+final_fd = os.path.join(config["BUILD_DIR_PATH"], "FV",
+"{}.fd".format(board_fd))
+
+if config["BIOS_INFO_GUID"]:
+# Generate the fit table
+print("Generating FIT ...")
+if os.path.isfile(final_fd):
+temp_fd = os.path.join(config["BUILD_DIR_PATH"], "FV",
+   "{}_.fd".format(board_fd))
+shell = True
+command = ["FitGen", "-D",
+   final_fd, temp_fd, "-NA",
+   "-I", config["BIOS_INFO_GUID"]]
+
+if os.name == "posix": # linux
+shell = False
+
+_, _, result, return_code = execute_script(command, config, 
shell=shell)
+if return_code != 0:
+print("Error while generating fit")
+else:
+

[edk2-devel] [edk2-platforms] [PATCH 0/5] Add FIT support using FitGen

[Agyeman, Prince]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2210
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2211

This series of patches
 1. Add the generation of the FitGen tool to build_bios.py
 2. Add Firmware Interface Table (FIT) to the final images of:
a. KabylakeOpenBoardPkg/KabylakeRvp3
b. KabylakeOpenBoardPkg/GalagoPro3
c. WhiskeylakeOpenBoardPkg/WhiskeylakeURvp

What each patch does:
0001-BoardModulePkg-Add-bios-info-HOB: Adds the gBiosInfoGuid
and the Bios Info struct for the Bios info HOB

0002-KabylakeOpenBoardPkg-KabylakeRvp3-Add-BIOS-Info-PEIM,
0003-KabylakeOpenBoardPkg-GalagoPro3-Add-BiosInfo-PEIM,
0004-WhiskeylakeOpenBoardPkg-Add-BIOS-INFO-PEIM:
Patches 0002,0003,0004 add bios info PEIM to KabylakeRvp3,
GalagoPro3 and WhiskeylakeURvp boards respectively

0005-Platform-Intel-Add-FIT-generation-tool
Adds the FitGen tool binary and FIT generation to
the BIOS build process.

Prince Agyeman (5):
  BoardModulePkg: Add bios info HOB
  KabylakeOpenBoardPkg/KabylakeRvp3: Add BIOS Info PEIM
  KabylakeOpenBoardPkg/GalagoPro3: Add BiosInfo PEIM
  WhiskeylakeOpenBoardPkg: Add BIOS INFO PEIM
  Platform/Intel: Add FIT generation tool

 .../Intel/BoardModulePkg/BoardModulePkg.dec   |  3 +
 .../BoardModulePkg/Include/Library/BiosInfo.h | 62 +
 .../GalagoPro3/BiosInfo/BiosInfo.c| 92 ++
 .../GalagoPro3/BiosInfo/BiosInfo.inf  | 48 ++
 .../GalagoPro3/OpenBoardPkg.dsc   |  2 +
 .../GalagoPro3/OpenBoardPkg.fdf   |  1 +
 .../GalagoPro3/build_config.cfg   |  1 +
 .../KabylakeRvp3/BiosInfo/BiosInfo.c  | 93 +++
 .../KabylakeRvp3/BiosInfo/BiosInfo.inf| 48 ++
 .../KabylakeRvp3/OpenBoardPkg.dsc |  2 +
 .../KabylakeRvp3/OpenBoardPkg.fdf |  1 +
 .../KabylakeRvp3/build_config.cfg |  1 +
 .../WhiskeylakeURvp/BiosInfo/BiosInfo.c   | 93 +++
 .../WhiskeylakeURvp/BiosInfo/BiosInfo.inf | 48 ++
 .../WhiskeylakeURvp/OpenBoardPkg.dsc  |  2 +
 .../WhiskeylakeURvp/OpenBoardPkg.fdf  |  1 +
 .../WhiskeylakeURvp/build_config.cfg  |  1 +
 Platform/Intel/build.cfg  |  1 +
 Platform/Intel/build_bios.py  | 57 
 19 files changed, 557 insertions(+)
 create mode 100644 Platform/Intel/BoardModulePkg/Include/Library/BiosInfo.h
 create mode 100644 
Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/BiosInfo/BiosInfo.c
 create mode 100644 
Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/BiosInfo/BiosInfo.inf
 create mode 100644 
Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/BiosInfo/BiosInfo.c
 create mode 100644 
Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/BiosInfo/BiosInfo.inf
 create mode 100644 
Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/BiosInfo/BiosInfo.c
 create mode 100644 
Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/BiosInfo/BiosInfo.inf

-- 
2.19.1.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48454): https://edk2.groups.io/g/devel/message/48454
Mute This Topic: https://groups.io/mt/34386696/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms] [PATCH 4/5] WhiskeylakeOpenBoardPkg: Add BIOS INFO PEIM

[Agyeman, Prince]

Added BIOS info PEIM to publish Bios Info
HOB. This PEIM currently publishes the microcode
FV info.

Cc: Ankit Sinha 
Cc: Nate DeSimone 
Cc: Kubacki Michael A 

Signed-off-by: Prince Agyeman 
---
 .../WhiskeylakeURvp/BiosInfo/BiosInfo.c   | 93 +++
 .../WhiskeylakeURvp/BiosInfo/BiosInfo.inf | 48 ++
 .../WhiskeylakeURvp/OpenBoardPkg.dsc  |  2 +
 .../WhiskeylakeURvp/OpenBoardPkg.fdf  |  1 +
 4 files changed, 144 insertions(+)
 create mode 100644 
Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/BiosInfo/BiosInfo.c
 create mode 100644 
Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/BiosInfo/BiosInfo.inf

diff --git 
a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/BiosInfo/BiosInfo.c 
b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/BiosInfo/BiosInfo.c
new file mode 100644
index 00..6a058a0fc2
--- /dev/null
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/BiosInfo/BiosInfo.c
@@ -0,0 +1,93 @@
+/** @file
+  Driver for BIOS Info support.
+
+  Copyright (c) 2019, Intel Corporation. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define INDEXPORT_TO_ADDRESS(x) (x)
+#define DATAPORT_TO_ADDRESS(x)  ((x) << 16)
+#define PORTWIDTH_TO_ADDRESS(x) ((x) << 32)
+#define PORTBITNUMBER_TO_ADDRESS(x) ((x) << 40)
+#define PORTINDEXNUMBER_TO_ADDRESS(x)   ((x) << 48)
+
+//
+// Internal
+//
+#pragma pack (1)
+
+typedef struct {
+  BIOS_INFO_HEADER  Header;
+  BIOS_INFO_STRUCT  Entry[1];
+} BIOS_INFO;
+#pragma pack ()
+
+GLOBAL_REMOVE_IF_UNREFERENCED BIOS_INFO  mBiosInfo = {
+  {
+BIOS_INFO_SIGNATURE,
+1,
+0,
+  },
+  {
+{
+  FIT_TYPE_01_MICROCODE,
+  BIOS_INFO_STRUCT_ATTRIBUTE_MICROCODE_WHOLE_REGION,
+  0x0100,
+  FixedPcdGet32 (PcdFlashMicrocodeFvSize),
+  FixedPcdGet32 (PcdFlashMicrocodeFvBase)
+}
+  }
+};
+
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_PPI_DESCRIPTOR  mBiosInfoPpiList = {
+  EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
+  ,
+  
+};
+
+/**
+  Installs BiosInfo Ppi.
+
+  @param  FileHandle  Handle of the file being invoked.
+  @param  PeiServices Describes the list of possible PEI Services.
+
+  @retval EFI_SUCCESS   Install the BiosInfo Ppi successfully.
+
+**/
+EFI_STATUS
+EFIAPI
+BiosInfoEntryPoint (
+  IN   EFI_PEI_FILE_HANDLE  FileHandle,
+  IN CONST EFI_PEI_SERVICES **PeiServices
+  )
+{
+  EFI_STATUS  Status;
+  VOID*HobData;
+
+  //
+  // Install PPI, so that other PEI module can add dependency.
+  //
+  Status = PeiServicesInstallPpi ();
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // Build hob, so that DXE module can also get the data.
+  //
+  HobData = BuildGuidHob (, sizeof (mBiosInfo));
+  ASSERT (HobData != NULL);
+  if (HobData == NULL) {
+return EFI_OUT_OF_RESOURCES;
+  }
+  CopyMem (HobData, , sizeof (mBiosInfo));
+
+  return EFI_SUCCESS;
+}
diff --git 
a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/BiosInfo/BiosInfo.inf 
b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/BiosInfo/BiosInfo.inf
new file mode 100644
index 00..f268de00f5
--- /dev/null
+++ 
b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/BiosInfo/BiosInfo.inf
@@ -0,0 +1,48 @@
+### @file
+#  Module Information description file for BIOS Info Driver
+#
+#  Copyright (c) 2019, Intel Corporation. All rights reserved.
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+###
+
+[Defines]
+  INF_VERSION= 0x00010017
+  BASE_NAME  = BiosInfo
+  FILE_GUID  = A842B2D2-5C88-44E9-A9E2-4830F26662B7
+  VERSION_STRING = 1.0
+  MODULE_TYPE= PEIM
+  ENTRY_POINT= BiosInfoEntryPoint
+#
+# The following information is for reference only and not required by the 
build tools.
+#
+# VALID_ARCHITECTURES IA32 X64
+#
+
+[LibraryClasses]
+  PeimEntryPoint
+  PeiServicesLib
+  HobLib
+  BaseMemoryLib
+  DebugLib
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  IntelSiliconPkg/IntelSiliconPkg.dec
+  CoffeelakeSiliconPkg/SiPkg.dec
+  CoffeeLakeFspBinPkg/CoffeeLakeFspBinPkg.dec
+  BoardModulePkg/BoardModulePkg.dec
+  MinPlatformPkg/MinPlatformPkg.dec
+
+[Pcd]
+  gSiPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase## CONSUMES
+  gSiPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize## CONSUMES
+
+[Sources]
+  BiosInfo.c
+
+[Guids]
+  gBiosInfoGuid ## PRODUCES
+
+[Depex]
+  TRUE
diff --git 
a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkg.dsc 
b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkg.dsc
index eea809140c..00a89f4507 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkg.dsc
+++ 

[edk2-devel] [edk2-platforms] [PATCH 1/5] BoardModulePkg: Add bios info HOB

[Agyeman, Prince]

Added gBiosInfoGuid to be used to
publish BIOS information HOB which
is needed in FIT generation

Also added the Bios Info header
file BiosInfo.h

Cc: Ankit Sinha 
Cc: Nate DeSimone 
Cc: Kubacki Michael A 

Signed-off-by: Prince Agyeman 
---
 .../Intel/BoardModulePkg/BoardModulePkg.dec   |  3 +
 .../BoardModulePkg/Include/Library/BiosInfo.h | 62 +++
 2 files changed, 65 insertions(+)
 create mode 100644 Platform/Intel/BoardModulePkg/Include/Library/BiosInfo.h

diff --git a/Platform/Intel/BoardModulePkg/BoardModulePkg.dec 
b/Platform/Intel/BoardModulePkg/BoardModulePkg.dec
index 6f13945ca8..035427b2f5 100644
--- a/Platform/Intel/BoardModulePkg/BoardModulePkg.dec
+++ b/Platform/Intel/BoardModulePkg/BoardModulePkg.dec
@@ -36,3 +36,6 @@
 [Guids]
   ## Include Include/Guid/BiosId.h
   gBiosIdGuid = { 0xC3E36D09, 0x8294, 0x4b97, { 0xA8, 0x57, 0xD5, 0x28, 0x8F, 
0xE3, 0x3E, 0x28 } }
+
+  ## GUID to publish BIOS information HOB
+  gBiosInfoGuid = { 0x09d0d15c, 0xe9f0, 0x4dfc, {0x9e, 0x0b, 0x39, 0x33, 0x1f, 
0xca, 0x66, 0x85} }
diff --git a/Platform/Intel/BoardModulePkg/Include/Library/BiosInfo.h 
b/Platform/Intel/BoardModulePkg/Include/Library/BiosInfo.h
new file mode 100644
index 00..bba1c07bff
--- /dev/null
+++ b/Platform/Intel/BoardModulePkg/Include/Library/BiosInfo.h
@@ -0,0 +1,62 @@
+/** @file
+
+  Driver for BIOS Info support.
+
+  Copyright (c) 2019, Intel Corporation. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef _BIOS_INFO_H_
+#define _BIOS_INFO_H_
+
+//
+// BIOS INFO data structure
+// This is self contained data structure for BIOS info for TXT
+//
+#pragma pack (1)
+#define BIOS_INFO_SIGNATURE  SIGNATURE_64 ('$', 'B', 'I', 'O', 'S', 'I', 'F', 
'$')
+typedef struct {
+  UINT64Signature;
+  UINT32EntryCount;
+  UINT32Reserved;
+//BIOS_INFO_STRUCT  Struct[EntryCount];
+} BIOS_INFO_HEADER;
+
+//
+// BIOS_INFO_STRUCT attributes
+// bits[0:3] means general attributes
+// bits[4:7] means type specific attributes
+//
+#define BIOS_INFO_STRUCT_ATTRIBUTE_GENERAL_EXCLUDE_FROM_FIT  0x01
+#define BIOS_INFO_STRUCT_ATTRIBUTE_MICROCODE_WHOLE_REGION0x10
+#define BIOS_INFO_STRUCT_ATTRIBUTE_BIOS_POST_IBB 0x10
+#define BIOS_INFO_STRUCT_ATTRIBUTE_BIOS_NON_IBB  0x20
+
+typedef struct {
+  //
+  // FitTable entry type
+  //
+  UINT8Type;
+  //
+  // BIOS_INFO_STRUCT attributes
+  //
+  UINT8Attributes;
+  //
+  // FitTable entry version
+  //
+  UINT16   Version;
+  //
+  // FitTable entry real size
+  //
+  UINT32   Size;
+  //
+  // FitTable entry address
+  //
+  UINT64   Address;
+} BIOS_INFO_STRUCT;
+
+extern EFI_GUID  gBiosInfoGuid;
+
+#pragma pack ()
+
+#endif
-- 
2.19.1.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48455): https://edk2.groups.io/g/devel/message/48455
Mute This Topic: https://groups.io/mt/34386697/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms] [PATCH 3/5] KabylakeOpenBoardPkg/GalagoPro3: Add BiosInfo PEIM

[Agyeman, Prince]

Added BiosInfo PEIM to publish the
Bios Info HOB. Currently this PEIM publishes
one Bios Info entry which is the microcode
region information.

Cc: Ankit Sinha 
Cc: Nate DeSimone 
Cc: Kubacki Michael A 

Signed-off-by: Prince Agyeman 
---
 .../GalagoPro3/BiosInfo/BiosInfo.c| 92 +++
 .../GalagoPro3/BiosInfo/BiosInfo.inf  | 48 ++
 .../GalagoPro3/OpenBoardPkg.dsc   |  2 +
 .../GalagoPro3/OpenBoardPkg.fdf   |  1 +
 4 files changed, 143 insertions(+)
 create mode 100644 
Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/BiosInfo/BiosInfo.c
 create mode 100644 
Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/BiosInfo/BiosInfo.inf

diff --git a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/BiosInfo/BiosInfo.c 
b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/BiosInfo/BiosInfo.c
new file mode 100644
index 00..4db9d4685c
--- /dev/null
+++ b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/BiosInfo/BiosInfo.c
@@ -0,0 +1,92 @@
+/** @file
+  Driver for BIOS Info support.
+
+Copyright (c) 2019, Intel Corporation. All rights reserved.
+SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define INDEXPORT_TO_ADDRESS(x) (x)
+#define DATAPORT_TO_ADDRESS(x)  ((x) << 16)
+#define PORTWIDTH_TO_ADDRESS(x) ((x) << 32)
+#define PORTBITNUMBER_TO_ADDRESS(x) ((x) << 40)
+#define PORTINDEXNUMBER_TO_ADDRESS(x)   ((x) << 48)
+
+//
+// Internal
+//
+#pragma pack (1)
+
+typedef struct {
+  BIOS_INFO_HEADER  Header;
+  BIOS_INFO_STRUCT  Entry[1];
+} BIOS_INFO;
+#pragma pack ()
+
+GLOBAL_REMOVE_IF_UNREFERENCED BIOS_INFO  mBiosInfo = {
+  {
+BIOS_INFO_SIGNATURE,
+1,
+0,
+  },
+  {
+{
+  FIT_TYPE_01_MICROCODE,
+  BIOS_INFO_STRUCT_ATTRIBUTE_MICROCODE_WHOLE_REGION,
+  0x0100,
+  FixedPcdGet32 (PcdFlashMicrocodeFvSize),
+  FixedPcdGet32 (PcdFlashMicrocodeFvBase)
+}
+  }
+};
+
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_PPI_DESCRIPTOR  mBiosInfoPpiList = {
+  EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
+  ,
+  
+};
+
+/**
+  Installs BiosInfo Ppi.
+
+  @param  FileHandle  Handle of the file being invoked.
+  @param  PeiServices Describes the list of possible PEI Services.
+
+  @retval EFI_SUCCESS   Install the BiosInfo Ppi successfully.
+
+**/
+EFI_STATUS
+EFIAPI
+BiosInfoEntryPoint (
+  IN   EFI_PEI_FILE_HANDLE  FileHandle,
+  IN CONST EFI_PEI_SERVICES **PeiServices
+  )
+{
+  EFI_STATUS  Status;
+  VOID*HobData;
+
+  //
+  // Install PPI, so that other PEI module can add dependency.
+  //
+  Status = PeiServicesInstallPpi ();
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // Build hob, so that DXE module can also get the data.
+  //
+  HobData = BuildGuidHob (, sizeof (mBiosInfo));
+  ASSERT (HobData != NULL);
+  if (HobData == NULL) {
+return EFI_OUT_OF_RESOURCES;
+  }
+  CopyMem (HobData, , sizeof (mBiosInfo));
+
+  return EFI_SUCCESS;
+}
diff --git 
a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/BiosInfo/BiosInfo.inf 
b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/BiosInfo/BiosInfo.inf
new file mode 100644
index 00..a0e18cff9c
--- /dev/null
+++ b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/BiosInfo/BiosInfo.inf
@@ -0,0 +1,48 @@
+### @file
+#  Module Information description file for BIOS Info Driver
+#
+#  Copyright (c) 2019, Intel Corporation. All rights reserved.
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+###
+
+[Defines]
+  INF_VERSION= 0x00010017
+  BASE_NAME  = BiosInfo
+  FILE_GUID  = 3132E669-D16B-4AA7-B09B-BC0EB5F40E1F
+  VERSION_STRING = 1.0
+  MODULE_TYPE= PEIM
+  ENTRY_POINT= BiosInfoEntryPoint
+#
+# The following information is for reference only and not required by the 
build tools.
+#
+# VALID_ARCHITECTURES IA32 X64
+#
+
+[LibraryClasses]
+  PeimEntryPoint
+  PeiServicesLib
+  HobLib
+  BaseMemoryLib
+  DebugLib
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  IntelSiliconPkg/IntelSiliconPkg.dec
+  KabylakeSiliconPkg/SiPkg.dec
+  KabylakeFspBinPkg/KabylakeFspBinPkg.dec
+  BoardModulePkg/BoardModulePkg.dec
+  MinPlatformPkg/MinPlatformPkg.dec
+
+[Pcd]
+  gSiPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase## CONSUMES
+  gSiPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize## CONSUMES
+
+[Sources]
+  BiosInfo.c
+
+[Guids]
+  gBiosInfoGuid ## PRODUCES
+
+[Depex]
+  TRUE
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc 
b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc
index d67e0cc000..64e25168c7 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc
+++ b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc
@@ -244,6 +244,8 @@
   }
   

Re: [edk2-devel] [PATCH v2 3/7] BaseTools: strip trailing whitespace

[Michael D Kinney]

Leif,

Reviewed-by: Michael D Kinney 

I am covering for Liming and Bob this week, so you do not
have to wait for a review from them to push these changes.

Mike

> -Original Message-
> From: devel@edk2.groups.io  On
> Behalf Of Leif Lindholm
> Sent: Tuesday, October 1, 2019 5:49 AM
> To: devel@edk2.groups.io
> Cc: Feng, Bob C ; Gao, Liming
> 
> Subject: [edk2-devel] [PATCH v2 3/7] BaseTools: strip
> trailing whitespace
> 
> Cc: Bob Feng 
> Cc: Liming Gao 
> Signed-off-by: Leif Lindholm 
> ---
> 
> Resubmitting a v2 with changes to external projects:
> - BrotliCompress
> - LzmaCompress
> - Pccts
> backed out for now.
> 
> BaseTools/Source/C/GNUmakefile |
> 2 +-
>  BaseTools/Source/C/Makefiles/app.makefile
> | 4 ++--
>  BaseTools/Source/C/Makefiles/footer.makefile
> | 4 ++--
>  BaseTools/Source/C/Makefiles/header.makefile
> | 8 
>  BaseTools/Source/C/Makefiles/lib.makefile
> | 2 +-
>  BaseTools/Source/C/Makefiles/ms.common
> | 4 ++--
>  BaseTools/Source/C/VfrCompile/GNUmakefile
> | 6 +++---
>  BaseTools/Source/Python/Ecc/Check.py
> | 2 +-
>  BaseTools/Source/Python/GenFds/GenFdsGlobalVariable.py
> | 2 +-
>  BaseTools/Source/Python/Makefile
> | 2 +-
>  10 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/BaseTools/Source/C/GNUmakefile
> b/BaseTools/Source/C/GNUmakefile index
> 37bcce519c7e..df4eb64ea95e 100644
> --- a/BaseTools/Source/C/GNUmakefile
> +++ b/BaseTools/Source/C/GNUmakefile
> @@ -77,7 +77,7 @@ $(SUBDIRS):
>  $(patsubst %,%-clean,$(sort $(SUBDIRS))):
>   -$(MAKE) -C $(@:-clean=) clean
> 
> -$(VFRAUTOGEN): VfrCompile/VfrSyntax.g
> +$(VFRAUTOGEN): VfrCompile/VfrSyntax.g
>   $(MAKE) -C VfrCompile VfrLexer.h
> 
>  clean:  $(patsubst %,%-clean,$(sort $(SUBDIRS))) diff -
> -git a/BaseTools/Source/C/Makefiles/app.makefile
> b/BaseTools/Source/C/Makefiles/app.makefile
> index fcadb4ed2194..6a2a8f5e8a0e 100644
> --- a/BaseTools/Source/C/Makefiles/app.makefile
> +++ b/BaseTools/Source/C/Makefiles/app.makefile
> @@ -12,9 +12,9 @@ include
> $(MAKEROOT)/Makefiles/header.makefile
>  APPLICATION = $(MAKEROOT)/bin/$(APPNAME)
> 
>  .PHONY:all
> -all: $(MAKEROOT)/bin $(APPLICATION)
> +all: $(MAKEROOT)/bin $(APPLICATION)
> 
> -$(APPLICATION): $(OBJECTS)
> +$(APPLICATION): $(OBJECTS)
>   $(LINKER) -o $(APPLICATION) $(BUILD_LFLAGS)
> $(OBJECTS) -L$(MAKEROOT)/libs $(LIBS)
> 
>  $(OBJECTS): $(MAKEROOT)/Include/Common/BuildVersion.h
> diff --git
> a/BaseTools/Source/C/Makefiles/footer.makefile
> b/BaseTools/Source/C/Makefiles/footer.makefile
> index e823246cfbb4..85c3374224f2 100644
> --- a/BaseTools/Source/C/Makefiles/footer.makefile
> +++ b/BaseTools/Source/C/Makefiles/footer.makefile
> @@ -14,10 +14,10 @@ $(MAKEROOT)/libs-$(HOST_ARCH):
>  install: $(MAKEROOT)/libs-$(HOST_ARCH) $(LIBRARY)
>   cp $(LIBRARY) $(MAKEROOT)/libs-$(HOST_ARCH)
> 
> -$(LIBRARY): $(OBJECTS)
> +$(LIBRARY): $(OBJECTS)
>   $(BUILD_AR) crs $@ $^
> 
> -%.o : %.c
> +%.o : %.c
>   $(BUILD_CC)  -c $(BUILD_CPPFLAGS) $(BUILD_CFLAGS) $<
> -o $@
> 
>  %.o : %.cpp
> diff --git
> a/BaseTools/Source/C/Makefiles/header.makefile
> b/BaseTools/Source/C/Makefiles/header.makefile
> index 52cbffcb4423..4e9b36d98bdb 100644
> --- a/BaseTools/Source/C/Makefiles/header.makefile
> +++ b/BaseTools/Source/C/Makefiles/header.makefile
> @@ -61,7 +61,7 @@ else
>  $(error Bad HOST_ARCH)
>  endif
> 
> -INCLUDE = $(TOOL_INCLUDE) -I $(MAKEROOT) -I
> $(MAKEROOT)/Include/Common -I $(MAKEROOT)/Include/ -I
> $(MAKEROOT)/Include/IndustryStandard -I
> $(MAKEROOT)/Common/ -I .. -I . $(ARCH_INCLUDE)
> +INCLUDE = $(TOOL_INCLUDE) -I $(MAKEROOT) -I
> $(MAKEROOT)/Include/Common
> +-I $(MAKEROOT)/Include/ -I
> $(MAKEROOT)/Include/IndustryStandard -I
> +$(MAKEROOT)/Common/ -I .. -I . $(ARCH_INCLUDE)
>  BUILD_CPPFLAGS = $(INCLUDE)
> 
>  # keep EXTRA_OPTFLAGS last
> @@ -82,7 +82,7 @@ BUILD_CXXFLAGS = -Wno-unused-result
> 
>  ifeq ($(HOST_ARCH), IA32)
>  #
> -# Snow Leopard  is a 32-bit and 64-bit environment.
> uname -m returns i386, but gcc defaults
> +# Snow Leopard  is a 32-bit and 64-bit environment.
> uname -m returns
> +i386, but gcc defaults
>  #  to x86_64. So make sure tools match uname -m. You
> can manual have a 64-bit kernal on Snow Leopard  #  so
> only do this is uname -m returns i386.
>  #
> @@ -96,7 +96,7 @@ endif
>  # keep BUILD_OPTFLAGS last
>  BUILD_CFLAGS   += $(BUILD_OPTFLAGS)
>  BUILD_CXXFLAGS += $(BUILD_OPTFLAGS)
> -
> +
>  # keep EXTRA_LDFLAGS last
>  BUILD_LFLAGS += $(EXTRA_LDFLAGS)
> 
> @@ -107,7 +107,7 @@ BUILD_LFLAGS += $(EXTRA_LDFLAGS)
>  all:
> 
>  $(MAKEROOT)/libs:
> - mkdir $(MAKEROOT)/libs
> + mkdir $(MAKEROOT)/libs
> 
>  $(MAKEROOT)/bin:
>   mkdir $(MAKEROOT)/bin
> diff --git a/BaseTools/Source/C/Makefiles/lib.makefile
> b/BaseTools/Source/C/Makefiles/lib.makefile
> index a9965fc628d9..2577c15380a9 100644
> --- a/BaseTools/Source/C/Makefiles/lib.makefile
> +++ b/BaseTools/Source/C/Makefiles/lib.makefile
> @@ -9,6 +9,6 @@ include
> $(MAKEROOT)/Makefiles/header.makefile

Re: [edk2-devel] [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache support

[Kubacki, Michael A]

I understand the concern. I'm aware code is sometimes marked as runtime 
compatible
though it is actually not and meeting all the criteria to work in the runtime 
environment
is non-trivial as you noted.

In response to series #2 patch #8, Andrew noted that the UEFI spec defines 
restrictions
on Runtime Services callers that prohibit re-entry into the GetVariable () and
GetNextVariableName () services. With this restriction in mind, I believe the 
polling can be
removed and the lock simply serve as an indicator to SMM whether a SMI that 
invokes
SetVariable () interrupted a runtime read.

Thanks,
Michael

> -Original Message-
> From: Laszlo Ersek 
> Sent: Thursday, October 3, 2019 4:01 AM
> To: Wu, Hao A ; Kubacki, Michael A
> ; devel@edk2.groups.io
> Cc: Bi, Dandan ; Ard Biesheuvel
> ; Dong, Eric ; Gao, Liming
> ; Kinney, Michael D ;
> Ni, Ray ; Wang, Jian J ; Yao,
> Jiewen 
> Subject: Re: [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable()
> cache support
> 
> On 10/03/19 10:04, Wu, Hao A wrote:
> > Before any comment on the patch, since I am not experienced in the
> > Variable driver, I would like to ask for help from other reviewers to
> > look into this patch and provide feedbacks as well. Thanks in advance.
> >
> > With the above fact, some comments provided below maybe wrong. So
> > please help to kindly correct me.
> >
> >
> > Some general comments:
> > 1. I am not sure if bringing the TimerLib dependency (delay in acquiring the
> >runtime cache read lock) to variable driver (a software driver for the 
> > most
> >part) is a good idea.
> 
> I agree. Most TimerLib instances do not expect sharing the hardware with
> the OS.
> 
> 
> Another complication is if the hardware is accessed via MMIO (that is, not IO
> ports). MMIO accesses are subject to page tables.
> 
> Assuming that MicroSecondDelay() is invoked from the runtime DXE driver at
> OS runtime, a platform would have to expose the MMIO area of the timer
> hardware in the UEFI memory map as "runtime MMIO". (Via GCD memory
> space operations in a platform driver or in the TimerLib constructor.)
> 
> Furthermore, the constructor function of the TimerLib instance would have
> to register a VirtualAddressChange event handler, and convert the MMIO
> address.
> 
> Thanks
> Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48452): https://edk2.groups.io/g/devel/message/48452
Mute This Topic: https://groups.io/mt/34318592/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V2 8/9] MdeModulePkg/Variable: Add RT GetNextVariableName() cache support

[Kubacki, Michael A]

Thanks Andrew, I had not seen that before.

From: devel@edk2.groups.io  On Behalf Of Andrew Fish via 
Groups.Io
Sent: Thursday, October 3, 2019 12:00 PM
To: devel@edk2.groups.io; Kubacki, Michael A 
Cc: Wu, Hao A ; 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 8/9] MdeModulePkg/Variable: Add RT 
GetNextVariableName() cache support



On Oct 3, 2019, at 1:52 PM, Kubacki, Michael A 
mailto:michael.a.kuba...@intel.com>> wrote:

-Done:
+  mVariableRuntimeCacheReadLock = FALSE;


Similar to the previous patch (7/9),
if timeout occurs when acquiring the read lock, should this flag be set to
FALSE in such case?

Given that the runtime service can be invoked in a multi-threaded OS 
environment,
it is possible that one thread could be stuck with the lock while another 
thread times
out waiting to acquire the lock. In that case, I believe the global lock should 
not be
released. I can move setting the flag to FALSE within the same conditional 
block in
which it is set.

UEFI Spec sets the rules is 8.1 ...

All callers of Runtime Services are restricted from calling the same or certain 
other Runtime Service functions prior to the completion and return of a 
previous Runtime Service call. These restrictions apply to:
• Runtime Services that have been interrupted
• Runtime Services that are active on another processor.
Callers are prohibited from using certain other services from another processor 
or on the same processor following an interrupt as specified in Table 35. For 
this table ‘Busy’ is defined as the state when a Runtime Service has been 
entered and has not returned to the caller.
The consequence of a caller violating these restrictions is undefined except 
for certain special cases described below.

Table 35 variable info:
If previous call is busy in:
GetVariable() GetNextVariableName() SetVariable() QueryVariableInfo() 
UpdateCapsule() QueryCapsuleCapabilities() GetNextHighMonotonicCount()

Forbidden to call:
GetVariable(), GetNextVariableName(), SetVariable(), QueryVariableInfo(), 
UpdateCapsule(), QueryCapsuleCapabilities(), GetNextHighMonotonicCount()

Thanks,

Andrew Fish

Thanks,
Michael

-Original Message-
From: Wu, Hao A mailto:hao.a...@intel.com>>
Sent: Thursday, October 3, 2019 1:05 AM
To: Kubacki, Michael A 
mailto:michael.a.kuba...@intel.com>>;
devel@edk2.groups.io
Cc: Bi, Dandan mailto:dandan...@intel.com>>; Ard Biesheuvel
mailto:ard.biesheu...@linaro.org>>; Dong, Eric 
mailto:eric.d...@intel.com>>; Laszlo Ersek
mailto:ler...@redhat.com>>; Gao, Liming 
mailto:liming@intel.com>>; Kinney, Michael
D mailto:michael.d.kin...@intel.com>>; Ni, Ray 
mailto:ray...@intel.com>>; Wang, Jian J
mailto:jian.j.w...@intel.com>>; Yao, Jiewen 
mailto:jiewen@intel.com>>
Subject: RE: [PATCH V2 8/9] MdeModulePkg/Variable: Add RT
GetNextVariableName() cache support

-Original Message-
From: 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, Liming;
Kinney, Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
Subject: [PATCH V2 8/9] MdeModulePkg/Variable: Add RT
GetNextVariableName() cache support

https://bugzilla.tianocore.org/show_bug.cgi?id=2220

This change implements the Runtime Service GetNextVariableName()
using
the Runtime Cache in VariableSmmRuntimeDxe. Runtime Service calls to
GetNextVariableName() will no longer trigger a SW SMI.

Overall system performance and stability will be improved by
eliminating an SMI for these calls as they typically result in a
relatively large number of invocations to retrieve all variable names
in all variable stores present.

Cc: Dandan Bi mailto:dandan...@intel.com>>
Cc: Ard Biesheuvel mailto:ard.biesheu...@linaro.org>>
Cc: Eric Dong mailto:eric.d...@intel.com>>
Cc: Laszlo Ersek mailto:ler...@redhat.com>>
Cc: Liming Gao mailto:liming@intel.com>>
Cc: Michael D Kinney 
mailto:michael.d.kin...@intel.com>>
Cc: Ray Ni mailto:ray...@intel.com>>
Cc: Jian J Wang mailto:jian.j.w...@intel.com>>
Cc: Hao A Wu mailto:hao.a...@intel.com>>
Cc: Jiewen Yao mailto:jiewen@intel.com>>
Signed-off-by: Michael Kubacki 
mailto:michael.a.kuba...@intel.com>>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe
.c | 118 +---
1 file changed, 50 insertions(+), 68 deletions(-)

diff --git
a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
xe.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
xe.c
index 46f69765a4..bc3b56b0ce 100644
---
a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
xe.c
+++
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
xe.c
@@ -799,87 +799,69 @@ RuntimeServiceGetNextVariableName (
  IN OUT  EFI_GUID  *VendorGuid
  )
{
-  EFI_STATUS

Re: [edk2-devel] [PATCH V2 8/9] MdeModulePkg/Variable: Add RT GetNextVariableName() cache support

[Andrew Fish via Groups.Io]

> On Oct 3, 2019, at 1:52 PM, Kubacki, Michael A  
> wrote:
> 
>>> -Done:
>>> +  mVariableRuntimeCacheReadLock = FALSE;
>> 
>> 
>> Similar to the previous patch (7/9),
>> if timeout occurs when acquiring the read lock, should this flag be set to
>> FALSE in such case?
>> 
> 
> Given that the runtime service can be invoked in a multi-threaded OS 
> environment,
> it is possible that one thread could be stuck with the lock while another 
> thread times
> out waiting to acquire the lock. In that case, I believe the global lock 
> should not be
> released. I can move setting the flag to FALSE within the same conditional 
> block in
> which it is set.
> 

UEFI Spec sets the rules is 8.1 ...

All callers of Runtime Services are restricted from calling the same or certain 
other Runtime Service functions prior to the completion and return of a 
previous Runtime Service call. These restrictions apply to:
• Runtime Services that have been interrupted
• Runtime Services that are active on another processor.
Callers are prohibited from using certain other services from another processor 
or on the same processor following an interrupt as specified in Table 35. For 
this table ‘Busy’ is defined as the state when a Runtime Service has been 
entered and has not returned to the caller.
The consequence of a caller violating these restrictions is undefined except 
for certain special cases described below.

Table 35 variable info:
If previous call is busy in:
GetVariable() GetNextVariableName() SetVariable() QueryVariableInfo() 
UpdateCapsule() QueryCapsuleCapabilities() GetNextHighMonotonicCount()

Forbidden to call:
GetVariable(), GetNextVariableName(), SetVariable(), QueryVariableInfo(), 
UpdateCapsule(), QueryCapsuleCapabilities(), GetNextHighMonotonicCount()

Thanks,

Andrew Fish

> Thanks,
> Michael
> 
>> -Original Message-
>> From: Wu, Hao A mailto:hao.a...@intel.com>>
>> Sent: Thursday, October 3, 2019 1:05 AM
>> To: Kubacki, Michael A > >;
>> devel@edk2.groups.io 
>> Cc: Bi, Dandan mailto:dandan...@intel.com>>; Ard 
>> Biesheuvel
>> mailto:ard.biesheu...@linaro.org>>; Dong, Eric 
>> mailto:eric.d...@intel.com>>; Laszlo Ersek
>> mailto:ler...@redhat.com>>; Gao, Liming 
>> mailto:liming@intel.com>>; Kinney, Michael
>> D mailto:michael.d.kin...@intel.com>>; Ni, Ray 
>> mailto:ray...@intel.com>>; Wang, Jian J
>> mailto:jian.j.w...@intel.com>>; Yao, Jiewen 
>> mailto:jiewen@intel.com>>
>> Subject: RE: [PATCH V2 8/9] MdeModulePkg/Variable: Add RT
>> GetNextVariableName() cache support
>> 
>>> -Original Message-
>>> From: 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, Liming;
>>> Kinney, Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
>>> Subject: [PATCH V2 8/9] MdeModulePkg/Variable: Add RT
>>> GetNextVariableName() cache support
>>> 
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=2220
>>> 
>>> This change implements the Runtime Service GetNextVariableName()
>> using
>>> the Runtime Cache in VariableSmmRuntimeDxe. Runtime Service calls to
>>> GetNextVariableName() will no longer trigger a SW SMI.
>>> 
>>> Overall system performance and stability will be improved by
>>> eliminating an SMI for these calls as they typically result in a
>>> relatively large number of invocations to retrieve all variable names
>>> in all variable stores present.
>>> 
>>> 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/VariableSmmRuntimeDxe
>>> .c | 118 +---
>>> 1 file changed, 50 insertions(+), 68 deletions(-)
>>> 
>>> diff --git
>>> 
>> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
>>> xe.c
>>> 
>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
>>> xe.c
>>> index 46f69765a4..bc3b56b0ce 100644
>>> ---
>>> 
>> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
>>> xe.c
>>> +++
>>> 
>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
>>> xe.c
>>> @@ -799,87 +799,69 @@ RuntimeServiceGetNextVariableName (
>>>   IN OUT  EFI_GUID  *VendorGuid
>>>   )
>>> {
>>> -  EFI_STATUS  Status;
>>> -  UINTN   PayloadSize;
>>> -  SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME
>>> *SmmGetNextVariableName;
>>> -  UINTN   OutVariableNameSize;
>>> -  UINTN   InVariableNameSize;
>>> +  EFI_STATUS  Status;
>>> +  UINTN   DelayIndex;
>>> +  UINTN   MaxLen;
>>> +  UINTN   VarNameSize;

Re: [edk2-devel] [PATCH V2 8/9] MdeModulePkg/Variable: Add RT GetNextVariableName() cache support

[Kubacki, Michael A]

> > -Done:
> > +  mVariableRuntimeCacheReadLock = FALSE;
> 
> 
> Similar to the previous patch (7/9),
> if timeout occurs when acquiring the read lock, should this flag be set to
> FALSE in such case?
>

Given that the runtime service can be invoked in a multi-threaded OS 
environment,
it is possible that one thread could be stuck with the lock while another 
thread times
out waiting to acquire the lock. In that case, I believe the global lock should 
not be
released. I can move setting the flag to FALSE within the same conditional 
block in
which it is set.

Thanks,
Michael

> -Original Message-
> From: Wu, Hao A 
> Sent: Thursday, October 3, 2019 1:05 AM
> 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: [PATCH V2 8/9] MdeModulePkg/Variable: Add RT
> GetNextVariableName() cache support
> 
> > -Original Message-
> > From: 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, Liming;
> > Kinney, Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> > Subject: [PATCH V2 8/9] MdeModulePkg/Variable: Add RT
> > GetNextVariableName() cache support
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=2220
> >
> > This change implements the Runtime Service GetNextVariableName()
> using
> > the Runtime Cache in VariableSmmRuntimeDxe. Runtime Service calls to
> > GetNextVariableName() will no longer trigger a SW SMI.
> >
> > Overall system performance and stability will be improved by
> > eliminating an SMI for these calls as they typically result in a
> > relatively large number of invocations to retrieve all variable names
> > in all variable stores present.
> >
> > 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/VariableSmmRuntimeDxe
> > .c | 118 +---
> >  1 file changed, 50 insertions(+), 68 deletions(-)
> >
> > diff --git
> >
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
> > xe.c
> >
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
> > xe.c
> > index 46f69765a4..bc3b56b0ce 100644
> > ---
> >
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
> > xe.c
> > +++
> >
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
> > xe.c
> > @@ -799,87 +799,69 @@ RuntimeServiceGetNextVariableName (
> >IN OUT  EFI_GUID  *VendorGuid
> >)
> >  {
> > -  EFI_STATUS  Status;
> > -  UINTN   PayloadSize;
> > -  SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME
> > *SmmGetNextVariableName;
> > -  UINTN   OutVariableNameSize;
> > -  UINTN   InVariableNameSize;
> > +  EFI_STATUS  Status;
> > +  UINTN   DelayIndex;
> > +  UINTN   MaxLen;
> > +  UINTN   VarNameSize;
> > +  VARIABLE_HEADER *VariablePtr;
> > +  VARIABLE_STORE_HEADER
> > *VariableStoreHeader[VariableStoreTypeMax];
> > +
> > +  Status = EFI_NOT_FOUND;
> >
> >if (VariableNameSize == NULL || VariableName == NULL || VendorGuid
> > ==
> > NULL) {
> >  return EFI_INVALID_PARAMETER;
> >}
> >
> > -  OutVariableNameSize   = *VariableNameSize;
> > -  InVariableNameSize= StrSize (VariableName);
> > -  SmmGetNextVariableName = NULL;
> > -
> >//
> > -  // If input string exceeds SMM payload limit. Return failure
> > +  // Calculate the possible maximum length of name string, including
> > + the
> > Null terminator.
> >//
> > -  if (InVariableNameSize > mVariableBufferPayloadSize - OFFSET_OF
> > (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name))
> {
> > +  MaxLen = *VariableNameSize / sizeof (CHAR16);  if ((MaxLen == 0) ||
> > + (StrnLenS (VariableName, MaxLen) == MaxLen)) {
> > +//
> > +// Null-terminator is not found in the first VariableNameSize
> > + bytes of the
> > input VariableName buffer,
> > +// follow spec to return EFI_INVALID_PARAMETER.
> > +//
> >  return EFI_INVALID_PARAMETER;
> >}
> >
> > -  AcquireLockOnlyAtBootTime();
> > +  AcquireLockOnlyAtBootTime ();
> >
> > -  //
> > -  // Init the communicate buffer. The buffer data size is:
> > -  // SMM_COMMUNICATE_HEADER_SIZE +
> > SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize.
> > -  //
> > -  if (OutVariableNameSize > mVariableBufferPayloadSize - OFFSET_OF
> > (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name))
> {
> > -//
> > -// If output buffer exceed SMM payload limit. Trim output buffer 

Re: [edk2-devel] [PATCH V2 5/9] MdeModulePkg/Variable: Add a file for NV variable functions

[Kubacki, Michael A]

I was debating on keeping this file in the patch series. I don't see a problem 
moving
those other functions. The goal was to break out some of the NV-specific content
from many of the other more generic functions in Variable.c Since you mentioned 
it,
I will make that change in V3, unless I hear back otherwise.

Thanks,
Michael

> -Original Message-
> From: Wu, Hao A 
> Sent: Thursday, October 3, 2019 1:04 AM
> 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: [PATCH V2 5/9] MdeModulePkg/Variable: Add a file for NV
> variable functions
> 
> > -Original Message-
> > From: 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, Liming;
> > Kinney, Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> > Subject: [PATCH V2 5/9] MdeModulePkg/Variable: Add a file for NV
> > variable functions
> >
> > This change adds a dedicated file for variable operations specific to
> > non-volatile variables. This decreases the overall length of the
> > relatively large Variable.c file.
> 
> 
> It is not clear to me what are the criteria for moving functions into the
> separate new file.
> 
> I guess the new file is for functions related with NV variables, but I saw 
> there
> are functions like:
> 
> InitRealNonVolatileVariableStore
> InitEmuNonVolatileVariableStore
> InitNonVolatileVariableStore
> 
> Not sure if they can be put into the new file as well.
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > 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/VariableRuntimeDxe.inf
> > |  2 ++
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf  |
> 2
> > ++
> >
> >
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
> > |  2 ++
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.h
> |
> > 25 +
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c   | 20 
> > +--
> --
> > -
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c
> |
> > 28 
> >  6 files changed, 60 insertions(+), 19 deletions(-)
> >
> > diff --git
> >
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> >
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> > index c35e5fe787..08a5490787 100644
> > ---
> >
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> > +++
> >
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> > @@ -36,6 +36,8 @@
> >Variable.c
> >VariableDxe.c
> >Variable.h
> > +  VariableNonVolatile.c
> > +  VariableNonVolatile.h
> >VariableParsing.c
> >VariableParsing.h
> >PrivilegePolymorphic.h
> > diff --git
> > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> > index 626738b9c7..6dc2721b81 100644
> > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> > @@ -45,6 +45,8 @@
> >Variable.c
> >VariableTraditionalMm.c
> >VariableSmm.c
> > +  VariableNonVolatile.c
> > +  VariableNonVolatile.h
> >VariableParsing.c
> >VariableParsing.h
> >VarCheck.c
> > diff --git
> >
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.i
> > nf
> >
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.
> > inf
> > index 1ba8f9ebfb..ca9d23ce9f 100644
> > ---
> >
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.i
> > nf
> > +++
> >
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.
> > inf
> > @@ -45,6 +45,8 @@
> >Variable.c
> >VariableSmm.c
> >VariableStandaloneMm.c
> > +  VariableNonVolatile.c
> > +  VariableNonVolatile.h
> >VariableParsing.c
> >VariableParsing.h
> >VarCheck.c
> > diff --git
> > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.h
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.h
> > new file mode 100644
> > index 00..82572262ef
> > --- /dev/null
> > +++
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.h
> > @@ -0,0 +1,25 @@
> > +/** @file
> > +  Common variable non-volatile store routines.
> > +
> > +Copyright (c) 2019, Intel Corporation. All rights reserved.
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#ifndef _VARIABLE_NON_VOLATILE_H_
> > +#define _VARIABLE_NON_VOLATILE_H_
> > +
> > +#include "Variable.h"
> > +
> > +/**
> > +  Get non-volatile maximum 

Re: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add local auth status in VariableParsing

[Kubacki, Michael A]

I will make the following changes in V3:

> InitVariableParsing() seems an internal function, the 'EFIAPI' keyword can be
> dropped. Please help to update the function definition in .C file as well.

I will remove the EFIAPI keyword.

> > 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 = PcdGet32
> > (PcdMaxVariableSize);
> >mVariableModuleGlobal->MaxAuthVariableSize = ((PcdGet32
> > (PcdMaxAuthVariableSize) != 0) ? PcdGet32 (PcdMaxAuthVariableSize) :
> > mVariableModuleGlobal->MaxVariableSize);
> >
> > +  Status = 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 = FALSE;
>   ...
>   //
>   // Restore AuthFormat
>   //
>   mVariableModuleGlobal->VariableGlobal.AuthFormat = 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'.
> 
>

You're right that will be a problem. I missed this temporary change in the 
value.
I'm going to have all the functions dependent on authentication status in
VariableParsing.c take it as a parameter and let the respective drivers linked
against it maintain their own single copy of the authentication state.

> >//
> >// 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 variable
> > format!\n"));
> > -//
> > -// Set AuthSupport to FALSE first, VariableWriteServiceInitialize() 
> > will
> > initialize it.
> > -//
> > -mVariableModuleGlobal->VariableGlobal.AuthSupport = FALSE;
> >  VariableGuid = 
> >} else {
> >  DEBUG ((EFI_D_INFO, "Variable driver will work without auth
> > variable support!\n"));
> > -mVariableModuleGlobal->VariableGlobal.AuthSupport = FALSE;
> 
> 
> Not sure why the above changes belong to this patch.
> Could you help to double confirm?

This was used during testing and is not needed. I will remove it.

Thanks,
Michael

> -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, Liming;
> > Kinney, Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> > Subject: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add local
> > 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 status
> > 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: 

Re: [edk2-devel] [PATCH V2 3/9] MdeModulePkg/Variable: Parameterize VARIABLE_INFO_ENTRY buffer

[Kubacki, Michael A]

Your understanding is correct.

Thanks,
Michael

> -Original Message-
> From: Wu, Hao A 
> Sent: Thursday, October 3, 2019 1:04 AM
> 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: [PATCH V2 3/9] MdeModulePkg/Variable: Parameterize
> VARIABLE_INFO_ENTRY buffer
> 
> > -Original Message-
> > From: 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, Liming;
> > Kinney, Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> > Subject: [PATCH V2 3/9] MdeModulePkg/Variable: Parameterize
> > VARIABLE_INFO_ENTRY buffer
> >
> > UpdateVariableInfo () currently accepts parameters regarding updates
> > to be made to a global variable of type VARIABLE_INFO_ENTRY. This
> > change passes the structure by pointer to UpdateVariableInfo () so
> > structures can be updated outside the fixed global variable.
> 
> 
> For:
> "... so structures can be updated outside the fixed global variable "
> 
> Do you mean:
> 
> VARIABLE_INFO_ENTRY structure pointers other than "" can
> be passed to UpdateVariableInfo().
> 
> Is my understanding correct? If so,
> Reviewed-by: Hao A Wu 
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > 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 | 18
> > +
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c| 14 +++---
> -
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c | 41
> > +++-
> >  3 files changed, 39 insertions(+), 34 deletions(-)
> >
> > diff --git
> > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> > index 0d231511ea..6f2000f3ee 100644
> > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> > @@ -286,13 +286,14 @@ GetNextVariableEx (
> >the transaction. Data is allocated by this routine, but never
> >freed.
> >
> > -  @param[in] VariableName   Name of the Variable to track.
> > -  @param[in] VendorGuid Guid of the Variable to track.
> > -  @param[in] Volatile   TRUE if volatile FALSE if non-volatile.
> > -  @param[in] Read   TRUE if GetVariable() was called.
> > -  @param[in] Write  TRUE if SetVariable() was called.
> > -  @param[in] Delete TRUE if deleted via SetVariable().
> > -  @param[in] Cache  TRUE for a cache hit.
> > +  @param[in]  VariableName   Name of the Variable to track.
> > +  @param[in]  VendorGuid Guid of the Variable to track.
> > +  @param[in]  Volatile   TRUE if volatile FALSE if non-volatile.
> > +  @param[in]  Read   TRUE if GetVariable() was called.
> > +  @param[in]  Write  TRUE if SetVariable() was called.
> > +  @param[in]  Delete TRUE if deleted via SetVariable().
> > +  @param[in]  Cache  TRUE for a cache hit.
> > +  @param[in,out]  VariableInfo   Pointer to a pointer of
> > VARIABLE_INFO_ENTRY structures.
> >
> >  **/
> >  VOID
> > @@ -303,7 +304,8 @@ UpdateVariableInfo (
> >IN  BOOLEAN Read,
> >IN  BOOLEAN Write,
> >IN  BOOLEAN Delete,
> > -  IN  BOOLEAN Cache
> > +  IN  BOOLEAN Cache,
> > +  IN OUT VARIABLE_INFO_ENTRY  **VariableInfo
> >);
> >
> >  #endif
> > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > index 816e8f7b8f..1a57d7e1ba 100644
> > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > @@ -1641,7 +1641,7 @@ UpdateVariable (
> >  // go to delete this variable in variable HOB and
> >  // try to flush other variables from HOB to flash.
> >  //
> > -UpdateVariableInfo (VariableName, VendorGuid, FALSE, FALSE,
> FALSE,
> > TRUE, FALSE);
> > +UpdateVariableInfo (VariableName, VendorGuid, FALSE, FALSE,
> > + FALSE,
> > TRUE, FALSE, );
> >  FlushHobVariableToFlash (VariableName, VendorGuid);
> >  return EFI_SUCCESS;
> >}
> > @@ -1758,7 +1758,7 @@ UpdateVariable (
> >   
> >   );
> >if (!EFI_ERROR (Status)) {
> > -UpdateVariableInfo (VariableName, VendorGuid, Variable->Volatile,
> > FALSE, FALSE, TRUE, FALSE);
> > +UpdateVariableInfo (VariableName, VendorGuid,
> > + Variable->Volatile,
> > FALSE, FALSE, TRUE, FALSE, );
> >  

Re: [edk2-devel] [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize GetNextVariableEx() store list

[Kubacki, Michael A]

Thanks for the feedback, I'll rename it back to 
VariableServiceGetNextVariableInternal ()
 and update the comments to refer to FindVariableEx () instead of FindVariable 
() in V3.

Thanks,
Michael

> -Original Message-
> From: Wu, Hao A 
> Sent: Thursday, October 3, 2019 1:03 AM
> 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: [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize
> GetNextVariableEx() store list
> 
> A couple of inline comments below:
> 
> 
> > -Original Message-
> > From: 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, Liming;
> > Kinney, Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> > Subject: [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize
> > GetNextVariableEx() store list
> >
> > The majority of logic related to GetNextVariableName () is currently
> > implemented in VariableServiceGetNextVariableInternal (). The list of
> > variable stores to search for the given variable name and variable
> > GUID is defined in the function body. This change renames the function
> > to GetNextVariableEx () since the function is no longer internal to a
> > specific source file and adds a new parameter so that the caller must
> > pass in the list of variable stores to be used in the variable search.
> 
> 
> I am not sure if 'GetNextVariableEx' is a good name for the function, since:
> 
> 1. There is no function named GetNextVariable(), so it is not clear what "Ex"
>means here.
> 
> 2. The origin function VariableServiceGetNextVariableInternal() does get
> used
>in multiple source files (Variable.c & VariableExLib.c). I am not sure what
>is the intention for such renaming.
> 
> 
> >
> > 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 | 15
> ++-
> > -
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c| 12 ++-
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c   |  8 +-
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c | 78
> > 
> >  4 files changed, 73 insertions(+), 40 deletions(-)
> >
> > diff --git
> > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> > index 9d77c4916c..0d231511ea 100644
> > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> > @@ -248,27 +248,30 @@ FindVariableEx (
> >);
> >
> >  /**
> > -  This code Finds the Next available variable.
> > +  This code finds the next available variable.
> >
> >Caution: This function may receive untrusted input.
> >This function may be invoked in SMM mode. This function will do
> > basic validation, before parse the data.
> >
> > -  @param[in]  VariableName  Pointer to variable name.
> > -  @param[in]  VendorGuidVariable Vendor Guid.
> > -  @param[out] VariablePtr   Pointer to variable header address.
> > +  @param[in]  VariableName  Pointer to variable name.
> > +  @param[in]  VendorGuidVariable Vendor Guid.
> > +  @param[in]  VariableStoreList A list of variable stores that should
> > + be used
> > to get the next variable.
> > +The maximum number of entries is the
> > + max value of
> > VARIABLE_STORE_TYPE.
> > +  @param[out] VariablePtr   Pointer to variable header address.
> >
> >@retval EFI_SUCCESS   The function completed successfully.
> >@retval EFI_NOT_FOUND The next variable was not found.
> > -  @retval EFI_INVALID_PARAMETER If VariableName is not an empty
> > string, while VendorGuid is NULL.
> > +  @retval EFI_INVALID_PARAMETER If VariableName is nt an empty
> > + string,
> > while VendorGuid is NULL.
> >@retval EFI_INVALID_PARAMETER The input values of VariableName and
> > VendorGuid are not a name and
> >  GUID of an existing variable.
> >
> >  **/
> >  EFI_STATUS
> >  EFIAPI
> > -VariableServiceGetNextVariableInternal (
> > +GetNextVariableEx (
> >IN  CHAR16*VariableName,
> >IN  EFI_GUID  *VendorGuid,
> > +  IN  VARIABLE_STORE_HEADER **VariableStoreList,
> >OUT VARIABLE_HEADER   **VariablePtr
> >);
> >
> > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > index 76536308e6..816e8f7b8f 100644
> > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > +++ 

Re: [edk2-devel] [platforms/devel-riscv-v2 PATCHv3 00/13] Updates to sync-up with

[Leif Lindholm]

On Mon, Sep 23, 2019 at 11:40:24AM +0800, Abner Chang wrote:
> This set of patches is updated to incorporate with patches sent for
> edk2-staging/RISC-V-V2 PATCH v2.
> 
> Abner Chang (13):
>   U500Pkg/OpenSbiPlatformLib: Use Fdtlib in EmbeddedPkg
>   U500Pkg/Sec: Remove unnecessary PCD reference
>   U500Pkg/Sec: Add information to header file references
>   U500Pkg/SerialIoLib: Header file reference change
>   U500Pkg: Update DEC revision
>   RiscV :Update INF revision
>   Silicon/SiFive: Update INF revision
>   Silicon/SiFive :Update DEC revision
>   U500Pkg/riscVPlatformTimerLib: Change source code to *.S
>   RiscV/Sec: Change source code to *.S
>   U500Pkg/SerialIoLib: Add copyrights
>   RiscV: Update DEC revision
>   U500Pkg: Leverage EmbeddedPkg modules

Thanks for these. I won't review these individually, but they all look
like things I have commented on either for the edk2 set or the v2 set
out of the edk2-platforms one.

Please merge these into the v4 submission.

Best Regards,

Leif

>  .../FirmwareContextProcessorSpecificLib.inf|   2 +-
>  .../RealTimeClockLibNull/RealTimeClockLibNull.inf  |   2 +-
>  Platform/RiscV/RiscVPlatformPkg.dec|   2 +-
>  .../OpenSbiPlatformLib/OpenSbiPlatformLib.inf  |  10 +-
>  .../U500Pkg/Library/OpenSbiPlatformLib/platform.c  | 242 
> +++--
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf|   2 +-
>  .../PlatformBootManagerLib.inf |   2 +-
>  .../RiscVPlatformTimerLib.inf  |   6 +-
>  .../U500Pkg/Library/SerialIoLib/SerialIoLib.inf|   2 +-
>  .../U500Pkg/Library/SerialIoLib/SerialPortLib.c|   3 +-
>  Platform/RiscV/SiFive/U500Pkg/U500.dec |   2 +-
>  Platform/RiscV/SiFive/U500Pkg/U500.dsc |   5 +-
>  Platform/RiscV/SiFive/U500Pkg/U500.fdf |   2 +-
>  .../FvbServicesRuntimeDxe.inf  |   2 +-
>  .../U500Pkg/Universal/Dxe/TimerDxe/TimerDxe.inf|   2 +-
>  .../Universal/Pei/PlatformPei/PlatformPei.inf  |   2 +-
>  Platform/RiscV/Universal/Sec/SecMain.c |  12 +-
>  Platform/RiscV/Universal/Sec/SecMain.inf   |   7 +-
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf|   2 +-
>  Silicon/SiFive/SiFive.dec  |   2 +-
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf|   2 +-
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf|   2 +-
>  22 files changed, 158 insertions(+), 157 deletions(-)
> 
> -- 
> 2.7.4
> 
> 
> 
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48444): https://edk2.groups.io/g/devel/message/48444
Mute This Topic: https://groups.io/mt/34260839/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 14/14] Platforms: Readme file updates

[Leif Lindholm]

On Thu, Sep 19, 2019 at 11:51:31AM +0800, Gilbert Chen wrote:
> Update Readme.md and Maintainers.txt for RISV-V platforms.
> 
> Signed-off-by: Gilbert Chen 
> ---
>  Maintainers.txt |  9 +
>  Readme.md   | 11 +++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/Maintainers.txt b/Maintainers.txt
> index 876ae561..c494c9d5 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -108,6 +108,11 @@ R: Marcin Wojtas 
>  Platform/SolidRun/Armada80x0McBin
>  R: Marcin Wojtas 
>  
> +Platform/RiscV
> +Platform/RiscV/SiFive/U500Pkg

Since we're on a -devel branch anyway ... could you turn these into F:
entries even though we haven't converted
edk2-platforms/Maintainers.txt to the new format yet?

> +R: Abner Chang 
> +R: Gilbert Chen 
> +
>  Silicon
>  M: Ard Biesheuvel 
>  M: Leif Lindholm 
> @@ -151,3 +156,7 @@ M: Liming Gao 
>  
>  Silicon/Marvell
>  R: Marcin Wojtas 
> +
> +Silicon/SiFive

Same here.

/
Leif

> +R: Abner Chang 
> +R: Gilbert Chen 
> diff --git a/Readme.md b/Readme.md
> index 63e59f60..0395e20a 100644
> --- a/Readme.md
> +++ b/Readme.md
> @@ -52,6 +52,7 @@ ARM | arm-linux-gnueabihf-
>  IA32| i?86-linux-gnu-* _or_ x86_64-linux-gnu-
>  IPF | ia64-linux-gnu
>  X64 | x86_64-linux-gnu-
> +RISCV64 | riscv64-unknown-elf-
>  
>  \* i386, i486, i586 or i686
>  
> @@ -62,6 +63,12 @@ and 
> [arm-linux-gnueabihf](https://releases.linaro.org/components/toolchain/binar
>  compiled to run on x86_64/i686 Linux and i686 Windows. Some Linux 
> distributions
>  provide their own packaged cross-toolchains.
>  
> +### GCC for RISC-V
> +RISC-V open source community provides GCC toolchains for
> +[riscv64-unknown-elf](https://github.com/riscv/riscv-gnu-toolchain)
> +compiled to run on x86 Linux. The commit ID 64879b24 is verified to build 
> RISC-V EDK2 platform and boot to EFI
> +SHELL successfully.
> +
>  ### clang
>  Clang does not require separate cross compilers, but it does need a
>  target-specific binutils. These are included with any prepackaged GCC 
> toolchain
> @@ -243,6 +250,10 @@ For more information, see the
>  ## Raspberry Pi
>  * [Pi 3](Platform/RaspberryPi/RPi3)
>  
> +## RISC-V
> +### SiFive
> +* [Freedom U500 VC707 FPGA](Platform/RiscV/SiFive/U500Pkg)
> +
>  ## Socionext
>  * [SynQuacer](Platform/Socionext/DeveloperBox)
>  
> -- 
> 2.12.0.windows.1
> 
> 
> 
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48443): https://edk2.groups.io/g/devel/message/48443
Mute This Topic: https://groups.io/mt/34196363/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 13/14] U500Pkg/PlatformPei: Platform initialization PEIM

[Leif Lindholm]

On Thu, Sep 19, 2019 at 11:51:30AM +0800, Gilbert Chen wrote:
> This is the platform-implementation specific library which is executed
>  in early PEI phase for platform initialization.
> 
> Signed-off-by: Gilbert Chen 
> ---
>  .../SiFive/U500Pkg/Universal/Pei/PlatformPei/Fv.c  |  49 
>  .../U500Pkg/Universal/Pei/PlatformPei/MemDetect.c  |  74 +
>  .../U500Pkg/Universal/Pei/PlatformPei/Platform.c   | 313 
> +
>  .../U500Pkg/Universal/Pei/PlatformPei/Platform.h   |  92 ++
>  .../Universal/Pei/PlatformPei/PlatformPei.inf  |  75 +
>  5 files changed, 603 insertions(+)
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Pei/PlatformPei/Fv.c
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Pei/PlatformPei/MemDetect.c
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Pei/PlatformPei/Platform.c
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Pei/PlatformPei/Platform.h
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Pei/PlatformPei/PlatformPei.inf
> 
> diff --git a/Platform/RiscV/SiFive/U500Pkg/Universal/Pei/PlatformPei/Fv.c 
> b/Platform/RiscV/SiFive/U500Pkg/Universal/Pei/PlatformPei/Fv.c
> new file mode 100644
> index ..74e4d433
> --- /dev/null
> +++ b/Platform/RiscV/SiFive/U500Pkg/Universal/Pei/PlatformPei/Fv.c
> @@ -0,0 +1,49 @@
> +/** @file
> +  Build FV related hobs for platform.
> +
> +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights 
> reserved.
> +  Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include "PiPei.h"
> +#include "Platform.h"
> +#include 
> +#include 
> +#include 
> +#include 

Please flip above two lines around.

> +
> +/**
> +  Publish PEI & DXE (Decompressed) Memory based FVs to let PEI
> +  and DXE know about them.
> +
> +  @retval EFI_SUCCESS   Platform PEI FVs were initialized successfully.
> +
> +**/
> +EFI_STATUS
> +PeiFvInitialization (
> +  VOID
> +  )
> +{
> +  DEBUG ((DEBUG_INFO, "Platform PEI Firmware Volume Initialization\n"));
> +  //
> +  // Let DXE know about the DXE FV
> +  //
> +  BuildFvHob (PcdGet32 (PcdRiscVDxeFvBase), PcdGet32 (PcdRiscVDxeFvSize));
> +  DEBUG ((DEBUG_INFO, "Platform builds DXE FV at %x, size %x.\n", PcdGet32 
> (PcdRiscVDxeFvBase), PcdGet32 (PcdRiscVDxeFvSize)));

Please wrap long line.

> +
> +  //
> +  // Let PEI know about the DXE FV so it can find the DXE Core
> +  //
> +  PeiServicesInstallFvInfoPpi (
> +NULL,
> +(VOID *)(UINTN) PcdGet32 (PcdRiscVDxeFvBase),
> +PcdGet32 (PcdRiscVDxeFvSize),
> +NULL,
> +NULL
> +);
> +
> +  return EFI_SUCCESS;
> +}
> diff --git 
> a/Platform/RiscV/SiFive/U500Pkg/Universal/Pei/PlatformPei/MemDetect.c 
> b/Platform/RiscV/SiFive/U500Pkg/Universal/Pei/PlatformPei/MemDetect.c
> new file mode 100644
> index ..dc99f2e0
> --- /dev/null
> +++ b/Platform/RiscV/SiFive/U500Pkg/Universal/Pei/PlatformPei/MemDetect.c
> @@ -0,0 +1,74 @@
> +/**@file
> +  Memory Detection for Virtual Machines.
> +
> +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights 
> reserved.
> +  Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +Module Name:
> +
> +  MemDetect.c
> +
> +**/
> +
> +//
> +// The package level header files this module uses
> +//
> +#include 
> +
> +//
> +// The Library classes this module consumes
> +//
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "Platform.h"
> +
> +
> +/**
> +  Publish PEI core memory
> +
> +  @return EFI_SUCCESS The PEIM initialized successfully.
> +
> +**/
> +EFI_STATUS
> +PublishPeiMemory (
> +  VOID
> +  )
> +{
> +  EFI_STATUS  Status;
> +  EFI_PHYSICAL_ADDRESSMemoryBase;
> +  UINT64  MemorySize;
> +
> +  MemoryBase = 0x8000UL + 0x100UL;
> +  MemorySize = 0x4000UL - 0x100UL; //1GB - 16MB
> +
> +  DEBUG((DEBUG_INFO, "%a: MemoryBase:0x%x MemorySize:%d\n", __FUNCTION__, 
> MemoryBase, MemorySize));
> +
> +  //
> +  // Publish this memory to the PEI Core
> +  //
> +  Status = PublishSystemMemory(MemoryBase, MemorySize);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  return Status;
> +}
> +
> +/**
> +  Publish system RAM and reserve memory regions
> +
> +**/
> +VOID
> +InitializeRamRegions (
> +  VOID
> +  )
> +{
> +  AddMemoryRangeHob(0x8100UL, 0x8100UL + 0x3F00UL);
> +
> +}
> diff --git 
> a/Platform/RiscV/SiFive/U500Pkg/Universal/Pei/PlatformPei/Platform.c 
> b/Platform/RiscV/SiFive/U500Pkg/Universal/Pei/PlatformPei/Platform.c
> new file mode 100644
> index ..45356399
> --- /dev/null
> +++ b/Platform/RiscV/SiFive/U500Pkg/Universal/Pei/PlatformPei/Platform.c
> @@ -0,0 +1,313 @@
> +/**@file
> +  Platform PEI driver
> +
> +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights 
> 

Re: [edk2-devel] [PATCH V2 1/9] MdeModulePkg/Variable: Consolidate common parsing functions

[Kubacki, Michael A]

The main reason is cohesiveness in the VariableParsing.c file. These are 
functions that
are commonly needed for general variable data structure parsing operations. 
Most of
them just read a member or two in a VARIABLE_STORE_HEADER or VARIABLE_HEADER
data structure,  perform a simple calculation if needed, and return some value. 
More
complex functions such as FindVariableEx (), do this in iteration across the 
variable store.
If a function is needed that performs these tasks, it is easier to have them 
grouped into a
cohesive file than search which one is in Variable.c and VariableParsing.c on a 
case-by-case
basis. UpdateVariableInfo () is the exception here, I can move this to a 
separate file
if you prefer.

Also, Variable.c is, in my opinion, far too large. It is on trend to only grow 
larger:
  * Today: ~4,600 LOC
  * ~2 years ago: ~4,200 LOC
  * ~4 years ago: ~4,100 LOC
  * ~5 years ago: ~3,440 LOC
  * ~8 years ago: ~2,600 LOC

I think moving out generic parsing services such as these better groups related
functionality in the short-term but also aids future refactoring efforts in the 
file.

Thanks,
Michael

> -Original Message-
> From: Wu, Hao A 
> Sent: Thursday, October 3, 2019 1:03 AM
> 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: [PATCH V2 1/9] MdeModulePkg/Variable: Consolidate common
> parsing functions
> 
> A couple of inline comments below:
> 
> 
> > -Original Message-
> > From: 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, Liming;
> Kinney,
> > Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> > Subject: [PATCH V2 1/9] MdeModulePkg/Variable: Consolidate common
> > parsing functions
> >
> > This change moves the following functions into a dedicated file
> > so they may be used in other variable files as needed. Furthermore,
> > it reduces the overall size of the common Variable.c file.
> >
> >  * DataSizeOfVariable ()
> >  * FindVariableEx ()
> >  * GetEndPointer ()
> >  * GetNextVariablePtr ()
> >  * GetStartPointer ()
> >  * GetVariableDataOffset ()
> >  * GetVariableDataPtr ()
> >  * GetVariableHeaderSize ()
> >  * GetVariableNamePtr ()
> >  * GetVariableStoreStatus ()
> >  * GetVendorGuidPtr ()
> >  * IsValidVariableHeader ()
> >  * NameSizeOfVariable ()
> >  * SetDataSizeOfVariable ()
> >  * SetNameSizeOfVariable ()
> >  * UpdateVariableInfo ()
> >  * VariableCompareTimeStampInternal ()
> >  * VariableServiceGetNextVariableInternal ()
> 
> 
> May I know what are the criteria for the above functions being moved to a
> separate file?
> 
> At first, I think all of them will be consumed by the new codes that
> implement
> the runtime cache. But I found that, for functions like
> GetVariableDataOffset(),
> GetVariableStoreStatus() and etc., their usages are still remained within file
> Variable.c (seems not related with the runtime cache).
> 
> 
> >
> > 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/VariableRuntimeDxe.inf
> > |   2 +
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf  |
> 2
> > +
> >
> >
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
> > |   7 +
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h   | 119 -
> ---
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h|
> 306
> > 
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c   | 726
> +--
> > 
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c  |   3
> +-
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c|
> 731
> > 
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c|   1
> +
> >  9 files changed, 1052 insertions(+), 845 deletions(-)
> 
> 
> For the below change in VariableStandaloneMm.inf:
> 
> [Guids]
> ...
>   ## SOMETIMES_CONSUMES   ## Variable:L"db"
>   ## SOMETIMES_CONSUMES   ## Variable:L"dbx"
>   ## SOMETIMES_CONSUMES   ## Variable:L"dbt"
>   gEfiImageSecurityDatabaseGuid
> ...
> 
> I think the above GUID is not used by the module specified by the above INF.
> Could you double confirm on this?
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > diff --git
> >
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> >
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> > index 641376c9c5..c35e5fe787 100644
> > ---
> >
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> > +++
> >
> 

Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 12/14] U500Pkg/TimerDxe: Platform Timer DXE driver

[Leif Lindholm]

On Thu, Sep 19, 2019 at 11:51:29AM +0800, Gilbert Chen wrote:
> Timer DXE driver for U500 platform based U500 platform implementation
>  specifc timer registers.
> 
> Signed-off-by: Gilbert Chen 
> ---
>  .../SiFive/U500Pkg/Universal/Dxe/TimerDxe/Timer.c  | 311 
> +
>  .../SiFive/U500Pkg/Universal/Dxe/TimerDxe/Timer.h  | 174 
>  .../U500Pkg/Universal/Dxe/TimerDxe/Timer.uni   |  14 +
>  .../U500Pkg/Universal/Dxe/TimerDxe/TimerDxe.inf|  48 
>  .../U500Pkg/Universal/Dxe/TimerDxe/TimerExtra.uni  |  12 +
>  5 files changed, 559 insertions(+)
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/TimerDxe/Timer.c
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/TimerDxe/Timer.h
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/TimerDxe/Timer.uni
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/TimerDxe/TimerDxe.inf
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/TimerDxe/TimerExtra.uni
> 
> diff --git a/Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/TimerDxe/Timer.c 
> b/Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/TimerDxe/Timer.c
> new file mode 100644
> index ..5cb42943
> --- /dev/null
> +++ b/Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/TimerDxe/Timer.c
> @@ -0,0 +1,311 @@
> +/** @file
> +  RISC-V Timer Architectural Protocol for U500 platform.
> +
> +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights 
> reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include "Timer.h"
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define CLINT_REG_MTIME 0x0200BFF8
> +#define CLINT_REG_MTIMECMP0 0x02004000
> +#define CLINT_REG_MTIMECMP1 0x02004008
> +#define CLINT_REG_MTIMECMP2 0x02004010
> +#define CLINT_REG_MTIMECMP3 0x02004018
> +#define CLINT_REG_MTIMECMP4 0x02004020
> +
> +static volatile void * const p_mtime = (void *)CLINT_REG_MTIME;
> +#define MTIME  (*p_mtime)
> +#define MTIMECMP(i)(p_mtimecmp[i])
> +
> +//
> +// The handle onto which the Timer Architectural Protocol will be installed
> +//
> +EFI_HANDLEmTimerHandle = NULL;

This one is exported? Or can it be STATIC?

> +
> +//
> +// The Timer Architectural Protocol that this driver produces
> +//
> +EFI_TIMER_ARCH_PROTOCOL   mTimer = {
> +  TimerDriverRegisterHandler,
> +  TimerDriverSetTimerPeriod,
> +  TimerDriverGetTimerPeriod,
> +  TimerDriverGenerateSoftInterrupt
> +};
> +
> +//
> +// Pointer to the CPU Architectural Protocol instance
> +//
> +EFI_CPU_ARCH_PROTOCOL *mCpu;

But this is only for internal use surely? STATIC?

> +
> +//
> +// The notification function to call on every timer interrupt.
> +// A bug in the compiler prevents us from initializing this here.
> +//
> +EFI_TIMER_NOTIFY mTimerNotifyFunction;

STATIC?

> +
> +//
> +// The current period of the timer interrupt
> +//
> +volatile UINT64 mTimerPeriod = 0;

This would be VOLATILE, but I dont think it has the effect you think it does.

> +
> +
> +/**
> +  8254 Timer #0 Interrupt Handler.
> +
> +  @param InterruptTypeThe type of interrupt that occured
> +  @param SystemContextA pointer to the system context when the interrupt 
> occured
> +**/
> +
> +VOID
> +EFIAPI
> +TimerInterruptHandler (
> +  IN EFI_EXCEPTION_TYPE   InterruptType,
> +  IN EFI_SYSTEM_CONTEXT   SystemContext
> +  )
> +{
> +  EFI_TPL OriginalTPL;
> +  UINT64 RiscvTimer;
> +
> +  csr_clear(CSR_SIE, MIP_STIP); // enable timer int
> +  OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> +  if (mTimerPeriod == 0) {
> +gBS->RestoreTPL (OriginalTPL);
> +mCpu->DisableInterrupt(mCpu);
> +return;
> +  }
> +  if (mTimerNotifyFunction != NULL) {
> +mTimerNotifyFunction (mTimerPeriod);
> +  }
> +  gBS->RestoreTPL (OriginalTPL);
> +
> +
> +  RiscvTimer = readq_relaxed(p_mtime);
> +  sbi_set_timer(RiscvTimer += mTimerPeriod);
> +  csr_set(CSR_SIE, MIP_STIP); // enable timer int
> +
> +}
> +
> +/**
> +
> +  This function registers the handler NotifyFunction so it is called every 
> time
> +  the timer interrupt fires.  It also passes the amount of time since the 
> last
> +  handler call to the NotifyFunction.  If NotifyFunction is NULL, then the
> +  handler is unregistered.  If the handler is registered, then EFI_SUCCESS is
> +  returned.  If the CPU does not support registering a timer interrupt 
> handler,
> +  then EFI_UNSUPPORTED is returned.  If an attempt is made to register a 
> handler
> +  when a handler is already registered, then EFI_ALREADY_STARTED is returned.
> +  If an attempt is made to unregister a handler when a handler is not 
> registered,
> +  then EFI_INVALID_PARAMETER is returned.  If an error occurs attempting to
> +  register the NotifyFunction with the timer interrupt, then EFI_DEVICE_ERROR
> +  is returned.
> +
> +  @param This The EFI_TIMER_ARCH_PROTOCOL instance.
> +  @param NotifyFunction   The function to call when a 

Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 11/14] U500Pkg/RamFvbServiceruntimeDxe: FVB driver for EFI variable.

[Leif Lindholm]

On Thu, Sep 19, 2019 at 11:51:28AM +0800, Gilbert Chen wrote:
> Firmware Volume Block driver instance for ram based EFI variable on U500
>  platform.
> 
> Signed-off-by: Gilbert Chen 
> ---
>  .../Dxe/RamFvbServicesRuntimeDxe/FvbInfo.c |  127 +++
>  .../FvbServicesRuntimeDxe.inf  |   81 ++
>  .../Dxe/RamFvbServicesRuntimeDxe/FwBlockService.c  | 1123 
> 
>  .../Dxe/RamFvbServicesRuntimeDxe/FwBlockService.h  |  187 
>  .../RamFvbServicesRuntimeDxe/FwBlockServiceDxe.c   |  151 +++
>  .../Dxe/RamFvbServicesRuntimeDxe/RamFlash.c|  144 +++
>  .../Dxe/RamFvbServicesRuntimeDxe/RamFlash.h|   85 ++
>  .../Dxe/RamFvbServicesRuntimeDxe/RamFlashDxe.c |   20 +
>  8 files changed, 1918 insertions(+)
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/RamFvbServicesRuntimeDxe/FvbInfo.c
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/RamFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/RamFvbServicesRuntimeDxe/FwBlockService.c
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/RamFvbServicesRuntimeDxe/FwBlockService.h
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/RamFvbServicesRuntimeDxe/FwBlockServiceDxe.c
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/RamFvbServicesRuntimeDxe/RamFlash.c
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/RamFvbServicesRuntimeDxe/RamFlash.h
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/RamFvbServicesRuntimeDxe/RamFlashDxe.c
> 
> diff --git 
> a/Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/RamFvbServicesRuntimeDxe/FvbInfo.c
>  
> b/Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/RamFvbServicesRuntimeDxe/FvbInfo.c
> new file mode 100644
> index ..1ade0d14
> --- /dev/null
> +++ 
> b/Platform/RiscV/SiFive/U500Pkg/Universal/Dxe/RamFvbServicesRuntimeDxe/FvbInfo.c
> @@ -0,0 +1,127 @@
> +/**@file
> +
> +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights 
> reserved.
> +  Copyright (c) 2006, Intel Corporation. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  Module Name:
> +
> +FvbInfo.c
> +
> +  Abstract:
> +
> +Defines data structure that is the volume header found.These data is 
> intent
> +to decouple FVB driver with FV header.
> +
> +**/
> +
> +//
> +// The package level header files this module uses
> +//
> +#include 
> +
> +//
> +// The protocols, PPI and GUID defintions for this module
> +//
> +#include 
> +//
> +// The Library classes this module consumes
> +//
> +#include 
> +#include 
> +
> +typedef struct {
> +  UINT64  FvLength;
> +  EFI_FIRMWARE_VOLUME_HEADER  FvbInfo;
> +  //
> +  // EFI_FV_BLOCK_MAP_ENTRYExtraBlockMap[n];//n=0
> +  //
> +  EFI_FV_BLOCK_MAP_ENTRY  End[1];
> +} EFI_FVB_MEDIA_INFO;
> +
> +EFI_FVB_MEDIA_INFO  mPlatformFvbMediaInfo[] = {
> +  //
> +  // Systen NvStorage FVB
> +  //
> +  {
> +FixedPcdGet32 (PcdFlashNvStorageVariableSize) +
> +FixedPcdGet32 (PcdFlashNvStorageFtwWorkingSize) +
> +FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize),
> +{
> +  {
> +0,
> +  },  // ZeroVector[16]
> +  EFI_SYSTEM_NV_DATA_FV_GUID,
> +  FixedPcdGet32 (PcdFlashNvStorageVariableSize) +
> +  FixedPcdGet32 (PcdFlashNvStorageFtwWorkingSize) +
> +  FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize),
> +  EFI_FVH_SIGNATURE,
> +  EFI_FVB2_MEMORY_MAPPED |
> +EFI_FVB2_READ_ENABLED_CAP |
> +EFI_FVB2_READ_STATUS |
> +EFI_FVB2_WRITE_ENABLED_CAP |
> +EFI_FVB2_WRITE_STATUS |
> +EFI_FVB2_ERASE_POLARITY |
> +EFI_FVB2_ALIGNMENT_16,
> +  sizeof (EFI_FIRMWARE_VOLUME_HEADER) + sizeof (EFI_FV_BLOCK_MAP_ENTRY),
> +  0,  // CheckSum
> +  0,  // ExtHeaderOffset
> +  {
> +0,
> +  },  // Reserved[1]
> +  2,  // Revision
> +  {
> +{
> +  (FixedPcdGet32 (PcdFlashNvStorageVariableSize) +
> +   FixedPcdGet32 (PcdFlashNvStorageFtwWorkingSize) +
> +   FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize)) /
> +  FixedPcdGet32 (PcdVariableFdBlockSize),
> +  FixedPcdGet32 (PcdVariableFdBlockSize),
> +}
> +  } // BlockMap[1]
> +},
> +{
> +  {
> +0,
> +0
> +  }
> +}  // End[1]
> +  }
> +};
> +
> +EFI_STATUS
> +GetFvbInfo (
> +  IN  UINT64FvLength,
> +  OUT EFI_FIRMWARE_VOLUME_HEADER**FvbInfo
> +  )
> +{
> +  STATIC BOOLEAN Checksummed = FALSE;
> +  UINTN Index;
> +
> +  if (!Checksummed) {
> +for (Index = 0;
> + Index < sizeof (mPlatformFvbMediaInfo) / sizeof 
> (EFI_FVB_MEDIA_INFO);
> + Index += 1) {
> +  UINT16 Checksum;
> +  mPlatformFvbMediaInfo[Index].FvbInfo.Checksum = 0;
> +  Checksum = CalculateCheckSum16 (
> +   (UINT16*) [Index].FvbInfo,
> +  

Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 10/14] U500Pkg/Library: Library instances of U500 platform library

[Leif Lindholm]

On Thu, Sep 19, 2019 at 11:51:27AM +0800, Gilbert Chen wrote:
> OpneSbiPlatformLib
> - In order to reduce the dependencies with RISC-V OpenSBI project
> (https://github.com/riscv/opensbi) and less burdens to EDK2 build
>  process, the implementation of RISC-V EDK2 platform is leverage
>  platform source code from OpenSBI code tree. The "platform.c"
>  under OpenSbiPlatformLib is cloned from RISC-V OpenSBI code tree
>  (in EDK2 RiscVPkg) and built based on EDK2 build environment.
> 
> PeiCoreInfoHobLib
> - This is the library to create RISC-V core characteristics for building
>  up RISC-V related SMBIOS records to support the unified boot loader
>  and OS image.
> 
> - RiscVPlatformTimerLib
> This is U500 platform timer library which has the platform-specific
>  timer implementation.
> 
> - SerialPortLib
> U500 serial port platform library

Please split this up into the 4 logical commits.
And add the specific header files with those commits.

> Signed-off-by: Gilbert Chen 
> ---
>  .../OpenSbiPlatformLib/OpenSbiPlatformLib.inf  |  47 
>  .../U500Pkg/Library/OpenSbiPlatformLib/platform.c  | 214 ++
>  .../Library/PeiCoreInfoHobLib/CoreInfoHob.c| 195 +
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf|  58 +
>  .../RiscVPlatformTimerLib/RiscVPlatformTimerLib.S  |  48 
>  .../RiscVPlatformTimerLib.inf  |  39 
>  .../U500Pkg/Library/SerialIoLib/SerialIoLib.inf|  31 +++
>  .../U500Pkg/Library/SerialIoLib/SerialPortLib.c| 241 
> +
>  .../Library/SerialIoLib/U500SerialPortLib.uni  |  16 ++
>  9 files changed, 889 insertions(+)
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Library/OpenSbiPlatformLib/OpenSbiPlatformLib.inf
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Library/OpenSbiPlatformLib/platform.c
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Library/PeiCoreInfoHobLib/CoreInfoHob.c
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Library/PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Library/RiscVPlatformTimerLib/RiscVPlatformTimerLib.S
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Library/RiscVPlatformTimerLib/RiscVPlatformTimerLib.inf
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Library/SerialIoLib/SerialIoLib.inf
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Library/SerialIoLib/SerialPortLib.c
>  create mode 100644 
> Platform/RiscV/SiFive/U500Pkg/Library/SerialIoLib/U500SerialPortLib.uni
> 
> diff --git 
> a/Platform/RiscV/SiFive/U500Pkg/Library/OpenSbiPlatformLib/OpenSbiPlatformLib.inf
>  
> b/Platform/RiscV/SiFive/U500Pkg/Library/OpenSbiPlatformLib/OpenSbiPlatformLib.inf
> new file mode 100644
> index ..473386d2
> --- /dev/null
> +++ 
> b/Platform/RiscV/SiFive/U500Pkg/Library/OpenSbiPlatformLib/OpenSbiPlatformLib.inf
> @@ -0,0 +1,47 @@
> +## @file
> +#  RISC-V OpenSbi Platform Library
> +#  This is the the required library which provides platform

Please don't use the word required. (If it wasn't, why would you
include it at all?)

> +#  level opensbi functions follow RISC-V opensbi implementation.
> +#
> +#  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights 
> reserved.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION= 0x00010005

Please bump specification version.

> +  BASE_NAME  = OpenSbiPlatformLib
> +  FILE_GUID  = 9424ED54-EBDA-4FB5-8FF6-8291B07BB151
> +  MODULE_TYPE= SEC
> +  VERSION_STRING = 1.0
> +  LIBRARY_CLASS  = OpenSbiPlatformLib

Seeing this, I have a bit of a feeling that this Opensbi isn't being
consistently treated/named:
- In edk2, we have RiscVPkg/Library/RiscVOpensbiLib which implements
  class RiscVOpensbiLib.
- Here, we have U500Pkg/Library/OpenSbiPlatformLib implementing the
  class OpenSbiPlatformLib.
- *Logically*, what we have is
  Platform/RiscV/Universal/Sec/SecMain.inf which depends on the
  OpensbiLib which depends on the OpenSbiPlatformLib.
  - However, there is no OpenSbiPlatformLib in edk2, which is very
unfortunate because that does not let us build edk2/RiscVPkg in
isolation (like we can with all the other packages in edk2).

Here is my preferred solution for untangling this:
1) Standardise on OpenSbi or Opensbi. Opensbi follows the pattern we
   already use for Openssl, so would be my preference.
2) Standardise on RiscVOpensbi or Opensbi - both for filenames and
   LibararyClasses. Either is fine, the former is less likely to clash
   with other imported projects in the future (but I would estimate
   this risk as *very* low to begin with).
3) Implement a (RiscV)OpensbiPlatformLibNull in edk2/RiscVPkg/Library,
   containing only what is required to build/link
   RiscVPkg/Library/(RiscV)OpensbiLib/.
   3.5) Map that to OpensbiPlatformLib in 

[edk2-devel] Upcoming Event: TianoCore Community Meeting - EMEA/NAMO - Thu, 10/03/2019 9:00am-10:00am #cal-reminder

[devel@edk2.groups.io Calendar]

*Reminder:* TianoCore Community Meeting - EMEA/NAMO

*When:* Thursday, 3 October 2019, 9:00am to 10:00am, (GMT-07:00) America/Los 
Angeles

*Where:* https://bluejeans.com/889357567?src=join_info

View Event ( https://edk2.groups.io/g/devel/viewevent?eventid=484143 )

*Organizer:* Stephano Cetola stephano.cet...@intel.com ( 
stephano.cet...@intel.com?subject=Re:%20Event:%20TianoCore%20Community%20Meeting%20-%20EMEA%2FNAMO
 )

*Description:*

Meeting URL
https://bluejeans.com/889357567?src=join_info

Meeting ID
889 357 567

Want to dial in from a phone?

Dial one of the following numbers:
+1.408.740.7256 (US (San Jose))
+1.408.317.9253 (US (Primary, San Jose))
(see all numbers - https://www.bluejeans.com/numbers)

Enter the meeting ID and passcode followed by #

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48437): https://edk2.groups.io/g/devel/message/48437
Mute This Topic: https://groups.io/mt/34383362/21656
Mute #cal-reminder: https://groups.io/mk?hashtag=cal-reminder=3846945
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES

[Lendacky, Thomas]

On 10/3/19 5:32 AM, Laszlo Ersek wrote:
> On 10/03/19 12:12, Laszlo Ersek wrote:
> 
>>   UINT32   ApEntryPoint;
>>   EFI_GUID SevEsFooterGuid;
>>   UINT16   Size;
> 
> It's probably better to reverse the order of "Size" and
> "SevEsFooterGuid", like this:
> 
>   UINT32   ApEntryPoint;
>   UINT16   Size;
>   EFI_GUID SevEsFooterGuid;
> 
> because then even the "Size" field can be changed (or resized), as a
> function of the footer GUID.

Cool, I'll look into doing this and see how it works out.

Thanks!
Tom

> 
> Thanks
> Laszlo
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48436): https://edk2.groups.io/g/devel/message/48436
Mute This Topic: https://groups.io/mt/34203585/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 33/35] StandaloneMmPkg/Core: stop abusing EFI_HANDLE for FwVolHeader tracking

[Achin Gupta]

Hi Laszlo,

Apologies for not getting back earlier as I was travelling. I will have a look
and get back by tomorrow.

cheers,
Achin

On Thu, Oct 03, 2019 at 01:10:53PM +0200, Laszlo Ersek wrote:
> Pinging StandaloneMmPkg maintainers again, for reviewing this patch.
>
> Thanks
> Laszlo
>
> On 09/26/19 14:48, Laszlo Ersek wrote:
> > Achin, Jiewen, Supreeth,
> >
> > can one of you guys please review this patch?
> >
> > Thanks
> > Laszlo
> >
> > On 09/17/19 21:49, Laszlo Ersek wrote:
> >> The FvHasBeenProcessed() and FvIsBeingProcesssed() functions make sure
> >> that every firmware volume is processed only once (every driver in every
> >> firmware volume should be discovered only once). For this, the functions
> >> use a linked list.
> >>
> >> In MdeModulePkg's DXE Core and SMM Core, the key used for identifying
> >> those firmware volumes that have been processed is the EFI_HANDLE on which
> >> the DXE or SMM firmware volume protocol is installed. In the
> >> StandaloneMmPkg core however, the key is the address of the firmware
> >> volume header; that is, it has type (EFI_FIRMWARE_VOLUME_HEADER*).
> >>
> >> (EFI_FIRMWARE_VOLUME_HEADER*) has nothing to do with EFI_HANDLE.
> >> EFI_HANDLE just happens to be specified as (VOID*), and therefore the
> >> conversion between (EFI_FIRMWARE_VOLUME_HEADER*) and EFI_HANDLE is silent.
> >>
> >> (The FvHasBeenProcessed() and FvIsBeingProcesssed() functions were likely
> >> copied verbatim from MdeModulePkg's DXE Core and/or the SMM Core, and not
> >> flagged by the compiler in StandaloneMmPkg due to UEFI regrettably
> >> specifying EFI_HANDLE as (VOID*), thereby enabling the above implicit
> >> conversion.)
> >>
> >> We should not exploit this circumstance. Represent the key type faithfully
> >> instead.
> >>
> >> This is a semantic fix; there is no change in operation.
> >>
> >> Cc: Achin Gupta 
> >> Cc: Jiewen Yao 
> >> Cc: Supreeth Venkatesh 
> >> Signed-off-by: Laszlo Ersek 
> >> ---
> >>
> >> Notes:
> >> build-tested only
> >>
> >>  StandaloneMmPkg/Core/StandaloneMmCore.h |  2 +-
> >>  StandaloneMmPkg/Core/Dispatcher.c   | 80 +++-
> >>  StandaloneMmPkg/Core/FwVol.c| 16 ++--
> >>  3 files changed, 52 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.h 
> >> b/StandaloneMmPkg/Core/StandaloneMmCore.h
> >> index dcf91bc5e916..23ddbe169faf 100644
> >> --- a/StandaloneMmPkg/Core/StandaloneMmCore.h
> >> +++ b/StandaloneMmPkg/Core/StandaloneMmCore.h
> >> @@ -67,7 +67,7 @@ typedef struct {
> >>
> >>LIST_ENTRY  ScheduledLink;// mScheduledQueue
> >>
> >> -  EFI_HANDLE  FvHandle;
> >> +  EFI_FIRMWARE_VOLUME_HEADER  *FwVolHeader;
> >>EFI_GUIDFileName;
> >>VOID*Pe32Data;
> >>UINTN   Pe32DataSize;
> >> diff --git a/StandaloneMmPkg/Core/Dispatcher.c 
> >> b/StandaloneMmPkg/Core/Dispatcher.c
> >> index 3788389f95ed..9853445a64a1 100644
> >> --- a/StandaloneMmPkg/Core/Dispatcher.c
> >> +++ b/StandaloneMmPkg/Core/Dispatcher.c
> >> @@ -5,7 +5,7 @@
> >>  is added to the mDiscoveredList. The Before, and After Depex 
> >> are
> >>  pre-processed as drivers are added to the mDiscoveredList. If 
> >> an Apriori
> >>  file exists in the FV those drivers are addeded to the
> >> -mScheduledQueue. The mFvHandleList is used to make sure a
> >> +mScheduledQueue. The mFwVolList is used to make sure a
> >>  FV is only processed once.
> >>
> >>Step #2 - Dispatch. Remove driver from the mScheduledQueue and load and
> >> @@ -40,13 +40,13 @@
> >>  //
> >>  // MM Dispatcher Data structures
> >>  //
> >> -#define KNOWN_HANDLE_SIGNATURE  SIGNATURE_32('k','n','o','w')
> >> +#define KNOWN_FWVOL_SIGNATURE  SIGNATURE_32('k','n','o','w')
> >>
> >>  typedef struct {
> >> -  UINTN   Signature;
> >> -  LIST_ENTRY  Link; // mFvHandleList
> >> -  EFI_HANDLE  Handle;
> >> -} KNOWN_HANDLE;
> >> +  UINTN  Signature;
> >> +  LIST_ENTRY Link; // mFwVolList
> >> +  EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader;
> >> +} KNOWN_FWVOL;
> >>
> >>  //
> >>  // Function Prototypes
> >> @@ -86,9 +86,10 @@ LIST_ENTRY  mDiscoveredList = 
> >> INITIALIZE_LIST_HEAD_VARIABLE (mDiscoveredList);
> >>  LIST_ENTRY  mScheduledQueue = INITIALIZE_LIST_HEAD_VARIABLE 
> >> (mScheduledQueue);
> >>
> >>  //
> >> -// List of handles who's Fv's have been parsed and added to the 
> >> mFwDriverList.
> >> +// List of firmware volume headers whose containing firmware volumes have 
> >> been
> >> +// parsed and added to the mFwDriverList.
> >>  //
> >> -LIST_ENTRY  mFvHandleList = INITIALIZE_LIST_HEAD_VARIABLE (mFvHandleList);
> >> +LIST_ENTRY  mFwVolList = INITIALIZE_LIST_HEAD_VARIABLE (mFwVolList);
> >>
> >>  //
> >>  // Flag for the MM Dispacher.  TRUE if dispatcher is execuing.
> >> @@ -769,26 

Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 09/29] MdePkg/BaseIoLibIntrinsic: RISC-V I/O intrinsic functions.

[Abner Chang]

> -Original Message-
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Leif Lindholm
> Sent: Thursday, October 3, 2019 4:38 PM
> To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> 
> Cc: af...@apple.com; Philippe Mathieu-Daudé ; Mike
> Kinney ; Liming Gao ;
> Palmer Dabbelt 
> Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 09/29]
> MdePkg/BaseIoLibIntrinsic: RISC-V I/O intrinsic functions.
> 
> On Thu, Oct 03, 2019 at 12:52:57AM +, Abner Chang wrote:
> > > > So is the plan to just copy IoLibArm.c to IoLibRiskV.c? I kind of
> > > > agree with Leif that having two copies of the same thing does not
> > > > make sense. I do see your point about naming, but maybe the issue
> > > > the IoLibArm.c name. I don't see anything ARM specific in
> > > > IoLibArm.c it seems to me it is generic C code for a platform that
> > > > does not have IO Ports. So I guess we could just change the file
> > > > name of IoLibArm.c to IoLibNoIo.c and have ARM and RISC-V point at
> the common file?
> >
> > Yes, naming is my concern. No technical issues here.
> > Thanks for this suggestion.
> >
> > > Works for me.
> > > We can untangle the remaining mess unrelated from the Risc-V
> upstreaming.
> >
> > Leif, I will rename IoLibRiscV.c to IoLibNoIo.c in the next version of
> > patches. You can adopt the new file in ARM side later.
> 
> The suggestion was to rename IoLibArm.c. If you're insisting on keeping your
> duplicated version, you're back to looking for another maintainer to convince
> that this is the better solution.
Ah, yes. Rename IoLibArm.c to IoLibNoIo.h. I am good with this.

> 
> Best Regards,
> 
> Leif
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48434): https://edk2.groups.io/g/devel/message/48434
Mute This Topic: https://groups.io/mt/34258203/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 33/35] StandaloneMmPkg/Core: stop abusing EFI_HANDLE for FwVolHeader tracking

[Laszlo Ersek]

Pinging StandaloneMmPkg maintainers again, for reviewing this patch.

Thanks
Laszlo

On 09/26/19 14:48, Laszlo Ersek wrote:
> Achin, Jiewen, Supreeth,
> 
> can one of you guys please review this patch?
> 
> Thanks
> Laszlo
> 
> On 09/17/19 21:49, Laszlo Ersek wrote:
>> The FvHasBeenProcessed() and FvIsBeingProcesssed() functions make sure
>> that every firmware volume is processed only once (every driver in every
>> firmware volume should be discovered only once). For this, the functions
>> use a linked list.
>>
>> In MdeModulePkg's DXE Core and SMM Core, the key used for identifying
>> those firmware volumes that have been processed is the EFI_HANDLE on which
>> the DXE or SMM firmware volume protocol is installed. In the
>> StandaloneMmPkg core however, the key is the address of the firmware
>> volume header; that is, it has type (EFI_FIRMWARE_VOLUME_HEADER*).
>>
>> (EFI_FIRMWARE_VOLUME_HEADER*) has nothing to do with EFI_HANDLE.
>> EFI_HANDLE just happens to be specified as (VOID*), and therefore the
>> conversion between (EFI_FIRMWARE_VOLUME_HEADER*) and EFI_HANDLE is silent.
>>
>> (The FvHasBeenProcessed() and FvIsBeingProcesssed() functions were likely
>> copied verbatim from MdeModulePkg's DXE Core and/or the SMM Core, and not
>> flagged by the compiler in StandaloneMmPkg due to UEFI regrettably
>> specifying EFI_HANDLE as (VOID*), thereby enabling the above implicit
>> conversion.)
>>
>> We should not exploit this circumstance. Represent the key type faithfully
>> instead.
>>
>> This is a semantic fix; there is no change in operation.
>>
>> Cc: Achin Gupta 
>> Cc: Jiewen Yao 
>> Cc: Supreeth Venkatesh 
>> Signed-off-by: Laszlo Ersek 
>> ---
>>
>> Notes:
>> build-tested only
>>
>>  StandaloneMmPkg/Core/StandaloneMmCore.h |  2 +-
>>  StandaloneMmPkg/Core/Dispatcher.c   | 80 +++-
>>  StandaloneMmPkg/Core/FwVol.c| 16 ++--
>>  3 files changed, 52 insertions(+), 46 deletions(-)
>>
>> diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.h 
>> b/StandaloneMmPkg/Core/StandaloneMmCore.h
>> index dcf91bc5e916..23ddbe169faf 100644
>> --- a/StandaloneMmPkg/Core/StandaloneMmCore.h
>> +++ b/StandaloneMmPkg/Core/StandaloneMmCore.h
>> @@ -67,7 +67,7 @@ typedef struct {
>>  
>>LIST_ENTRY  ScheduledLink;// mScheduledQueue
>>  
>> -  EFI_HANDLE  FvHandle;
>> +  EFI_FIRMWARE_VOLUME_HEADER  *FwVolHeader;
>>EFI_GUIDFileName;
>>VOID*Pe32Data;
>>UINTN   Pe32DataSize;
>> diff --git a/StandaloneMmPkg/Core/Dispatcher.c 
>> b/StandaloneMmPkg/Core/Dispatcher.c
>> index 3788389f95ed..9853445a64a1 100644
>> --- a/StandaloneMmPkg/Core/Dispatcher.c
>> +++ b/StandaloneMmPkg/Core/Dispatcher.c
>> @@ -5,7 +5,7 @@
>>  is added to the mDiscoveredList. The Before, and After Depex are
>>  pre-processed as drivers are added to the mDiscoveredList. If 
>> an Apriori
>>  file exists in the FV those drivers are addeded to the
>> -mScheduledQueue. The mFvHandleList is used to make sure a
>> +mScheduledQueue. The mFwVolList is used to make sure a
>>  FV is only processed once.
>>  
>>Step #2 - Dispatch. Remove driver from the mScheduledQueue and load and
>> @@ -40,13 +40,13 @@
>>  //
>>  // MM Dispatcher Data structures
>>  //
>> -#define KNOWN_HANDLE_SIGNATURE  SIGNATURE_32('k','n','o','w')
>> +#define KNOWN_FWVOL_SIGNATURE  SIGNATURE_32('k','n','o','w')
>>  
>>  typedef struct {
>> -  UINTN   Signature;
>> -  LIST_ENTRY  Link; // mFvHandleList
>> -  EFI_HANDLE  Handle;
>> -} KNOWN_HANDLE;
>> +  UINTN  Signature;
>> +  LIST_ENTRY Link; // mFwVolList
>> +  EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader;
>> +} KNOWN_FWVOL;
>>  
>>  //
>>  // Function Prototypes
>> @@ -86,9 +86,10 @@ LIST_ENTRY  mDiscoveredList = 
>> INITIALIZE_LIST_HEAD_VARIABLE (mDiscoveredList);
>>  LIST_ENTRY  mScheduledQueue = INITIALIZE_LIST_HEAD_VARIABLE 
>> (mScheduledQueue);
>>  
>>  //
>> -// List of handles who's Fv's have been parsed and added to the 
>> mFwDriverList.
>> +// List of firmware volume headers whose containing firmware volumes have 
>> been
>> +// parsed and added to the mFwDriverList.
>>  //
>> -LIST_ENTRY  mFvHandleList = INITIALIZE_LIST_HEAD_VARIABLE (mFvHandleList);
>> +LIST_ENTRY  mFwVolList = INITIALIZE_LIST_HEAD_VARIABLE (mFwVolList);
>>  
>>  //
>>  // Flag for the MM Dispacher.  TRUE if dispatcher is execuing.
>> @@ -769,26 +770,30 @@ MmInsertOnScheduledQueueWhileProcessingBeforeAndAfter (
>>  }
>>  
>>  /**
>> -  Return TRUE if the Fv has been processed, FALSE if not.
>> +  Return TRUE if the firmware volume has been processed, FALSE if not.
>>  
>> -  @param  FvHandle  The handle of a FV that's being tested
>> +  @param  FwVolHeader   The header of the firmware volume that's 
>> being
>> +   

Re: [edk2-devel] [PATCH 27/35] SecurityPkg: stop abusing EFI_EVENT for protocol notify registration

[Laszlo Ersek]

Pinging SecurityPkg maintainers again, for reviewing this patch.

Thanks
Laszlo

On 09/26/19 14:46, Laszlo Ersek wrote:
> Chao, Jian, Jiewen,
> 
> can you please review this patch?
> 
> Thanks,
> Laszlo
> 
> 
> On 09/17/19 21:49, Laszlo Ersek wrote:
>> EfiCreateProtocolNotifyEvent() takes a (VOID**) for "Registration",
>> similarly to gBS->RegisterProtocolNotify(). We should pass the address of
>> an actual pointer-to-VOID, and not the address of an EFI_EVENT. EFI_EVENT
>> just happens to be specified as (VOID*), and has nothing to do with the
>> registration.
>>
>> This change is a no-op in practice; it's a semantic improvement.
>>
>> Cc: Chao Zhang 
>> Cc: Jian Wang 
>> Cc: Jiewen Yao 
>> Signed-off-by: Laszlo Ersek 
>> ---
>>
>> Notes:
>> build-tested only
>>
>>  SecurityPkg/HddPassword/HddPasswordDxe.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/SecurityPkg/HddPassword/HddPasswordDxe.c 
>> b/SecurityPkg/HddPassword/HddPasswordDxe.c
>> index b0d795b6597f..051e64091d7f 100644
>> --- a/SecurityPkg/HddPassword/HddPasswordDxe.c
>> +++ b/SecurityPkg/HddPassword/HddPasswordDxe.c
>> @@ -2770,7 +2770,7 @@ HddPasswordDxeInit (
>>  {
>>EFI_STATUS Status;
>>HDD_PASSWORD_DXE_PRIVATE_DATA  *Private;
>> -  EFI_EVENT  Registration;
>> +  VOID   *Registration;
>>EFI_EVENT  EndOfDxeEvent;
>>EDKII_VARIABLE_LOCK_PROTOCOL   *VariableLock;
>>  
>>
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48432): https://edk2.groups.io/g/devel/message/48432
Mute This Topic: https://groups.io/mt/34180229/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 26/35] SecurityPkg: fix UninstallMultipleProtocolInterfaces() calls

[Laszlo Ersek]

Pinging SecurityPkg maintainers again, for reviewing this patch.

Thanks
Laszlo

On 09/26/19 14:45, Laszlo Ersek wrote:
> Chao, Jian, Jiewen,
> 
> can you please review this patch?
> 
> Thanks,
> Laszlo
> 
> On 09/17/19 21:49, Laszlo Ersek wrote:
>> Unlike the InstallMultipleProtocolInterfaces() boot service, which takes
>> an (EFI_HANDLE*) as first parameter, the
>> UninstallMultipleProtocolInterfaces() boot service takes an EFI_HANDLE as
>> first parameter.
>>
>> These are actual bugs. They must have remained hidden until now because
>> they are all in Unload() functions, which are probably exercised
>> infrequently. Fix the UninstallMultipleProtocolInterfaces() calls.
>>
>> Cc: Chao Zhang 
>> Cc: Jian Wang 
>> Cc: Jiewen Yao 
>> Signed-off-by: Laszlo Ersek 
>> ---
>>
>> Notes:
>> build-tested only
>>
>>  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c   
>>| 2 +-
>>  SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c  
>>| 2 +-
>>  
>> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDriver.c
>>  | 2 +-
>>  3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c 
>> b/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c
>> index 54155a338100..9052eced757d 100644
>> --- a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c
>> +++ b/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c
>> @@ -443,7 +443,7 @@ Tcg2ConfigDriverUnload (
>>ASSERT (PrivateData->Signature == TCG2_CONFIG_PRIVATE_DATA_SIGNATURE);
>>  
>>gBS->UninstallMultipleProtocolInterfaces (
>> - ,
>> + ImageHandle,
>>   ,
>>   PrivateData,
>>   NULL
>> diff --git a/SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c 
>> b/SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c
>> index 341879e4c4ba..fb06624fdb8f 100644
>> --- a/SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c
>> +++ b/SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c
>> @@ -138,7 +138,7 @@ TcgConfigDriverUnload (
>>ASSERT (PrivateData->Signature == TCG_CONFIG_PRIVATE_DATA_SIGNATURE);
>>  
>>gBS->UninstallMultipleProtocolInterfaces (
>> - ,
>> + ImageHandle,
>>   ,
>>   PrivateData,
>>   NULL
>> diff --git 
>> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDriver.c
>>  
>> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDriver.c
>> index 798ef9cfbc01..6c0294151e6c 100644
>> --- 
>> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDriver.c
>> +++ 
>> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDriver.c
>> @@ -115,7 +115,7 @@ SecureBootConfigDriverUnload (
>>ASSERT (PrivateData->Signature == 
>> SECUREBOOT_CONFIG_PRIVATE_DATA_SIGNATURE);
>>  
>>gBS->UninstallMultipleProtocolInterfaces (
>> - ,
>> + ImageHandle,
>>   ,
>>   PrivateData,
>>   NULL
>>
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48431): https://edk2.groups.io/g/devel/message/48431
Mute This Topic: https://groups.io/mt/34180228/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 18/35] NetworkPkg/DxeNetLib: fix type typo in NetLibGetMacAddress()

[Laszlo Ersek]

Pinging NetworkPkg maintainers again. Please?

Thanks
Laszlo

On 09/26/19 14:14, Laszlo Ersek wrote:
> Jiaxin, Siyuan,
> 
> can you please review this patch?
> 
> Thanks
> Laszlo
> 
> On 09/17/19 21:49, Laszlo Ersek wrote:
>> NetLibGetSnpHandle() returns an EFI_HANDLE, not an (EFI_HANDLE*).
>> NetLibGetMacAddress() only uses the return value ("SnpHandle") for a
>> NULL-check. Fix the type of "SnpHandle".
>>
>> This patch is a no-op.
>>
>> Cc: Jiaxin Wu 
>> Cc: Siyuan Fu 
>> Signed-off-by: Laszlo Ersek 
>> ---
>>
>> Notes:
>> lightly tested: MAC strings are displayed in UiApp
>>
>>  NetworkPkg/Library/DxeNetLib/DxeNetLib.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/NetworkPkg/Library/DxeNetLib/DxeNetLib.c 
>> b/NetworkPkg/Library/DxeNetLib/DxeNetLib.c
>> index 8e2f720666ea..a39c20be3d34 100644
>> --- a/NetworkPkg/Library/DxeNetLib/DxeNetLib.c
>> +++ b/NetworkPkg/Library/DxeNetLib/DxeNetLib.c
>> @@ -2182,7 +2182,7 @@ NetLibGetMacAddress (
>>EFI_SIMPLE_NETWORK_MODE  SnpModeData;
>>EFI_MANAGED_NETWORK_PROTOCOL *Mnp;
>>EFI_SERVICE_BINDING_PROTOCOL *MnpSb;
>> -  EFI_HANDLE   *SnpHandle;
>> +  EFI_HANDLE   SnpHandle;
>>EFI_HANDLE   MnpChildHandle;
>>  
>>ASSERT (MacAddress != NULL);
>>
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48430): https://edk2.groups.io/g/devel/message/48430
Mute This Topic: https://groups.io/mt/34180219/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache support

[Laszlo Ersek]

On 10/03/19 10:04, Wu, Hao A wrote:
> Before any comment on the patch, since I am not experienced in the Variable
> driver, I would like to ask for help from other reviewers to look into this
> patch and provide feedbacks as well. Thanks in advance.
> 
> With the above fact, some comments provided below maybe wrong. So please help
> to kindly correct me.
> 
> 
> Some general comments:
> 1. I am not sure if bringing the TimerLib dependency (delay in acquiring the
>runtime cache read lock) to variable driver (a software driver for the most
>part) is a good idea.

I agree. Most TimerLib instances do not expect sharing the hardware with
the OS.


Another complication is if the hardware is accessed via MMIO (that is,
not IO ports). MMIO accesses are subject to page tables.

Assuming that MicroSecondDelay() is invoked from the runtime DXE driver
at OS runtime, a platform would have to expose the MMIO area of the
timer hardware in the UEFI memory map as "runtime MMIO". (Via GCD memory
space operations in a platform driver or in the TimerLib constructor.)

Furthermore, the constructor function of the TimerLib instance would
have to register a VirtualAddressChange event handler, and convert the
MMIO address.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48429): https://edk2.groups.io/g/devel/message/48429
Mute This Topic: https://groups.io/mt/34318592/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES

[Laszlo Ersek]

On 10/03/19 12:12, Laszlo Ersek wrote:

>   UINT32   ApEntryPoint;
>   EFI_GUID SevEsFooterGuid;
>   UINT16   Size;

It's probably better to reverse the order of "Size" and
"SevEsFooterGuid", like this:

  UINT32   ApEntryPoint;
  UINT16   Size;
  EFI_GUID SevEsFooterGuid;

because then even the "Size" field can be changed (or resized), as a
function of the footer GUID.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48428): https://edk2.groups.io/g/devel/message/48428
Mute This Topic: https://groups.io/mt/34203585/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES

[Laszlo Ersek]

On 10/02/19 20:07, Lendacky, Thomas wrote:
> On 10/2/19 10:26 AM, Laszlo Ersek wrote:
>> On 10/02/19 17:15, Laszlo Ersek wrote:
>>> Adding Phil.
>>>
>>> I'm looking at this patch only because one thing caught my attention in
>>> the previous one, "OvmfPkg: Add support for SEV-ES AP reset vector
>>> re-directing":
>>>
>>> On 09/19/19 21:53, Lendacky, Thomas wrote:
 From: Tom Lendacky 

 BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198

 Typically, an AP is booted using the INIT-SIPI-SIPI sequence. This
 sequence is intercepted by the hypervisor, which sets the AP's registers
 to the values requested by the sequence. At that point, the hypervisor can
 start the AP, which will then begin execution at the appropriate location.

 Under SEV-ES, AP booting presents some challenges since the hypervisor is
 not allowed to alter the AP's register state. In this situation, we have
 to distinguish between the AP's first boot and AP's subsequent boots.

 First boot:
  Once the AP's register state has been defined (which is before the guest
  is first booted) it cannot be altered. Should the hypervisor attempt to
  alter the register state, the change would be detected by the hardware
  and the VMRUN instruction would fail. Given this, the first boot for the
  AP is required to begin execution with this initial register state, which
  is typically the reset vector. This prevents the BSP from directing the
  AP startup location through the INIT-SIPI-SIPI sequence.

  To work around this, provide a four-byte field at offset 0xffd0 that
  can contain an IP / CS register combination, that if non-zero, causes
  the AP to perform a far jump to that location instead of a near jump to
  EarlyBspInitReal16. Before booting the AP for the first time, the BSP
  should set the IP / CS value for the AP based on the value that would be
  derived from the INIT-SIPI-SIPI sequence.
>>>
>>> I don't understand how this can work: the guest-phys address 0xffd0
>>> is backed by read-only pflash in most OVMF deployments.
>>>
>>> In addition:
>>>
>>> [...]
>>>
 @@ -1002,6 +1204,7 @@ WakeUpAP (
CpuMpData->InitFlag   != ApInitDone) {
  ResetVectorRequired = TRUE;
  AllocateResetVector (CpuMpData);
 +AllocateSevEsAPMemory (CpuMpData);
  FillExchangeInfoData (CpuMpData);
  SaveLocalApicTimerSetting (CpuMpData);
}
 @@ -1038,6 +1241,15 @@ WakeUpAP (
}
  }
  if (ResetVectorRequired) {
 +  //
 +  // For SEV-ES, set the jump address for initial AP boot
 +  //
 +  if (CpuMpData->SevEsActive) {
 +SEV_ES_AP_JMP_FAR *JmpFar = (SEV_ES_AP_JMP_FAR *)0xFFD0;
 +
 +JmpFar->ApStart.Rip = 0;
 +JmpFar->ApStart.Segment = (UINT16) (ExchangeInfo->BufferStart >> 
 4);
 +  }
>>>
>>> Even if the address is backed by a single "unified" pflash, mapped r/w
>>> -- which we can call a "non-standard OVMF deployment" nowadays --, a
>>> normal store doesn't appear sufficient to me. The first write to pflash
>>> will flip it to "programming mode", and the values stored are supposed
>>> to be pflash commands (not just the raw data we intend to put in place).
>>>
>>> See for example the QemuFlashWrite() function in
>>> "OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c". (But, again, that
>>> is used with the pflash chip that hosts the variable store, and is
>>> therefore mapped r/w.)
>>>
>>>
>>> Taking a step back... I don't think APs execute any code from pflash,
>>> when MpInitLib boots them.
>>>
>>> In OVMF, PcdCpuApLoopMode is ApInHltLoop (value 1), therefore
>>> "CpuMpData->WakeUpByInitSipiSipi" should be TRUE, and
>>> "ResetVectorRequired" too should be TRUE, at first AP boot.
>>> Consequently, the reset vector seems to be allocated with
>>> AllocateResetVector().
>>>
>>> AllocateResetVector() has separate implementations for PEI and DXE, but
>>> in both cases, it returns RAM. So I don't see where the AP accesses (or
>>> executes) pflash.
>>
>> ... I believe I understand that this is precisely what cannot work under
>> SEV-ES -- because we cannot launch an AP at an address that's
>> dynamically chosen by the firmware (and passed to the hypervisor), like
>> with INIT-SIPI-SIPI.
> 
> Correct.
> 
>>
>> And so firmware and hypervisor have to agree upon a *constant* AP reset
>> vector address, in advance.
>>
>> We have two options:
>>
>> - pick the reset vector address *constant* such that it falls into RAM, or
>>
>> - let the AP reset vector reside in pflash, but then the code in pflash
>> has to look for a parameter block at a fixed address in RAM.
>>
>> So in the end, both options require the same -- we need a RAM address
>> constant that is determined at firmware build time. Either for the reset
>> vector itself, or for the reset vector's parameter block.
> 
> As long 

Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES

[Laszlo Ersek]

On 10/02/19 19:58, Lendacky, Thomas wrote:
> On 10/2/19 10:15 AM, Laszlo Ersek via Groups.Io wrote:
>> Adding Phil.
>>
>> I'm looking at this patch only because one thing caught my attention in
>> the previous one, "OvmfPkg: Add support for SEV-ES AP reset vector
>> re-directing":
>>
>> On 09/19/19 21:53, Lendacky, Thomas wrote:
>>> From: Tom Lendacky 
>>>
>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>>>
>>> Typically, an AP is booted using the INIT-SIPI-SIPI sequence. This
>>> sequence is intercepted by the hypervisor, which sets the AP's registers
>>> to the values requested by the sequence. At that point, the hypervisor can
>>> start the AP, which will then begin execution at the appropriate location.
>>>
>>> Under SEV-ES, AP booting presents some challenges since the hypervisor is
>>> not allowed to alter the AP's register state. In this situation, we have
>>> to distinguish between the AP's first boot and AP's subsequent boots.
>>>
>>> First boot:
>>>  Once the AP's register state has been defined (which is before the guest
>>>  is first booted) it cannot be altered. Should the hypervisor attempt to
>>>  alter the register state, the change would be detected by the hardware
>>>  and the VMRUN instruction would fail. Given this, the first boot for the
>>>  AP is required to begin execution with this initial register state, which
>>>  is typically the reset vector. This prevents the BSP from directing the
>>>  AP startup location through the INIT-SIPI-SIPI sequence.
>>>
>>>  To work around this, provide a four-byte field at offset 0xffd0 that
>>>  can contain an IP / CS register combination, that if non-zero, causes
>>>  the AP to perform a far jump to that location instead of a near jump to
>>>  EarlyBspInitReal16. Before booting the AP for the first time, the BSP
>>>  should set the IP / CS value for the AP based on the value that would be
>>>  derived from the INIT-SIPI-SIPI sequence.
>>
>> I don't understand how this can work: the guest-phys address 0xffd0
>> is backed by read-only pflash in most OVMF deployments.
>>
>> In addition:
>>
>> [...]
>>
>>> @@ -1002,6 +1204,7 @@ WakeUpAP (
>>>CpuMpData->InitFlag   != ApInitDone) {
>>>  ResetVectorRequired = TRUE;
>>>  AllocateResetVector (CpuMpData);
>>> +AllocateSevEsAPMemory (CpuMpData);
>>>  FillExchangeInfoData (CpuMpData);
>>>  SaveLocalApicTimerSetting (CpuMpData);
>>>}
>>> @@ -1038,6 +1241,15 @@ WakeUpAP (
>>>}
>>>  }
>>>  if (ResetVectorRequired) {
>>> +  //
>>> +  // For SEV-ES, set the jump address for initial AP boot
>>> +  //
>>> +  if (CpuMpData->SevEsActive) {
>>> +SEV_ES_AP_JMP_FAR *JmpFar = (SEV_ES_AP_JMP_FAR *)0xFFD0;
>>> +
>>> +JmpFar->ApStart.Rip = 0;
>>> +JmpFar->ApStart.Segment = (UINT16) (ExchangeInfo->BufferStart >> 
>>> 4);
>>> +  }
>>
>> Even if the address is backed by a single "unified" pflash, mapped r/w
>> -- which we can call a "non-standard OVMF deployment" nowadays --, a
>> normal store doesn't appear sufficient to me. The first write to pflash
>> will flip it to "programming mode", and the values stored are supposed
>> to be pflash commands (not just the raw data we intend to put in place).
> 
> There is a corresponding patch in Qemu that does not set the
> KVM_MEM_READONLY flag for the ROM when starting an SEV-ES guest, thus
> allowing the MP library to update the location with desired address.

Isn't that a price too high to pay? It seems to allow the memory mapped
contents of the pflash chip to diverge from the host-side file, even if
the QEMU command line specifies that the pflash chip is to be mapped r/o.

With regard to SMM, I think this could even be considered a security
problem -- if the pflash region is not marked as readonly, then the
guest OS could write to it as well (with direct hardware access, like
the code seen above). The write would not trap to QEMU, and QEMU
couldn't prevent the write.

The OS could use this to inject code into the pflash region (e.g., SEC).
Although the host-side pflash file would not be modified, if the OS
performed a warm reboot (or S3 cycle) afterwards, the code it injected
into the flash region would be executed with firmware privileges.

AMD Publication #56421 states that SMM is currently out of scope for
SEV-ES, so I don't think there's a problem right now; I'm just generally
concerned that here we're enabling something I've always considered a
big no-no for SMM builds.

I still have to read your next response though (to my suggestion to use
a fixed RAM address, I believe).

Thanks!
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48426): https://edk2.groups.io/g/devel/message/48426
Mute This Topic: https://groups.io/mt/34203585/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH v2 37/44] OvmfPkg: Add support for SEV-ES AP reset vector re-directing

[Laszlo Ersek]

On 10/02/19 19:33, Lendacky, Thomas wrote:
> On 10/2/19 9:54 AM, Laszlo Ersek wrote:
>> On 09/19/19 21:53, Lendacky, Thomas wrote:
>>> From: Tom Lendacky 
>>>
>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>>>
>>> A hypervisor is not allowed to update an SEV-ES guests register state,
>>> so when booting an SEV-ES guest AP, the hypervisor is not allowed to
>>> set the RIP to the guest requested value. Instead an SEV-ES AP must be
>>> re-directed from within the guest to the actual requested staring location
>>> as specified in the INIT-SIPI-SIPI sequence.
>>>
>>> Provide reset vector code that contains support to jump to the desired
>>> RIP location after having been started. This is required for only the
>>> very first AP reset.
>>
>> (1) In the commit message, can you mention the build mechanism by which
>> this file overrides the original in UefiCpuPkg?
>>
>> Is it due to include path order?
> 
> Yes, this is due to the BuildOptions include path order. I'll update the
> commit message.
> 
>>
>>>
>>> Cc: Jordan Justen 
>>> Cc: Laszlo Ersek 
>>> Cc: Ard Biesheuvel 
>>> Signed-off-by: Tom Lendacky 
>>> ---
>>>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 80 
>>>  1 file changed, 80 insertions(+)
>>>  create mode 100644 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>>>
>>> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm 
>>> b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>>> new file mode 100644
>>> index ..1ac8b7ca7e85
>>> --- /dev/null
>>> +++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>>> @@ -0,0 +1,80 @@
>>> +;--
>>> +; @file
>>> +; First code executed by processor after resetting.
>>> +; Derived from UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm
>>> +;
>>> +; Copyright (c) 2019, AMD Inc. All rights reserved.
>>> +; SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> (2) Thanks for the "derived from" hint -- but in that case, you should
>> probably keep the original copyright notice too.
> 
> Ok, will do.
> 
>>
>>> +;
>>> +;--
>>> +
>>> +BITS16
>>> +
>>> +ALIGN   16
>>> +
>>> +;
>>> +; Pad the image size to 4k when page tables are in VTF0
>>> +;
>>> +; If the VTF0 image has page tables built in, then we need to make
>>> +; sure the end of VTF0 is 4k above where the page tables end.
>>> +;
>>> +; This is required so the page tables will be 4k aligned when VTF0 is
>>> +; located just below 0x1 (4GB) in the firmware device.
>>> +;
>>> +%ifdef ALIGN_TOP_TO_4K_FOR_PAGING
>>> +TIMES (0x1000 - ($ - EndOfPageTables) - 0x20) DB 0
>>> +%endif
>>> +
>>> +;
>>> +; SEV-ES Processor Reset support
>>> +;
>>> +; standardProcessorSevEsReset:(0xffd0)
>>> +;   When using the Application Processors entry point, always perform a
>>> +;   far jump to the RIP/CS value contained at this location.  This will
>>> +;   default to EarlyBspInitReal16 unless specifically overridden.
>>> +
>>> +standardProcessorSevEsReset:
>>> +DW  0x
>>> +DW  0x
>>> +
>>> +ALIGN   16
>>> +
>>> +applicationProcessorEntryPoint:
>>> +;
>>> +; Application Processors entry point
>>> +;
>>> +; GenFv generates code aligned on a 4k boundary which will jump to this
>>> +; location.  (0xffe0)  This allows the Local APIC Startup IPI to be
>>> +; used to wake up the application processors.
>>> +;
>>> +jmp EarlyApInitReal16
>>> +
>>> +ALIGN   8
>>> +
>>> +DD  0
>>> +
>>> +;
>>> +; The VTF signature
>>> +;
>>> +; VTF-0 means that the VTF (Volume Top File) code does not require
>>> +; any fixups.
>>> +;
>>> +vtfSignature:
>>> +DB  'V', 'T', 'F', 0
>>> +
>>> +ALIGN   16
>>> +
>>> +resetVector:
>>> +;
>>> +; Reset Vector
>>> +;
>>> +; This is where the processor will begin execution
>>> +;
>>> +cmp dword [CS:0xFFD0], 0
>>> +je  EarlyBspInitReal16
>>> +jmp far [CS:0xFFD0]
>>> +
>>> +ALIGN   16
>>> +
>>> +fourGigabytes:
>>> +
>>>
>>
>> It's worth looking at this patch as a diff against the original:
>>
>>> --- UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm2019-09-25 
>>> 17:09:42.856850422 +0200
>>> +++ OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm2019-10-02 
>>> 16:40:55.906630335 +0200
>>> @@ -1,8 +1,9 @@
>>>  
>>> ;--
>>>  ; @file
>>>  ; First code executed by processor after resetting.
>>> +; Derived from UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm
>>>  ;
>>> -; Copyright (c) 2008 - 2014, Intel Corporation. All rights reserved.
>>> +; Copyright (c) 2019, AMD Inc. All rights reserved.
>>>  ; SPDX-License-Identifier: BSD-2-Clause-Patent
>>>  ;
>>>  
>>> ;--
>>> @@ -24,6 +25,20 @@
>>>  TIMES (0x1000 - ($ - EndOfPageTables) - 0x20) DB 0
>>>  %endif
>>>
>>> +;
>>> +; SEV-ES 

Re: [edk2-devel] [RFC PATCH v2 10/44] OvmfPkg: A per-CPU variable area for #VC usage

[Laszlo Ersek]

On 10/02/19 18:06, Lendacky, Thomas wrote:
> On 10/2/19 6:51 AM, Laszlo Ersek wrote:

>> ... Side question: actually, do we support S3 with SEV enabled, at the
>> moment? Last week or so I tried to test it, and it didn't work. I don't
>> remember if we *intended* to support S3 in SEV guests at all. If we
>> never cared, then we should document that, plus I shouldn't make the
>> SEV-ES work needlessly difficult with S3 remarks... Brijesh, what's your
>> recollection?
>>
>> If the intent has always been to ignore S3 in SEV guests, then we should
>> modify the S3Verification() function to catch QEMU configs where both
>> features are enabled, and force the user to disable at least one of
>> them. Otherwise, the user might suspend the OS to S3, and then lose data
>> when resume fails. In such cases, the user should be forced -- during
>> early boot -- to explicitly disable S3 on the QEMU cmdline, and to
>> re-launch the guest. And then the OS won't ever attempt S3.
>>
>> Hm I've now found some internal correspondence at Red Hat, from Aug
>> 2017. I wrote,
>>
>>> With SEV enabled, the S3 boot script would have to manipulate page
>>> tables (which might require more memory pre-allocation), in order to
>>> continue using the currently pre-reserved memory areas for guest-host
>>> communication during S3 resume.
> 
> I guess I need to understand more about this. Does the page table
> manipulation occur in the guest or hypervisor? If in the guest, then that
> is ok. But the page tables can't be successfully manipulated by the
> hypervisor.

It's all on the guest side. That's not the issue, the issue is (again)
complexity, and possibly also the limited expressiveness of the S3 boot
script "language" (the set of opcodes).

Roughly, this is the story: during normal boot, DXE drivers locate the
S3 Save State protocol, and call it to append a number of opcodes to the
S3 boot script. These opcodes allow platform device drivers to "stash"
various chipset programming actions for S3 resume time.

At a certain point in BDS (when the DXE SMM Ready To Lock protocol is
installed by Platform BDS), the boot script is saved into secure storage
(a "lock box" in SMRAM). Furthermore, BootScriptExecutorDxe saves itself
(the executable) into another lock box.

At S3 resume time, at the end of the PEI phase, S3Resume2Pei restores
BootScriptExecutorDxe from SMRAM, restores the boot script, and invokes
BootScriptExecutorDxe to execute the boot script; thereby re-programming
various chipset registers (as queued by platform DXE drivers during
normal boot). Finally control is transferred to the OS waking vector
(per ACPI FACS).

A number of platform drivers in OVMF queue boot script opcodes such that
those opcodes implement fw_cfg actions (fw_cfg DMA transfers) during S3.
Queueing these opcodes is very messy, therefore OVMF has a helper
library for that, QemuFwCfgS3Lib.

For the fw_cfg DMA transfers, the underlying pages need to be decrypted
& re-encrypted, as always. During normal boot, QemuFwCfgLib handles this:

- In the SEC and PEI phases, QemuFwCfgLib uses the IO port access
method, which is slower, and does not support fw_cfg writes. But for
SEC/PEI, that's enough.

- In DXE, QemuFwCfgLib uses the DMA access method, which is faster,
supports fw_cfg writes. It is SEV-aware, and uses the IOMMU protocol for
decrypting / encrypting the relevant pages.

The S3 boot script opcodes saved by QemuFwCfgS3Lib must use fw_cfg DMA
(because they need fw_cfg writes too, which are only supported by the
DMA method), and so they'd need extra page table actions in SEV guests.
But those page table actions appear difficult to express through S3 boot
script opcodes.

Anyway, this is just some background info; I'm certainly not suggesting
that we spend *any* resources on enabling S3 for SEV. SEV (not SEV-ES)
has been available in OVMF for a good while now, and we've seen no
reports related to S3. For another data point, S3 has been en-bloc
unsupported in RHEL downstream, regardless of SEV (it is disabled in
downstream QEMU by default, and if you force-enable it, that will
"taint" the domain). Mainly due to S3 depending very much on guest
driver cooperation (primarily video drivers), and that has been brittle,
in our experience.

So in the end I think we should update S3Verification() to catch S3+SEV
configs.

> 
>>>
>>> This kind of page table manipulation is very difficult to do with the
>>> currently specified / standardized boot script opcodes.
>>> EFI_BOOT_SCRIPT_DISPATCH_2_OPCODE *might* prove usable to call custom
>>> code during S3 resume, from the boot script, but the callee seems to
>>> need a custom assembly trampoline, and likely some magic for code
>>> relocation too (or the code must be position independent). One example
>>> seems to exist in the edk2 tree, but for OVMF this is uncharted
>>> territory.
>>
>> And then the participants in that discussion seemed to set S3+SEV aside,
>> indefinitely.
>>
>> ... I've also found some S3 references 

Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 09/29] MdePkg/BaseIoLibIntrinsic: RISC-V I/O intrinsic functions.

[Leif Lindholm]

On Thu, Oct 03, 2019 at 12:52:57AM +, Abner Chang wrote:
> > > So is the plan to just copy IoLibArm.c to IoLibRiskV.c? I kind of
> > > agree with Leif that having two copies of the same thing does not make
> > > sense. I do see your point about naming, but maybe the issue the
> > > IoLibArm.c name. I don't see anything ARM specific in IoLibArm.c it
> > > seems to me it is generic C code for a platform that does not have IO
> > > Ports. So I guess we could just change the file name of IoLibArm.c to
> > > IoLibNoIo.c and have ARM and RISC-V point at the common file?
>
> Yes, naming is my concern. No technical issues here.
> Thanks for this suggestion.
> 
> > Works for me.
> > We can untangle the remaining mess unrelated from the Risc-V upstreaming.
>
> Leif, I will rename IoLibRiscV.c to IoLibNoIo.c in the next version
> of patches. You can adopt the new file in ARM side later.

The suggestion was to rename IoLibArm.c. If you're insisting on
keeping your duplicated version, you're back to looking for another
maintainer to convince that this is the better solution.

Best Regards,

Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48423): https://edk2.groups.io/g/devel/message/48423
Mute This Topic: https://groups.io/mt/34258203/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V2 8/9] MdeModulePkg/Variable: Add RT GetNextVariableName() cache support

[Wu, Hao A]

> -Original Message-
> From: 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, Liming; Kinney,
> Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> Subject: [PATCH V2 8/9] MdeModulePkg/Variable: Add RT
> GetNextVariableName() cache support
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=2220
> 
> This change implements the Runtime Service GetNextVariableName()
> using the Runtime Cache in VariableSmmRuntimeDxe. Runtime Service
> calls to GetNextVariableName() will no longer trigger a SW SMI.
> 
> Overall system performance and stability will be improved by
> eliminating an SMI for these calls as they typically result in a
> relatively large number of invocations to retrieve all variable
> names in all variable stores present.
> 
> 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/VariableSmmRuntimeDxe
> .c | 118 +---
>  1 file changed, 50 insertions(+), 68 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
> xe.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
> xe.c
> index 46f69765a4..bc3b56b0ce 100644
> ---
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
> xe.c
> +++
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD
> xe.c
> @@ -799,87 +799,69 @@ RuntimeServiceGetNextVariableName (
>IN OUT  EFI_GUID  *VendorGuid
>)
>  {
> -  EFI_STATUS  Status;
> -  UINTN   PayloadSize;
> -  SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME
> *SmmGetNextVariableName;
> -  UINTN   OutVariableNameSize;
> -  UINTN   InVariableNameSize;
> +  EFI_STATUS  Status;
> +  UINTN   DelayIndex;
> +  UINTN   MaxLen;
> +  UINTN   VarNameSize;
> +  VARIABLE_HEADER *VariablePtr;
> +  VARIABLE_STORE_HEADER
> *VariableStoreHeader[VariableStoreTypeMax];
> +
> +  Status = EFI_NOT_FOUND;
> 
>if (VariableNameSize == NULL || VariableName == NULL || VendorGuid ==
> NULL) {
>  return EFI_INVALID_PARAMETER;
>}
> 
> -  OutVariableNameSize   = *VariableNameSize;
> -  InVariableNameSize= StrSize (VariableName);
> -  SmmGetNextVariableName = NULL;
> -
>//
> -  // If input string exceeds SMM payload limit. Return failure
> +  // Calculate the possible maximum length of name string, including the
> Null terminator.
>//
> -  if (InVariableNameSize > mVariableBufferPayloadSize - OFFSET_OF
> (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) {
> +  MaxLen = *VariableNameSize / sizeof (CHAR16);
> +  if ((MaxLen == 0) || (StrnLenS (VariableName, MaxLen) == MaxLen)) {
> +//
> +// Null-terminator is not found in the first VariableNameSize bytes of 
> the
> input VariableName buffer,
> +// follow spec to return EFI_INVALID_PARAMETER.
> +//
>  return EFI_INVALID_PARAMETER;
>}
> 
> -  AcquireLockOnlyAtBootTime();
> +  AcquireLockOnlyAtBootTime ();
> 
> -  //
> -  // Init the communicate buffer. The buffer data size is:
> -  // SMM_COMMUNICATE_HEADER_SIZE +
> SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize.
> -  //
> -  if (OutVariableNameSize > mVariableBufferPayloadSize - OFFSET_OF
> (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) {
> -//
> -// If output buffer exceed SMM payload limit. Trim output buffer to SMM
> payload size
> -//
> -OutVariableNameSize = mVariableBufferPayloadSize - OFFSET_OF
> (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name);
> +  for (DelayIndex = 0; mVariableRuntimeCacheReadLock && DelayIndex <
> VARIABLE_RT_CACHE_READ_LOCK_TIMEOUT; DelayIndex++) {
> +MicroSecondDelay (10);
>}
> -  //
> -  // Payload should be Guid + NameSize + MAX of Input & Output buffer
> -  //
> -  PayloadSize = OFFSET_OF
> (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name) +
> MAX (OutVariableNameSize, InVariableNameSize);
> -
> -  Status = InitCommunicateBuffer ((VOID **),
> PayloadSize, SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME);
> -  if (EFI_ERROR (Status)) {
> -goto Done;
> -  }
> -  ASSERT (SmmGetNextVariableName != NULL);
> -
> -  //
> -  // SMM comm buffer->NameSize is buffer size for return string
> -  //
> -  SmmGetNextVariableName->NameSize = OutVariableNameSize;
> -
> -  CopyGuid (>Guid, VendorGuid);
> -  //
> -  // Copy whole string
> -  //
> -  CopyMem (SmmGetNextVariableName->Name, VariableName,
> InVariableNameSize);
> -  if (OutVariableNameSize > InVariableNameSize) {
> -ZeroMem ((UINT8 *) SmmGetNextVariableName->Name +
> 

Re: [edk2-devel] [PATCH V2 6/9] MdeModulePkg VariableInfo: Always consider RT DXE and SMM stats

[Wu, Hao A]

> -Original Message-
> From: 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, Liming; Kinney,
> Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> Subject: [PATCH V2 6/9] MdeModulePkg VariableInfo: Always consider RT
> DXE and SMM stats
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2220
> 
> The current VariableInfo application only checks for variable
> statistics from SMM if the variable information entries are
> not present in the UEFI System Configuration table as published
> by the DXE UEFI variable driver (VariableRuntimeDxe).
> 
> This change first checks for variable information entries in the
> UEFI System Configuration but always checks for entries in SMM
> as well. If the SMM variable driver is not present, an instance of
> EFI_SMM_VARIABLE_PROTOCOL will not be found and the search for
> SMM variable statistics will be aborted (an SW SMI to get variable
> statistics will not be triggered).
> 
> In the case variable statistics are provided by both a Runtime DXE
> driver (e.g. VariableSmmRuntimeDxe) and a SMM driver (VariableSmm),
> this change will clearly identify statistics from each respective
> driver.
> 
> 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/Application/VariableInfo/VariableInfo.c | 37 ++--
> 
>  1 file changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/MdeModulePkg/Application/VariableInfo/VariableInfo.c
> b/MdeModulePkg/Application/VariableInfo/VariableInfo.c
> index f213471e9a..c04ba18213 100644
> --- a/MdeModulePkg/Application/VariableInfo/VariableInfo.c
> +++ b/MdeModulePkg/Application/VariableInfo/VariableInfo.c
> @@ -3,7 +3,7 @@
>this utility will print out the statistics information. You can use console
>redirection to capture the data.
> 
> -  Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
> +  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
>SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -126,7 +126,7 @@ PrintInfoFromSmm (
>ASSERT (CommBuffer != NULL);
>ZeroMem (CommBuffer, RealCommSize);
> 
> -  Print (L"Non-Volatile SMM Variables:\n");
> +  Print (L"SMM Driver Non-Volatile Variables:\n");
>do {
>  CommSize = RealCommSize;
>  Status = GetVariableStatisticsData (CommBuffer, );
> @@ -155,7 +155,7 @@ PrintInfoFromSmm (
>  }
>} while (TRUE);
> 
> -  Print (L"Volatile SMM Variables:\n");
> +  Print (L"SMM Driver Volatile Variables:\n");
>ZeroMem (CommBuffer, RealCommSize);
>do {
>  CommSize = RealCommSize;
> @@ -207,24 +207,18 @@ UefiMain (
>IN EFI_SYSTEM_TABLE  *SystemTable
>)
>  {
> -  EFI_STATUSStatus;
> +  EFI_STATUSRuntimeDxeStatus;
> +  EFI_STATUSSmmStatus;
>VARIABLE_INFO_ENTRY   *VariableInfo;
>VARIABLE_INFO_ENTRY   *Entry;
> 
> -  Status = EfiGetSystemConfigurationTable (, (VOID
> **));
> -  if (EFI_ERROR (Status) || (Entry == NULL)) {
> -Status = EfiGetSystemConfigurationTable
> (, (VOID **));
> +  RuntimeDxeStatus = EfiGetSystemConfigurationTable (,
> (VOID **) );
> +  if (EFI_ERROR (RuntimeDxeStatus) || (Entry == NULL)) {
> +RuntimeDxeStatus = EfiGetSystemConfigurationTable
> (, (VOID **) );
>}
> 
> -  if (EFI_ERROR (Status) || (Entry == NULL)) {
> -Status = PrintInfoFromSmm ();
> -if (!EFI_ERROR (Status)) {
> -  return Status;
> -}
> -  }
> -
> -  if (!EFI_ERROR (Status) && (Entry != NULL)) {
> -Print (L"Non-Volatile EFI Variables:\n");
> +  if (!EFI_ERROR (RuntimeDxeStatus) && (Entry != NULL)) {
> +Print (L"Runtime DXE Driver Non-Volatile EFI Variables:\n");
>  VariableInfo = Entry;
>  do {
>if (!VariableInfo->Volatile) {
> @@ -242,7 +236,7 @@ UefiMain (
>VariableInfo = VariableInfo->Next;
>  } while (VariableInfo != NULL);
> 
> -Print (L"Volatile EFI Variables:\n");
> +Print (L"Runtime DXE Driver Volatile EFI Variables:\n");
>  VariableInfo = Entry;
>  do {
>if (VariableInfo->Volatile) {
> @@ -258,14 +252,19 @@ UefiMain (
>}
>VariableInfo = VariableInfo->Next;
>  } while (VariableInfo != NULL);
> +  }
> 
> -  } else {
> +  SmmStatus = PrintInfoFromSmm ();
> +
> +  if (EFI_ERROR (RuntimeDxeStatus) && EFI_ERROR (SmmStatus)) {
>  Print (L"Warning: Variable Dxe/Smm driver doesn't enable the feature of
> statistical information!\n");
>  Print (L"If you want to see this info, please:\n");
>  Print (L"  1. Set PcdVariableCollectStatistics as TRUE\n");
>  Print (L"  2. Rebuild Variable Dxe/Smm driver\n");
>  Print (L"  3. Run \"VariableInfo\" cmd again\n");
> +
> +return EFI_NOT_FOUND;
>}
> 
> -  return Status;
> +  return EFI_SUCCESS;
> 

Re: [edk2-devel] [PATCH V2 3/9] MdeModulePkg/Variable: Parameterize VARIABLE_INFO_ENTRY buffer

[Wu, Hao A]

> -Original Message-
> From: 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, Liming; Kinney,
> Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> Subject: [PATCH V2 3/9] MdeModulePkg/Variable: Parameterize
> VARIABLE_INFO_ENTRY buffer
> 
> UpdateVariableInfo () currently accepts parameters regarding updates
> to be made to a global variable of type VARIABLE_INFO_ENTRY. This
> change passes the structure by pointer to UpdateVariableInfo ()
> so structures can be updated outside the fixed global variable.


For:
"... so structures can be updated outside the fixed global variable "

Do you mean:

VARIABLE_INFO_ENTRY structure pointers other than "" can be
passed to UpdateVariableInfo().

Is my understanding correct? If so,
Reviewed-by: Hao A Wu 

Best Regards,
Hao Wu


> 
> 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 | 18
> +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c| 14 +++
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c | 41
> +++-
>  3 files changed, 39 insertions(+), 34 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> index 0d231511ea..6f2000f3ee 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> @@ -286,13 +286,14 @@ GetNextVariableEx (
>the transaction. Data is allocated by this routine, but never
>freed.
> 
> -  @param[in] VariableName   Name of the Variable to track.
> -  @param[in] VendorGuid Guid of the Variable to track.
> -  @param[in] Volatile   TRUE if volatile FALSE if non-volatile.
> -  @param[in] Read   TRUE if GetVariable() was called.
> -  @param[in] Write  TRUE if SetVariable() was called.
> -  @param[in] Delete TRUE if deleted via SetVariable().
> -  @param[in] Cache  TRUE for a cache hit.
> +  @param[in]  VariableName   Name of the Variable to track.
> +  @param[in]  VendorGuid Guid of the Variable to track.
> +  @param[in]  Volatile   TRUE if volatile FALSE if non-volatile.
> +  @param[in]  Read   TRUE if GetVariable() was called.
> +  @param[in]  Write  TRUE if SetVariable() was called.
> +  @param[in]  Delete TRUE if deleted via SetVariable().
> +  @param[in]  Cache  TRUE for a cache hit.
> +  @param[in,out]  VariableInfo   Pointer to a pointer of
> VARIABLE_INFO_ENTRY structures.
> 
>  **/
>  VOID
> @@ -303,7 +304,8 @@ UpdateVariableInfo (
>IN  BOOLEAN Read,
>IN  BOOLEAN Write,
>IN  BOOLEAN Delete,
> -  IN  BOOLEAN Cache
> +  IN  BOOLEAN Cache,
> +  IN OUT VARIABLE_INFO_ENTRY  **VariableInfo
>);
> 
>  #endif
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index 816e8f7b8f..1a57d7e1ba 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -1641,7 +1641,7 @@ UpdateVariable (
>  // go to delete this variable in variable HOB and
>  // try to flush other variables from HOB to flash.
>  //
> -UpdateVariableInfo (VariableName, VendorGuid, FALSE, FALSE, FALSE,
> TRUE, FALSE);
> +UpdateVariableInfo (VariableName, VendorGuid, FALSE, FALSE, FALSE,
> TRUE, FALSE, );
>  FlushHobVariableToFlash (VariableName, VendorGuid);
>  return EFI_SUCCESS;
>}
> @@ -1758,7 +1758,7 @@ UpdateVariable (
>   
>   );
>if (!EFI_ERROR (Status)) {
> -UpdateVariableInfo (VariableName, VendorGuid, Variable->Volatile,
> FALSE, FALSE, TRUE, FALSE);
> +UpdateVariableInfo (VariableName, VendorGuid, Variable->Volatile,
> FALSE, FALSE, TRUE, FALSE, );
>  if (!Variable->Volatile) {
>CacheVariable->CurrPtr->State = State;
>FlushHobVariableToFlash (VariableName, VendorGuid);
> @@ -1777,7 +1777,7 @@ UpdateVariable (
>//
>// Variable content unchanged and no need to update timestamp, just
> return.
>//
> -  UpdateVariableInfo (VariableName, VendorGuid, Variable->Volatile,
> FALSE, TRUE, FALSE, FALSE);
> +  UpdateVariableInfo (VariableName, VendorGuid, Variable->Volatile,
> FALSE, TRUE, FALSE, FALSE, );
>Status = EFI_SUCCESS;
>goto Done;
>  } else if ((CacheVariable->CurrPtr->State == VAR_ADDED) ||
> @@ -2006,7 +2006,7 @@ 

Re: [edk2-devel] [PATCH V2 5/9] MdeModulePkg/Variable: Add a file for NV variable functions

[Wu, Hao A]

> -Original Message-
> From: 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, Liming; Kinney,
> Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> Subject: [PATCH V2 5/9] MdeModulePkg/Variable: Add a file for NV variable
> functions
> 
> This change adds a dedicated file for variable operations specific
> to non-volatile variables. This decreases the overall length of the
> relatively large Variable.c file.


It is not clear to me what are the criteria for moving functions into the
separate new file.

I guess the new file is for functions related with NV variables, but I saw
there are functions like:

InitRealNonVolatileVariableStore
InitEmuNonVolatileVariableStore
InitNonVolatileVariableStore

Not sure if they can be put into the new file as well.

Best Regards,
Hao Wu


> 
> 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/VariableRuntimeDxe.inf
> |  2 ++
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf  |  2
> ++
> 
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
> |  2 ++
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.h|
> 25 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c   | 20 
> +
> -
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c|
> 28 
>  6 files changed, 60 insertions(+), 19 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> index c35e5fe787..08a5490787 100644
> ---
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> +++
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> @@ -36,6 +36,8 @@
>Variable.c
>VariableDxe.c
>Variable.h
> +  VariableNonVolatile.c
> +  VariableNonVolatile.h
>VariableParsing.c
>VariableParsing.h
>PrivilegePolymorphic.h
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> index 626738b9c7..6dc2721b81 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> @@ -45,6 +45,8 @@
>Variable.c
>VariableTraditionalMm.c
>VariableSmm.c
> +  VariableNonVolatile.c
> +  VariableNonVolatile.h
>VariableParsing.c
>VariableParsing.h
>VarCheck.c
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.i
> nf
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.
> inf
> index 1ba8f9ebfb..ca9d23ce9f 100644
> ---
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.i
> nf
> +++
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.
> inf
> @@ -45,6 +45,8 @@
>Variable.c
>VariableSmm.c
>VariableStandaloneMm.c
> +  VariableNonVolatile.c
> +  VariableNonVolatile.h
>VariableParsing.c
>VariableParsing.h
>VarCheck.c
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.h
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.h
> new file mode 100644
> index 00..82572262ef
> --- /dev/null
> +++
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.h
> @@ -0,0 +1,25 @@
> +/** @file
> +  Common variable non-volatile store routines.
> +
> +Copyright (c) 2019, Intel Corporation. All rights reserved.
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef _VARIABLE_NON_VOLATILE_H_
> +#define _VARIABLE_NON_VOLATILE_H_
> +
> +#include "Variable.h"
> +
> +/**
> +  Get non-volatile maximum variable size.
> +
> +  @return Non-volatile maximum variable size.
> +
> +**/
> +UINTN
> +GetNonVolatileMaxVariableSize (
> +  VOID
> +  );
> +
> +#endif
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index 53d797152c..5da2354aa5 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  **/
> 
>  #include "Variable.h"
> +#include "VariableNonVolatile.h"
>  #include "VariableParsing.h"
> 
>  VARIABLE_MODULE_GLOBAL  *mVariableModuleGlobal;
> @@ -3006,25 +3007,6 @@ ReclaimForOS(
>}
>  }
> 
> -/**
> -  Get non-volatile maximum variable size.
> -
> -  @return Non-volatile maximum variable size.
> -
> -**/
> -UINTN
> -GetNonVolatileMaxVariableSize (
> -  VOID
> -  )
> -{
> -  if (PcdGet32 (PcdHwErrStorageSize) != 0) {
> -return MAX (MAX (PcdGet32 (PcdMaxVariableSize), 

Re: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add local auth status in VariableParsing

[Wu, Hao A]

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, Liming; Kinney,
> Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> Subject: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add local
> 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 status 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 authenticated
> variables are supported
> +
> +  @retval  EFI_SUCCESS Initialized successfully
> +  @retval  Others  An error occurred during 
> initialization
> +**/
> +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 well.


> +  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 = PcdGet32
> (PcdMaxVariableSize);
>mVariableModuleGlobal->MaxAuthVariableSize = ((PcdGet32
> (PcdMaxAuthVariableSize) != 0) ? PcdGet32 (PcdMaxAuthVariableSize) :
> mVariableModuleGlobal->MaxVariableSize);
> 
> +  Status = 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 = FALSE;
  ...
  //
  // Restore AuthFormat
  //
  mVariableModuleGlobal->VariableGlobal.AuthFormat = 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 variable
> format!\n"));
> -//
> -// Set AuthSupport to FALSE first, VariableWriteServiceInitialize() will
> initialize it.
> -//
> -mVariableModuleGlobal->VariableGlobal.AuthSupport = FALSE;
>  VariableGuid = 
>} else {
>  DEBUG 

Re: [edk2-devel] [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize GetNextVariableEx() store list

[Wu, Hao A]

A couple of inline comments below:


> -Original Message-
> From: 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, Liming; Kinney,
> Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> Subject: [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize
> GetNextVariableEx() store list
> 
> The majority of logic related to GetNextVariableName () is currently
> implemented in VariableServiceGetNextVariableInternal (). The list
> of variable stores to search for the given variable name and variable
> GUID is defined in the function body. This change renames the function
> to GetNextVariableEx () since the function is no longer internal to
> a specific source file and adds a new parameter so that the caller
> must pass in the list of variable stores to be used in the variable
> search.


I am not sure if 'GetNextVariableEx' is a good name for the function, since:

1. There is no function named GetNextVariable(), so it is not clear what "Ex"
   means here.

2. The origin function VariableServiceGetNextVariableInternal() does get used
   in multiple source files (Variable.c & VariableExLib.c). I am not sure what
   is the intention for such renaming.


> 
> 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 | 15 ++-
> -
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c| 12 ++-
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c   |  8 +-
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c | 78
> 
>  4 files changed, 73 insertions(+), 40 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> index 9d77c4916c..0d231511ea 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> @@ -248,27 +248,30 @@ FindVariableEx (
>);
> 
>  /**
> -  This code Finds the Next available variable.
> +  This code finds the next available variable.
> 
>Caution: This function may receive untrusted input.
>This function may be invoked in SMM mode. This function will do basic
> validation, before parse the data.
> 
> -  @param[in]  VariableName  Pointer to variable name.
> -  @param[in]  VendorGuidVariable Vendor Guid.
> -  @param[out] VariablePtr   Pointer to variable header address.
> +  @param[in]  VariableName  Pointer to variable name.
> +  @param[in]  VendorGuidVariable Vendor Guid.
> +  @param[in]  VariableStoreList A list of variable stores that should be used
> to get the next variable.
> +The maximum number of entries is the max 
> value of
> VARIABLE_STORE_TYPE.
> +  @param[out] VariablePtr   Pointer to variable header address.
> 
>@retval EFI_SUCCESS   The function completed successfully.
>@retval EFI_NOT_FOUND The next variable was not found.
> -  @retval EFI_INVALID_PARAMETER If VariableName is not an empty string,
> while VendorGuid is NULL.
> +  @retval EFI_INVALID_PARAMETER If VariableName is nt an empty string,
> while VendorGuid is NULL.
>@retval EFI_INVALID_PARAMETER The input values of VariableName and
> VendorGuid are not a name and
>  GUID of an existing variable.
> 
>  **/
>  EFI_STATUS
>  EFIAPI
> -VariableServiceGetNextVariableInternal (
> +GetNextVariableEx (
>IN  CHAR16*VariableName,
>IN  EFI_GUID  *VendorGuid,
> +  IN  VARIABLE_STORE_HEADER **VariableStoreList,
>OUT VARIABLE_HEADER   **VariablePtr
>);
> 
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index 76536308e6..816e8f7b8f 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -2358,6 +2358,7 @@ VariableServiceGetNextVariableName (
>UINTN   MaxLen;
>UINTN   VarNameSize;
>VARIABLE_HEADER *VariablePtr;
> +  VARIABLE_STORE_HEADER
> *VariableStoreHeader[VariableStoreTypeMax];
> 
>if (VariableNameSize == NULL || VariableName == NULL || VendorGuid ==
> NULL) {
>  return EFI_INVALID_PARAMETER;
> @@ -2377,7 +2378,16 @@ VariableServiceGetNextVariableName (
> 
>AcquireLockOnlyAtBootTime(
> >VariableGlobal.VariableServicesLock);
> 
> -  Status = VariableServiceGetNextVariableInternal (VariableName,
> VendorGuid, );
> +  //
> +  // 0: Volatile, 1: HOB, 2: Non-Volatile.
> +  // The index and attributes mapping must be kept in this order as
> FindVariable
> +  // makes use of this 

Re: [edk2-devel] [PATCH V2 1/9] MdeModulePkg/Variable: Consolidate common parsing functions

[Wu, Hao A]

A couple of inline comments below:


> -Original Message-
> From: 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, Liming; Kinney,
> Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> Subject: [PATCH V2 1/9] MdeModulePkg/Variable: Consolidate common
> parsing functions
> 
> This change moves the following functions into a dedicated file
> so they may be used in other variable files as needed. Furthermore,
> it reduces the overall size of the common Variable.c file.
> 
>  * DataSizeOfVariable ()
>  * FindVariableEx ()
>  * GetEndPointer ()
>  * GetNextVariablePtr ()
>  * GetStartPointer ()
>  * GetVariableDataOffset ()
>  * GetVariableDataPtr ()
>  * GetVariableHeaderSize ()
>  * GetVariableNamePtr ()
>  * GetVariableStoreStatus ()
>  * GetVendorGuidPtr ()
>  * IsValidVariableHeader ()
>  * NameSizeOfVariable ()
>  * SetDataSizeOfVariable ()
>  * SetNameSizeOfVariable ()
>  * UpdateVariableInfo ()
>  * VariableCompareTimeStampInternal ()
>  * VariableServiceGetNextVariableInternal ()


May I know what are the criteria for the above functions being moved to a
separate file?

At first, I think all of them will be consumed by the new codes that implement
the runtime cache. But I found that, for functions like GetVariableDataOffset(),
GetVariableStoreStatus() and etc., their usages are still remained within file
Variable.c (seems not related with the runtime cache).


> 
> 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/VariableRuntimeDxe.inf
> |   2 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf  |   2
> +
> 
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
> |   7 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h   | 119 
> 
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h| 306
> 
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c   | 726 +--
> 
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c  |   3 +-
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c| 731
> 
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c|   1 +
>  9 files changed, 1052 insertions(+), 845 deletions(-)


For the below change in VariableStandaloneMm.inf:

[Guids]
...
  ## SOMETIMES_CONSUMES   ## Variable:L"db"
  ## SOMETIMES_CONSUMES   ## Variable:L"dbx"
  ## SOMETIMES_CONSUMES   ## Variable:L"dbt"
  gEfiImageSecurityDatabaseGuid
...

I think the above GUID is not used by the module specified by the above INF.
Could you double confirm on this?

Best Regards,
Hao Wu


> 
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> index 641376c9c5..c35e5fe787 100644
> ---
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> +++
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> @@ -36,6 +36,8 @@
>Variable.c
>VariableDxe.c
>Variable.h
> +  VariableParsing.c
> +  VariableParsing.h
>PrivilegePolymorphic.h
>Measurement.c
>TcgMorLockDxe.c
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> index 0a160d269d..626738b9c7 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> @@ -45,6 +45,8 @@
>Variable.c
>VariableTraditionalMm.c
>VariableSmm.c
> +  VariableParsing.c
> +  VariableParsing.h
>VarCheck.c
>Variable.h
>PrivilegePolymorphic.h
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.i
> nf
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.
> inf
> index 21bc81163b..1ba8f9ebfb 100644
> ---
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.i
> nf
> +++
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.
> inf
> @@ -45,6 +45,8 @@
>Variable.c
>VariableSmm.c
>VariableStandaloneMm.c
> +  VariableParsing.c
> +  VariableParsing.h
>VarCheck.c
>Variable.h
>PrivilegePolymorphic.h
> @@ -99,6 +101,11 @@
>## SOMETIMES_PRODUCES   ## Variable:L"Lang"
>gEfiGlobalVariableGuid
> 
> +  ## SOMETIMES_CONSUMES   ## Variable:L"db"
> +  ## SOMETIMES_CONSUMES   ## Variable:L"dbx"
> +  ## SOMETIMES_CONSUMES   ## Variable:L"dbt"
> +  gEfiImageSecurityDatabaseGuid
> +
>gEfiMemoryOverwriteControlDataGuid## SOMETIMES_CONSUMES
> ## Variable:L"MemoryOverwriteRequestControl"
>gEfiMemoryOverwriteRequestControlLockGuid ##
> 

Re: [edk2-devel] [edk2-platforms] [PATCH V1] KabylakeOpenBoardPkg: Wrong instance of PlatformSecLib is used.

[Nate DeSimone]

Good catch, thank you!

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Philippe 
Mathieu-Daudé
Sent: Thursday, October 3, 2019 12:40 AM
To: devel@edk2.groups.io; Desimone, Nathaniel L 
Cc: Chiu, Chasel ; Kubacki, Michael A 
; Jeremy Soller 
Subject: Re: [edk2-devel] [edk2-platforms] [PATCH V1] KabylakeOpenBoardPkg: 
Wrong instance of PlatformSecLib is used.

On 10/2/19 9:40 AM, Nate DeSimone wrote:
> The GalagoPro3 platform in KabylakeOpenBoardPkg is using the instance 
> of PlatformSecLib in MinPlatformPkg instead of the instance in 
> KabylakeOpenBoardPkg. The version in MinPlatformPkg does not support 
> FSP 2.1 Dispatch Mode, whearas the version in

typo: "whereas"

> KabylakeOpenBoardPkg does.
> 
> Cc: Chasel Chiu 
> Cc: Michael Kubacki 
> Cc: Jeremy Soller 
> Signed-off-by: Nate DeSimone 
> ---
>   Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git 
> a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc 
> b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc
> index d67e0cc000..f3dd2b0c91 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc
> @@ -85,7 +85,7 @@
> 
> PlatformHookLib|$(PROJECT)/Library/BasePlatformHookLib/BasePlatformHoo
> kLib.inf
>   
> 
> FspWrapperHobProcessLib|$(PLATFORM_PACKAGE)/FspWrapper/Library/PeiFspW
> rapperHobProcessLib/PeiFspWrapperHobProcessLib.inf
> -  
> PlatformSecLib|$(PLATFORM_PACKAGE)/FspWrapper/Library/SecFspWrapperPla
> tformSecLib/SecFspWrapperPlatformSecLib.inf
> +  
> + PlatformSecLib|$(PLATFORM_BOARD_PACKAGE)/FspWrapper/Library/SecFspWr
> + apperPlatformSecLib/SecFspWrapperPlatformSecLib.inf
>   
> 
> FspWrapperApiLib|IntelFsp2WrapperPkg/Library/BaseFspWrapperApiLib/BaseFspWrapperApiLib.inf
> 
> FspWrapperApiTestLib|IntelFsp2WrapperPkg/Library/PeiFspWrapperApiTestL
> ib/PeiFspWrapperApiTestLib.inf
> 





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48414): https://edk2.groups.io/g/devel/message/48414
Mute This Topic: https://groups.io/mt/34367442/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms] [PATCH V1] KabylakeOpenBoardPkg: Wrong instance of PlatformSecLib is used.

[Nate DeSimone]

Thanks Michael. Another thing we should consider doing a some point in the 
future is moving the PlatformSecLib implementation that supports dispatch mode 
from KabylakeOpenBoardPkg to MinPlatformPkg.

-Original Message-
From: Kubacki, Michael A  
Sent: Wednesday, October 2, 2019 6:35 PM
To: devel@edk2.groups.io; Desimone, Nathaniel L 
Cc: Chiu, Chasel ; Jeremy Soller 
Subject: RE: [edk2-devel] [edk2-platforms] [PATCH V1] KabylakeOpenBoardPkg: 
Wrong instance of PlatformSecLib is used.

Reviewed-by: Michael Kubacki 

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Nate 
> DeSimone
> Sent: Wednesday, October 2, 2019 12:41 AM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel ; Kubacki, Michael A 
> ; Jeremy Soller 
> Subject: [edk2-devel] [edk2-platforms] [PATCH V1] KabylakeOpenBoardPkg:
> Wrong instance of PlatformSecLib is used.
> 
> The GalagoPro3 platform in KabylakeOpenBoardPkg is using the instance 
> of PlatformSecLib in MinPlatformPkg instead of the instance in 
> KabylakeOpenBoardPkg. The version in MinPlatformPkg does not support 
> FSP 2.1 Dispatch Mode, whearas the version in KabylakeOpenBoardPkg does.
> 
> Cc: Chasel Chiu 
> Cc: Michael Kubacki 
> Cc: Jeremy Soller 
> Signed-off-by: Nate DeSimone 
> ---
>  Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc | 2
> +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git
> a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc
> b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc
> index d67e0cc000..f3dd2b0c91 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc
> +++
> b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc
> @@ -85,7 +85,7 @@
> 
> PlatformHookLib|$(PROJECT)/Library/BasePlatformHookLib/BasePlatformHo
> okLib.inf
> FspWrapperHobProcessLib|$(PLATFORM_PACKAGE)/FspWrapper/Library/P
> eiFspWrapperHobProcessLib/PeiFspWrapperHobProcessLib.inf-
> PlatformSecLib|$(PLATFORM_PACKAGE)/FspWrapper/Library/SecFspWrapp
> erPlatformSecLib/SecFspWrapperPlatformSecLib.inf+
> PlatformSecLib|$(PLATFORM_BOARD_PACKAGE)/FspWrapper/Library/SecFs
> pWrapperPlatformSecLib/SecFspWrapperPlatformSecLib.inf
> FspWrapperApiLib|IntelFsp2WrapperPkg/Library/BaseFspWrapperApiLib/Ba
> seFspWrapperApiLib.inf
> FspWrapperApiTestLib|IntelFsp2WrapperPkg/Library/PeiFspWrapperApiTest
> Lib/PeiFspWrapperApiTestLib.inf--
> 2.23.0.windows.1
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#48362): 
> https://edk2.groups.io/g/devel/message/48362
> Mute This Topic: https://groups.io/mt/34367442/1772268
> Group Owner: devel+ow...@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [michael.a.kuba...@intel.com] -=-=-=-=-=-=



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48413): https://edk2.groups.io/g/devel/message/48413
Mute This Topic: https://groups.io/mt/34367442/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms][PATCH V1 0/3] Add FW Boot Media Device Indicator

[Kubacki, Michael A]

Thanks for sharing. You articulated well why I also don't think this is 
required in MdePkg. My
description of the reset vector problem was kind of incomplete. While the x86 
reset vector
is fixed at the top of the address space, that does not mean that a FV has to 
be mapped to it
(or any SPI flash based content). SEC has made that assumption. For example, 
the top
address space could be mapped to another random access memory with x86 
instructions.
In this case, assumptions previously made in SecCore such as calculating the 
BFV size as follows are invalid:
  SecCoreData.BootFirmwareVolumeSize = (UINTN)(0x1ULL - (UINTN) 
BootFirmwareVolume);

In a system with NAND media, the BFV could be located in CAR at a base of 
4GB-xKB/MB with
execution entry via a jump from the code at top of memory. In this particular 
case, the FvLength
field can more reliably determine the FV size.

These patches use a GUIDed HOB. It just hides that detail behind a function API 
largely added
for convenience so firmware boot media consumers do not need to handle the HOB 
related logic.

Thanks,
Michael

From: af...@apple.com 
Sent: Wednesday, October 2, 2019 9:20 PM
To: devel@edk2.groups.io; Kubacki, Michael A 
Cc: Chaganty, Rangasai V ; Dong, Eric 
; Gao, Liming ; Ni, Ray 

Subject: Re: [edk2-devel] [edk2-platforms][PATCH V1 0/3] Add FW Boot Media 
Device Indicator

Since I was around back in the Intel Tiano days and I've worked on all the PI 
specs I can share the history.

The reset vector is a hardware thing. It is usually at the top or bottom of the 
address space. For x86 it is at the TOP of the ROM and that is why the FV has a 
VoluteTop file GUID that places the file at the end of the FV, thus the end of 
that file is the reset vector. If the reset vector is low then the FV header 
starts with a 16 byte ZeroVector that can contain the reset jump instruction. 
The other architecture that is out there is a mask ROM, and the mask ROM is the 
reset vector, and that code hands off to the 2nd level, and sometimes that can 
be in DRAM or ROM.

The PI FVs (Firmware Volumes) are abstracted by Firmware Volume Blocks (FVB) 
the FVB instances abstract the storage media for the firmware. Thus from a PI 
spec point of view there is not need to define the storage type of the "ROM". 
That is just an implementation detail, that needs to be managed by the 
implementation.

If you think about in UEFI disk are abstracted by the Block IO protocol. We can 
boot an OS from any Block device, and the firmware does not need to know what 
kind of disk it is. The purpose of the Block IO protocol is to abstract all 
possible implementations o block devices. For the "Boot ROM", FVB is the same 
kind of abstraction. So you can implement PI with out knowing the media type. 
Managing the media type was is a platform abstraction, thus there is no need 
for a boot media abstraction in the MdePkg.

That being said the PI architecture is abstract to a fault. It was designed to 
abstract all possible future platforms. If there are a family of platforms that 
share common properties it makes sense to build a platform abstraction package 
that a set of platforms can share. This is the intent of the PI architecture 
and the EDKII source base.

The MdePkg implements the UEFI and PI specs, and other industry standards, with 
some common libs thrown in. So the MdePkg is not the place to put some 
implementation hint about the Media Device. You could use a GUIDed HOB for that 
if needed?

Thanks,

Andrew Fish


On Oct 2, 2019, at 8:02 PM, Kubacki, Michael A 
mailto:michael.a.kuba...@intel.com>> wrote:

In platforms built for boot media other than SPI flash there has been a 
compelling
need for silicon and platform code to be aware of the firmware boot media but
apart from the UEFI variable driver (which is a special case being addressed
here - https://github.com/makubacki/edk2/tree/storage_agnostic_uefi_variables),
this has not been the case for edk2 repository packages. In some cases, code in 
SEC
has made assumptions about the reset vector or FIT pointer that do not 
necessarily
translate to storage media that does not support MMIO. These cases have been
handled more gracefully than checking the firmware boot media technology. This 
is
just an observation, not necessarily a case for it to stay in IntelSiliconPkg 
(which
does make it accessible to Intel silicon and platform code).

I suppose the firmware boot media properties could be treated in a similar 
manner
to Boot Mode as defined in the Platform Initialization spec. If this was done, 
it may
make more sense to abstract the technology impact onto firmware, for example,
whether it supports MMIO or not (NOR vs NAND flash) instead of what is defined
here with specific technologies such as eMMC and UFS. Otherwise, the 
specification
describing this would be subject to constant expansion over time keeping pace 
with
new technologies and cross-generation code (not silicon or platform specific 
drivers)

Re: [edk2-devel] [edk2-platforms] [PATCH V1] KabylakeOpenBoardPkg: Wrong instance of PlatformSecLib is used.

[Philippe Mathieu-Daudé]

On 10/2/19 9:40 AM, Nate DeSimone wrote:

The GalagoPro3 platform in KabylakeOpenBoardPkg is using the
instance of PlatformSecLib in MinPlatformPkg instead of the
instance in KabylakeOpenBoardPkg. The version in MinPlatformPkg
does not support FSP 2.1 Dispatch Mode, whearas the version in


typo: "whereas"


KabylakeOpenBoardPkg does.

Cc: Chasel Chiu 
Cc: Michael Kubacki 
Cc: Jeremy Soller 
Signed-off-by: Nate DeSimone 
---
  Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc 
b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc
index d67e0cc000..f3dd2b0c91 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc
+++ b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/OpenBoardPkg.dsc
@@ -85,7 +85,7 @@

PlatformHookLib|$(PROJECT)/Library/BasePlatformHookLib/BasePlatformHookLib.inf
  
FspWrapperHobProcessLib|$(PLATFORM_PACKAGE)/FspWrapper/Library/PeiFspWrapperHobProcessLib/PeiFspWrapperHobProcessLib.inf

-  
PlatformSecLib|$(PLATFORM_PACKAGE)/FspWrapper/Library/SecFspWrapperPlatformSecLib/SecFspWrapperPlatformSecLib.inf
+  
PlatformSecLib|$(PLATFORM_BOARD_PACKAGE)/FspWrapper/Library/SecFspWrapperPlatformSecLib/SecFspWrapperPlatformSecLib.inf
  
FspWrapperApiLib|IntelFsp2WrapperPkg/Library/BaseFspWrapperApiLib/BaseFspWrapperApiLib.inf


FspWrapperApiTestLib|IntelFsp2WrapperPkg/Library/PeiFspWrapperApiTestLib/PeiFspWrapperApiTestLib.inf




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48411): https://edk2.groups.io/g/devel/message/48411
Mute This Topic: https://groups.io/mt/34367442/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-