Message-ID: <CAMj1kXE5OYTxxBEO38dRyYt_J1FNpU-tdkaU8rxvrMLd_k_beg@mail.gmail.com>
Date: Tue, 9 Jul 2024 08:49:43 +0200
From: Ard Biesheuvel <ardb@...nel.org>
To: Steve Wahl <steve.wahl@....com>
Cc: Borislav Petkov <bp@...en8.de>, Ashish Kalra <ashish.kalra@....com>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, Andy Lutomirski <luto@...nel.org>, 
	Peter Zijlstra <peterz@...radead.org>, Thomas Gleixner <tglx@...utronix.de>, 
	Ingo Molnar <mingo@...hat.com>, x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>, 
	linux-kernel@...r.kernel.org, Pavin Joseph <me@...injoseph.com>, 
	Eric Hagberg <ehagberg@...il.com>, Simon Horman <horms@...ge.net.au>, 
	Eric Biederman <ebiederm@...ssion.com>, Dave Young <dyoung@...hat.com>, Sarah Brofeldt <srhb@....dk>, 
	Russ Anderson <rja@....com>, Dimitri Sivanich <sivanich@....com>, 
	Hou Wenlong <houwenlong.hwl@...group.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	Baoquan He <bhe@...hat.com>, Yuntao Wang <ytcoode@...il.com>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Joerg Roedel <jroedel@...e.de>, Michael Roth <michael.roth@....com>
Subject: Re: [PATCH 0/3] Resolve problems with kexec identity mapping

On Mon, 8 Jul 2024 at 23:20, Steve Wahl <steve.wahl@....com> wrote:
>
> On Mon, Jul 08, 2024 at 11:05:29PM +0200, Ard Biesheuvel wrote:
> > On Mon, 8 Jul 2024 at 22:41, Steve Wahl <steve.wahl@....com> wrote:
> > >
> > > On Mon, Jul 08, 2024 at 09:58:10PM +0200, Borislav Petkov wrote:
> > > > On Mon, Jul 08, 2024 at 02:29:05PM -0500, Steve Wahl wrote:
> > > > > Yes, this is about AMD machines which support SEV, running bare metal.
> > > > > ("Server" is in question, one of my testers is known to be using a
> > > > > laptop, so the facilities must be present in non-servers as well.)
> > > >
> > > > No, they can't be. SEV is supported only on server, not on client. This laptop
> > > > has a different problem it seems.
> > >
> > > Ahhh.  On the laptop, it's not looking *at* the CC blob that's the
> > > problem.
> > >
> > > Its looking *for* the CC blob in the EFI config table; the CC blob
> > > probably does not exist in that table on the laptop.  But the EFI
> > > config table needs to be identity mapped, to look through it and see
> > > that the CC blob is not there, and the EFI config table is not mapped.
> > >
> > > I think the existence of the CC blob in the EFI config table is being
> > > used, more or less, as a flag as to whether we need to do SEV related
> > > code.  Without mapping the EFI config table, we can't look for that
> > > blob.
> > >
> >
> > We have run into this exact problem before - I don't have time to
> > check lore right now (it's 11pm here) but 'CC blob' and 'EFI config
> > table' are the keywords that may help you track down the thread.
> >
> > So first of all, let's define some terminology:
> > - the EFI system table is the EFI root table that contains some magic
> > numbers and pointers to various other assets in memory, one of which
> > is:
> > - the EFI config table array, which is just a list of (GUID, pointer)
> > tuples, the length of which is recorded in the EFI system table
> > - an EFI config table is some asset elsewhere in memory that is
> > identified by its GUID.
> >
> > The EFI config table array can grow and shrink at boot time, which is
> > why it is a separate allocation, as this allows it to be realloc()'ed.
> > This means any bootloader that intends to map the primary EFI table
> > should also map the EFI config table array, which may be elsewhere
> > entirely.
> >
> > > > > As far as I can see it, the effort you're putting into finding a
> > > > > different solution must mean you find something less than desirable
> > > > > about the solution I have offered.  But at this point, I don't
> > > > > understand why;
> > > >
> > > > Why would we parse the CC blob which is destined *solely* for a SEV- *guest*,
> > > > when booting the baremetal kernel which is *not* a guest?
> > > >
> > > > This is the solution I'm chasing - don't do something you're not supposed to
> > > > or needed to do.
> > >
> > > What you're saying suggests that, maybe, my patch #2 will not be
> > > necessary.  The CC blob will never be present except for in a guest.
> > > But can you do a kexec to a new kernel within that guest?  If so,
> > > patch #2 might still be necessary.
> > >
> > > Anyway, I think the references you're trying to eliminate when they're
> > > not needed are the references used to determine if the SEV feature is
> > > to be used in this specific boot iteration or not.
> > >
> >
> > It would be better if we did not have to rely on page fault handling
> > to map the EFI config table array this early. This is not strictly
> > related to SEV, but the CC blob happens to be the EFI config table
> > that is accessed before the page fault handler is installed.
> >
> > So regardless of how we fix any SEV-guest specific issues, we should
> > ensure that kexec infrastructure creates the mappings of the EFI
> > system table and the EFI config table array upfront.
>
> I think that's exactly what this patch series does.
>
> The mapping of the EFI system table is/was already present in
> map_efi_systab.  Patch #1 in this series adds the efi_config_table to
> what gets mapped.
>

Excellent. Please update the commit log to make it very clear that it
is the EFI config table *array* (the GUID/pointer tuple list) that is
being mapped, without any regard for the meaning of the individual
entries.

Also, the patch seems to be lacking your signed-off-by.

> Patch #2 adds the CC blob to the identity map as well, if present,
> since if present it is also dereferenced before the page fault handler
> can be put into place.  Given what's been discussed, this patch might
> not be necessary; I don't know enough to say whether kexec-ing a new
> kernel within a SEV guest makes sense.  I'm pretty certain it can
> cause no harm, though.
>

I'd prefer it if that is addressed within the context of the SEV guest
work. The memory setup is quite intricate, and dealing with individual
types of EFI config tables is something we should avoid in general. I
still maintain that the best approach would be to map all of DRAM 1:1
instead of mapping patches left and right (as this is what EFI does),
but if we need to do so, let's keep it as generic as we possibly can.

> And patch #3 fixes the UV platform problem as discussed, which will
> not cause the previous reported regression if patches #1 and #2 are
> already in place.
>

I wasn't cc'ed on any of the patches so I don't know exactly what was
discussed.

Please cc me and linux-efi@ on your next revision.

Powered by blists - more mailing lists