Re: [edk2-devel] [PATCH v3 0/3] Support CloudHv guests with >1TB of RAM

["Thomas Barrett" <tbarrett1200@gmail.com>]

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

Hey all,

Yeah I don’t disagree Laszlo. While developing these patches, It was pretty
confusing to determine which parts of PlatformLibInit were compatible with
CloudHv and which parts weren’t.

I have no idea how much work would be involved in splitting out
cloud-hypervisor code. It would be good to hear input from the
cloud-hypervisor maintainers on this.

Best,
Thomas

On Mon, Jan 15, 2024 at 9:52 AM Ard Biesheuvel <ardb@kernel.org> wrote:

> On Mon, 15 Jan 2024 at 18:40, Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > On 1/12/24 19:31, Thomas Barrett wrote:
> > > This series adds support for cloud-hypervisor guests with >1TiB
> > > of RAM to Ovmf. This bug was solved for Qemu back in 2017
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1468526. The bug is
> > > still occuring for CloudHv guests because the PlatformScanE820
> > > utility is not currently supported for CloudHv.
> > >
> > > My working branch for these changes can be found here:
> > > https://github.com/thomasbarrett/edk2/tree/cloud-hv-1tb-ram
> > >
> > > Cc: Anatol Belski <anbelski@linux.microsoft.com>
> > > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > > Cc: Jianyong Wu <jianyong.wu@arm.com>
> > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Cc: Rob Bradford <rbradford@rivosinc.com>
> > >
> > > Thomas Barrett (3):
> > >   OvmfPkg: Add CloudHv support to PlatformScanE820 utility function.
> > >   OvmfPkg: Update PlatformAddressWidthInitialization for CloudHv
> > >   OvmfPkg: CloudHv: Enable PcdUse1GPageTable
> > >
> > >  OvmfPkg/CloudHv/CloudHvX64.dsc              |   2 +
> > >  OvmfPkg/Library/PlatformInitLib/MemDetect.c | 107 ++++++++++++++------
> > >  2 files changed, 79 insertions(+), 30 deletions(-)
> > >
> >
> > Thanks for posting v3, this one looks well-formed, so I can make some
> > comments.
> >
> > TBH, I'm super uncomfortable with a new bunch of CLOUDHV_DEVICE_ID
> > branches introduced to PlatformInitLib.
> >
> > OVMF supports multiple hypervisors, and the general idea has been that a
> > single module (or even a single platform DSC) should preferably not
> > attempt to support multiple hypervisors. That's why we have a separate
> > Xen platform, and a big number of Xen-customized, standalone modules.
> >
> > The idea with this is that any particular developer is very unlikely to
> > develop for, and test on, multiple hypervisors. Therefore unification (=
> > elimination of all possible code duplication) between distinct
> > hypervisor code snippets is actually detrimental for maintenance,
> > because it technically enables a developer to regress a platform that
> > they never actively work with.
> >
> > I believe bhyve is similarly separated out (just like Xen).
> >
> > It's one thing to collect support for multiple board types (machine
> > types) for QEMU into a given module; that's still the same hypervisor --
> > testing only requires a different "-M" option on the qemu command line.
> >
> > But fusing Cloud Hypervisor code with QEMU code makes me fidget in my
> seat.
> >
> > I've now grepped the OvmfPkg directory tree for existent instances of
> > CLOUDHV_DEVICE_ID, and I'm very much not liking the result list. It has
> > seeped into a whole bunch of modules that should only be QEMU. If you
> > need those modules customized for CloudHv, it'd be best to detach them
> > for CloudHv, and eliminate the dead code (such as anything that depends
> > on QEMU fw_cfg), and *enforce* that the underlying platform is CloudHv.
> > Both communities will benefit. Again, this is all based on the empirical
> > fact that QEMU and CloudHv developers don't tend to cross-develop.
> >
> > I understand that it's always just a small addition; in each specific
> > case, it's just one or two more "if"s in common code. But the end result
> > is terrible to maintain in the long term.
> >
> > Of course this all depends on having a separate platform DSC for
> > CloudHv, but that one already exists: "CloudHv/CloudHvX64.dsc".
> >
> > So I'd suggest (a) isolating current CloudHv logic to new library
> > instances, drivers, etc, (b) adding this enhancement to CloudHv's own
> > instance of PlatformInitLib.
> >
> > Counter-arguments, objections etc welcome -- feel free to prove me wrong
> > (e.g. whatever I'm saying about prior art / status quo).
> >
> > Also I'm not necessarily blocking this patch set; if others don't mind
> > this, they can ACK it (and I won't NACK).
> >
>
> Thanks Laszlo.
>
> I'm afraid I seem to have made your last point moot :-)
>
> But I agree with your points above: EDK2 makes the fork&tweak approach
> very easy, so CloudHv can keep its own versions of many of the QEMU
> specific glue libraries. It does, however, require a certain degree of
> hygiene on the part of the developer to introduce abstractions where
> possible, to avoid forking huge drivers or libraries for a 2 line
> delta between QEMU and CloudHv.
>
> So let's decree that future CloudHv contributions that follow this old
> pattern will not be considered until/unless there is some confidence
> on our part that there is a long term plan in motion that cleans this
> all up and repays the accumulated technical debt.
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113842): https://edk2.groups.io/g/devel/message/113842
Mute This Topic: https://groups.io/mt/103689730/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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