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.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>

Zhichao Gao (4):
  MdePkg/UefiSpec.h: Add define of runtime services support
  MdePkg: Add new pcd PcdRuntimeServicesSupport
  MdeModulePkg/RuntimeDxe: Set RuntimeServicesSupport base on Pcd
  MdeModulePkg/CapsuleRuntimeDxe: Implement RuntimeServicesSupported

 MdeModulePkg/Core/RuntimeDxe/Runtime.c        | 65 +++++++++++++++++-
 MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf   |  8 ++-
 .../CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf   |  2 +
 .../CapsuleRuntimeDxe/CapsuleService.c        | 68 +++++++++++++++++++
 MdePkg/Include/Uefi/UefiSpec.h                | 15 ++++
 MdePkg/MdePkg.dec                             | 19 ++++++
 MdePkg/MdePkg.uni                             | 19 ++++++
 7 files changed, 194 insertions(+), 2 deletions(-)

-- 
2.21.0.windows.1


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

View/Reply Online (#44028): https://edk2.groups.io/g/devel/message/44028
Mute This Topic: https://groups.io/mt/32524668/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


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

View/Reply Online (#44082): https://edk2.groups.io/g/devel/message/44082
Mute This Topic: https://groups.io/mt/32524668/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 07/22/19 05:17, Gao, Zhichao wrote:
> 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

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

You don't have to change all platforms in existence, in this patch
series, just those that live inside the core edk2 repository.


> If I missed some, those platforms would not boot because of the patch.

Yes, and that's exactly the point.

The above code will cause an assertion failure for such platforms.
People will look at the error message, will locate the relevant source
code, will run "git blame" and "git log" on the source file, and they
will learn about the subject TianoCore BZ, and the new responsibility
for their platform DSC.

Openly forcing downstream platforms to implement a very simple change (a
PCD setting in the platform DSC) is a whole lot better than silently
breaking spec conformance for them.

(Obviously, it would even be better if we could write code that kept
those platforms spec-conformant by default. But that's not possible,
because the change in UEFI-2.8 spells out a new requirement.)


> And I think miss this change for DxeCapsuleLibNull wouldn't violate the spec.

Well, I disagree. :)


> I'd better to hear more comments about this.

Sure, absolutely! Feedback is welcome, like always.


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

Even if no platform sets PcdCapsuleInRamSupport to FALSE at this time, a
platform can choose to do so later. And, at that later point, any
inconsistency between PcdCapsuleInRamSupport and
PcdRuntimeServicesSupport should be caught, and reported.

Whether you should identify and fix up such individual inconsistencies
in specific platforms, as part of this patch series, is a different
question. For platforms that live inside the edk2 tree, the answer is
"yes". For other platforms, the answer is "no" -- they will have to fix
up the inconsistency for themselves. But, at least, the above ASSERT
will notify them, so they will learn about the new task.

Thanks!
Laszlo

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

View/Reply Online (#44144): https://edk2.groups.io/g/devel/message/44144
Mute This Topic: https://groups.io/mt/32524668/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-