Re: [edk2-devel] [RESEND PATCH RFC v3 00/22] Add AMD Secure Nested Paging (SEV-SNP) support

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

On 06/04/21 15:09, Laszlo Ersek wrote:
> On 06/04/21 13:50, Brijesh Singh wrote:

>> The main issue is I typed wrong branch name in the cover letter. The
>> branch name should be "sev-snp-rfc-3" and not "sev-snp-rfc-2". I
>> apologies for it :(. Ray asked the branch name and I replied him with
>> the correct branch.
> 
> Hmmm... indeed, but that discussion happened off-list, namely under the
> original posting of this v3 RFC set that did not reach the list. And now
> I was working with my list folder.
>>
>> https://github.com/AMDESE/ovmf/tree/sev-snp-rfc-3
>>
>> This branch was based on commit 5531fd48ded1271b8775725355ab83994e4bc77c
>> from the upstream. 
> 
> Right, this branch indeed averts problem (2); it is in sync with the
> posted series. Thanks!
> 
> Problem (1) stays the same -- git-rebase reports the same issue as
> git-am above, for patch#21. So, I'm going to review this version on the
> list, but I'll skip patch#21 (or perhaps I'll attempt to make useful
> comments there too, if I can).

I re-reviewed patch #3 today, and reviewed patch #4 as well.

Because the data flow was not explained in advance, regarding the
"SevSnpEnabled" and "HypervisorFeatures" fields, I wasted a huge amount
of time reviewing ResetVector details that ultimately proved irrelevant.

Additionally, I've found that several patches modify multiple modules in
one step, typically ResetVector and PlatformPei. Honestly, this is
inexplicable to me, given the edk2 coding style. The edk2 style permits
multi-module patches only in the most exceptional cases. In a typical
producer/consumer setup, the producer module patch comes first, the
consumer module patch comes second. Breaking this edk2 rule is very
detrimental to my efficiency as a reviewer.

Either way, I'm done reviewing RFCv3; primarily because tomorrow and the
day after I have some time-sensitive work to do, and afterwards, I'm
going to disapper for a good while, to salvage the shreds of my brain
that I've been left with, after the last two weeks.

In the meantime, assuming no other reviewer wishes to comment RFCv3,
please rework this series carefully -- even if it has "RFC" status,
that's not a license to break edk2 coding style, structure patches
incorrectly, diverge from spec-dictated constants, omit detailed
explanations in commit messages (data flow!), duplicate code (assembly
code!) mindlessly, and so on.

Thanks
Laszlo