Re: [edk2-devel] [PATCH v7 01/43] MdeModulePkg: Create PCDs to be used in support of SEV-ES

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

On 05/06/20 15:19, Tom Lendacky wrote:
> On 5/5/20 8:53 PM, Dong, Eric wrote:
>>
>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>>> Laszlo Ersek
>>> Sent: Tuesday, May 5, 2020 11:30 PM
>>> To: Tom Lendacky <thomas.lendacky@amd.com>; Dong, Eric
>>> <eric.dong@intel.com>; devel@edk2.groups.io
>>> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org>; Kinney, Michael D
>>> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Ni,
>>> Ray
>>> <ray.ni@intel.com>; Brijesh Singh <brijesh.singh@amd.com>; Wang, Jian J
>>> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
>>> Subject: Re: [edk2-devel] [PATCH v7 01/43] MdeModulePkg: Create PCDs to
>>> be used in support of SEV-ES
>>>
>>> On 05/04/20 18:41, Tom Lendacky wrote:
>>>
>>>> Is there an easy way to run everything that this link points, too? Is
>>>> it just creating a pull request that does this? I don't want to take
>>>> up a lot of your time, so if there's some documentation on how to run
>>>> an integration test to find and fix issues like this, just point me
>>>> to it.
>>>
>>> Just create a pull request; it will set off CI, and you can review VS
>>> build errors
>>> there (if any).
>>>
>>> Your PR will automatically be closed (rejected) regardless of whether CI
>>> succeeds or not. PRs are merged -- in fact, *auto*-merged, by the
>>> "mergify
>>> bot" -- if and only if (a) the CI run succeeds, and (b) the PR has
>>> the "push"
>>> label set.
>>>
>>> And only edk2 maintainers have permission to set the "push" label.
>>> Any PR
>>> without the "push" label qualifies as a "personal test build". So you
>>> can freely
>>> experiment with PRs, because you can't (even unwittingly) satisfy
>>> condition
>>> (b).
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Development-&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7C9cff3475aff84a95728508d7f1604c99%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243268217382019&amp;sdata=3%2FIKB174QaVLaqO0u1gdrL0izXmhEZ%2Byvj3iC13UYBc%3D&amp;reserved=0
>>>
>>> Process
>>>
>>
>> Thanks Laszlo for your explanation.
>>
>> I found this patch serial is incompatible for the existed platforms.
>> Can you help to fix the build failure for these platforms in
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2-platforms&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7C9cff3475aff84a95728508d7f1604c99%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243268217382019&amp;sdata=jU0qrB%2BV6ZvFmPzjcxGo9o2Pu1%2FrhRW0gUZTMv%2BiXDQ%3D&amp;reserved=0
>>
>>
> 
> I have fixed all of the build issues associated with the VS compiler
> using the pull request method that Laszlo mentioned. I then successfully
> built the RPi4 platform under GCC (build -n 32 -a AARCH64 -t GCC5 -p
> Platform/RaspberryPi/RPi4/RPi4.dsc) using the AARCH64 cross compiler.
> 
> Is there a particular platform that experiences an issue or are the
> failures related to the VS compiler errors that my next series will have
> fixed?

I think that Eric refers to various (as of yet, unspecified) platform
DSCs in the edk2-platforms tree, which consume edk2.

https://github.com/tianocore/edk2-platforms/

I don't fully agree with Eric on this observation. I agree somewhat.

I have always made the argument that *all* platform code should reside
in edk2; in other words, we should not have a separate edk2-platforms
repository. Then it is feasible for a contributor to adapt dependent
platform code in the same patch series that modifies core code.

But I have not been successful in arguing against the edk2-platforms
tree's existence, and so we have two projects now, with separate git
histories. Therefore, the pattern sometiems followed is to split the
edk2 (core) patch series into multiple sub-series, and to post an
edk2-platforms series in the middle, to help edk2-platforms cope with
the edk2 changes.

The question is of course how complicated this is. If the edk2-platforms
patches are not difficult, then I think the edk2-platforms maintainers
may ask the contributor to submit patches for edk2-platforms too, as a
*courtesy*. But if the edk2-platforms patches are many or complicated,
then I think it's *unfair* to block edk2 changes *only* due to
edk2-platforms dependencies. The edk2 contributor may not be familiar
with the edk2-platforms stuff at all; worse, firmware platforms in the
latter tree have often very quirky build instructions.

Inside a single git tree, a contributor is justifiedly expected to track
down dependencies and to update them (see for example Tom's patches for
UefiPayloadPkg!), but in other trees, that's not necessarily the case.

If we'd like Tom to adapt edk2-platforms to the edk2 changes, and maybe
even restructure the edk2 series in order to accommodate this, then the
*bare minimum* is to provide specific error messages and build instructions.

> 
>> I think you also needs to add an wiki page to explain what need to do
>> if an platform needs to integrate your changes, also it's better to
>> explain this feature in the page.
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7C9cff3475aff84a95728508d7f1604c99%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243268217382019&amp;sdata=xLkoV4zWhxtsbqszqPc0lEAl%2BYLL%2B2wg1nIXql8a64E%3D&amp;reserved=0
>>
> 
> I don't see any platform other than OVMF using this feature as it is a
> virtualization feature. Having said that I can add an explanation of
> what is needed should another virtualization platform be created under
> EDK2 that wants to support SEV-ES. And, as you said, I can also explain
> the feature overall on the page.

This should be approached from the "out of tree platform build errors"
angle described above.

For example, if there's a pre-existent / widely used library instance in
UefiCpuPkg, and you add a new library class dependency to it, such that
even the library class is entirely new in edk2, that will require
existent platforms to resolve the new lib class to a proper lib instence
in their DSC files -- otherwise they will get a build error. The wiki
page should basically explain such DSC updates in natural language.

If there are new PCDs in UefiCpuPkg.dec that a platform might want to
customize in its DSC file, that could be useful to mention in the Wiki
page too.

Because SEV-ES is only really relevant for OVMF at this point, I think
the wiki page should only help out of tree platform owners prevent build
errors, at this point.

But for that, Eric should please provide build instructions and error
messages for such out of tree platforms that actually break, with this
edk2 series applied.

> 
>>
>>
>> If you want to include this change in the next edk2 release, you need
>> to add one item for it in the release plan page, sample can be found
>> in below pages:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Release-Planning&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7C9cff3475aff84a95728508d7f1604c99%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243268217382019&amp;sdata=kcDVjYHMS9bRRZOlKEk5ynFNT39AnxchJAMak%2Bn870I%3D&amp;reserved=0
>>
> 
> Thanks. Is there anyone in particular that I need to request this
> feature be added?

Here's a general approach:

(1) Clone the git repository that is the edk2 wiki, locally:

  git://github.com/tianocore/tianocore.github.io.wiki

or

  https://github.com/tianocore/tianocore.github.io.wiki.git

(2) On a topic branch, implement your wiki changes.

In this case, the "EDK-II-Release-Planning.md" file should be updated.
Pick one of the upcoming stable releases, and list
<https://bugzilla.tianocore.org/show_bug.cgi?id=2198> under it. Use
existent items as example.

(3) In order to check a "rendered" view of your edk2 wiki changes, I
recommend having a personal fork of the edk2 project on github. Then,
you can use the wiki associated with *that* personal project as a
personal review space for your edk2 wiki changes.

For example, the personal wiki that I use for this purpose has the
following URL in my clone:

  git@github.com:lersek/edk2.wiki.git
                 ^^^^^^ ^^^^

and I use the following URL in my browser:

  https://github.com/lersek/edk2/wiki
                     ^^^^^^ ^^^^

(4) Push your local changes (on the local topic branch) to the remote
*master* branch (github.com ignores branches different from "master" in
wiki repositories). Verify the "rendered" view of your wiki changes.

(5) Post the patch to edk2-devel, using the subject prefix

  "edk2-wiki PATCH"

and also include a URL to the "rendered view" from (4).

I don't know the exact set of people having push access to the wiki; CC
me on your wiki patch, and I'll push it.

Thanks
Laszlo