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

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

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