Re: [edk2-devel] [PATCH V2 0/4] Add a pcd PcdBootManagerInBootOrder to control whether BootManager is in BootOrder

["Gao, Zhichao" <zhichao.gao@intel.com>]

Hi Laszlo,

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Friday, July 19, 2019 10:15 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> Michael Turner <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: Re: [edk2-devel] [PATCH V2 0/4] Add a pcd
> PcdBootManagerInBootOrder to control whether BootManager is in
> BootOrder
> 
> Hi Zhichao,
> 
> On 07/19/19 10:09, Zhichao Gao wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1979
> >
> > V1:
> > UEFI spec 2.8 introduce a new variable L"RuntimeServicesSupported". If
> > some runtime sevices is not supported at runtime phase, the variable
> > should present at boot services. It is a bitmask value, the bit value
> > of zero indicate the related runtime services is not supported at
> > runtime phase.
> > Add the difinition and use it to control Capsule runtime services.
> >
> > V2:
> > Adjust the indent of uni file.
> > Move the set variable function from CapsuleRuntimeDxe to RuntimeDxe.
> > Add 'EFIAPI' to the event function "UpdateRuntimeServicesSupported",
> > lacking of it would cause the GCC build failure.
> 
> (1) First of all, I think something must have gone wrong with your posting.
> Your cover letter carries the subject
> 
>   Add a pcd PcdBootManagerInBootOrder to control whether BootManager is
>   in BootOrder
> 
> and references TianoCore#1979.
> 
> However, all four patches in the series belong to TianoCore#1907, and the
> *contents* of the cover letter are also related to TianoCore#1907.
> 
> So basically I think the subject line and the BZ reference in your cover letter
> are incorrect.

Sorry I mixed the two patch I am working on. The BZ link should be https://bugzilla.tianocore.org/show_bug.cgi?id=1907.
And the title should be MdePkg/MdeModulePkg: Introduce a pcd to control runtime capsule servives.

> 
> 
> (2) I have read your answers at:
> 
>   http://mid.mail-
> archive.com/3CE959C139B4C44DBEA1810E3AA6F9000B808772@SHSMSX101.c
> cr.corp.intel.com
>   https://edk2.groups.io/g/devel/message/43899
> 
> If I understand correctly, you said that the new PCD / standardized UEFI
> variable is a pure addition, and that platforms can *transparently* inherit this
> feature enablement in the runtime DXE core and CapsuleRuntimeDxe.
> 
> Did I understand your answer correctly?

I didn't think of * transparent* things before.

> 
> If so, then I disagree. In my opinion, this is *not* a transparent change for
> platforms. And that's because of the following change in the UEFI
> specification:
> 
> * In UEFI v2.7 Errata B, the EFI_UNSUPPORTED return status is documented
>   as follows, for the UpdateCapsule() runtime service:
> 
>     "The capsule type is not supported on this platform."
> 
>   And for the QueryCapsuleCapabilities() runtime service:
> 
>     "The capsule type is not supported on this platform, and
>     /MaximumCapsuleSize/ and /ResetType/ are undefined."
> 
> * In UEFI v2.8, the same return status specifications are preserved, but
>   the following ones are added too (for EFI_UNSUPPORTED), under both
>   UpdateCapsule() and QueryCapsuleCapabilities():
> 
>     "This call is not supported by this platform at the time the call is
>     made. The platform must correctly reflect this behavior in the
>     /RuntimeServicesSupported/ variable."
> 
> Therefore, if a platform knows that it will return EFI_UNSUPPORTED
> *consistently* (due to platform limitations) from these runtime services,
> then UEFI-2.8 *requires* the platform to advertize that fact in the new UEFI
> variable.

The new pcd is set by the platform and the platform should aware that the pcd would set a new variable L"RuntimeServicesSupported".
It there some implementations that conflict with your description above?

> 
> 
> (3) If a platform links DxeCapsuleLibNull into CapsuleRuntimeDxe, that has
> the following consequences:
> 
> - QueryCapsuleCapabilities()
>   [MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c] calls
>   SupportCapsuleImage()
>   [MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.c].
> 
>   The return status is EFI_UNSUPPORTED, consistently.
> 
> - UpdateCapsule()
>   [MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c] calls
> both
>   SupportCapsuleImage() -- see above -- and ProcessCapsuleImage()
>   [MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.c].
> 
>   The return status is EFI_UNSUPPORTED, consistently.
> 
> Meaning that, if a platform uses DxeCapsuleLibNull, it *must* clear the
> EFI_RT_SUPPORTED_UPDATE_CAPSULE and
> EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES bits in the
> "RuntimeServicesSupported" variable.
> 
> Now, your patch introduces "PcdRuntimeServicesSupport" in the
> [PcdsFixedAtBuild] section of "MdePkg.dec". Based on that, I think we
> should add a CONSTRUCTOR function to DxeCapsuleLibNull, as a separate
> patch.
> 
> The constructor function should do:
> 
>   if (((FixedPcdGet16 (PcdRuntimeServicesSupport) &
>         EFI_RT_SUPPORTED_UPDATE_CAPSULE) != 0) ||
>       ((FixedPcdGet16 (PcdRuntimeServicesSupport) &
>         EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES) != 0)) {
>     //
>     // This library instance is unsuitable for implementing the
>     // UpdateCapsule() and SupportCapsuleImage() runtime services.
>     //
>     return EFI_UNSUPPORTED;
>   }
>   return EFI_SUCCESS;
> 
> Why is this important? Because it will *force* platforms to expose their lack
> of capsule support in the new PCD. Otherwise, the firmware will not boot --
> and that is impossible to miss.

I see your point. The platforms which use null version CapsuleLib should setting the related bit in the new PCD. That's right.
But changing the whole related platforms which use the null version is a challenge. If I missed some, those platforms would not boot because of the patch.
And I think miss this change for DxeCapsuleLibNull wouldn't violate the spec. I'd better to hear more comments about this.

> 
> 
> (4) The situation is somewhat similar with "PcdCapsuleInRamSupport". If
> "PcdCapsuleInRamSupport" is FALSE, then UpdateCapsule() will always
> return EFI_UNSUPPORTED.
> 
> Therefore, the entry point function of CapsuleRuntimeDxe --
> CapsuleServiceInitialize() -- should get the following assertion:
> 
>   ASSERT (
>     PcdGetBool (PcdCapsuleInRamSupport) ||
>     ((FixedPcdGet16 (PcdRuntimeServicesSupport) &
>       EFI_RT_SUPPORTED_UPDATE_CAPSULE) == 0)
>     );
> 
> 
> (5) For each platform in the edk2 tree that either uses DxeCapsuleLibNull or
> sets "PcdCapsuleInRamSupport" to FALSE, the corresponding bits should be
> cleared in "PcdRuntimeServicesSupport", in the platform DSC files.
> 
> This would mean a number of new patches for this series.

(4) and (5) would force the platform to set PcdRuntimeServicesSupport base on PcdCapsuleInRamSupport. That' fine. But I should know the specific platforms that already set "PcdCapsuleInRamSupport". If the PcdCapsuleInRamSupport is only an introduction, that means no platform sets it, no patch is required.

> 
> 
> (6) With the sanity checks from points (3) and (4) implemented, I agree that
> CapsuleRuntimeDxe is permitted to consume "PcdRuntimeServicesSupport",
> in patch#4, and to introduce new EFI_UNSUPPORTED exit points into
> QueryCapsuleCapabilities() and UpdateCapsule().
> 
> However:
> 
> (6a) In patch#4, CapsuleRuntimeDxe consumes the new *UEFI variable*, and
> not the new *PCD*. I think that's wrong; or at least sub-optiomal.
> 
> Earlier Mike wrote, in
> 
>   http://mid.mail-
> archive.com/E92EE9817A31E24EB0585FDF735412F5B9D77345@ORSMSX113.a
> mr.corp.intel.com
>   https://edk2.groups.io/g/devel/message/43890
> 
> that the runtime DXE Core should set the variable, and that individual
> runtime drivers providing some runtime services should consume the *PCD*.
> See the quote below, from Mike:
> 
> > I agree that each RT driver that populates the RT Services Table with
> > a RT services can consume the new bitmask PCD and use the PCD to
> > determine if the RT Service should return EFI_UNSUPPORTED after
> > ExitBootServices().
> 
> So, CapsuleRuntimeDxe should base those new exit points on the PCD, and
> the GetVariable() call should be removed.

Agree.

> 
> (6b) The current bitmask checks in patch #4 are wrong:
> 
> > +  if (!(mRuntimeServicesSupported |
> EFI_RT_SUPPORTED_UPDATE_CAPSULE)) {
> > +    return EFI_UNSUPPORTED;
> > +  }
> > +
> >
> > +  if (!(mRuntimeServicesSupported |
> EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES)) {
> > +    return EFI_UNSUPPORTED;
> > +  }
> > +
> 
> First, the relevant bits should be extracted with the bitwise AND operator,
> not the bitwise OR operator.

Agree. My mistake.

> 
> Second, after the extraction, the edk2 coding style dictates an explicit
> comparison with zero, to my understanding. The logical negation operator is
> only acceptable with BOOLEAN variables, and with such sub-expressions that
> evaluate to FALSE/TRUE.

I would follow that in the next patch.

Before I make next version patch, I want to hear more comments. Expecially for your (3).

Thanks,
Zhichao

> 
> Thanks,
> Laszlo
> 
>