Message-ID: <Zo1QkP0GQOkyrvTx@swahl-home.5wahls.com>
Date: Tue, 9 Jul 2024 10:00:32 -0500
From: Steve Wahl <steve.wahl@....com>
To: Ard Biesheuvel <ardb@...nel.org>
Cc: Steve Wahl <steve.wahl@....com>, 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 Tue, Jul 09, 2024 at 08:49:43AM +0200, Ard Biesheuvel wrote:
> 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:
> > > 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.

I wrote in the non-permanent-comment section of that patch:

------------------------------
> I (Steve Wahl) modified the above commit message, but did not modify
> the code.  I am not clear if that requires additional Co-developed-by:
> and Signed-off-by: lines.  If so, copy them from here:
> 
> Co-developed-by: Steve Wahl <steve.wahl@....com>
> Signed-off-by: Steve Wahl <steve.wahl@....com>
------------------------------

I take it you believe I should add them to the for-posterity portion
of the patch.  I will do so.

> > 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.

I understand.  It's hard to kick yourself out of proactive mode when
you're battling a problem that you can't reproduce in your own
hands. :-)

This is one reason why I kept it as a separate patch in the first
place, though.

If you will, keep in mind for the future that mapping all of DRAM 1:1,
without regards to what areas the BIOS has marked "reserved" in the
E820 tables and such, allows processor speculation into said reserved
areas, which causes system halts on our (SGI UV) platform.  Patch #3
is all about this.

Mapping all of DRAM *except* areas marked reserved would work on our
platform.

> > 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.

Will do.

--> Steve
-- 
Steve Wahl, Hewlett Packard Enterprise

Powered by blists - more mailing lists