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

["Laszlo Ersek" <lersek@redhat.com>]

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