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

[Wu, Hao A]

Inline replies below:


> -Original Message-
> From: Kubacki, Michael A
> Sent: Friday, October 04, 2019 5:54 AM
> 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 caches will now be present.
> > > 1. "Runtime Cache" - Maintained in VariableSmmRuntimeDxe. Used to
> > > service
> > >Runtime Services GetVariable () and GetNextVariableName () callers.
> > > 2. "SMM Cache" - Maintained 

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

[Kubacki, Michael A]

> On 10/03/19 23:53, Kubacki, Michael A wrote:
> > #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.
> 
> Thanks!
> 
> (I'm also happy with the lock / timeout resolution. I had known about the
> reentrancy restriction in the UEFI spec (I happened to look at something in
> the kernel just the other day that reminded me of that part of the spec), but
> it wasn't clear to me that the lock + timeout in the patch series were
> intended to protect against just that. Kudos to Andrew for the comment!)
> 
> (
> 
> Meanwhile, I've received help from my colleagues wrt.
> QueryVariableInfo(), but right now it's too early to tell if we'll be able to
> settle on that in the long term:
> 
> [PATCH] efi/efi_test: require CAP_SYS_ADMIN to open the chardev
> http://mid.mail-archive.com/20191003100712.31045-1-javierm@redhat.com
> 
> )
> 

Thanks for the QueryVariableInfo () update.

> For the next version of this edk2 patch set (where you plan to include the
> new FeaturePCD, if I understand correctly), I'd like to request the
> following: either set the DEC default to FALSE please, or please include a
> patch for OvmfPkg where you set the PCD to FALSE in all the OvmfPkg DSC
> files.
> 
> I think the next stable release should be made like that. Then, for the stable
> release following that, we can re-evaluate the question, and might decide to
> invert the PCD in OVMF (enabling the feature), assuming
> QueryVariableInfo() proves usable in Fedora, by then.
> 
> 

I'd like to propose the DEC default be set to TRUE and I make the changes in
all the OvmfPkg DSC files to set the PCD to FALSE. 

> Two independent questions:
> 
> - Has this work been regression-tested on ARM / AARCH64? (For example,
> ArmVirtPkg platforms use the unified runtime DXE driver, not the split
> runtime/SMM drivers. So no change in behavior is expected; we should test
> that.)
> 
> In the "Testing Performed" section of your blurb, item#3 suggests something
> similar ("Boot from S5 to EFI shell with DXE variables enabled"), but I 
> figured
> I'd raise AARCH64 specifically.
> 
> 

I have not tested on ARM / AARCH64. I will add this test for V3.

> - Can you please confirm that the handling of *volatile* variables is not
> affected? ArmVirtPkg and OvmfPkg use quite different sizes for volatile and
> non-volatile variables; see:
> 
>   - 9c7d0d499296 ("OvmfPkg/TlsAuthConfigLib: configure trusted CA certs
>   for HTTPS boot", 2018-03-30)
>   - ffe048a0807b ("ArmVirtPkg: handle NETWORK_TLS_ENABLE in
>   ArmVirtQemu*", 2019-06-28)
> 

The handling of volatile variables will not be affected.

> Thank you!
> Laszlo

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

View/Reply Online (#48479): https://edk2.groups.io/g/devel/message/48479
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 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache support

[Kubacki, Michael A]

> On 10/04/19 01:31, Kubacki, Michael A wrote:
> > I agree, I will make the default to enable the runtime cache.
> 
> I've just made a request for the opposite :) , before reading this part
> of the thread.
> 
> Let me revise my request then, seeing the above preference. From the
> following three options:
> 
> (1) set DEC default to FALSE,
> 
> (2) set the DEC default to TRUE, but set the PCD in OvmfPkg DSC files to
> FALSE,
> 
> (3) set the DEC default to TRUE, and inherit it in OvmfPkg,
> 
> I'd ask for (2) in the short or mid term, and entertain (3) in the long
> term (dependent on the upstream kernel patch I linked elsewhere in the
> thread:
>  javi...@redhat.com>).
> 
> 
> With the PCD available in the DEC file, I agree that downstream OVMF
> packages could easily be built with the PCD set to FALSE, e.g. on the
> "build" command line, regardless of the upstream OvmfPkg DSC setting.
> Given that, option (2) might appear superfluous.
> 
> However, I'd like upstream OvmfPkg's DSC setting to reflect that as yet,
> there is no alternative to GetVariable(), for exercising the SMM stack
> built into OVMF without side effects to the variable store. Once the
> kernel patch is merged and QueryVariableInfo() becomes *recommendable*
> as a practical substitute for GetVariable(), we can switch upstream
> OvmfPkg from option (2) to option (3).
> 
> Does that sound acceptable?
> 

I have no problem setting the PCD to FALSE in all the OvmfPkg DSC files. 

> Thanks!
> Laszlo
> 
> 
> >
> >> -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-
> > 

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

[Laszlo Ersek]

On 10/04/19 01:31, Kubacki, Michael A wrote:
> I agree, I will make the default to enable the runtime cache.

I've just made a request for the opposite :) , before reading this part
of the thread.

Let me revise my request then, seeing the above preference. From the
following three options:

(1) set DEC default to FALSE,

(2) set the DEC default to TRUE, but set the PCD in OvmfPkg DSC files to
FALSE,

(3) set the DEC default to TRUE, and inherit it in OvmfPkg,

I'd ask for (2) in the short or mid term, and entertain (3) in the long
term (dependent on the upstream kernel patch I linked elsewhere in the
thread:
).


With the PCD available in the DEC file, I agree that downstream OVMF
packages could easily be built with the PCD set to FALSE, e.g. on the
"build" command line, regardless of the upstream OvmfPkg DSC setting.
Given that, option (2) might appear superfluous.

However, I'd like upstream OvmfPkg's DSC setting to reflect that as yet,
there is no alternative to GetVariable(), for exercising the SMM stack
built into OVMF without side effects to the variable store. Once the
kernel patch is merged and QueryVariableInfo() becomes *recommendable*
as a practical substitute for GetVariable(), we can switch upstream
OvmfPkg from option (2) to option (3).

Does that sound acceptable?

Thanks!
Laszlo


> 
>> -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
>
>
>>> 

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

[Laszlo Ersek]

On 10/03/19 23:53, Kubacki, Michael A wrote:
> #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.

Thanks!

(I'm also happy with the lock / timeout resolution. I had known about
the reentrancy restriction in the UEFI spec (I happened to look at
something in the kernel just the other day that reminded me of that part
of the spec), but it wasn't clear to me that the lock + timeout in the
patch series were intended to protect against just that. Kudos to Andrew
for the comment!)

(

Meanwhile, I've received help from my colleagues wrt.
QueryVariableInfo(), but right now it's too early to tell if we'll be
able to settle on that in the long term:

[PATCH] efi/efi_test: require CAP_SYS_ADMIN to open the chardev
http://mid.mail-archive.com/20191003100712.31045-1-javierm@redhat.com

)

For the next version of this edk2 patch set (where you plan to include
the new FeaturePCD, if I understand correctly), I'd like to request the
following: either set the DEC default to FALSE please, or please include
a patch for OvmfPkg where you set the PCD to FALSE in all the OvmfPkg
DSC files.

I think the next stable release should be made like that. Then, for the
stable release following that, we can re-evaluate the question, and
might decide to invert the PCD in OVMF (enabling the feature), assuming
QueryVariableInfo() proves usable in Fedora, by then.


Two independent questions:

- Has this work been regression-tested on ARM / AARCH64? (For example,
ArmVirtPkg platforms use the unified runtime DXE driver, not the split
runtime/SMM drivers. So no change in behavior is expected; we should
test that.)

In the "Testing Performed" section of your blurb, item#3 suggests
something similar ("Boot from S5 to EFI shell with DXE variables
enabled"), but I figured I'd raise AARCH64 specifically.


- Can you please confirm that the handling of *volatile* variables is
not affected? ArmVirtPkg and OvmfPkg use quite different sizes for
volatile and non-volatile variables; see:

  - 9c7d0d499296 ("OvmfPkg/TlsAuthConfigLib: configure trusted CA certs
  for HTTPS boot", 2018-03-30)
  - ffe048a0807b ("ArmVirtPkg: handle NETWORK_TLS_ENABLE in
  ArmVirtQemu*", 2019-06-28)

Thank you!
Laszlo

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

View/Reply Online (#48469): https://edk2.groups.io/g/devel/message/48469
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 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 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 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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Kubacki, Michael A]

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 the SMM variable driver (SMM Cache hits). SMM Cache hits for
GetVariable () will occur when SMM modules invoke GetVariable ().

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/VariableSmmRuntimeDxe.inf |  31 +-
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf  |   2 +
 MdeModulePkg/Include/Guid/SmmVariableCommon.h|  29 +-
 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h|  39 +-
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.h|  47 ++
 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c|  44 +-
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.c| 153 
+
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 114 +++-
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c   | 608 
+---
 11 files changed, 966 insertions(+), 105 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf 
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
index 08a5490787..ceea5d1ff9 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
@@ -40,6 +40,8 @@
   VariableNonVolatile.h
   VariableParsing.c
   VariableParsing.h
+  VariableRuntimeCache.c
+  VariableRuntimeCache.h
   PrivilegePolymorphic.h
   Measurement.c
   TcgMorLockDxe.c
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf 
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
index 6dc2721b81..bc3033588d 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
@@ -49,6 +49,8 @@
   VariableNonVolatile.h
   VariableParsing.c
   VariableParsing.h
+  VariableRuntimeCache.c
+  VariableRuntimeCache.h
   VarCheck.c
   Variable.h
   PrivilegePolymorphic.h
diff --git 
a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf 
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
index 14894e6f13..70837ac6e0 100644
---