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 56CD1D806D1 for ; Tue, 16 Jan 2024 05:33:40 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=FTce1bDhYLrN5BNVYKimMk0ysIxdIXPN+L8XbqNF1eE=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1705383219; v=1; b=Xlv90pT74JgJmr1HVK2ihOlHvyCHBa8HZUCejHW9QSK7t+om2FOyIOWxuFxs4lCObc/wpxaa Z17fNZUdXKnZMaM/shEMX4ICXsSyaoLGLfPYzGCTP9W56HmsjaRl3aoL3RCDeq6NNS97mS737jG Qask6r/9uw+LGTGXpVB5QkHs= X-Received: by 127.0.0.2 with SMTP id bG5RYY7687511xf2TJ7xuj9L; Mon, 15 Jan 2024 21:33:39 -0800 X-Received: from mail-pl1-f172.google.com (mail-pl1-f172.google.com [209.85.214.172]) by mx.groups.io with SMTP id smtpd.web10.90875.1705343218674978402 for ; Mon, 15 Jan 2024 10:26:58 -0800 X-Received: by mail-pl1-f172.google.com with SMTP id d9443c01a7336-1d5cdb4a444so9447685ad.1 for ; Mon, 15 Jan 2024 10:26:58 -0800 (PST) X-Gm-Message-State: 9sYxneAM1zJRfAUIu9VH09GJx7686176AA= X-Google-Smtp-Source: AGHT+IG8P7CEE5eB4ooHif7pPsO2knxdJIPBSMpF/a/lJ330Mtgt5IxYZz+qcc0GQcfTZHkH7t5O851vJQZKDR9i4ow= X-Received: by 2002:a17:90a:c08:b0:28c:8eaa:e5e3 with SMTP id 8-20020a17090a0c0800b0028c8eaae5e3mr3943254pjs.17.1705343217915; Mon, 15 Jan 2024 10:26:57 -0800 (PST) MIME-Version: 1.0 References: <30871515-6b4f-fd6c-5b56-d911d5f15463@redhat.com> In-Reply-To: From: "Thomas Barrett" Date: Mon, 15 Jan 2024 10:26:47 -0800 Message-ID: Subject: Re: [edk2-devel] [PATCH v3 0/3] Support CloudHv guests with >1TB of RAM To: Ard Biesheuvel Cc: Anatol Belski , Gerd Hoffmann , Jianyong Wu , Jiewen Yao , Laszlo Ersek , Rob Bradford , devel@edk2.groups.io 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 Reply-To: devel@edk2.groups.io,tbarrett1200@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="000000000000b95aa7060f002880" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=Xlv90pT7; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.com (policy=none) --000000000000b95aa7060f002880 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hey all, Yeah I don=E2=80=99t disagree Laszlo. While developing these patches, It wa= s pretty confusing to determine which parts of PlatformLibInit were compatible with CloudHv and which parts weren=E2=80=99t. 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 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): > > > 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 (= =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 empirica= l > > 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 resul= t > > 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 wron= g > > (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. > -=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 (#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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --000000000000b95aa7060f002880 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hey all,

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

I have no idea how much work would be involved= in splitting out cloud-hypervisor code. It would be good to hear input fro= m 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 >1Ti= B
> > 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/thomasbarret= t/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):
> >=C2=A0 =C2=A0OvmfPkg: Add CloudHv support to PlatformScanE820 util= ity function.
> >=C2=A0 =C2=A0OvmfPkg: Update PlatformAddressWidthInitialization fo= r 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<= br> > 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 sepa= rate
> Xen platform, and a big number of Xen-customized, standalone modules.<= br> >
> The idea with this is that any particular developer is very unlikely t= o
> 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<= br> > 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 (machin= e
> types) for QEMU into a given module; that's still the same hypervi= sor --
> testing only requires a different "-M" option on the qemu co= mmand 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. I= t has
> seeped into a whole bunch of modules that should only be QEMU. If you<= br> > need those modules customized for CloudHv, it'd be best to detach = them
> for CloudHv, and eliminate the dead code (such as anything that depend= s
> on QEMU fw_cfg), and *enforce* that the underlying platform is CloudHv= .
> Both communities will benefit. Again, this is all based on the empiric= al
> fact that QEMU and CloudHv developers don't tend to cross-develop.=
>
> I understand that it's always just a small addition; in each speci= fic
> case, it's just one or two more "if"s in common code. Bu= t 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&quo= t;.
>
> So I'd suggest (a) isolating current CloudHv logic to new library<= br> > instances, drivers, etc, (b) adding this enhancement to CloudHv's = own
> instance of PlatformInitLib.
>
> Counter-arguments, objections etc welcome -- feel free to prove me wro= ng
> (e.g. whatever I'm saying about prior art / status quo).
>
> Also I'm not necessarily blocking this patch set; if others don= 9;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<= br> 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<= br> 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:

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

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

_._,_._,_
--000000000000b95aa7060f002880--