Re: [edk2-devel] [PATCH v2 5/5] OvmfPkg: link SM3 support into Tcg2Pei and Tcg2Dxe

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

On 05/28/19 22:40, Imran Desai wrote:
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1781
> 
> EDK2 Support for SM3 digest algorithm is needed to enable TPM with SM3 PCR
> banks. This digest algorithm is part of the China Crypto algorithm suite.
> This integration has dependency on the openssl_1_1_1b integration into
> edk2.
> This patch links SM3 support into Tcg2Pei and Tcg2Dxe.
> 
> 
> Signed-off-by: Imran Desai <imran.desai@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc    | 2 ++
>  OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
>  OvmfPkg/OvmfPkgX64.dsc     | 2 ++
>  3 files changed, 6 insertions(+)

Wow, what just happened here?

I'm noticing now that this patch has been pushed to the master branch as
commit a7c7d21ffa9a.

However, *NONE* of the OvmfPkg co-maintainers or reviewers have reviewed
this patch!

The commit message includes "Cc:" lines, but that's a lie. Probably not
an intentional lie, but a lie nonetheless. These patches have *never*
been delivered to my inbox, and if I look at the address list on the
message instance that was reflected by the mailing list, that address
list confirms the same. I'm pretty sure Imran's git configuration has a
bug related to CC's. (I've extended the address list now, manually.)

Jian: please revert this patch immediately, stating, as reason, that the
patch review process was not honored. I'm sorry but I cannot let this
slide -- if you look at commit a7c7d21ffa9a now, it suggests that the
OvmfPkg maintainers / reviewers were CC'd (they weren't), but they
ignored the patch (they didn't -- they couldn't see it), and that
another maintainer pushed the patch just the same (which is factual, but
*wrong*).

After the revert, Imran can resubmit the patch, with *functional* CC's,
and then we can discuss it.

In general, it is fine for one maintainer to push a series that touches
multiple top-level packages. However, that maintainer *MUST* make sure
that each patch has sufficient "M" reviews, and he/she is responsible
for picking up the feedback tags for *all* patches from the list.

Come on now, guys -- have you really known me to be a person that
*silently ignores* an OvmfPkg patch for more than a month? No automated
out-of-office reply, no "please give me some time to review" reply, just
silence?

And even if I missed a patch like that, don't you think a maintainer
deserves a ping first?

... Why are we, as a community, *still* failing at this process?

Laszlo

> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 578fc6c98ec8..fb5944aa6945 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -628,6 +628,7 @@ [Components]
>        NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>        NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
>        NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> +      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
>    }
>  !if $(TPM2_CONFIG_ENABLE) == TRUE
>    SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> @@ -914,5 +915,6 @@ [Components]
>        NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>        NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
>        NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> +      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
>    }
>  !endif
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index eade8f62d3de..64c231f735c2 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -636,6 +636,7 @@ [Components.IA32]
>        NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>        NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
>        NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> +      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
>    }
>  !if $(TPM2_CONFIG_ENABLE) == TRUE
>    SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> @@ -924,5 +925,6 @@ [Components.X64]
>        NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>        NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
>        NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> +      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
>    }
>  !endif
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 733a4c9d8a43..7e46d401a36f 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -635,6 +635,7 @@ [Components]
>        NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>        NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
>        NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> +      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
>    }
>  !if $(TPM2_CONFIG_ENABLE) == TRUE
>    SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> @@ -922,5 +923,6 @@ [Components]
>        NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>        NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
>        NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> +      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
>    }
>  !endif
>