Re: multiple levels of support for MOR / MORLock

["Yao, Jiewen" <jiewen.yao@intel.com>]

Agree. Adding a hook function is OK for me, as long as the hook function implementation is in TcgMorLockXXX.c

Thank you
Yao Jiewen


From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Saturday, September 30, 2017 12:39 AM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Ladi Prosek <lprosek@redhat.com>
Subject: Re: [edk2] multiple levels of support for MOR / MORLock

On 09/29/17 15:26, Yao, Jiewen wrote:
> Sure. Feel free to submit patch.
>
> For the fix, I suggest we keep all MORL related code into TcgMorLockXXX.c.
> I do not suggest we mix the MORL stuff into VariableXXX.c.

I've had more thoughts about this, since sending my last email.

Currently SmmEndOfDxeCallback()
[MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c] performs the
following actions:

- sets "mEndOfDxe" to TRUE
- calls VarCheckLibInitializeAtEndOfDxe()
- calls InitializeVariableQuota()
- calls ReclaimForOS() optionally

If we register a separate callback for gEfiSmmEndOfDxeProtocolGuid, in
order to create the MORL variable, then that callback could be invoked
after SmmEndOfDxeCallback(). I think it would not be optimal to create
MORL after SmmEndOfDxeCallback() has run, even if it worked, technically
speaking. This suggests to hook the MORL creation to the top of
SmmEndOfDxeCallback(). What do you suggest?

> A reminder that, the PRC team will have the PRC national holiday in next week. As such, we won't have too much activity in next week.
>
> If you submit the patch, I will try to review in next week to unblock the Linux work.

Ah, sorry, I haven't been aware of the holidays.

I might send a patch soon, but please don't let me disturb you during
the holidays! Especially because this patch might need more discussion
and more versins. Holidays are for getting rested from work! :) We can
sort it out after you return.

> In the patch, if you can add the detailed test result, that will be very helpful for us.
> For example, test SMM/non-SMM version, test with MOR present/absent, etc.

For the "no MOR, no MORL" case, I'll have to rely on Ladi, for test
results (Ladi offered to help test that case, thanks again for that, Ladi).

However, for regression-testing the "both MOR and MORL" case, I can only
rely on Intel developers and quality engineers -- OVMF doesn't have any
TPM / MOR solution at the moment. (If we had one, then we wouldn't have
found or reported this issue in the first place. :) )

The fact that I can't regression-test the MOR+MORL case myself supports
the idea that we should postpone the review and commit (if appropriate)
of the patch until after the PRC national holiday.

Thanks!
Laszlo


> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Friday, September 29, 2017 7:06 PM
> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> Cc: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
>
> On 09/29/17 03:52, Yao, Jiewen wrote:
>> Thanks Laszlo.
>>
>> Yes, I agree it is bug. Would you please help to file a bugzilar in EDKII?
>>
>> For the fix, I think we have a way to resolve it without PCD. (I do not want to bother a platform developer to set a new PCD.)
>>
>> The only invalid case we need handle is: MOR is absent, but MORL is present.
>>
>> My thought is to let Variable driver check if MOR is present. Variable driver can defer the MORL setting at EndOfDxe event based upon the presence of MOR. If MOR driver is present, it sets MOR at entrypoint. EndOfDxe is good enough to know the state.
>>
>> Also, because EndOfDxe is PI event, the UEFI OS is not aware of that.
>
> Sounds great; thanks a lot!
>
> I've filed:
>
>   https://bugzilla.tianocore.org/show_bug.cgi?id=727
>
> If possible I'd like the fix to be committed sometime next week. Is it
> OK if I try and submit a patch soon? My plan is the following:
>
> - In MorLockInit() [TcgMorLockSmm.c], call
> gSmst->SmmRegisterProtocolNotify() in order to register a callback for
> gEfiSmmEndOfDxeProtocolGuid
>
> - In the callback function, call VariableServiceGetVariable(), with size
> 0, to see if MOR is present -- if EFI_BUFFER_TOO_SMALL is returned, MOR
> is present; otherwise MOR is absent.
>
> - If MOR is present, then call SetMorLockVariable(0) from the callback
> function; like MorLockInit() does now. Otherwise, do nothing.
>
> - It looks like there are no circumstances under which I should
> de-register the callback. (I.e. call SmmRegisterProtocolNotify() with a
> NULL Function argument.)
>
>
> Now, I can see that VariableSmm.c already installs such a callback --
> SmmEndOfDxeCallback(). Should I hook into that callback function through
> a new BOOLEAN variable, such as "mDelayedMorLockInit" (and then
> MorLockInit() would only set this variable to TRUE), or should I install
> a separate callback?
>
> Either way, I don't think that I should do the MOR/MORL stuff in the
> current SmmEndOfDxeCallback() function *unconditionally*, because that
> callback is set up when the *read* half of the variable services is
> initialized, but MORL only becomes relevant when the *write* half of the
> variable services is initialized (which occurs in the
> SmmFtwNotificationEvent() callback, i.e. when the FaultTolerantWrite SMM
> protocol becomes available). Hence I think we need either a separate
> callback registration, or a new boolean for the existent callback.
>
> Thanks!
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> https://lists.01.org/mailman/listinfo/edk2-devel
>