Re: [edk2-devel] [PATCH edk2-platforms 1/2] QemuOpenBoardPkg: Redo PCI bus initialization

["Isaac Oram" <isaac.w.oram@intel.com>]

We are doing some work to enable CI on edk2-platforms and would like to add QemuOpenBoardPkg to the CI list.  I do think that this will become more active over time.

Is there a V2 coming?  If so, I found a compiler issue with VS2019 and VS2015.
It doesn't like the local variable PlatformData having the same name as the global variable PlatformData.
I would suggest changing the global to mPlatformData.

Regards,
Isaac

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pedro Falcato
Sent: Monday, January 30, 2023 3:28 AM
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: devel@edk2.groups.io; Oram, Isaac W <isaac.w.oram@intel.com>; Theo Jehl <theojehl76@gmail.com>
Subject: Re: [edk2-devel] [PATCH edk2-platforms 1/2] QemuOpenBoardPkg: Redo PCI bus initialization

On Fri, Jan 13, 2023 at 6:34 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > diff --git a/Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/Memory.c 
> > b/Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/Memory.c
> > index 21705256191b..4f312c36016e 100644
> > --- a/Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/Memory.c
> > +++ b/Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/Memory.c
>
> There is OvmfPkg/Library/PlatformInitLib which you could use instead 
> of reinventing the wheel ...
>
> If there are changes needed to make PlatformInitLib work for you feel 
> free to propose patches (same goes for eventually moving code from 
> OvmfPkg/PlatformPei to the Library).

Gerd,

Thank you for the comments.

That is an interesting idea, however I believe OvmfPkg/Library/PlatformInitLib is ATM a bit too tightly coupled with the rest of OVMF itself.
For instance, this work was partially inspired by your recent-ish work in similar OvmfPkg code and also drove me to remove the dependency on PciHostBridgeLib, et al (replacing it with the "standard"
MinPlatformPkg PCI libs). My solution is based on dynamic PCD war-crimes but it Just Works(tm).
Changing it in OvmfPkg would be too invasive, I feel; it's not like my solution is good or anything.

And because of the way the repos are split, there is no way to currently dedup OVMF's PciHostBridgeLib by also making OVMF depend on MinPlatformPkg's, because why would there be, right?

Anyway, doing deep changes to OVMF for QemuOpenBoardPkg is uncomfortable for me because its place in Tiano is not defined. OVMF and Intel folks have said very few things about QOBP.
At the moment, it's the typical "GSoC project stagnating in edk2-platforms/-staging" that Tianocore seems to love so much. I don't see much point in it being this way.
QemuOBP has been a nice fun exercise in reducing code duplication, which shows a lot of promise, but it also doesn't support some of the things OvmfPkg does, like PEI-less booting (and TDX/SEV but those could certainly be worked in).

If this kicks off discussion on the future of QemuOpenBoardPkg, I'll be happy.

Until that happens, I am reluctant to change OVMF internals for an obscure QEMU PlatformPkg hidden in the best worst repo, edk2-platforms
:)

> > +  // It's worth noting that QEMU also grew an option to change 
> > + lowmem based on the  // user's preferences. Because of all of 
> > + this, it's near impossible to hardcode  // a range, so we grab 
> > + TOLUD (not in a literal way, since QEMU does not implement  // 
> > + that register ;)) and calculate our PCI MMIO based on that. This also makes it so  // the DSDT built by QEMU will have correct _CRS ranges.
> > +  // hw/pci-host/q35.c explicitly says our PCI hole ranges from [TOLUD, IO APIC].
> > +  // As far as I can tell, we seem to be allowed to add a 64-bit resource range anywhere.
>
> There is a etc/reserved-memory-end fw_cfg FwCfg file.  If present it 
> the 64-bit resource range should be placed above the address specified 
> there (qemu uses that to reserve address space if needed, happens for 
> example when you enable memory hotplug).

ACK, will change on v2 (together with Isaac's comments).

> The patch should be splitted up into smaller pieces to make it easier 
> to review the changes.

Agreed, will try to devise a strategy to break it up in smaller, logical chunks.
Again, thank you so much for your time looking at something you're not even a maintainer for :))

--
Pedro