| Message ID | 20211210154332.11526-25-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
On 12/10/21 7:43 AM, Brijesh Singh wrote: > +/* > + * Helpers for early access to EFI configuration table > + * > + * Copyright (C) 2021 Advanced Micro Devices, Inc. > + * > + * Author: Michael Roth <michael.roth@amd.com> > + */ It doesn't seem quite right to slap this copyright on a file that's full of content that came from other files. It would be one thing if arch/x86/boot/compressed/acpi.c had this banner in it already. Also, a bunch of the lines in this file seem to come from: commit 33f0df8d843deb9ec24116dcd79a40ca0ea8e8a9 Author: Chao Fan <fanc.fnst@cn.fujitsu.com> Date: Wed Jan 23 19:08:46 2019 +0800
On Fri, Dec 10, 2021 at 10:54:35AM -0800, Dave Hansen wrote: > On 12/10/21 7:43 AM, Brijesh Singh wrote: > > +/* > > + * Helpers for early access to EFI configuration table > > + * > > + * Copyright (C) 2021 Advanced Micro Devices, Inc. > > + * > > + * Author: Michael Roth <michael.roth@amd.com> > > + */ > > It doesn't seem quite right to slap this copyright on a file that's full > of content that came from other files. It would be one thing if > arch/x86/boot/compressed/acpi.c had this banner in it already. Also, a Yah, acpi.c didn't have any copyright banner so I used my 'default' template for new files here to cover any additions, but that does give a misleading impression. I'm not sure how this is normally addressed, but I'm planning on just continuing the acpi.c tradition of *not* adding copyright notices for new code, and simply document that the contents of the file are mostly movement from acpi.c > arch/x86/boot/compressed/acpi.c had this banner in it already. Also, a > bunch of the lines in this file seem to come from: > > commit 33f0df8d843deb9ec24116dcd79a40ca0ea8e8a9 > Author: Chao Fan <fanc.fnst@cn.fujitsu.com> > Date: Wed Jan 23 19:08:46 2019 +0800 AFAICT the full author list for the changes in question are, in alphabetical order: Chao Fan <fanc.fnst@cn.fujitsu.com> Junichi Nomura <j-nomura@ce.jp.nec.com> Borislav Petkov <bp@suse.de> Chao, Junichi, Borislav, If you would like to be listed as an author in efi.c (which is mainly just a movement of EFI config table parsing code from acpi.c into re-usable helper functions in efi.c), please let me know and I'll add you. Otherwise, I'll plan on adopting the acpi.c precedent for this as well, which is to not list individual authors, since it doesn't seem right to add Author fields retroactively without their permission.
On 12/13/21 7:47 AM, Michael Roth wrote: > Otherwise, I'll plan on adopting the acpi.c precedent for this as well, which > is to not list individual authors, since it doesn't seem right to add Author > fields retroactively without their permission. That's fine with me, especially if it follows precedent in the subsystem. Could you also please take a quick scan over the rest of the series to make sure there are no more of these?
On Mon, Dec 13, 2021 at 08:21:44AM -0800, Dave Hansen wrote: > On 12/13/21 7:47 AM, Michael Roth wrote: > > Otherwise, I'll plan on adopting the acpi.c precedent for this as well, which > > is to not list individual authors, since it doesn't seem right to add Author > > fields retroactively without their permission. > > That's fine with me, especially if it follows precedent in the subsystem. > > Could you also please take a quick scan over the rest of the series to > make sure there are no more of these? Outside of the guest driver there's only one other new file addition in the series: arch/x86/include/asm/cpuid.h where I moved some code out of arch/x86/kvm/cpuid.c in similar fashion, so cpuid.h should probably inherit cpuid.c's copyright banner. I'll make that change for the next spin as well. Thanks for the catches.
On 2021-12-10 09:43:16 -0600, 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. Incrementally 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> Reviewed-by: Venu Busireddy <venu.busireddy@oracle.com> > --- > arch/x86/boot/compressed/Makefile | 1 + > arch/x86/boot/compressed/acpi.c | 60 ++++++++++---------------- > arch/x86/boot/compressed/efi.c | 72 +++++++++++++++++++++++++++++++ > arch/x86/boot/compressed/misc.h | 14 ++++++ > 4 files changed, 109 insertions(+), 38 deletions(-) > create mode 100644 arch/x86/boot/compressed/efi.c > > diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile > index 431bf7f846c3..d364192c2367 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.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..9e784bd7b2e6 100644 > --- a/arch/x86/boot/compressed/acpi.c > +++ b/arch/x86/boot/compressed/acpi.c > @@ -86,8 +86,8 @@ static acpi_physical_address kexec_get_rsdp_addr(void) > { > efi_system_table_64_t *systab; > struct efi_setup_data *esd; > - struct efi_info *ei; > - char *sig; > + bool efi_64; > + int ret; > > esd = (struct efi_setup_data *)get_kexec_setup_data_addr(); > if (!esd) > @@ -98,18 +98,16 @@ static acpi_physical_address kexec_get_rsdp_addr(void) > return 0; > } > > - ei = &boot_params->efi_info; > - sig = (char *)&ei->efi_loader_signature; > - if (strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) { > + /* Get systab from boot params. */ > + ret = efi_get_system_table(boot_params, (unsigned long *)&systab, &efi_64); > + if (ret) > + error("EFI system table not found in kexec boot_params."); > + > + if (!efi_64) { > debug_putstr("Wrong kexec EFI loader signature.\n"); > return 0; > } > > - /* Get systab from boot params. */ > - systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32)); > - if (!systab) > - error("EFI system table not found in kexec boot_params."); > - > return __efi_get_rsdp_addr((unsigned long)esd->tables, systab->nr_tables, true); > } > #else > @@ -119,45 +117,31 @@ static acpi_physical_address kexec_get_rsdp_addr(void) { return 0; } > static acpi_physical_address efi_get_rsdp_addr(void) > { > #ifdef CONFIG_EFI > - unsigned long systab, config_tables; > + unsigned long systab_tbl_pa, config_tables; > unsigned int nr_tables; > - struct efi_info *ei; > bool efi_64; > - char *sig; > - > - ei = &boot_params->efi_info; > - sig = (char *)&ei->efi_loader_signature; > - > - if (!strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) { > - efi_64 = true; > - } else if (!strncmp(sig, EFI32_LOADER_SIGNATURE, 4)) { > - efi_64 = false; > - } else { > - debug_putstr("Wrong EFI loader signature.\n"); > - return 0; > - } > + int ret; > > - /* Get systab from boot params. */ > -#ifdef CONFIG_X86_64 > - systab = ei->efi_systab | ((__u64)ei->efi_systab_hi << 32); > -#else > - if (ei->efi_systab_hi || ei->efi_memmap_hi) { > - debug_putstr("Error getting RSDP address: EFI system table located above 4GB.\n"); > + /* > + * This function is called even for non-EFI BIOSes, and callers expect > + * failure to locate the EFI system table to result in 0 being returned > + * as indication that EFI is not available, rather than outright > + * failure/abort. > + */ > + ret = efi_get_system_table(boot_params, &systab_tbl_pa, &efi_64); > + if (ret == -EOPNOTSUPP) > return 0; > - } > - systab = ei->efi_systab; > -#endif > - if (!systab) > - error("EFI system table not found."); > + if (ret) > + error("EFI support advertised, but unable to locate system table."); > > /* Handle EFI bitness properly */ > if (efi_64) { > - efi_system_table_64_t *stbl = (efi_system_table_64_t *)systab; > + efi_system_table_64_t *stbl = (efi_system_table_64_t *)systab_tbl_pa; > > config_tables = stbl->tables; > nr_tables = stbl->nr_tables; > } else { > - efi_system_table_32_t *stbl = (efi_system_table_32_t *)systab; > + efi_system_table_32_t *stbl = (efi_system_table_32_t *)systab_tbl_pa; > > config_tables = stbl->tables; > nr_tables = stbl->nr_tables; > diff --git a/arch/x86/boot/compressed/efi.c b/arch/x86/boot/compressed/efi.c > new file mode 100644 > index 000000000000..1c626d28f07e > --- /dev/null > +++ b/arch/x86/boot/compressed/efi.c > @@ -0,0 +1,72 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Helpers for early access to EFI configuration table > + * > + * Copyright (C) 2021 Advanced Micro Devices, Inc. > + * > + * Author: Michael Roth <michael.roth@amd.com> > + */ > + > +#include "misc.h" > +#include <linux/efi.h> > +#include <asm/efi.h> > + > +/** > + * efi_get_system_table - Given boot_params, retrieve the physical address of > + * EFI system table. > + * > + * @boot_params: pointer to boot_params > + * @sys_tbl_pa: location to store physical address of system table > + * @is_efi_64: location to store whether using 64-bit EFI or not > + * > + * Return: 0 on success. On error, return params are left unchanged. > + * > + * Note: Existing callers like ACPI will call this unconditionally even for > + * non-EFI BIOSes. In such cases, those callers may treat cases where > + * bootparams doesn't indicate that a valid EFI system table is available as > + * non-fatal errors to allow fall-through to non-EFI alternatives. This > + * class of errors are reported as EOPNOTSUPP and should be kept in sync with > + * callers who check for that specific error. > + */ > +int efi_get_system_table(struct boot_params *boot_params, unsigned long *sys_tbl_pa, > + bool *is_efi_64) > +{ > + unsigned long sys_tbl; > + struct efi_info *ei; > + bool efi_64; > + char *sig; > + > + if (!sys_tbl_pa || !is_efi_64) > + return -EINVAL; > + > + ei = &boot_params->efi_info; > + sig = (char *)&ei->efi_loader_signature; > + > + if (!strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) { > + efi_64 = true; > + } else if (!strncmp(sig, EFI32_LOADER_SIGNATURE, 4)) { > + efi_64 = false; > + } else { > + debug_putstr("Wrong EFI loader signature.\n"); > + return -EOPNOTSUPP; > + } > + > + /* Get systab from boot params. */ > +#ifdef CONFIG_X86_64 > + sys_tbl = ei->efi_systab | ((__u64)ei->efi_systab_hi << 32); > +#else > + if (ei->efi_systab_hi || ei->efi_memmap_hi) { > + debug_putstr("Error: EFI system table located above 4GB.\n"); > + return -EOPNOTSUPP; > + } > + sys_tbl = ei->efi_systab; > +#endif > + if (!sys_tbl) { > + debug_putstr("EFI system table not found."); > + return -ENOENT; > + } > + > + *sys_tbl_pa = sys_tbl; > + *is_efi_64 = efi_64; > + return 0; > +} > diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h > index 01cc13c12059..165640f64b71 100644 > --- a/arch/x86/boot/compressed/misc.h > +++ b/arch/x86/boot/compressed/misc.h > @@ -23,6 +23,7 @@ > #include <linux/screen_info.h> > #include <linux/elf.h> > #include <linux/io.h> > +#include <linux/efi.h> > #include <asm/page.h> > #include <asm/boot.h> > #include <asm/bootparam.h> > @@ -176,4 +177,17 @@ void boot_stage2_vc(void); > > unsigned long sev_verify_cbit(unsigned long cr3); > > +#ifdef CONFIG_EFI > +/* helpers for early EFI config table access */ > +int efi_get_system_table(struct boot_params *boot_params, > + unsigned long *sys_tbl_pa, bool *is_efi_64); > +#else > +static inline int > +efi_get_system_table(struct boot_params *boot_params, > + unsigned long *sys_tbl_pa, bool *is_efi_64) > +{ > + return -ENOENT; > +} > +#endif /* CONFIG_EFI */ > + > #endif /* BOOT_COMPRESSED_MISC_H */ > -- > 2.25.1 >
diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile index 431bf7f846c3..d364192c2367 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.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..9e784bd7b2e6 100644 --- a/arch/x86/boot/compressed/acpi.c +++ b/arch/x86/boot/compressed/acpi.c @@ -86,8 +86,8 @@ static acpi_physical_address kexec_get_rsdp_addr(void) { efi_system_table_64_t *systab; struct efi_setup_data *esd; - struct efi_info *ei; - char *sig; + bool efi_64; + int ret; esd = (struct efi_setup_data *)get_kexec_setup_data_addr(); if (!esd) @@ -98,18 +98,16 @@ static acpi_physical_address kexec_get_rsdp_addr(void) return 0; } - ei = &boot_params->efi_info; - sig = (char *)&ei->efi_loader_signature; - if (strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) { + /* Get systab from boot params. */ + ret = efi_get_system_table(boot_params, (unsigned long *)&systab, &efi_64); + if (ret) + error("EFI system table not found in kexec boot_params."); + + if (!efi_64) { debug_putstr("Wrong kexec EFI loader signature.\n"); return 0; } - /* Get systab from boot params. */ - systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32)); - if (!systab) - error("EFI system table not found in kexec boot_params."); - return __efi_get_rsdp_addr((unsigned long)esd->tables, systab->nr_tables, true); } #else @@ -119,45 +117,31 @@ static acpi_physical_address kexec_get_rsdp_addr(void) { return 0; } static acpi_physical_address efi_get_rsdp_addr(void) { #ifdef CONFIG_EFI - unsigned long systab, config_tables; + unsigned long systab_tbl_pa, config_tables; unsigned int nr_tables; - struct efi_info *ei; bool efi_64; - char *sig; - - ei = &boot_params->efi_info; - sig = (char *)&ei->efi_loader_signature; - - if (!strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) { - efi_64 = true; - } else if (!strncmp(sig, EFI32_LOADER_SIGNATURE, 4)) { - efi_64 = false; - } else { - debug_putstr("Wrong EFI loader signature.\n"); - return 0; - } + int ret; - /* Get systab from boot params. */ -#ifdef CONFIG_X86_64 - systab = ei->efi_systab | ((__u64)ei->efi_systab_hi << 32); -#else - if (ei->efi_systab_hi || ei->efi_memmap_hi) { - debug_putstr("Error getting RSDP address: EFI system table located above 4GB.\n"); + /* + * This function is called even for non-EFI BIOSes, and callers expect + * failure to locate the EFI system table to result in 0 being returned + * as indication that EFI is not available, rather than outright + * failure/abort. + */ + ret = efi_get_system_table(boot_params, &systab_tbl_pa, &efi_64); + if (ret == -EOPNOTSUPP) return 0; - } - systab = ei->efi_systab; -#endif - if (!systab) - error("EFI system table not found."); + if (ret) + error("EFI support advertised, but unable to locate system table."); /* Handle EFI bitness properly */ if (efi_64) { - efi_system_table_64_t *stbl = (efi_system_table_64_t *)systab; + efi_system_table_64_t *stbl = (efi_system_table_64_t *)systab_tbl_pa; config_tables = stbl->tables; nr_tables = stbl->nr_tables; } else { - efi_system_table_32_t *stbl = (efi_system_table_32_t *)systab; + efi_system_table_32_t *stbl = (efi_system_table_32_t *)systab_tbl_pa; config_tables = stbl->tables; nr_tables = stbl->nr_tables; diff --git a/arch/x86/boot/compressed/efi.c b/arch/x86/boot/compressed/efi.c new file mode 100644 index 000000000000..1c626d28f07e --- /dev/null +++ b/arch/x86/boot/compressed/efi.c @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Helpers for early access to EFI configuration table + * + * Copyright (C) 2021 Advanced Micro Devices, Inc. + * + * Author: Michael Roth <michael.roth@amd.com> + */ + +#include "misc.h" +#include <linux/efi.h> +#include <asm/efi.h> + +/** + * efi_get_system_table - Given boot_params, retrieve the physical address of + * EFI system table. + * + * @boot_params: pointer to boot_params + * @sys_tbl_pa: location to store physical address of system table + * @is_efi_64: location to store whether using 64-bit EFI or not + * + * Return: 0 on success. On error, return params are left unchanged. + * + * Note: Existing callers like ACPI will call this unconditionally even for + * non-EFI BIOSes. In such cases, those callers may treat cases where + * bootparams doesn't indicate that a valid EFI system table is available as + * non-fatal errors to allow fall-through to non-EFI alternatives. This + * class of errors are reported as EOPNOTSUPP and should be kept in sync with + * callers who check for that specific error. + */ +int efi_get_system_table(struct boot_params *boot_params, unsigned long *sys_tbl_pa, + bool *is_efi_64) +{ + unsigned long sys_tbl; + struct efi_info *ei; + bool efi_64; + char *sig; + + if (!sys_tbl_pa || !is_efi_64) + return -EINVAL; + + ei = &boot_params->efi_info; + sig = (char *)&ei->efi_loader_signature; + + if (!strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) { + efi_64 = true; + } else if (!strncmp(sig, EFI32_LOADER_SIGNATURE, 4)) { + efi_64 = false; + } else { + debug_putstr("Wrong EFI loader signature.\n"); + return -EOPNOTSUPP; + } + + /* Get systab from boot params. */ +#ifdef CONFIG_X86_64 + sys_tbl = ei->efi_systab | ((__u64)ei->efi_systab_hi << 32); +#else + if (ei->efi_systab_hi || ei->efi_memmap_hi) { + debug_putstr("Error: EFI system table located above 4GB.\n"); + return -EOPNOTSUPP; + } + sys_tbl = ei->efi_systab; +#endif + if (!sys_tbl) { + debug_putstr("EFI system table not found."); + return -ENOENT; + } + + *sys_tbl_pa = sys_tbl; + *is_efi_64 = efi_64; + return 0; +} diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h index 01cc13c12059..165640f64b71 100644 --- a/arch/x86/boot/compressed/misc.h +++ b/arch/x86/boot/compressed/misc.h @@ -23,6 +23,7 @@ #include <linux/screen_info.h> #include <linux/elf.h> #include <linux/io.h> +#include <linux/efi.h> #include <asm/page.h> #include <asm/boot.h> #include <asm/bootparam.h> @@ -176,4 +177,17 @@ void boot_stage2_vc(void); unsigned long sev_verify_cbit(unsigned long cr3); +#ifdef CONFIG_EFI +/* helpers for early EFI config table access */ +int efi_get_system_table(struct boot_params *boot_params, + unsigned long *sys_tbl_pa, bool *is_efi_64); +#else +static inline int +efi_get_system_table(struct boot_params *boot_params, + unsigned long *sys_tbl_pa, bool *is_efi_64) +{ + return -ENOENT; +} +#endif /* CONFIG_EFI */ + #endif /* BOOT_COMPRESSED_MISC_H */