Re: [PATCH Part1 RFC v4 24/36] x86/compressed/acpi: move EFI config table access to common code

[Michael Roth <michael.roth@amd.com>]

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