From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f50.google.com (mail-ej1-f50.google.com [209.85.218.50]) by mx.groups.io with SMTP id smtpd.web09.6552.1662125828859557067 for ; Fri, 02 Sep 2022 06:37:09 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=dQ/iJGVL; spf=pass (domain: gmail.com, ip: 209.85.218.50, mailfrom: theojehl76@gmail.com) Received: by mail-ej1-f50.google.com with SMTP id bj12so3880325ejb.13 for ; Fri, 02 Sep 2022 06:37:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=dHfk+vcIZiN/NkdkJS35hesxm4Ol7G9Esez7+r8g2NE=; b=dQ/iJGVLFNM2HP1vUHYRl6nc8LswtVz1BfYpEBxyrrw01TxkYemUJW0YbfbYrvpNwD TePZCVrKaewODS8+4S3gAEmGYW/6xA8MD4I7nl1YRPGAUCizr/yfNMmdEgGMeP0386sX wDTOkwFeKYcB6FYlMVd3aSebgW21QKP237TbjIo/bkhRHeAh1cJOu+DdYGuO8t2a4zSG vLkeJ3R8plZ+r5F0tt76q8KWNsF2mqO12Ro/YJrJ4YM9Ue8M8zE37fzkjMNUupnY8WxM HMArz4J39reH3kKQiX2alHM/Lcsg5yo7s9QLVWGwR+lAmuyBbcDmtGuL5iexZkapBL4n AMkw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=dHfk+vcIZiN/NkdkJS35hesxm4Ol7G9Esez7+r8g2NE=; b=0Wc7wEBCIFbBZAhTr8QHMyUjgp/kIrZHtr8GIB+xSlxoeGRDHufhMnrbQa/lBxXGk5 e3NZSYk8NQFX4ThDQoB5TnKPSk2Kh/jttLuTk9s79DVvOzyDn+CnNilDsLSWCiptqOpy nIsA3FTIKxIfExYIX0vwgnV6sdKt1fcCPRPpLK3ovl4S59IQ6INQqW1AvlJ84UgoGIwQ hP3oCOp4hPM8efCe/HKifMmh0QOYdVzQVoxNmO4zbHAYLDHSexgajsrum8o3geJTDV1b MzBjN7IxWmcSMeIdWAWONvky6tKJtRy22ilKnW4wE2SDaSzvkolb4nT8v5DX+4RVV6kJ 00Og== X-Gm-Message-State: ACgBeo3Q0o6D5n6KQoJ5r0+CdbkjX/iDg3QysH4DEozg1+x8NxlBwHoj +STKGSgp/6Q+vXq2NiG3kB0qPEOTy28opq7Ow/g= X-Google-Smtp-Source: AA6agR6zy3B3k9Xz5S2DCU3S1lDXIydWQf8E9gRgATbQV1U9rdqfZloj/T4WpxSAw9sEyqLEUpW/KhtfPvoJBQEsTOQ= X-Received: by 2002:a17:907:1ca9:b0:741:4f9a:5984 with SMTP id nb41-20020a1709071ca900b007414f9a5984mr20166070ejc.86.1662125827103; Fri, 02 Sep 2022 06:37:07 -0700 (PDT) MIME-Version: 1.0 References: <20220827000201.22235-1-theojehl76@gmail.com> <20220827000201.22235-2-theojehl76@gmail.com> <20220902100245.m5mgcpvub4gmwj54@sirius.home.kraxel.org> In-Reply-To: <20220902100245.m5mgcpvub4gmwj54@sirius.home.kraxel.org> From: =?UTF-8?B?VGjDqW8gSmVobA==?= Date: Fri, 2 Sep 2022 15:36:56 +0200 Message-ID: Subject: Re: [edk2-platforms][PATCH v1 01/02] QemuOpenBoardPkg: Add QemuOpenBoardPkg To: Gerd Hoffmann Cc: devel@edk2.groups.io, Leif Lindholm , Michael D Kinney , Isaac Oram , Pedro Falcato , Stefan Hajnoczi Content-Type: multipart/alternative; boundary="0000000000007f1fc505e7b1d39a" --0000000000007f1fc505e7b1d39a Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable > > 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=C3=A9o Le ven. 2 sept. 2022 =C3=A0 12:02, Gerd Hoffmann a =C3= =A9crit : > On Sat, Aug 27, 2022 at 02:02:00AM +0200, Th=C3=A9o wrote: > > From: Th=C3=A9o Jehl > > > > 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 bo= ot > > 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 > > --0000000000007f1fc505e7b1d39a Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
That is a rather short description for a = patch of this size.=C2=A0 It
probably makes sense to break that down int= o smaller pieces and make a
patch series out of it because you can descr= ibe the specific pieces much
better then.

I was thinking about breaking down the main patch into smaller ones corre= sponding to=C2=A0
MinPlatform stages implementations to make it e= asier to follow along.=C2=A0

It seems = the goal is to have a MinPlatform board which can easily be
used to lear= n about and experiment with MinPlatform, is that correct?
= =C2=A0
The package goal is to be a simple implementation for deve= lopers=C2=A0wanting to get started with board ports indeed.

<= /div>
Why duplicate that lib instead of just using the Ovmf= Pkg 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 e= ssentials. And this simple implementation is used in my custom libs, the Ov= mf
version is used by OvmfPkg modules and libs. If indeed my lib = doesn't bring anything new or useful I can=C2=A0
remove it an= d switch to OvmfPkg QemuFwCfgLib instead.

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

For both GSOC purposes and MinPlatfo= rm compliance, I didn't want this package to be OVMF but repacked,
PlatformInit from OVMF is very complete, maybe too much when we are t= argeting a simple board port.
QemuOpenBoardPkg's=C2=A0Platfor= mInit only performs the necessary and nothing more, to make reading and und= erstanding
the package easier.
It's also up to deba= te, but concerning OVMF code, all the dependencies we have with OVMF makes = understanding
the boot flow a bit harder, I was thinking about sl= owly phasing out OVMF modules with lot of external dependencies=C2=A0
=
in favor of new ones, keeping the "only do the necessary" pa= ttern, just like what happened with PlatformInitPei.
=C2=A0
=
Regards,
Th=C3=A9o

Le=C2=A0ven. 2 sept. 2022 =C3=A0=C2= =A012:02, Gerd Hoffmann <kraxel@red= hat.com> a =C3=A9crit=C2=A0:
On Sat, Aug 2= 7, 2022 at 02:02:00AM +0200, Th=C3=A9o wrote:
> From: Th=C3=A9o 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 edk= 2 and
> MinPlatform Arch.
> Currently we implement up to stage 4 of the MinPlatform spec and can b= oot
> Windows/Linux.

That is a rather short description for a patch of this size.=C2=A0 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.=C2=A0 Can&#= 39;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?

>=C2=A0 Platform/Qemu/QemuOpenBoardPkg/Library/OpenQemuFwCfgLib/OpenQemu= FwCfgLib.inf=C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 23 +
>=C2=A0 Platform/Qemu/QemuOpenBoardPkg/Include/Library/OpenQemuFwCfgLib.= h=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 102 +++ >=C2=A0 Platform/Qemu/QemuOpenBoardPkg/Library/OpenQemuFwCfgLib/OpenQemu= FwCfgLib.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 130 ++++

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

>=C2=A0 Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/Cpu.c=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 56 ++
>=C2=A0 Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/Memory.c=C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 | 244 ++++++++
>=C2=A0 Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/Pci.c=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 59 ++
>=C2=A0 Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/Pcie.c=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 |=C2=A0 91 +++
>=C2=A0 Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/PlatformInit.c=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |= =C2=A0 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,
=C2=A0 Gerd

--0000000000007f1fc505e7b1d39a--