From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id D9643740034 for ; Tue, 16 Jan 2024 06:40:40 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=tt02P1/MGyqtw6A3FKlZ5vOh5PjlV1S6jbLJmGoTgNA=; c=relaxed/simple; d=groups.io; h=DKIM-Filter:Message-ID:Subject:From:Reply-To:To:Cc:Date:In-Reply-To:References:User-Agent:MIME-Version:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1705387239; v=1; b=em2/HF8Xn32Xz0NZqE72KeiqAK55p2NWx7+1Dwm+ulDvpDD6qzBCNYP2my5eTfuNjviS38ZX x+inX2Yk0bqGuBus33vCoj1hOkE3kY9D9krbmilqiPZ3DaRR+wiiavMhphWBdF99eSFr8v2xkc3 +LqZAfBTlzoJYVCCcVjYbGCI= X-Received: by 127.0.0.2 with SMTP id esjNYY7687511xKJO9ZWeUrh; Mon, 15 Jan 2024 22:40:39 -0800 X-Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.6840.1705387239010083565 for ; Mon, 15 Jan 2024 22:40:39 -0800 X-Received: from de-anbelski-t480.fritz.box (80-77-142-46.pool.kielnet.net [46.142.77.80]) by linux.microsoft.com (Postfix) with ESMTPSA id 4CBB320DEE9B; Mon, 15 Jan 2024 22:40:36 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 4CBB320DEE9B Message-ID: <0e25a41073f5eeec01e7be6017b75208f416a697.camel@linux.microsoft.com> Subject: Re: [edk2-devel] [PATCH v3 0/3] Support CloudHv guests with >1TB of RAM From: "Anatol Belski" Reply-To: devel@edk2.groups.io,anbelski@linux.microsoft.com To: devel@edk2.groups.io, tbarrett1200@gmail.com, Ard Biesheuvel Cc: Gerd Hoffmann , Jianyong Wu , Jiewen Yao , Laszlo Ersek , Rob Bradford Date: Tue, 16 Jan 2024 07:40:32 +0100 In-Reply-To: References: <30871515-6b4f-fd6c-5b56-d911d5f15463@redhat.com> User-Agent: Evolution 3.44.4-0ubuntu2 MIME-Version: 1.0 Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 7FboKHMJTVjHmoX4QDUMYif9x7686176AA= Content-Type: multipart/alternative; boundary="=-oZsteYhsmT2E4nphdP7J" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="em2/HF8X"; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=linux.microsoft.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --=-oZsteYhsmT2E4nphdP7J Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi, i was late to the party as well. At least i went now for a manual build and some tests with Cloud Hypervisor and it looks good. On Mon, 2024-01-15 at 10:26 -0800, Thomas Barrett wrote: > Hey all, >=20 > Yeah I don=E2=80=99t disagree Laszlo. While developing these patches, It = was > pretty confusing to determine which parts of PlatformLibInit were > compatible with CloudHv and which parts weren=E2=80=99t. >=20 > 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. >=20 Such a refactoring appears to be for sure meaningful. We should create an issue on the Cloud Hypervisor side so this topic isn't lost and so we can track it there, too. Thanks! Anatol > Best, > Thomas >=20 > On Mon, Jan 15, 2024 at 9:52 AM Ard Biesheuvel > wrote: > > On Mon, 15 Jan 2024 at 18:40, Laszlo Ersek > > 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=3D1468526. 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 > > > > Cc: Ard Biesheuvel > > > > Cc: Gerd Hoffmann > > > > Cc: Jianyong Wu > > > > Cc: Jiewen Yao > > > > Cc: Laszlo Ersek > > > > Cc: Rob Bradford > > > > > > > > Thomas Barrett (3): > > > >=C2=A0 =C2=A0OvmfPkg: Add CloudHv support to PlatformScanE820 utilit= y > > function. > > > >=C2=A0 =C2=A0OvmfPkg: Update PlatformAddressWidthInitialization for > > CloudHv > > > >=C2=A0 =C2=A0OvmfPkg: CloudHv: Enable PcdUse1GPageTable > > > > > > > >=C2=A0 OvmfPkg/CloudHv/CloudHvX64.dsc=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 |=C2=A0 =C2=A02 + > > > >=C2=A0 OvmfPkg/Library/PlatformInitLib/MemDetect.c | 107 > > ++++++++++++++------ > > > >=C2=A0 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 (=3D > > > 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). > > > > >=20 > > Thanks Laszlo. > >=20 > > I'm afraid I seem to have made your last point moot :-) > >=20 > > 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. > >=20 > > 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. >=20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113866): https://edk2.groups.io/g/devel/message/113866 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --=-oZsteYhsmT2E4nphdP7J Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable
Hi,

i was late to th= e party as well. At least i went now for a manual build and some tests with= Cloud Hypervisor and it looks good.

On Mon, 2024-= 01-15 at 10:26 -0800, Thomas Barrett wrote:
Hey all,

Yeah I don=E2=80=99t disagree Laszlo. While developing these patches, I= t was pretty confusing to determine which parts of PlatformLibInit were com= patible with CloudHv and which parts weren=E2=80=99t.

I have no idea how much work would be involve= d in splitting out cloud-hypervisor code. It would be good to hear input fr= om the cloud-hypervisor maintainers on this.

Such a refactoring appears to be for sure meaningful. = We should create an issue on the Cloud Hypervisor side so this topic isn't = lost and so we can track it there, too.

Thanks!

Anatol


Best,
Thomas

On Mon, Jan 15, 2024 at 9:52 AM Ard Biesheuvel &l= t;ardb@kernel.org> wrote:
On Mon, 15 Jan 2024 at 18:40, Laszlo Erse= k <lersek@redhat.= com> wrote:
>
> On 1/12/24 19:31, Thomas Barrett wrote:<= br>> > 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=3D1468526. The bug is
> > still occuring for CloudH= v guests because the PlatformScanE820
> > utility is not currently= supported for CloudHv.
> >
> > My working branch for the= se changes can be found here:
> > https://github.com/thomasbarrett/edk2/tree/cloud-hv-1tb-ram
> &= gt;
> > Cc: Anatol Belski <anbelski@linux.microsoft.com>
> = > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Gerd H= offmann <kraxel@r= edhat.com>
> > Cc: Jianyong Wu <jianyong.wu@arm.com>
> > C= c: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
&g= t; > Cc: Rob Bradford <rbradford@rivosinc.com>
> >
> > Thom= as Barrett (3):
> >   OvmfPkg: Add CloudHv support to Pl= atformScanE820 utility function.
> >   OvmfPkg: Update P= latformAddressWidthInitialization for CloudHv
> >   Ovmf= Pkg: CloudHv: Enable PcdUse1GPageTable
> >
> >  Ovmf= Pkg/CloudHv/CloudHvX64.dsc              = |   2 +
> >  OvmfPkg/Library/PlatformInitLib/MemDet= ect.c | 107 ++++++++++++++------
> >  2 files changed, 79 ins= ertions(+), 30 deletions(-)
> >
>
> Thanks for posting= v3, this one looks well-formed, so I can make some
> comments.
&g= t;
> TBH, I'm super uncomfortable with a new bunch of CLOUDHV_DEVICE_= ID
> branches introduced to PlatformInitLib.
>
> OVMF sup= ports multiple hypervisors, and the general idea has been that a
> si= ngle module (or even a single platform DSC) should preferably not
> a= ttempt to support multiple hypervisors. That's why we have a separate
&g= t; Xen platform, and a big number of Xen-customized, standalone modules.>
> The idea with this is that any particular developer is very u= nlikely to
> develop for, and test on, multiple hypervisors. Therefor= e unification (=3D
> elimination of all possible code duplication) be= tween 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 b= elieve bhyve is similarly separated out (just like Xen).
>
> It= 's one thing to collect support for multiple board types (machine
> t= ypes) 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 t= ree 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 mod= ules that should only be QEMU. If you
> need those modules customized= for CloudHv, it'd be best to detach them
> for CloudHv, and eliminat= e the dead code (such as anything that depends
> on QEMU fw_cfg), and= *enforce* that the underlying platform is CloudHv.
> Both communitie= s 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<= br>> is terrible to maintain in the long term.
>
> Of course= this all depends on having a separate platform DSC for
> CloudHv, bu= t that one already exists: "CloudHv/CloudHvX64.dsc".
>
> So I'd= suggest (a) isolating current CloudHv logic to new library
> instanc= es, drivers, etc, (b) adding this enhancement to CloudHv's own
> inst= ance of PlatformInitLib.
>
> Counter-arguments, objections etc = welcome -- feel free to prove me wrong
> (e.g. whatever I'm saying ab= out prior art / status quo).
>
> Also I'm not necessarily block= ing 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 :-)

B= ut I agree with your points above: EDK2 makes the fork&tweak approachvery easy, so CloudHv can keep its own versions of many of the QEMU
sp= ecific glue libraries. It does, however, require a certain degree of
hyg= iene on the part of the developer to introduce abstractions where
possib= le, to avoid forking huge drivers or libraries for a 2 line
delta betwee= n QEMU and CloudHv.

So let's decree that future CloudHv c= ontributions that follow this old
pattern will not be considered until/u= nless there is some confidence
on our part that there is a long term pla= n in motion that cleans this
all up and repays the accumulated technical= debt.

<= span>
_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#113866) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--=-oZsteYhsmT2E4nphdP7J--