Adding Bhyve support into upstream EDK2

Adding Bhyve support into upstream EDK2

[Rebecca Cran]

I'm currently working on updating EDK2 support for Bhyve 
(https://bhyve.org/) from the edk2-stable201903 tag to 
edk2-stable202002. It's currently kept in a separate repo 
(https://github.com/freebsd/uefi-edk2), but I'd like to discuss pushing 
support upstream into the main edk2 repo (I guess into edk2-staging as a 
first step?).


Would that be something people would be open to considering, or should 
it remain separate? Should it be a new top-level package (e.g. BhyvePkg) 
or could it be just a configuration option when building OVMF? It's 
currently maintained as a set of patches against OvmfPkg, which seems to 
work quite well.


-- 
Rebecca Cran

Re: Adding Bhyve support into upstream EDK2

[Laszlo Ersek]

On 03/06/20 17:09, Rebecca Cran wrote:
> I'm currently working on updating EDK2 support for Bhyve
> (https://bhyve.org/) from the edk2-stable201903 tag to
> edk2-stable202002. It's currently kept in a separate repo
> (https://github.com/freebsd/uefi-edk2), but I'd like to discuss pushing
> support upstream into the main edk2 repo (I guess into edk2-staging as a
> first step?).
> 
> 
> Would that be something people would be open to considering, or should
> it remain separate? Should it be a new top-level package (e.g. BhyvePkg)
> or could it be just a configuration option when building OVMF? It's
> currently maintained as a set of patches against OvmfPkg, which seems to
> work quite well.

It depends:

- on the amount of patches you have,

- on the complexity the patches introduce.

For example, ArmVirtPkg platforms use a large number (large proportion)
of OvmfPkg modules. I still very much like ArmVirtPkg to stay a separate
package.

For another example, IA32/X64 Xen support is presently "mixed into"
OvmfPkg code that otherwise targets QEMU. This mixture can be witnessed
at two levels:

- Xen-specific modules pulled into OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf},

- dynamically enabled/disabled Xen-guest code that's part of the same
source files (or same modules) that otherwise target QEMU.

There is no general recipe for keeping things shared by default, and
coding up the exceptions one by one, versus keeping things duplicated by
default, and extracting the common parts one by one. Again, it depends
on code size and complexity.

As I mentioned, I strongly prefer ArmVirtPkg to stay separate from
OvmfPkg. Leif disagreed with that, IIRC. I don't remember how Ard feels
about it.

Regarding Xen, I seem to recall that both Ard and myself prefer to keep
Xen as a separate *platform* (DSC, FDF) in both ArmVirtPkg and OvmfPkg.

ArmVirtPkg has always been like that (Ard did the Xen enablement that
way, up-front -- see commit 22455087dc37).

In OvmfPkg, we had started from the opposite direction, and it's quite
recent that Anthony has proposed to isolate Xen support to a dedicated
platform -- see <https://bugzilla.tianocore.org/show_bug.cgi?id=2122>.
(Which is a very welcome proposal, as far as I'm concerned.)

So... if you have a small number of trivial patches (or at least
well-separable drivers, libraries etc) for supporting bhyve, those could
be added to OvmfPkg, perhaps.

If you needed to hook a bunch of complex / deep "if"s into existent
code, then I would very much *NOT* like that, on the other hand.

Because the latter kind of code:
- makes human reasoning difficult,
- is virtually a recipe for regressions.

Regressions because: imagine there is a patch (motivated by a QEMU
feature) that rearranges some code that's also used, in a *slightly*
customized manner, on bhyve. If we mess up the code for bhyve, we don't
notice, because we don't *test* on bhyve. So in such cases, code
duplication / separation is actually beneficial, because it prevents
users / developers of platform X from regressing platform Y. As you see
such separation is mostly driven by what contributors run on, and test on.

Quick request: please do not ask me to look at the current bhyve
patches, off-list. Feel free to post them as an RFC series, instead.

Another reason I'd likely prefer bhyve to be reasonably separate is
maintenance responsibility. We have a pathname pattern based
Maintainers.txt now -- I would want to be able to assign BZs, and patch
reviews, immediately to you, for anything bhyve-related.

(I mean... it's not like I am some special "arbitrator" or whatever;
instead, the Maintainers.txt patterns should tell *anyone* asking, that
you would be responsible for bhyve patch review.)

In that sense, BhyvePkg would surely be my preference. I don't use
bhyve, so I want none of that responsibility, and also no increased
chance of regressions (in either direction: I don't want bhyve patches
to break OVMF on QEMU, and I don't want to break bhyve support with
QEMU-oriented patches).

FWIW, I do agree that bhyve support would be nice to have up-stream, and
specifically in "edk2", not "edk2-platforms". Virtual platforms are very
useful for testing core code updates, and therefore they get a pass into
"edk2". (I specifically remember an on-list discussion about the edk2
vs. edk2-platforms split, and virtual platforms and emulators were
*generally* permitted in edk2.)

Thanks
Laszlo

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Laszlo Ersek]

On 03/06/20 20:54, Laszlo Ersek wrote:
> On 03/06/20 17:09, Rebecca Cran wrote:
>> I'm currently working on updating EDK2 support for Bhyve
>> (https://bhyve.org/) from the edk2-stable201903 tag to
>> edk2-stable202002. It's currently kept in a separate repo
>> (https://github.com/freebsd/uefi-edk2), but I'd like to discuss pushing
>> support upstream into the main edk2 repo (I guess into edk2-staging as a
>> first step?).
>>
>>
>> Would that be something people would be open to considering, or should
>> it remain separate? Should it be a new top-level package (e.g. BhyvePkg)
>> or could it be just a configuration option when building OVMF? It's
>> currently maintained as a set of patches against OvmfPkg, which seems to
>> work quite well.
> 
> It depends:
> 
> - on the amount of patches you have,
> 
> - on the complexity the patches introduce.
> 
> For example, ArmVirtPkg platforms use a large number (large proportion)
> of OvmfPkg modules. I still very much like ArmVirtPkg to stay a separate
> package.
> 
> For another example, IA32/X64 Xen support is presently "mixed into"
> OvmfPkg code that otherwise targets QEMU. This mixture can be witnessed
> at two levels:
> 
> - Xen-specific modules pulled into OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf},
> 
> - dynamically enabled/disabled Xen-guest code that's part of the same
> source files (or same modules) that otherwise target QEMU.
> 
> There is no general recipe for keeping things shared by default, and
> coding up the exceptions one by one, versus keeping things duplicated by
> default, and extracting the common parts one by one. Again, it depends
> on code size and complexity.
> 
> As I mentioned, I strongly prefer ArmVirtPkg to stay separate from
> OvmfPkg. Leif disagreed with that, IIRC. I don't remember how Ard feels
> about it.
> 
> Regarding Xen, I seem to recall that both Ard and myself prefer to keep
> Xen as a separate *platform* (DSC, FDF) in both ArmVirtPkg and OvmfPkg.
> 
> ArmVirtPkg has always been like that (Ard did the Xen enablement that
> way, up-front -- see commit 22455087dc37).
> 
> In OvmfPkg, we had started from the opposite direction, and it's quite
> recent that Anthony has proposed to isolate Xen support to a dedicated
> platform -- see <https://bugzilla.tianocore.org/show_bug.cgi?id=2122>.
> (Which is a very welcome proposal, as far as I'm concerned.)
> 
> So... if you have a small number of trivial patches (or at least
> well-separable drivers, libraries etc) for supporting bhyve, those could
> be added to OvmfPkg, perhaps.
> 
> If you needed to hook a bunch of complex / deep "if"s into existent
> code, then I would very much *NOT* like that, on the other hand.
> 
> Because the latter kind of code:
> - makes human reasoning difficult,
> - is virtually a recipe for regressions.
> 
> Regressions because: imagine there is a patch (motivated by a QEMU
> feature) that rearranges some code that's also used, in a *slightly*
> customized manner, on bhyve. If we mess up the code for bhyve, we don't
> notice, because we don't *test* on bhyve. So in such cases, code
> duplication / separation is actually beneficial, because it prevents
> users / developers of platform X from regressing platform Y. As you see
> such separation is mostly driven by what contributors run on, and test on.
> 
> Quick request: please do not ask me to look at the current bhyve
> patches, off-list. Feel free to post them as an RFC series, instead.
> 
> Another reason I'd likely prefer bhyve to be reasonably separate is
> maintenance responsibility. We have a pathname pattern based
> Maintainers.txt now -- I would want to be able to assign BZs, and patch
> reviews, immediately to you, for anything bhyve-related.
> 
> (I mean... it's not like I am some special "arbitrator" or whatever;
> instead, the Maintainers.txt patterns should tell *anyone* asking, that
> you would be responsible for bhyve patch review.)
> 
> In that sense, BhyvePkg would surely be my preference. I don't use
> bhyve, so I want none of that responsibility, and also no increased
> chance of regressions (in either direction: I don't want bhyve patches
> to break OVMF on QEMU, and I don't want to break bhyve support with
> QEMU-oriented patches).
> 
> FWIW, I do agree that bhyve support would be nice to have up-stream, and
> specifically in "edk2", not "edk2-platforms". Virtual platforms are very
> useful for testing core code updates, and therefore they get a pass into
> "edk2". (I specifically remember an on-list discussion about the edk2
> vs. edk2-platforms split, and virtual platforms and emulators were
> *generally* permitted in edk2.)

A further question is code size -- if we were to introduce bhyve support
as additional drivers, I would want to permit downstreams to easily
disable those, without downstream patches for the DSC/FDF files, i.e.,
with a simple -D flag.

Thanks
Laszlo

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Yao, Jiewen]

I can share some of my experience, for your information only.

0) If the patch is generic, not specific to Bhyve, but benefit to current EDKII pkg, you can submit them directly. No need to wait for Bhyve.

1) If the patch is very simple, you can merge into current PKG with current DSC.
If there is something special to the Bhyve that can be detected at runtime, then detect at runtime.
If there is something special to the Byhve that need to be determine at build time, then you can introduce a PCD (such as PcdBhyveXXX) and configurate at build time.

2) If the patch is big, you can introduce a standalone driver and put to current PKG and introduce a new DSC file (such as OvmfBhyve.dsc). You can control and build Byhve with the new DSC file.

3) If the patch is extremely big and has architecture difference, you can introduce a new pkg (BhyvePkg) and put all new drivers there. You can still refer to some drivers in OvmfPkg, which introduce a dependency (BhyvePkg => OvmfPkg). The OvmfPkg change may impact BhyvePkg build or running.

X) Last but not least important, if the Bhyve has a different *security requirement* or *threat model* with current Pkg, then you had better introduce a new pkg or update the current Pkg with same threat model. Before that, you had better not use any driver in other package and keep them separate. It is easy for future audit purpose.

Above is the generic rule. I think OvmfPkg maintainer can provide more comment on that.

Can you post the patch? :-)

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Rebecca
> Cran
> Sent: Saturday, March 7, 2020 12:10 AM
> To: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [edk2-devel] Adding Bhyve support into upstream EDK2
> 
> I'm currently working on updating EDK2 support for Bhyve
> (https://bhyve.org/) from the edk2-stable201903 tag to
> edk2-stable202002. It's currently kept in a separate repo
> (https://github.com/freebsd/uefi-edk2), but I'd like to discuss pushing
> support upstream into the main edk2 repo (I guess into edk2-staging as a
> first step?).
> 
> 
> Would that be something people would be open to considering, or should
> it remain separate? Should it be a new top-level package (e.g. BhyvePkg)
> or could it be just a configuration option when building OVMF? It's
> currently maintained as a set of patches against OvmfPkg, which seems to
> work quite well.
> 
> 
> --
> Rebecca Cran
> 
> 
> 
> 

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Rebecca Cran]

On 3/6/20 6:29 PM, Yao, Jiewen wrote:
> Can you post the patch? :-)

Thanks, It's just about ready for review I think. There's perhaps a bit 
more deduplication between BhyvePkg and OvmfPkg to be done.

Since the patch is 1.7MB, I've uploaded it to 
https://bex.dev/bhyve-edk2-stable202002.diff .


-- 
Rebecca Cran

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Laszlo Ersek]

On 03/24/20 02:34, Rebecca Cran wrote:
> On 3/6/20 6:29 PM, Yao, Jiewen wrote:
>> Can you post the patch? :-)
> 
> Thanks, It's just about ready for review I think. There's perhaps a bit
> more deduplication between BhyvePkg and OvmfPkg to be done.
> 
> Since the patch is 1.7MB, I've uploaded it to
> https://bex.dev/bhyve-edk2-stable202002.diff .

Umm... :) This is way too large, I think.

Just because I indeed recommend creating a separate BhyvePkg, it's
really not advisable to create a complete copy of OvmfPkg, as first
step. I assume most modules can be reused from under OvmfPkg; can't they?

Consider for example ArmVirtPkg. The ArmVirtQemu DSC and FDF files refer
to a bunch of content that resides under OvmfPkg. That's what BhyvePkg
should do too:

- introduce its own DEC, DSC and FDF files,
- reuse everything possible verbatim from under OvmfPkg,
- if changes are necessary:
  - tweak existent OvmfPkg PCDs in the BhyvePkg DSC file,
  - introduce new library instances for library classes,
    and link those into OvmfPkg modules (and any other edk2 modules) via
    the BhyvePkg DSC file,
  - in the worst case, copy a *small* subset of OvmfPkg modules, and
    tweak the source under BhyvePkg.
- add totally BHYVE specific modules (drivers) under BhyvePkg.

Basically any given platform (DSC / FDF) is supposed to cherry-pick
whatever it can reuse from edk2 -- that's why edk2 is a "kit". And
virtual platforms are most welcome to depend on (consume modules from)
OvmfPkg.

Again, it's impossible to tell in advance, but in some cases, the tweaks
might be minimal enough to upstream them into OvmfPkg (conditionally on
a PCD, or conditionally on some small / easy runtime detection of
BHYVE). Then BhyvePkg only has to activate said PCD (or just rely on the
runtme detection). Again, there's no general rule; it depends on how
much the bhyve specifics would complicate the OvmfPkg code.

Thanks,
Laszlo

Re: [EXTERNAL] Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Bret Barkelew]

[-- Attachment #1: Type: text/plain, Size: 2914 bytes --]

Should it though? It seems like it should go the opposite way. ArmVirt->Ovmf and ArmVirt->Bhyve. Which one is the more core package? By naming it would *seem* that ArmVirt is more core, but maybe I’m wrong.

- Bret

From: Laszlo Ersek via Groups.Io<mailto:lersek=redhat.com@groups.io>
Sent: Tuesday, March 24, 2020 5:05 PM
To: Rebecca Cran<mailto:rebecca@bsdio.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>; Justen, Jordan L<mailto:jordan.l.justen@intel.com>; Ard Biesheuvel<mailto:ard.biesheuvel@linaro.org>
Subject: [EXTERNAL] Re: [edk2-devel] Adding Bhyve support into upstream EDK2

On 03/24/20 02:34, Rebecca Cran wrote:
> On 3/6/20 6:29 PM, Yao, Jiewen wrote:
>> Can you post the patch? :-)
>
> Thanks, It's just about ready for review I think. There's perhaps a bit
> more deduplication between BhyvePkg and OvmfPkg to be done.
>
> Since the patch is 1.7MB, I've uploaded it to
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbex.dev%2Fbhyve-edk2-stable202002.diff&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7C6b91de2e92934a455baf08d7d0503889%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637206915279358560&amp;sdata=YIqsjfqvNXGqZnEt86sCxMl3%2F1YVpkEJf7%2FPFseEAyM%3D&amp;reserved=0 .

Umm... :) This is way too large, I think.

Just because I indeed recommend creating a separate BhyvePkg, it's
really not advisable to create a complete copy of OvmfPkg, as first
step. I assume most modules can be reused from under OvmfPkg; can't they?

Consider for example ArmVirtPkg. The ArmVirtQemu DSC and FDF files refer
to a bunch of content that resides under OvmfPkg. That's what BhyvePkg
should do too:

- introduce its own DEC, DSC and FDF files,
- reuse everything possible verbatim from under OvmfPkg,
- if changes are necessary:
  - tweak existent OvmfPkg PCDs in the BhyvePkg DSC file,
  - introduce new library instances for library classes,
    and link those into OvmfPkg modules (and any other edk2 modules) via
    the BhyvePkg DSC file,
  - in the worst case, copy a *small* subset of OvmfPkg modules, and
    tweak the source under BhyvePkg.
- add totally BHYVE specific modules (drivers) under BhyvePkg.

Basically any given platform (DSC / FDF) is supposed to cherry-pick
whatever it can reuse from edk2 -- that's why edk2 is a "kit". And
virtual platforms are most welcome to depend on (consume modules from)
OvmfPkg.

Again, it's impossible to tell in advance, but in some cases, the tweaks
might be minimal enough to upstream them into OvmfPkg (conditionally on
a PCD, or conditionally on some small / easy runtime detection of
BHYVE). Then BhyvePkg only has to activate said PCD (or just rely on the
runtme detection). Again, there's no general rule; it depends on how
much the bhyve specifics would complicate the OvmfPkg code.

Thanks,
Laszlo





[-- Attachment #2: Type: text/html, Size: 5407 bytes --]

Re: [EXTERNAL] Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Laszlo Ersek]

On 03/25/20 19:18, Bret Barkelew wrote:
> Should it though? It seems like it should go the opposite way.
> ArmVirt->Ovmf and ArmVirt->Bhyve. Which one is the more core package?
> By naming it would *seem* that ArmVirt is more core, but maybe I’m
> wrong.

OvmfPkg is the more core package.

Thanks
Laszlo

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Rebecca Cran]

On 3/24/20 6:04 PM, Laszlo Ersek wrote:

> Umm... :) This is way too large, I think.


Oh, it definitely is! I've still got a couple of days of deduplication 
work to go I think: I've already removed lots of packages from under 
BhyvePkg that it can just use from OvmfPkg.

Thanks, I wasn't sure if I should suggest making changes to files under 
OvmfPkg to support Bhyve. Since that's okay, there are some small 
changes I'd like to make, for example adding the Bhyve PCI IDs into 
PciHostBridgeLib.


--

Rebecca Cran

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Yao, Jiewen]

Just saw Laszlo's email. Similar feedback. Especially, I like the regression test part.

I am not sure how many virtual platforms we will have eventually.
If there are more and more, maybe we can create a new edk2-virt-platform repo, and put them together there. (Similar to edk2-platform repo for the physical platform)


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
> Sent: Saturday, March 7, 2020 9:30 AM
> To: devel@edk2.groups.io; rebecca@bsdio.com; Laszlo Ersek
> <lersek@redhat.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2-devel] Adding Bhyve support into upstream EDK2
> 
> I can share some of my experience, for your information only.
> 
> 0) If the patch is generic, not specific to Bhyve, but benefit to current EDKII pkg,
> you can submit them directly. No need to wait for Bhyve.
> 
> 1) If the patch is very simple, you can merge into current PKG with current DSC.
> If there is something special to the Bhyve that can be detected at runtime, then
> detect at runtime.
> If there is something special to the Byhve that need to be determine at build
> time, then you can introduce a PCD (such as PcdBhyveXXX) and configurate at
> build time.
> 
> 2) If the patch is big, you can introduce a standalone driver and put to current
> PKG and introduce a new DSC file (such as OvmfBhyve.dsc). You can control and
> build Byhve with the new DSC file.
> 
> 3) If the patch is extremely big and has architecture difference, you can
> introduce a new pkg (BhyvePkg) and put all new drivers there. You can still refer
> to some drivers in OvmfPkg, which introduce a dependency (BhyvePkg =>
> OvmfPkg). The OvmfPkg change may impact BhyvePkg build or running.
> 
> X) Last but not least important, if the Bhyve has a different *security
> requirement* or *threat model* with current Pkg, then you had better introduce
> a new pkg or update the current Pkg with same threat model. Before that, you
> had better not use any driver in other package and keep them separate. It is easy
> for future audit purpose.
> 
> Above is the generic rule. I think OvmfPkg maintainer can provide more
> comment on that.
> 
> Can you post the patch? :-)
> 
> Thank you
> Yao Jiewen
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Rebecca
> > Cran
> > Sent: Saturday, March 7, 2020 12:10 AM
> > To: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Justen, Jordan
> L
> > <jordan.l.justen@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Subject: [edk2-devel] Adding Bhyve support into upstream EDK2
> >
> > I'm currently working on updating EDK2 support for Bhyve
> > (https://bhyve.org/) from the edk2-stable201903 tag to
> > edk2-stable202002. It's currently kept in a separate repo
> > (https://github.com/freebsd/uefi-edk2), but I'd like to discuss pushing
> > support upstream into the main edk2 repo (I guess into edk2-staging as a
> > first step?).
> >
> >
> > Would that be something people would be open to considering, or should
> > it remain separate? Should it be a new top-level package (e.g. BhyvePkg)
> > or could it be just a configuration option when building OVMF? It's
> > currently maintained as a set of patches against OvmfPkg, which seems to
> > work quite well.
> >
> >
> > --
> > Rebecca Cran
> >
> >
> >
> >
> 
> 
> 

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Laszlo Ersek]

Hi Jiewen,

On 03/07/20 02:43, Yao, Jiewen wrote:
> Just saw Laszlo's email. Similar feedback. Especially, I like the regression test part.

Thanks.

> I am not sure how many virtual platforms we will have eventually.
> If there are more and more, maybe we can create a new edk2-virt-platform repo, and put them together there. (Similar to edk2-platform repo for the physical platform)

Regarding the last part ("move them together here") -- I'm 100% opposed
to removing OvmfPkg and ArmVirtPkg from edk2. They *must* remain in the
exact same git repository where the core (MdePkg, MdeModulePkg,
CryptoPkg, SecurityPkg, UefiCpuPkg, ...) lives too, and share a common
git history.

ArmVirtPkg and OvmfPkg move very closely together with the core, most
significant ArmVirtPkg and OvmfPkg contributions need changes (and
therefore introduce new dependencies) on the core. Managing such
dependencies is a nightmare evein with git submodules; it only works if
the git history is shared. This problem is not theoretical, it already
has a bad effect on edk2-platforms.

For a recent example, my latest OvmfPkg patch series:

https://bugzilla.tianocore.org/show_bug.cgi?id=1512#c18

merged as commit range 61d3b2d4279e..1158fc8e2c7b, started by improving
the logging in MdeModulePkg/PiSmmCore (a1ddad95933e), and fixing a bug
in UefiCpuPkg/PiSmmCpuDxeSmm (90e11edd16c7).

I don't necessarily mind if *new* virtual platforms are outside of the
edk2 tree, but if I'm completely honest about "why", it's because I
don't use those new platforms. And that's a *selfish* reason -- if I
want ArmVirtPkg and OvmfPkg to benefit from sharing and interleaving
their histories with the core, then other virtual platforms deserve the
same, even if I don't use them.

(In fact, I think that even edk2-platforms should never have been split
out of edk2 -- but that ship has sailed. I believe I argued against
separating edk2-platforms, but my reasons weren't strong or convincing
enough.)

Thanks,
Laszlo


>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
>> Sent: Saturday, March 7, 2020 9:30 AM
>> To: devel@edk2.groups.io; rebecca@bsdio.com; Laszlo Ersek
>> <lersek@redhat.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Ard
>> Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: Re: [edk2-devel] Adding Bhyve support into upstream EDK2
>>
>> I can share some of my experience, for your information only.
>>
>> 0) If the patch is generic, not specific to Bhyve, but benefit to current EDKII pkg,
>> you can submit them directly. No need to wait for Bhyve.
>>
>> 1) If the patch is very simple, you can merge into current PKG with current DSC.
>> If there is something special to the Bhyve that can be detected at runtime, then
>> detect at runtime.
>> If there is something special to the Byhve that need to be determine at build
>> time, then you can introduce a PCD (such as PcdBhyveXXX) and configurate at
>> build time.
>>
>> 2) If the patch is big, you can introduce a standalone driver and put to current
>> PKG and introduce a new DSC file (such as OvmfBhyve.dsc). You can control and
>> build Byhve with the new DSC file.
>>
>> 3) If the patch is extremely big and has architecture difference, you can
>> introduce a new pkg (BhyvePkg) and put all new drivers there. You can still refer
>> to some drivers in OvmfPkg, which introduce a dependency (BhyvePkg =>
>> OvmfPkg). The OvmfPkg change may impact BhyvePkg build or running.
>>
>> X) Last but not least important, if the Bhyve has a different *security
>> requirement* or *threat model* with current Pkg, then you had better introduce
>> a new pkg or update the current Pkg with same threat model. Before that, you
>> had better not use any driver in other package and keep them separate. It is easy
>> for future audit purpose.
>>
>> Above is the generic rule. I think OvmfPkg maintainer can provide more
>> comment on that.
>>
>> Can you post the patch? :-)
>>
>> Thank you
>> Yao Jiewen
>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Rebecca
>>> Cran
>>> Sent: Saturday, March 7, 2020 12:10 AM
>>> To: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Justen, Jordan
>> L
>>> <jordan.l.justen@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Subject: [edk2-devel] Adding Bhyve support into upstream EDK2
>>>
>>> I'm currently working on updating EDK2 support for Bhyve
>>> (https://bhyve.org/) from the edk2-stable201903 tag to
>>> edk2-stable202002. It's currently kept in a separate repo
>>> (https://github.com/freebsd/uefi-edk2), but I'd like to discuss pushing
>>> support upstream into the main edk2 repo (I guess into edk2-staging as a
>>> first step?).
>>>
>>>
>>> Would that be something people would be open to considering, or should
>>> it remain separate? Should it be a new top-level package (e.g. BhyvePkg)
>>> or could it be just a configuration option when building OVMF? It's
>>> currently maintained as a set of patches against OvmfPkg, which seems to
>>> work quite well.
>>>
>>>
>>> --
>>> Rebecca Cran
>>>
>>>
>>>
>>>
>>
>>
>> 
> 

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Ard Biesheuvel]

On Sat, 7 Mar 2020 at 08:39, Laszlo Ersek <lersek@redhat.com> wrote:
>
> Hi Jiewen,
>
> On 03/07/20 02:43, Yao, Jiewen wrote:
> > Just saw Laszlo's email. Similar feedback. Especially, I like the regression test part.
>
> Thanks.
>
> > I am not sure how many virtual platforms we will have eventually.
> > If there are more and more, maybe we can create a new edk2-virt-platform repo, and put them together there. (Similar to edk2-platform repo for the physical platform)
>
> Regarding the last part ("move them together here") -- I'm 100% opposed
> to removing OvmfPkg and ArmVirtPkg from edk2. They *must* remain in the
> exact same git repository where the core (MdePkg, MdeModulePkg,
> CryptoPkg, SecurityPkg, UefiCpuPkg, ...) lives too, and share a common
> git history.
>

Agreed.

> ArmVirtPkg and OvmfPkg move very closely together with the core, most
> significant ArmVirtPkg and OvmfPkg contributions need changes (and
> therefore introduce new dependencies) on the core. Managing such
> dependencies is a nightmare evein with git submodules; it only works if
> the git history is shared. This problem is not theoretical, it already
> has a bad effect on edk2-platforms.
>
> For a recent example, my latest OvmfPkg patch series:
>
> https://bugzilla.tianocore.org/show_bug.cgi?id=1512#c18
>
> merged as commit range 61d3b2d4279e..1158fc8e2c7b, started by improving
> the logging in MdeModulePkg/PiSmmCore (a1ddad95933e), and fixing a bug
> in UefiCpuPkg/PiSmmCpuDxeSmm (90e11edd16c7).
>
> I don't necessarily mind if *new* virtual platforms are outside of the
> edk2 tree, but if I'm completely honest about "why", it's because I
> don't use those new platforms. And that's a *selfish* reason -- if I
> want ArmVirtPkg and OvmfPkg to benefit from sharing and interleaving
> their histories with the core, then other virtual platforms deserve the
> same, even if I don't use them.
>
> (In fact, I think that even edk2-platforms should never have been split
> out of edk2 -- but that ship has sailed. I believe I argued against
> separating edk2-platforms, but my reasons weren't strong or convincing
> enough.)
>

Yes, keeping platforms in sync with the core is more painful than it
should be. If we move all platforms out of the core, what are we going
to do for validation? Sure, we'll get a nice tickbox from Azure that
all the semicolons line up nicely, but being able to build something
that can be tested on actual hardware is essential IMO.

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Rebecca Cran]

On 3/7/2020 12:52 AM, Ard Biesheuvel wrote:

> Yes, keeping platforms in sync with the core is more painful than it
> should be. If we move all platforms out of the core, what are we going
> to do for validation? Sure, we'll get a nice tickbox from Azure that
> all the semicolons line up nicely, but being able to build something
> that can be tested on actual hardware is essential IMO.

Talking of which, it would be nice if we could get that sort of 
validation into the CI builds. I'm hoping somebody's already working on 
that, since it was mentioned during the initial RFC process?


-- 

Rebecca Cran

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Sean]

[-- Attachment #1: Type: text/plain, Size: 1737 bytes --]

Ard/Lazlo,

I find your position on OvmfPkg, ArmVirtPkg,and edk2-platforms in edk2 to be detrimental to the overall success of the edk2 project.  The majority of edk2 consumers already have to deal with their platform not being part of the edk2 git repo and the fact that changes to edk2 may not work or cause conflict on their platforms.  This is the reality of working with edk2 and building a large diverse set of products.  In fact, because OVMF's preferential treatment and presence in the edk2 tree, others in the community are burdened by those changes (mailing list noise, git history/commits, git repo size, etc).   From someone who has developed and maintained platforms with edk2 for the last decade +, I can tell you that having OVMF in edk2 hasn't kept the tree free from regressions. I also don’t use it as a source for integration requirements because it is vastly different than physical platforms and has very little resemblance to my projects.

I share your concerns about regressions, ease of development, etc. I want to see tianocore develop workflows and processes that work for the full community.  That means we must take into account the needs of those that have platforms outside the edk2 repo.  We must build tools that make managing all platforms dependencies tenable.  We must create CI that can leverage a vast number of platforms as an indicator of breaking changes, yet not be gated on their success. We must have a process that lets all interested platforms efficiently interact with and be part of.  The tianocore community is in need of the larger UEFI ecosystem engagement and we must remove the barriers to get them involved.

As always I appreciate the discussion.

Thanks
Sean

[-- Attachment #2: Type: text/html, Size: 2150 bytes --]

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Laszlo Ersek]

On 03/09/20 07:08, Sean via Groups.Io wrote:
> Ard/Lazlo,
>
> I find your position on OvmfPkg, ArmVirtPkg,and edk2-platforms in edk2
> to be detrimental to the overall success of the edk2 project.  The
> majority of edk2 consumers already have to deal with their platform
> not being part of the edk2 git repo and the fact that changes to edk2
> may not work or cause conflict on their platforms.  This is the
> reality of working with edk2

This may be the reality for proprietary downstreams that don't, or
hardly, upstream their changes,

The reality that I have known, since joining the project in 2011, is
that OVMF had been in the tree before I joined. And we added ArmVirtPkg
in 2014 (or so) similarly. (It bore a different name back then.)

> and building a large diverse set of products.  In fact, because OVMF's
> preferential treatment

You are making it appear as if I wanted to keep OVMF in the tree and at
the same time kick other platforms out of the tree, or otherwise stymie
core contributions motivated by other platforms' needs.

That couldn't be farther from the truth. I have stated on multiple
occasions that I wish all edk2 consuming platforms to be in-tree.

> and presence in the edk2 tree, others in the community are burdened by
> those changes (mailing list noise, git history/commits, git repo size,
> etc).

Please run git log and check my contributions in the edk2 history that
fall outside of OvmfPkg and ArmVirtPkg.

$ git log --oneline --reverse \
  --author='Laszlo Ersek <lersek@redhat.com>' master -- \
  ':!OvmfPkg' \
  ':!ArmVirtPkg' \
  ':!ArmPlatformPkg/ArmVirtualizationPkg' \
| wc -l

With master being at a3e25cc8a1dd, that's ~474 commits, out of my ~1266
total. (Those numbers are not precise minimally because, at the start of
the git history, while we used SVN, authorship wasn't captured
correctly.)

IOW, approx. 37 percent of my patches have been for neither OvmfPkg nor
ArmVirtPkg.

My latest commit that fixes a bug in a core package is five days old
(90e11edd16c7, "UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU
hotplug", 2020-03-04).

- "mailing list noise": I have processed the complete traffic of the
mailing list for 9 years now (not read, but processed). edk2-devel has a
low amount of traffic relative to other open source development lists.
It is a fraction of qemu-devel, for example. If you see a large OvmfPkg
patch series in your list folder, it will say "OvmfPkg" in the blurb
subject, and then it takes a single keypress to mark the whole thread
read.

We could introduce topical mailing lists (different subsystem patches
would be CC'd to different lists, but everything would still need to go
to the main list as well).

- "git repo size": for a current master checkout, OvmfPkg weighs in at
5.5M. In comparison, NetworkPkg is 7.2M, MdePkg is 18M, MdeModulePkg is
27M. Even considering all git objects and such, the repo is not larger
than 1.1GiB or so.

I assume your platforms must be larger than 5.5M. I'd be happy to have
them in the tree.

- "git history / commits": they are easy to filter to the packages you
care about. We take care not to straddle multiple packages with single
commits.

May those others in the community please speak up about the burden
that OvmfPkg development has subjected them to.

> From someone who has developed and maintained platforms with edk2 for
> the last decade +, I can tell you that having OVMF in edk2 hasn't kept
> the tree free from regressions.

Of course. Changes to core packages (even spec changes) are always
motivated by platform goals. And regressions are unavoidable, as much as
we try to prevent them (even *spec* regressions). I hope you're not
claiming that OVMF has been a major source of regressions in the core
packages.

And, you have the advantage of *plausibly* claiming that OVMF-oriented
changes have caused regressions in the core -- plausibly because those
contributions, and the dependent OVMF changes, are upstream. Whereas I
can't classify any other regressions as associated by Microsoft platform
needs, simply because those platform needs have hardly ever been public.
(One of my never-ending crusades has been "better commit messages",
i.e., with use case spelled out.)

> I also don't use it as a source for integration requirements because
> it is vastly different than physical platforms and has very little
> resemblance to my projects.

That's fine. There's a whole bunch of stuff in the tree that's
irrelevant to my purposes, I just don't go ahead and claim that they
"clutter" the git history for me.

- BaseTools -- check. I don't really care about BaseTools internals, I
just want them to function OK. In fact, BaseTools used to live outside
of the tree, with huge code drops / syncs, and that was no end of
misery, for example because it was un-bisectable.

(BTW, you are throwing the possibility of bisecting a platform and the
core together out the window. Why should any given platform lose this
very important trait just because other platforms have never had it, out
of their own choice?)

- DynamicTablesPkg -- check. No interest, yet it doesn't bother me.

- EmulatorPkg,
  FmpDevicePkg,
  IntelFsp2Pkg,
  IntelFsp2WrapperPkg,
  SignedCapsulePkg,
  SourceLevelDebugPkg,
  StandaloneMmPkg,
  UefiPayloadPkg,
  UnitTestFrameworkPkg -- check, all irrelevant to me.

It's very easy to ignore patches, emails, and discussions around these
packages. It's also easy to join them.

Even when AppPkg and StdLib were split out to edk2-libc, in
<https://bugzilla.tianocore.org/show_bug.cgi?id=1734>, I made the
following clear in my agreement:

  https://edk2.groups.io/g/devel/message/35321

    I'm not sure how closely the StdLib internals are tied to day-to-day
    changes in core edk2; that is, whether we should keep those
    histories interlinked. That's something for the StdLib maintainers
    to evaluate. Personally I don't remember many StdLib changes, so
    there seems to be a genuine separation that supports the new repo
    idea.

I was willing to listen to the StdLib maintainers, if they had wanted to
keep the package inside.

> I share your concerns about regressions, ease of development, etc. I
> want to see tianocore develop workflows and processes that work for
> the full community.  That means we must take into account the needs of
> those that have platforms outside the edk2 repo.  We must build tools
> that make managing all platforms dependencies tenable.  We must create
> CI that can leverage a vast number of platforms as an indicator of
> breaking changes, yet not be gated on their success.

(Side comment: it wasn't me who filed this:
<https://bugzilla.tianocore.org/show_bug.cgi?id=2570>.)

> We must have a process that lets all interested platforms efficiently
> interact with and be part of.  The tianocore community is in need of
> the larger UEFI ecosystem engagement and we must remove the barriers
> to get them involved.

I don't want to keep anyone out. I just want to keep OvmfPkg and
ArmVirtPkg in.

The goals you describe in this paragraph don't conflict with OvmfPkg and
ArmVirtPkg, as they are, i.e., in-tree.

Laszlo

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Laszlo Ersek]

On 03/09/20 23:54, Laszlo Ersek wrote:
> On 03/09/20 07:08, Sean via Groups.Io wrote:

>> From someone who has developed and maintained platforms with edk2 for
>> the last decade +, I can tell you that having OVMF in edk2 hasn't kept
>> the tree free from regressions.
> 
> Of course. Changes to core packages (even spec changes) are always
> motivated by platform goals. And regressions are unavoidable, as much as
> we try to prevent them (even *spec* regressions). I hope you're not
> claiming that OVMF has been a major source of regressions in the core
> packages.
> 
> And, you have the advantage of *plausibly* claiming that OVMF-oriented
> changes have caused regressions in the core -- plausibly because those
> contributions, and the dependent OVMF changes, are upstream. Whereas I
> can't classify any other regressions as associated by Microsoft platform
> needs, simply because those platform needs have hardly ever been public.
> (One of my never-ending crusades has been "better commit messages",
> i.e., with use case spelled out.)

I think I misread your point.

IIUC, you weren't blaming OVMF for regressions, instead, you were
questioning the usefulness of OVMF in catching regressions in the core.

Putting aside your rhetorical expression "keep the tree free from
regressions" -- because how could any single platform keep the whole
tree free of regressions? --, OVMF *has* caught regressions. In fact,
running OVMF on QEMU/KVM has been one of the best ways for rooting out
multiprocessing / synchronization bugs in UefiCpuPkg, due to the VCPU
jitter that KVM tends to introduce. This is why I asked (years ago) to
be CC'd on all UefiCpuPkg patches, so I could regression-test them
before they are pushed. (Hence my "R:" role in UefiCpuPkg.)

$ git log \
  --oneline \
  --reverse \
  --grep='Regression-tested-by: Laszlo Ersek <lersek@redhat.com>' \
  master -- \
  ':!OvmfPkg' \
  ':!ArmVirtPkg' \
  ':!ArmPlatformPkg/ArmVirtualizationPkg' \
| wc -l

(83 patches)


This is also (part of) why I wrote:

https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt


Two semi-recent commits that fixed regressions (exposed by OVMF) have been:

- 4d05a4b709ce ("MdeModulePkg/BdsDxe: Fix calling
PlatformBootManagerWaitCallback on 0", 2019-10-15)

- a52355624440 ("UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting
regression for PDEs", 2020-01-17)

Thanks
Laszlo

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Sean]

[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]

Sorry let me clarify a little more.  I don't doubt that you spending the time to validate a patch using OVMF catches issues and regressions.  I am very appreciative of that effort, my point is you could do that without being in the edk2 tree and in fact we need to enable and request that more do that for their given platforms (Platform CI needs to make this possible).  I also didn't mean to imply that you only wanted your platform in the tree.  My ask is that we evaluate a strategy where all platforms and all things not "core" get removed from edk2 proper.   This means silicon specific modules move into silicon specific repos and packages full of limited use modules get moved out too. At that point we can focus edk2 on delivering a small, high quality core and focus the community on workflows that leverage multiple repos.  There are numerous ways to keep multiple repos in sync that allow a user to root cause regressions.  Using submodules is one way to easily track the version of edk2 that was/is used in the platform repo and when issues are identified the commit history can be bisected to find the problem.  One other great benefit of multiple repos is that the repo maintainers can define their own workflows and requirements.

I am not as proficient as you at mailing list management.  Nor are the people I interact with given that we all often miss many things we would like to comment on or change until post commit/integration.  I equate much of this to the mailing list review process and the value to noise ratio of the mailing list.   So although one package may not overwhelm, it does continue to distract from the most critical of changes in the core.

Thanks
Sean

[-- Attachment #2: Type: text/html, Size: 1790 bytes --]

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Laszlo Ersek]

On 03/10/20 02:50, sean.brogan via [] wrote:
> [...] There are numerous ways to keep multiple repos in sync that
> allow a user to root cause regressions.  Using submodules is one way
> to easily track the version of edk2 that was/is used in the platform
> repo and when issues are identified the commit history can be
> bisected to find the problem.  [...]
I disagree.

Running git-bisect on a commit range in the superproject will identify
the submodule update commit as the one introducing the regression, yes.

But I don't see a way forward from that point onward. The goal of
git-bisect is to identify (pinpoint) the smallest atomic change that's
related to a regression. A submodule update is the polar opposite of an
atomic change.

Delving into the submodule itself, for a git-bisect -- i.e., bisecting
the submodule between the boundaries set by the superproject's submodule
update commit -- is not something that can be relied upon.

Namely, a core trait of git-bisect is that it tests states of the whole
project that have *existed* at some point. But a combination of:

- the superproject checked out at any particular point in its own history,

- and the submodule checked out strictly *between* two such adjacent
commits that have ever been actively consumed by the superproject,

is not a state like that.

Such states may not even build. They may never have been tested, put
through CI, and so whether they work or not is inconsequential -- they
should not be *expected* to work. Such states don't qualify as
"checkpoints". That's because a submodule is unaware of all the
superprojects that consume it. Whereas, in a shared git history, every
single commit counts as a checkpoint -- a combined state that has
existed at some point.

You can solve this problem (in the separate repository model) only if
you build and test every known platform (= every known superproject)
against every single patch (not series!) that is proposed for the core
(= the submodule). Importantly, it's not enough to build and test the
superprojects at the ends of the submodule (= core) patch sets, because
that still leaves the checkpoints two far apart. The full platform
testing (for all platforms) would have to be done at every stage of the
submodule (= core) patch series.

Turning a codebase (such as core edk2) into a submodule for a
superproject (such as OVMF) is equivalent to saying, "we trust the
submodule that, at distinguished checkouts, it holds up all its
contracts". That's exactly what I don't trust the core of edk2 to do,
and those contract violations are exactly what git-bisect is designed to
root out.

The edk2 bisection that I've performed most recently was less than nine
hours ago. It identified a regression in a core (non-platform) change,
namely "ArmPkg/Library/ArmMmuLib":

https://edk2.groups.io/g/devel/message/55701

It was doable because git-bisect was aware of six patches that had been
applied in a row on ArmMmuLib against a single ArmVirtQemu platform state.

I may not be knowledgeable enough about git-bisect, of course; I've
asked my teammates internally if they have positive experience and/or
recommendations with git-bisect across submodules.

Thanks
Laszlo

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Sean]

[-- Attachment #1: Type: text/plain, Size: 1799 bytes --]

I don't see the difference besides the mechanics of the operation (which you have described clearly).  To guarantee a repo or repos is "git-bisectable" you need to build and test every commit on your platform.  For example in the recent ArmMmuLib patchset, you were able to build every commit in the patch to identify which one caused the break.  There isn't an enforced process in place to ensure that happens within Edk2.  Thankfully the review process and the developers knowledge allowed the commits to be made in such a way that this was possible.  That doesn't have to change when you move to a submodule.  Also you could put automation in place to enforce and/or test for this scenario.  You can put automation in place to "integrate" into your super project at every commit if you really wanted to and had the resources to run tests on every one of those commits.  Is this type of CI done today for OVMF?

Again this is what nearly all platforms have to do today and we have a lot of experience with bisecting within the submodule to find the error.  The longer you wait between integrations the more costly the bisect is if you have to do it, but this is a choice of the super project owner / platform owner.  Today I assume you make those choices too, they just happen to be within the same repo.  I also assume that if you found the MmuLib bug in a few days you probably wouldn't bisect all the changes but you would review the history to intelligently guess at the most likely candidates and bisect within those commits.

In the end I just don't see the big difference to the platform (OVMF in this case) but I do see the reduced size/noise/content helping all platforms.  Success still relies on good development practices, regular builds, and testing.

Thanks
Sean

[-- Attachment #2: Type: text/html, Size: 1930 bytes --]

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Ard Biesheuvel]

For me, the issue is more fundamental than what Laszlo describes.

The platforms that target qemu, which is available to anyone, can run
on any host, and can boot various OSes beyond Linux (including
windows), which is their primary target. It even supports
virtualization, which is highly significant on arm, given the tricky
cache maintenance and things like aligned access and dc zva
instructions, which qemu does not catch in emulation mode.

This means we can reasonably require any contributor not to regress on
those platforms, given how they have full access to it, and this is
actually where i would like us to take the next step when it comes to
ci automation, i.e., automatically flag PRs that break the boot on a
selected set of qemu based configs.

Putting those platforms in a separate repository complicates this to
the extent where it is no longer feasible to reason about the core
repository being in a working or broken state, given how intrusive
changes usually require changes on the platform description side as
well, and I don’t see the validation  tools handling two repositories
in parallel.

Beyond this, i have no fundamental objections to moving  things out of
the core, and i’d remove more cruft from ArmPlatformPkg or EmbeddedPkg
if i could (including, e.g., the PrePi SEC code that should have never
existed)



On 10/03/2020, Sean via Groups.Io <sean.brogan=microsoft.com@groups.io> wrote:
> I don't see the difference besides the mechanics of the operation (which you
> have described clearly).  To guarantee a repo or repos is "git-bisectable"
> you need to build and test every commit on your platform.  For example in
> the recent ArmMmuLib patchset, you were able to build every commit in the
> patch to identify which one caused the break.  There isn't an enforced
> process in place to ensure that happens within Edk2.  Thankfully the review
> process and the developers knowledge allowed the commits to be made in such
> a way that this was possible.  That doesn't have to change when you move to
> a submodule.  Also you could put automation in place to enforce and/or test
> for this scenario.  You can put automation in place to "integrate" into your
> super project at every commit if you really wanted to and had the resources
> to run tests on every one of those commits.  Is this type of CI done today
> for OVMF?
>
> Again this is what nearly all platforms have to do today and we have a lot
> of experience with bisecting within the submodule to find the error.  The
> longer you wait between integrations the more costly the bisect is if you
> have to do it, but this is a choice of the super project owner / platform
> owner.  Today I assume you make those choices too, they just happen to be
> within the same repo.  I also assume that if you found the MmuLib bug in a
> few days you probably wouldn't bisect all the changes but you would review
> the history to intelligently guess at the most likely candidates and bisect
> within those commits.
>
> In the end I just don't see the big difference to the platform (OVMF in this
> case) but I do see the reduced size/noise/content helping all platforms.
> Success still relies on good development practices, regular builds, and
> testing.
>
> Thanks
> Sean
>
> 
>
>

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Sean]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]

On Tue, Mar 10, 2020 at 10:54 AM, Ard Biesheuvel wrote:

> 
> This means we can reasonably require any contributor not to regress on
> those platforms, given how they have full access to it, and this is
> actually where i would like us to take the next step when it comes to
> ci automation, i.e., automatically flag PRs that break the boot on a
> selected set of qemu based configs.

I appreciate the viewpoint but I don't think edk2 has "required" this yet and I would push back on that if it was brought up for community discussion.  As you know I believe contributing to edk2 is much too hard already, and filled with custom edk2 workflows, and this one would only make it more of a burden to contribute.

I do however agree that if this is important to the edk2 community, systems/automation should be setup to run this type of thing.  If that happens, i don't think it is much harder to do it for multi-repo than single repo and if done right for multi-repo then other platforms can opt in as well and the efforts to build this infrastructure will scale well beyond a single virtual platform.

Thanks
Sean

[-- Attachment #2: Type: text/html, Size: 1198 bytes --]

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 1797 bytes --]

Sean,

This is the reason that OvmfPkg was kept in the edk2 repo so only a single repo is required for local dev testing and CI testing.  Same reason for the EmulatorPkg.

The current rule for the edk2 repo is common firmware packages and virtual/emulated platforms.
The fact that there has not been bandwidth to implement the CI testing for OvmfPkg or EmulatorPkg is not a reasonable reason to remove them.

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean via Groups.Io
Sent: Tuesday, March 10, 2020 12:11 PM
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; devel@edk2.groups.io
Subject: Re: [edk2-devel] Adding Bhyve support into upstream EDK2

On Tue, Mar 10, 2020 at 10:54 AM, Ard Biesheuvel wrote:
This means we can reasonably require any contributor not to regress on
those platforms, given how they have full access to it, and this is
actually where i would like us to take the next step when it comes to
ci automation, i.e., automatically flag PRs that break the boot on a
selected set of qemu based configs.
I appreciate the viewpoint but I don't think edk2 has "required" this yet and I would push back on that if it was brought up for community discussion.  As you know I believe contributing to edk2 is much too hard already, and filled with custom edk2 workflows, and this one would only make it more of a burden to contribute.

I do however agree that if this is important to the edk2 community, systems/automation should be setup to run this type of thing.  If that happens, i don't think it is much harder to do it for multi-repo than single repo and if done right for multi-repo then other platforms can opt in as well and the efforts to build this infrastructure will scale well beyond a single virtual platform.

Thanks
Sean


[-- Attachment #2: Type: text/html, Size: 42114 bytes --]

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Sean]

[-- Attachment #1: Type: text/plain, Size: 1298 bytes --]

If I look around i don't see that documented.
https://github.com/tianocore/edk2#code-contributions
https://github.com/tianocore/tianocore.github.io/wiki/Code-Contributions
https://github.com/tianocore/tianocore.github.io/wiki/How-To-Contribute
https://github.com/tianocore/tianocore.github.io/wiki/How-to-Become-a-Contributor

In Laszlo's massive write up that is very detailed for the contribution process there is nothing calling out that the contributor must test on any given platform.
He does however suggest that maybe as a maintainer you should test it but again no indication of how/what/where.
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#maint-13

In all the teams i have worked with over the years no one has used OVMF or emulator package as the verification for an edk2 patch.  Maybe that is just my experience.

Overall my point is I don't think this is clear to the community and I believe that adding this requirement without actually building the full infrastructure online will be detrimental to the contribution process.  And if building the infrastructure online then build it to scale to N platforms to cover more of the edk2 community and not just those in the edk2 tree.

Thanks
Sean

[-- Attachment #2: Type: text/html, Size: 1898 bytes --]

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Rebecca Cran]

[-- Attachment #1: Type: text/plain, Size: 1643 bytes --]

I think the expectation is that any OVMF testing requirements would be added to the CI system, so contributors would simply have to wait a few extra minutes when submitting a change?

Rebecca Cran 
> On Mar 10, 2020, at 1:44 PM, Sean via Groups.Io <sean.brogan=microsoft.com@groups.io> wrote:
> 
> If I look around i don't see that documented. 
> https://github.com/tianocore/edk2#code-contributions
> https://github.com/tianocore/tianocore.github.io/wiki/Code-Contributions
> https://github.com/tianocore/tianocore.github.io/wiki/How-To-Contribute
> https://github.com/tianocore/tianocore.github.io/wiki/How-to-Become-a-Contributor
> 
> In Laszlo's massive write up that is very detailed for the contribution process there is nothing calling out that the contributor must test on any given platform.
> He does however suggest that maybe as a maintainer you should test it but again no indication of how/what/where.
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#maint-13
> 
> In all the teams i have worked with over the years no one has used OVMF or emulator package as the verification for an edk2 patch.  Maybe that is just my experience.
> 
> Overall my point is I don't think this is clear to the community and I believe that adding this requirement without actually building the full infrastructure online will be detrimental to the contribution process.  And if building the infrastructure online then build it to scale to N platforms to cover more of the edk2 community and not just those in the edk2 tree.
> 
> Thanks
> Sean
> 
> 


[-- Attachment #2: Type: text/html, Size: 2483 bytes --]

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Laszlo Ersek]

(This message seems to have broken the threading, so I'll make an
attempt below, to quote the context in a way that I think is logical.)

On 03/10/20 20:23, Michael D Kinney wrote:
> Sean,
>
> This is the reason that OvmfPkg was kept in the edk2 repo so only a
> single repo is required for local dev testing and CI testing.  Same
> reason for the EmulatorPkg.
>
> The current rule for the edk2 repo is common firmware packages and
> virtual/emulated platforms.
> The fact that there has not been bandwidth to implement the CI testing
> for OvmfPkg or EmulatorPkg is not a reasonable reason to remove them.
>
> Mike

On 03/10/20 20:44, Sean via Groups.Io wrote:
> If I look around i don't see that documented.
> https://github.com/tianocore/edk2#code-contributions
> https://github.com/tianocore/tianocore.github.io/wiki/Code-Contributions
> https://github.com/tianocore/tianocore.github.io/wiki/How-To-Contribute
> https://github.com/tianocore/tianocore.github.io/wiki/How-to-Become-a-Contributor
>
> In Laszlo's massive write up that is very detailed for the
> contribution process there is nothing calling out that the contributor
> must test on any given platform.
> He does however suggest that maybe as a maintainer you should test it
> but again no indication of how/what/where.
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#maint-13
>
> In all the teams i have worked with over the years no one has used
> OVMF or emulator package as the verification for an edk2 patch.  Maybe
> that is just my experience.
>
> Overall my point is I don't think this is clear to the community and I
> believe that adding this requirement without actually building the
> full infrastructure online will be detrimental to the contribution
> process. And if building the infrastructure online then build it to
> scale to N platforms to cover more of the edk2 community and not just
> those in the edk2 tree.

I remember that "virtual platforms should remain in the tree"
(paraphrased) was part of one of Mike's RFCs for the split-out
repositories (edk2-platforms or similar). I can't find the exact message
now (even though I recall responding to it, with a "thank you"), but I
have found subsequent statements:

* http://mid.mail-archive.com/E92EE9817A31E24EB0585FDF735412F5B8B385D8@ORSMSX113.amr.corp.intel.com
  https://lists.01.org/hyperkitty/list/edk2-devel@lists.01.org/message/BG7IWV7SWJCX44T4RFAE36GJNJGBBPRH/

* https://bugzilla.tianocore.org/show_bug.cgi?id=1374#c0
* https://bugzilla.tianocore.org/show_bug.cgi?id=1467#c0
* https://bugzilla.tianocore.org/show_bug.cgi?id=1793#c0

Not claiming that any of these are official documentation, it's just
illustration that it has been "operating knowledge / agreement" for
many.

Thanks
Laszlo

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Sean]

[-- Attachment #1: Type: text/plain, Size: 1026 bytes --]

Well time to let this one simmer a while.  I appreciate the viewpoints.

For what it is worth, I am unconvinced that leaving OVMF, ArmVirt, or Emulator packages in the edk2 tree is the right choice.  I would be opposed to adding more as that would just continue the growth and noise of platforms within a "core" repo.  In my view there are other, more consistent options for how to develop platforms using edk2 leveraging submodules (or a locked manifest).  See Project Mu ( https://microsoft.github.io/mu/ ( https://microsoft.github.io/mu/ ) ) for a strategy I find extremely successful that supports open source and closed source projects equally.  Respectfully, Edk2-Platforms is another great example of doing it wrong, but lets not get into that on this thread.  :)

I too believe that a CI system in which platforms can report their compatibility with a change is critical and will let core code developers make educated decisions about their changes and their impact on downstream platforms.

Thanks
Sean

[-- Attachment #2: Type: text/html, Size: 1110 bytes --]

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Liming Gao]

[-- Attachment #1: Type: text/plain, Size: 1923 bytes --]

Sean:
  We mostly use Emulator to verify and debug the code change in the common module, such as BDS, HII, DxeCore, PeiCore. Compared to the real platform, Emulator and OVMF are more easier to be built and verified. So, I request to add Open CI build test for Ovmf and Emulator (BZ 2570). Later, we may consider to add boot test on them.

Thanks
Liming
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean via Groups.Io
Sent: Wednesday, March 11, 2020 3:45 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] Adding Bhyve support into upstream EDK2

If I look around i don't see that documented.
https://github.com/tianocore/edk2#code-contributions
https://github.com/tianocore/tianocore.github.io/wiki/Code-Contributions
https://github.com/tianocore/tianocore.github.io/wiki/How-To-Contribute
https://github.com/tianocore/tianocore.github.io/wiki/How-to-Become-a-Contributor

In Laszlo's massive write up that is very detailed for the contribution process there is nothing calling out that the contributor must test on any given platform.
He does however suggest that maybe as a maintainer you should test it but again no indication of how/what/where.
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#maint-13

In all the teams i have worked with over the years no one has used OVMF or emulator package as the verification for an edk2 patch.  Maybe that is just my experience.

Overall my point is I don't think this is clear to the community and I believe that adding this requirement without actually building the full infrastructure online will be detrimental to the contribution process.  And if building the infrastructure online then build it to scale to N platforms to cover more of the edk2 community and not just those in the edk2 tree.

Thanks
Sean


[-- Attachment #2: Type: text/html, Size: 5323 bytes --]

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Laszlo Ersek]

On 03/10/20 18:25, sean.brogan via [] wrote:
> I don't see the difference besides the mechanics of the operation (which
> you have described clearly).  To guarantee a repo or repos is
> "git-bisectable" you need to build and test every commit on your
> platform.  For example in the recent ArmMmuLib patchset, you were able
> to build every commit in the patch to identify which one caused the
> break.  There isn't an enforced process in place to ensure that happens
> within Edk2.  Thankfully the review process and the developers knowledge
> allowed the commits to be made in such a way that this was possible. 
> That doesn't have to change when you move to a submodule.  Also you
> could put automation in place to enforce and/or test for this scenario. 
> You can put automation in place to "integrate" into your super project
> at every commit if you really wanted to and had the resources to run
> tests on every one of those commits.  Is this type of CI done today for
> OVMF?  

Consider a patch series that needs to modify at least one core module
and at least one platform, in order to reach a desired result. Such
patch series are not infrequent: one of my recent contributions (commit
range 61d3b2d4279e..1158fc8e2c7b) modified MdeModulePkg, UefiCpuPkg, and
OvmfPkg.

When I work on a patch series (any patch series, really) I not only test
that the work builds at every stage, but also that it *functions* at
every stage (in other words that no stage in the series regresses
existing functionality, and that it indeed implements the desired step
of the feature or bugfix). This is done specifically so that, in case I
miss a use case completely, and regress it, another developer can return
to any stage across my work (regardless of "core module" vs "platform
code" distinction), via bisection, and pinpoint my mistake.

So: this kind of CI is not being done (it's not enforced), but I take it
seriously.

If we split platforms to different repos from the core, I can't even
propose a patch set, in the first place, that modifies both kinds of
modules in one logical go.

> 
> Again this is what nearly all platforms have to do today and we have a
> lot of experience with bisecting within the submodule to find the
> error.  The longer you wait between integrations the more costly the
> bisect is if you have to do it, but this is a choice of the super
> project owner / platform owner.  Today I assume you make those choices
> too, they just happen to be within the same repo.  I also assume that if
> you found the MmuLib bug in a few days you probably wouldn't bisect all
> the changes but you would review the history to intelligently guess at
> the most likely candidates and bisect within those commits.

My thinking is the inverse. Bisection cost grows logarithmically --
covering a range of 256 commits takes just one build more than covering
a range of 128 commits. To me, bisection comes first, exactly so I don't
have to think (or intelligently guess), before the issue is narrowed
down to a single patch. I have seen such a huge amount of incredible /
stunning bisection results ("I would have never thought of this!") that
I don't even try to guess, any more.

Regarding the six patches that I bisected a few days ago -- those hadn't
even been upstreamed, I was preparing to merge them, for Ard. I
collected them from three separate series (posted & reviewed on the
list), in the logical order explained by the cover letters. They were
super quirky and over my head (aarch64 MMU programming? thanks but no
thanks); I would have had zero chance to guess which one caused the
crash with "grubaa64.efi".

The bisection allowed me at least to tell Ard which one of his patches
looked risky.

... I've got almost entirely mechanical Linux kernel bisections behind
me that took 5-8 hours and pinpointed very unexpected culprit patches,
from multiple thousands of commits between releases. I could have never
tracked those bisections myself, if I had had to dig into submodules
manually.

I understand that I'm appearing too stubborn. I promise it's not just
for the sake of arguing.

Thanks,
Laszlo

> 
> In the end I just don't see the big difference to the platform (OVMF in
> this case) but I do see the reduced size/noise/content helping all
> platforms.  Success still relies on good development practices, regular
> builds, and testing.    
> 
> Thanks
> Sean
> 
>   

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Leif Lindholm]

Hi Sean,

On Sun, Mar 08, 2020 at 23:08:54 -0700, Sean via Groups.Io wrote:
> Ard/Lazlo,
> 
> I find your position on OvmfPkg, ArmVirtPkg,and edk2-platforms in
> edk2 to be detrimental to the overall success of the edk2 project.
>
> The majority of edk2 consumers already have to deal with their
> platform not being part of the edk2 git repo and the fact that
> changes to edk2 may not work or cause conflict on their platforms.

The community took the decision to include virtual platforms in edk2
in order to make it possible for anyone to build and test complete
platform ports against the core tree.

Referring to this as the position of Ard and Laszlo is out of line.
I think you owe them both an apology.

If you want to change that decision, I suggest you join in, become
part of the community and try to change things from within rather than
continuing to stand on the sidelines shouting "YOU'RE DOING IT WRONG".

/
    Leif

Re: [edk2-devel] Adding Bhyve support into upstream EDK2

[Laszlo Ersek]

On 03/07/20 08:39, Laszlo Ersek wrote:
> Hi Jiewen,
> 
> On 03/07/20 02:43, Yao, Jiewen wrote:
>> Just saw Laszlo's email. Similar feedback. Especially, I like the regression test part.
> 
> Thanks.
> 
>> I am not sure how many virtual platforms we will have eventually.
>> If there are more and more, maybe we can create a new edk2-virt-platform repo, and put them together there. (Similar to edk2-platform repo for the physical platform)
> 
> Regarding the last part ("move them together here") -- I'm 100% opposed
> to removing OvmfPkg and ArmVirtPkg from edk2. They *must* remain in the
> exact same git repository where the core (MdePkg, MdeModulePkg,
> CryptoPkg, SecurityPkg, UefiCpuPkg, ...) lives too, and share a common
> git history.
> 
> ArmVirtPkg and OvmfPkg move very closely together with the core, most
> significant ArmVirtPkg and OvmfPkg contributions need changes (and
> therefore introduce new dependencies) on the core. Managing such
> dependencies is a nightmare evein with git submodules; it only works if
> the git history is shared. This problem is not theoretical, it already
> has a bad effect on edk2-platforms.
> 
> [...]

... now I expect this might raise the question why my stance on
consuming submodules in edk2 itself is different -- i.e., why it is that
I *like* edk2 to consume OpenSSL, brotli (TianoCore#2558,
TianoCore#2559), Oniguruma (TianoCore#2073) etc through git submodules.

Here's why: because we mostly treat those projects as black boxes.

- The contributor audiences for those projects hardly overlap with edk2
developers.

- The development workflows sharply differ.

- The rate of change introduced into those projects, for addressing
consumer (i.e., edk2) needs, is very low.

This is very much not the case between edk2, and: ArmVirtPkg and OvmfPkg
(and, again edk2-platforms).

Thanks
Laszlo