Re: Patch List for 202005 stable tag

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

On 05/19/20 14:04, Leif Lindholm wrote:
> On Tue, May 19, 2020 at 11:09:42 +0000, Gao, Liming wrote:
>> Hi Stewards and all:
>>   I collect current patch lists in devel mail list. Those patch contributors request to add them for 200205 stable tag. Because we have enter into Soft Feature Freeze, I want to collect your feedback for them. If any patches are missing, please reply this mail to add them.
>>
>> Feature List (pass review):
>> https://edk2.groups.io/g/devel/message/59578 [PATCH V7 0/1] Disable safe string constraint assertions
> 
> This one I might even be persuaded to wave through in hard
> freeze. Yes, please include.

Agreed.

>> https://edk2.groups.io/g/devel/message/59589 [PATCH V7 0/6] Add definitions introduced in UEFI 2.8a
> 
> 1,3-4 only adds definitions - I'm fine with those.
> 
> 2/6 looks out of place compared with the set description, but as it
> falls within the scope of "align codebase with current spec", and it
> is a correction, it can stay. (It should also not break anything, and
> if it does, that's automatically a bug isn't it?)

Patches v7 #1 through #4 should be merged, because they were posted
before the SFF, and they already carried an R-b from Liming (MdePkg
co-maintainer).

> 5-6, I have no comment on. Someone else will have to make that call.

Patches #5 and #6 seem to have been superseded by the following separate
series:

  [edk2-devel] [PATCH 0/2] Add FMP Capsule Image Header extension
  https://edk2.groups.io/g/devel/message/59652
  http://mid.mail-archive.com/20200515073848.13920-1-wei6.xu@intel.com

Now, this separate series was also posted (even if barely: 22 minutes)
before the SFF. The patches also contained some R-b tags upon posting.

Patch #1 had an R-b from Liming, upon posting. The files it modifies are:
- MdeModulePkg/Application/CapsuleApp/CapsuleDump.c
- MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c

I've run "BaseTools/Scripts/GetMaintainer.py" with the "-l" option on
both files. Both invocations list Liming as a designated reviewer. (From
section "MdeModulePkg: Firmware Update modules", as far as I can tell.)

Patch #2 had an R-b from Liming and Chao Zhang, on posting. The patch
modifies the following file:
- SignedCapsulePkg/Universal/RecoveryModuleLoadPei/RecoveryModuleLoadPei.c

"GetMaintainer.py" does list "Chao Zhang" with the "-l" option. (See the
"SignedCapsulePkg" section in Maintainers.txt".)

So, assuming both patches included those R-b's *justifiedly* when there
were posted (that is, the R-b's were given earlier on the list), I think
this 2-part series too should be merged. Technically speaking, it was
fully reviewed as soon as it was posted, and it was posted in time.

> 
>> Bug List (pass review):
>> https://edk2.groups.io/g/devel/message/59735 [PATCH v3] SecurityPkg: Change default value source
> 
> Patch is pretty garbled in groups.io. Thankfully, it looks better in
> gmail.
> 
> I have a minor concern in that neither the comments on v2 nor the
> changelog in v3 indicates the (useful) addition of the bit-value
> lookup table for PcdTcg2PhysicalPresenceFlags in SecurityPkg.dec
> and its associated dropping of the line
> "# Default setting is TCG2_BIOS_TPM_MANAGEMENT_FLAG_DEFAULT | TCG2_BIOS_STORAGE_MANAGEMENT_FLAG_DEFAULT".
> 
> But if SecurityPkg maintainers are happy they haven't missed any other
> changes in v3, I'm OK with this going in.

I think even a v4 would be eligible for merging (with the commit message
improvements that you are suggesting). Even during the hard feature
freeze. (Possibly justifying an extension to the hard feature freeze.)

Here's why I think that:
<https://bugzilla.tianocore.org/show_bug.cgi?id=2713#c0> is a good issue
report. It explains that this patch is a bugfix. Namely, commit
710174e01174 ("SecurityPkg: Tcg2PhysicalPresence: Define TCG2 PP Flags
Initial Pcd", 2016-12-29) was incomplete; it created an inconsistency in
physical presence flag defaults, between different code paths. And the
new patch fixes that.

(On the negative side, three versions of the patch have been posted to
the list, and the bugzilla ticket has pointers to... none of those. The
last comment remains, at this time, "Still investigate the solution to
fix it". I find that really frustrating and sad.)

Thanks,
Laszlo