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

Fri Jul 19 14:15:20 UTC 2019

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.

(2) I have read your answers at:

  http://mid.mail-archive.com/3CE959C139B4C44DBEA1810E3AA6F9000B808772@SHSMSX101.ccr.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?

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.

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

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

(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.amr.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.

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

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.

Thanks,
Laszlo

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

View/Reply Online (#44044): https://edk2.groups.io/g/devel/message/44044
Mute This Topic: https://groups.io/mt/32524668/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-