Re: [PATCH v7 6/6] x86/efi: Safely enable unaccepted memory in UEFI

[Ard Biesheuvel]

On Wed, 5 Apr 2023 at 15:42, Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:
>
> On Wed, Apr 05, 2023 at 09:46:59AM +0200, Ard Biesheuvel wrote:
> > On Tue, 4 Apr 2023 at 23:02, Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:
> > >
> > > On Tue, Apr 04, 2023 at 10:41:02PM +0200, Ard Biesheuvel wrote:
> > > > On Tue, 4 Apr 2023 at 22:24, Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Tue, Apr 04, 2023 at 09:49:52PM +0200, Ard Biesheuvel wrote:
> > > > > > On Tue, 4 Apr 2023 at 20:09, Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Tue, Apr 04, 2023 at 10:57:52AM -0700, Dave Hansen wrote:
> > > > > > > > On 4/4/23 10:45, Kirill A. Shutemov wrote:
> > > > > > > > > I still think it is a bad idea.
> > > > > > > > >
> > > > > > > > > As I asked before, please include my
> > > > > > > > >
> > > > > > > > > Nacked-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> > > > > > > > >
> > > > > > > > > into the patch.
> > > > > > > >
> > > > > > > > I was pretty opposed to this when I first saw it too.  But, Tom and
> > > > > > > > company have worn down my opposition a bit.
> > > > > > > >
> > > > > > > > The fact is that we have upstream kernels out there with SEV-SNP support
> > > > > > > > that don't know anything about unaccepted memory.  They're either
> > > > > > > > relegated to using the pre-accepted memory (4GB??) or _some_ entity
> > > > > > > > needs to accept the memory.  That entity obviously can't be the kernel
> > > > > > > > unless we backport unaccepted memory support.
> > > > > > > >
> > > > > > > > This both lets the BIOS be the page-accepting entity _and_ allows the
> > > > > > > > entity to delegate that to the kernel when it needs to.
> > > > > > > >
> > > > > > > > As much as I want to nak this and pretend that that those existing
> > > > > > > > kernel's don't exist, my powers of self-delusion do have their limits.
> > > > > > > >
> > > > > > > > If our AMD friends don't do this, what is their alternative?
> > > > > > >
> > > > > > > The alternative is coordination on the host side: VMM can load a BIOS that
> > > > > > > pre-accepts all memory if the kernel is older.
> > > > > > >
> > > > > >
> > > > > > And how does one identify such a kernel? How does the VMM know which
> > > > > > kernel the guest is going to load after it boots?
> > > > >
> > > > > VMM has to know what it is running. Yes, it is cumbersome. But enabling
> > > > > phase for a feature is often rough. It will get smoother overtime.
> > > > >
> > > >
> > > > So how does the VMM get informed about what it is running? How does it
> > > > distinguish between kernels that support unaccepted memory and ones
> > > > that don't? And how does it predict which kernel a guest is going to
> > > > load?
> > >
> > > User will specify if it wants unaccepted memory or not for the VM. And if
> > > it does it is his responsibility to have kernel that supports it.
> > >
> > > And you have not addressed my question:
> > >
> > >         How is it different from any other feature the kernel is not [yet] aware
> > >         of?
> > >
> >
> > It is the same problem, but this is just a better solution.
>
> Okay, we at least agree that there are more then one solution to the
> problem.
>
> > Having a BIOS menu option (or similar) to choose between unaccepted
> > memory or not (or to expose CXL memory via the EFI memory map, which is
> > another hack I have seen) is just unnecessary complication, if the
> > kernel can simply inform the loader about what it supports. We do this
> > all the time with things like OsIndications.
>
> It assumes that kernel calls ExitBootServices() which is not always true.
> A bootloader in between will make impossible for kernel to use any of
> futures exposed this way.
>
> But we talked about this before.
>

Yes, we have. But this is a theoretical concern, as nobody who is
deploying this stuff is interested in booting the kernel without the
stub: even the trenchboot folks are bending over backwards to
incorporate execution of the kernel's EFI stub into the D-RTM
bootflow, and all of the confidential compute attestation logic is
based on EFI protocols as well. So using a bootloader that calls
ExitBootServices() and subsequently boots the Linux kernel using the
legacy boot protocol is simply not something anyone is interested in
doing. But don't take my word for it.

> BTW, can we at least acknowledge the limitation in the commit message?
>

Sure.

> > We can phase out the protocol implementation from the firmware once we
> > no longer need it, at which point the LocateProtocol() call just
> > becomes a NOP (we do the same thing for UGA support, which has
> > disappeared a long time ago, but we still look for the protocol in the
> > EFI stub).
> >
> > Once the firmware stops exposing this protocol (and ceases to accept
> > memory on the OS's behalf), we can phase it out from the kernel as
> > well.
>
> It is unlikely to ever happen. In few year everybody will forget about
> this conversation. Regardless of what is written in commit message.
>
> Everything works, why bother?
>

That is a good question. If it doesn't get in the way and does not
prevent us from doing any of the things we want to do, why would we
even care?

But as I argued in my reply to Dave, we can actually drop it from
mainline later if we provide an upgrade path for legacy workloads that
want to upgrade their kernels.

> > The only other potential solution I see is exposing the unaccepted
> > memory as coldplugged ACPI memory objects, and implementing the accept
> > calls via PRM methods. But PRM has had very little test coverage, so
> > it is anybody's guess whether it works for the stable kernels that we
> > need to support with this. It would also mean that the new unaccepted
> > memory logic would need to be updated and cross reference these memory
> > regions with EFI unaccepted memory regions and avoid claiming them
> > both.
>
> Nah. That is a lot of complexity for no particular reason.
>

Good, at least we agree on that :-)