linux-coco.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: Michael Roth <michael.roth@amd.com>
To: Borislav Petkov <bp@alien8.de>
Cc: Brijesh Singh <brijesh.singh@amd.com>,
	x86@kernel.org, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org, linux-efi@vger.kernel.org,
	platform-driver-x86@vger.kernel.org, linux-coco@lists.linux.dev,
	linux-mm@kvack.org, linux-crypto@vger.kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Joerg Roedel <jroedel@suse.de>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Ard Biesheuvel <ardb@kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <seanjc@google.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>,
	Andy Lutomirski <luto@kernel.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Sergio Lopez <slp@redhat.com>, Peter Gonda <pgonda@google.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
	David Rientjes <rientjes@google.com>,
	Dov Murik <dovmurik@linux.ibm.com>,
	Tobin Feldman-Fitzthum <tobin@ibm.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	tony.luck@intel.com, brijesh.ksingh@gmail.com
Subject: Re: [PATCH Part1 RFC v4 24/36] x86/compressed/acpi: move EFI config table access to common code
Date: Thu, 19 Aug 2021 09:58:31 -0500	[thread overview]
Message-ID: <20210819145831.42uszc4lcsffebzu@amd.com> (raw)
In-Reply-To: <YR42323cUxsbQo5h@zn.tnic>

On Thu, Aug 19, 2021 at 12:47:59PM +0200, Borislav Petkov wrote:
> On Wed, Jul 07, 2021 at 01:14:54PM -0500, Brijesh Singh wrote:
> > From: Michael Roth <michael.roth@amd.com>
> > 
> > Future patches for SEV-SNP-validated CPUID will also require early
> > parsing of the EFI configuration. Move the related code into a set of
> > helpers that can be re-used for that purpose.
> > 
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > ---
> >  arch/x86/boot/compressed/Makefile           |   1 +
> >  arch/x86/boot/compressed/acpi.c             | 124 +++++---------
> >  arch/x86/boot/compressed/efi-config-table.c | 180 ++++++++++++++++++++
> >  arch/x86/boot/compressed/misc.h             |  50 ++++++
> >  4 files changed, 272 insertions(+), 83 deletions(-)
> >  create mode 100644 arch/x86/boot/compressed/efi-config-table.c
> 

Hi Boris,

Thanks for reviewing. Just FYI, Brijesh is prepping v5 to post soon, and I
will work to get all your comments addressed as part of that, but there has
also been a change to the CPUID handling in the #VC handlers in case you
wanted to wait for that to land.

> arch/x86/boot/compressed/efi.c
> 
> should be good enough.
> 
> And in general, this patch is hard to review because it does a bunch of
> things at the same time. You should split it:
> 
> - the first patch sould carve out only the functionality into helpers
> without adding or changing the existing functionality.
> 
> - later ones should add the new functionality, in single logical steps.

Not sure what you mean here. All the interfaces introduced here are used
by acpi.c. There is another helper added later (efi_bp_find_vendor_table())
in "enable SEV-SNP-validated CPUID in #VC handler", since it's not used
here by acpi.c.

> 
> Some preliminary comments below as far as I can:
> 
> > diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> > index 431bf7f846c3..b41aecfda49c 100644
> > --- a/arch/x86/boot/compressed/Makefile
> > +++ b/arch/x86/boot/compressed/Makefile
> > @@ -100,6 +100,7 @@ endif
> >  vmlinux-objs-$(CONFIG_ACPI) += $(obj)/acpi.o
> >  
> >  vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_thunk_$(BITS).o
> > +vmlinux-objs-$(CONFIG_EFI) += $(obj)/efi-config-table.o
> >  efi-obj-$(CONFIG_EFI_STUB) = $(objtree)/drivers/firmware/efi/libstub/lib.a
> >  
> >  $(obj)/vmlinux: $(vmlinux-objs-y) $(efi-obj-y) FORCE
> > diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> > index 8bcbcee54aa1..e087dcaf43b3 100644
> > --- a/arch/x86/boot/compressed/acpi.c
> > +++ b/arch/x86/boot/compressed/acpi.c
> > @@ -24,42 +24,36 @@ struct mem_vector immovable_mem[MAX_NUMNODES*2];
> >   * Search EFI system tables for RSDP.  If both ACPI_20_TABLE_GUID and
> >   * ACPI_TABLE_GUID are found, take the former, which has more features.
> >   */
> > +#ifdef CONFIG_EFI
> > +static bool
> > +rsdp_find_fn(efi_guid_t guid, unsigned long vendor_table, bool efi_64,
> > +	     void *opaque)
> > +{
> > +	acpi_physical_address *rsdp_addr = opaque;
> > +
> > +	if (!(efi_guidcmp(guid, ACPI_TABLE_GUID))) {
> > +		*rsdp_addr = vendor_table;
> > +	} else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID))) {
> > +		*rsdp_addr = vendor_table;
> > +		return false;
> 
> No "return false" in the ACPI_TABLE_GUID branch above? Maybe this has to
> do with the preference to ACPI_20_TABLE_GUID.

Right, current acpi.c keeps searching in case the preferred
ACPI_20_TABLE_GUID is found.

> 
> In any case, this looks silly. Please do the iteration simple
> and stupid without the function pointer and get rid of that
> efi_foreach_conf_entry() thing - this is not firmware.

There is the aforementioned efi_bp_find_vendor_table() that does the
simple iteration, but I wasn't sure how to build the "find one of these,
but this one is preferred" logic into it in a reasonable way.

I could just call it once for each of these GUIDs though. I was hesitant
to do so since it's less efficient than existing code, but if it's worth
it for the simplification then I'm all for it.

So I'll pull efi_bp_find_vendor_table() into this patch, rename to
efi_find_vendor_table(), and drop efi_foreach_conf_entry() in favor
of it.

> 
> > diff --git a/arch/x86/boot/compressed/efi-config-table.c b/arch/x86/boot/compressed/efi-config-table.c
> > new file mode 100644
> > index 000000000000..d1a34aa7cefd
> > --- /dev/null
> > +++ b/arch/x86/boot/compressed/efi-config-table.c
> 
> ...
> 
> > +/*
> 
> If you're going to add proper comments, make them kernel-doc. I.e., it
> should start with
> 
> /**
> 
> and then use
> 
> ./scripts/kernel-doc -none arch/x86/boot/compressed/efi-config-table.c
> 
> to check them all they're proper.

Nice, thanks for the tip.

> 
> 
> > + * Given boot_params, retrieve the physical address of EFI system table.
> > + *
> > + * @boot_params:        pointer to boot_params
> > + * @sys_table_pa:       location to store physical address of system table
> > + * @is_efi_64:          location to store whether using 64-bit EFI or not
> > + *
> > + * Returns 0 on success. On error, return params are left unchanged.
> > + */
> > +int
> > +efi_bp_get_system_table(struct boot_params *boot_params,
> 
> There's no need for the "_bp_" - just efi_get_system_table(). Ditto for
> the other naming.

There used to also be an efi_get_conf_table() that did the lookup given
a direct pointer to systab rather than going through bootparams, so I
needed some way to differentiate, but looks like I dropped that at some
point, so I'll rename these as suggested.

> 
> I'll review the rest properly after you've split it.
> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&amp;data=04%7C01%7Cmichael.roth%40amd.com%7C5a35d1d920024a99451608d962febc38%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637649668538296788%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ED6tez8A1ktSULe%2FhJTxeqPlp4LVb0Yt4i44P9gytAw%3D&amp;reserved=0

  reply	other threads:[~2021-08-19 15:15 UTC|newest]

Thread overview: 86+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-07 18:14 [PATCH Part1 RFC v4 00/36] Add AMD Secure Nested Paging (SEV-SNP) Guest Support Brijesh Singh
2021-07-07 18:14 ` [PATCH Part1 RFC v4 01/36] x86/sev: shorten GHCB terminate macro names Brijesh Singh
2021-07-07 18:14 ` [PATCH Part1 RFC v4 02/36] x86/sev: Save the negotiated GHCB version Brijesh Singh
2021-08-10  9:17   ` Borislav Petkov
2021-08-10 13:15     ` Brijesh Singh
2021-08-10 10:01   ` Borislav Petkov
2021-08-10 13:17     ` Brijesh Singh
2021-07-07 18:14 ` [PATCH Part1 RFC v4 03/36] x86/sev: Add support for hypervisor feature VMGEXIT Brijesh Singh
2021-08-10 11:22   ` Borislav Petkov
2021-08-10 13:39     ` Brijesh Singh
2021-08-10 14:03       ` Borislav Petkov
2021-07-07 18:14 ` [PATCH Part1 RFC v4 04/36] x86/mm: Add sev_feature_enabled() helper Brijesh Singh
2021-07-08  8:50   ` Dr. David Alan Gilbert
2021-07-08  8:53     ` Paolo Bonzini
2021-08-10 11:25   ` Borislav Petkov
2021-08-10 14:57     ` Brijesh Singh
2021-07-07 18:14 ` [PATCH Part1 RFC v4 05/36] x86/sev: Define the Linux specific guest termination reasons Brijesh Singh
2021-08-10 11:33   ` Borislav Petkov
2021-08-10 14:59     ` Brijesh Singh
2021-08-10 19:30       ` Tom Lendacky
2021-08-10 21:52         ` Borislav Petkov
2021-07-07 18:14 ` [PATCH Part1 RFC v4 06/36] x86/sev: check SEV-SNP features support Brijesh Singh
2021-07-07 18:14 ` [PATCH Part1 RFC v4 07/36] x86/sev: Add a helper for the PVALIDATE instruction Brijesh Singh
2021-07-07 18:14 ` [PATCH Part1 RFC v4 08/36] x86/sev: check the vmpl level Brijesh Singh
2021-08-13  7:25   ` Borislav Petkov
2021-08-13 13:13     ` Brijesh Singh
2021-08-13 15:16       ` Borislav Petkov
2021-07-07 18:14 ` [PATCH Part1 RFC v4 09/36] x86/compressed: Add helper for validating pages in the decompression stage Brijesh Singh
2021-08-13 10:22   ` Borislav Petkov
2021-08-13 14:21     ` Brijesh Singh
2021-08-13 15:19       ` Borislav Petkov
2021-07-07 18:14 ` [PATCH Part1 RFC v4 10/36] x86/compressed: Register GHCB memory when SEV-SNP is active Brijesh Singh
2021-08-13 10:47   ` Borislav Petkov
2021-07-07 18:14 ` [PATCH Part1 RFC v4 11/36] x86/sev: " Brijesh Singh
2021-07-07 18:14 ` [PATCH Part1 RFC v4 12/36] x86/sev: Add helper for validating pages in early enc attribute changes Brijesh Singh
2021-08-13 11:13   ` Borislav Petkov
2021-07-07 18:14 ` [PATCH Part1 RFC v4 13/36] x86/kernel: Make the bss.decrypted section shared in RMP table Brijesh Singh
2021-08-13 17:09   ` Borislav Petkov
2021-07-07 18:14 ` [PATCH Part1 RFC v4 14/36] x86/kernel: Validate rom memory before accessing when SEV-SNP is active Brijesh Singh
2021-07-07 18:14 ` [PATCH Part1 RFC v4 15/36] x86/mm: Add support to validate memory when changing C-bit Brijesh Singh
2021-08-17 17:27   ` Borislav Petkov
2021-08-17 18:07     ` Brijesh Singh
2021-08-17 18:17       ` Borislav Petkov
2021-08-17 18:18         ` Brijesh Singh
2021-08-17 20:34     ` Brijesh Singh
2021-08-17 20:44       ` Borislav Petkov
2021-07-07 18:14 ` [PATCH Part1 RFC v4 16/36] KVM: SVM: define new SEV_FEATURES field in the VMCB Save State Area Brijesh Singh
2021-08-17 17:54   ` Borislav Petkov
2021-08-17 17:59     ` Borislav Petkov
2021-08-17 18:11     ` Brijesh Singh
2021-07-07 18:14 ` [PATCH Part1 RFC v4 17/36] KVM: SVM: Create a separate mapping for the SEV-ES save area Brijesh Singh
2021-07-07 18:14 ` [PATCH Part1 RFC v4 18/36] KVM: SVM: Create a separate mapping for the GHCB " Brijesh Singh
2021-07-07 18:14 ` [PATCH Part1 RFC v4 19/36] KVM: SVM: Update the SEV-ES save area mapping Brijesh Singh
2021-07-07 18:14 ` [PATCH Part1 RFC v4 20/36] x86/sev: Use SEV-SNP AP creation to start secondary CPUs Brijesh Singh
2021-08-17 20:04   ` Borislav Petkov
2021-08-17 22:13     ` Tom Lendacky
2021-08-18  8:38       ` Borislav Petkov
2021-07-07 18:14 ` [PATCH Part1 RFC v4 21/36] x86/head/64: set up a startup %gs for stack protector Brijesh Singh
2021-08-19  9:34   ` Borislav Petkov
2021-07-07 18:14 ` [PATCH Part1 RFC v4 22/36] x86/sev: move MSR-based VMGEXITs for CPUID to helper Brijesh Singh
2021-08-19  9:45   ` Borislav Petkov
2021-08-19 15:37     ` Michael Roth
2021-08-19 16:46       ` Borislav Petkov
2021-08-20  3:29         ` Michael Roth
2021-08-23  4:50           ` Borislav Petkov
2021-07-07 18:14 ` [PATCH Part1 RFC v4 23/36] KVM: x86: move lookup of indexed CPUID leafs " Brijesh Singh
2021-08-19 10:07   ` Borislav Petkov
2021-07-07 18:14 ` [PATCH Part1 RFC v4 24/36] x86/compressed/acpi: move EFI config table access to common code Brijesh Singh
2021-08-19 10:47   ` Borislav Petkov
2021-08-19 14:58     ` Michael Roth [this message]
2021-08-19 17:09       ` Borislav Petkov
2021-08-19 23:42         ` Michael Roth
2021-08-23  4:52           ` Borislav Petkov
2021-07-07 18:14 ` [PATCH Part1 RFC v4 25/36] x86/boot: Add Confidential Computing type to setup_data Brijesh Singh
2021-08-19 11:06   ` Borislav Petkov
2021-07-07 18:14 ` [PATCH Part1 RFC v4 26/36] x86/compressed/64: enable SEV-SNP-validated CPUID in #VC handler Brijesh Singh
2021-07-07 18:14 ` [PATCH Part1 RFC v4 27/36] x86/boot: add a pointer to Confidential Computing blob in bootparams Brijesh Singh
2021-07-07 18:14 ` [PATCH Part1 RFC v4 28/36] x86/compressed/64: store Confidential Computing blob address " Brijesh Singh
2021-07-07 18:14 ` [PATCH Part1 RFC v4 29/36] x86/compressed/64: add identity mapping for Confidential Computing blob Brijesh Singh
2021-07-07 18:15 ` [PATCH Part1 RFC v4 30/36] x86/sev: enable SEV-SNP-validated CPUID in #VC handlers Brijesh Singh
2021-07-07 18:15 ` [PATCH Part1 RFC v4 31/36] x86/sev: Provide support for SNP guest request NAEs Brijesh Singh
2021-07-07 18:15 ` [PATCH Part1 RFC v4 32/36] x86/sev: Add snp_msg_seqno() helper Brijesh Singh
2021-07-07 18:15 ` [PATCH Part1 RFC v4 33/36] x86/sev: Register SNP guest request platform device Brijesh Singh
2021-07-07 18:15 ` [PATCH Part1 RFC v4 34/36] virt: Add SEV-SNP guest driver Brijesh Singh
2021-07-07 18:15 ` [PATCH Part1 RFC v4 35/36] virt: sevguest: Add support to derive key Brijesh Singh
2021-07-07 18:15 ` [PATCH Part1 RFC v4 36/36] virt: sevguest: Add support to get extended report Brijesh Singh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210819145831.42uszc4lcsffebzu@amd.com \
    --to=michael.roth@amd.com \
    --cc=ardb@kernel.org \
    --cc=bp@alien8.de \
    --cc=brijesh.ksingh@gmail.com \
    --cc=brijesh.singh@amd.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=dovmurik@linux.ibm.com \
    --cc=hpa@zytor.com \
    --cc=jmattson@google.com \
    --cc=jroedel@suse.de \
    --cc=kvm@vger.kernel.org \
    --cc=linux-coco@lists.linux.dev \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=luto@kernel.org \
    --cc=mingo@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=peterz@infradead.org \
    --cc=pgonda@google.com \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=rientjes@google.com \
    --cc=seanjc@google.com \
    --cc=slp@redhat.com \
    --cc=srinivas.pandruvada@linux.intel.com \
    --cc=tglx@linutronix.de \
    --cc=thomas.lendacky@amd.com \
    --cc=tobin@ibm.com \
    --cc=tony.luck@intel.com \
    --cc=vbabka@suse.cz \
    --cc=vkuznets@redhat.com \
    --cc=wanpengli@tencent.com \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH Part1 RFC v4 24/36] x86/compressed/acpi: move EFI config table access to common code' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).