Re: [edk2-platforms][PATCH v1 01/02] QemuOpenBoardPkg: Add QemuOpenBoardPkg

["Théo Jehl" <theojehl76@gmail.com>]

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

>
> That is a rather short description for a patch of this size.  It
> probably makes sense to break that down into smaller pieces and make a
> patch series out of it because you can describe the specific pieces much
> better then.


I was thinking about breaking down the main patch into smaller ones
corresponding to
MinPlatform stages implementations to make it easier to follow along.

It seems the goal is to have a MinPlatform board which can easily be
> used to learn about and experiment with MinPlatform, is that correct?


The package goal is to be a simple implementation for developers wanting to
get started with board ports indeed.

Why duplicate that lib instead of just using the OvmfPkg version (which
> you do elsewhere)?


It was supposed to be for training purposes, I was discussing with my
mentors about that,
it stayed in the project because of its simplicity, the OvmfPkg is way more
complete but this
one provides only the essentials. And this simple implementation is used in
my custom libs, the Ovmf
version is used by OvmfPkg modules and libs. If indeed my lib doesn't bring
anything new or useful I can
remove it and switch to OvmfPkg QemuFwCfgLib instead.

Note that OvmfPkg got a PlatformInitLib recently which you might be able
> to use to reduce code duplication (didn't check the code though and
> maybe MinPlatform init is different enough that this doesn't help much).


For both GSOC purposes and MinPlatform compliance, I didn't want this
package to be OVMF but repacked,
PlatformInit from OVMF is very complete, maybe too much when we are
targeting a simple board port.
QemuOpenBoardPkg's PlatformInit only performs the necessary and nothing
more, to make reading and understanding
the package easier.
It's also up to debate, but concerning OVMF code, all the dependencies we
have with OVMF makes understanding
the boot flow a bit harder, I was thinking about slowly phasing out OVMF
modules with lot of external dependencies
in favor of new ones, keeping the "only do the necessary" pattern, just
like what happened with PlatformInitPei.

Regards,
Théo

Le ven. 2 sept. 2022 à 12:02, Gerd Hoffmann <kraxel@redhat.com> a écrit :

> On Sat, Aug 27, 2022 at 02:02:00AM +0200, Théo wrote:
> > From: Théo Jehl <theojehl76@gmail.com>
> >
> > QemuOpenBoardPkg adds a MinPlatform port to Qemu x86_64
> > It can boots UEFI Linux and Windows, and works on PIIX4 and Q35
> > This board port provides a simple starting place for investigating edk2
> and
> > MinPlatform Arch.
> > Currently we implement up to stage 4 of the MinPlatform spec and can boot
> > Windows/Linux.
>
> That is a rather short description for a patch of this size.  It
> probably makes sense to break that down into smaller pieces and make a
> patch series out of it because you can describe the specific pieces much
> better then.
>
> I'm not familiar with MinPlatform, just skimmed the manual.  Can't
> comment much on those details.
>
> It seems the goal is to have a MinPlatform board which can easily be
> used to learn about and experiment with MinPlatform, is that correct?
>
> >
> Platform/Qemu/QemuOpenBoardPkg/Library/OpenQemuFwCfgLib/OpenQemuFwCfgLib.inf
>      |  23 +
> >  Platform/Qemu/QemuOpenBoardPkg/Include/Library/OpenQemuFwCfgLib.h
>             | 102 +++
> >
> Platform/Qemu/QemuOpenBoardPkg/Library/OpenQemuFwCfgLib/OpenQemuFwCfgLib.c
>        | 130 ++++
>
> Why duplicate that lib instead of just using the OvmfPkg version (which
> you do elsewhere)?
>
> >  Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/Cpu.c
>            |  56 ++
> >  Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/Memory.c
>             | 244 ++++++++
> >  Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/Pci.c
>            |  59 ++
> >  Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/Pcie.c
>             |  91 +++
> >  Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/PlatformInit.c
>             |  67 ++
>
> Note that OvmfPkg got a PlatformInitLib recently which you might be able
> to use to reduce code duplication (didn't check the code though and
> maybe MinPlatform init is different enough that this doesn't help much).
>
> take care,
>   Gerd
>
>

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