The snp_init() in boot/compressed/sev.c is local to that file and is not
called from outside of the file. Change the name so that it is not tied
to the function definition in arch/x86/include/asm/sev.h. Move the renamed
snp_init() and related functions up in the file to avoid having to add a
forward declaration and make the function static, too.
This will allow the snp_init() function in arch/x86/kernel/sev.c to be
changed without having to make the same change in boot/compressed/sev.c.
Signed-off-by: Tom Lendacky <[email protected]>
---
arch/x86/boot/compressed/sev.c | 162 ++++++++++++++++-----------------
1 file changed, 81 insertions(+), 81 deletions(-)
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index ec71846d28c9..5ad0ff4664f1 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -413,6 +413,85 @@ void snp_check_features(void)
}
}
+/* Search for Confidential Computing blob in the EFI config table. */
+static struct cc_blob_sev_info *find_cc_blob_efi(struct boot_params *bp)
+{
+ unsigned long cfg_table_pa;
+ unsigned int cfg_table_len;
+ int ret;
+
+ ret = efi_get_conf_table(bp, &cfg_table_pa, &cfg_table_len);
+ if (ret)
+ return NULL;
+
+ return (struct cc_blob_sev_info *)efi_find_vendor_table(bp, cfg_table_pa,
+ cfg_table_len,
+ EFI_CC_BLOB_GUID);
+}
+
+/*
+ * Initial set up of SNP relies on information provided by the
+ * Confidential Computing blob, which can be passed to the boot kernel
+ * by firmware/bootloader in the following ways:
+ *
+ * - via an entry in the EFI config table
+ * - via a setup_data structure, as defined by the Linux Boot Protocol
+ *
+ * Scan for the blob in that order.
+ */
+static struct cc_blob_sev_info *find_cc_blob(struct boot_params *bp)
+{
+ struct cc_blob_sev_info *cc_info;
+
+ cc_info = find_cc_blob_efi(bp);
+ if (cc_info)
+ goto found_cc_info;
+
+ cc_info = find_cc_blob_setup_data(bp);
+ if (!cc_info)
+ return NULL;
+
+found_cc_info:
+ if (cc_info->magic != CC_BLOB_SEV_HDR_MAGIC)
+ sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
+
+ return cc_info;
+}
+
+/*
+ * Indicate SNP based on presence of SNP-specific CC blob. Subsequent checks
+ * will verify the SNP CPUID/MSR bits.
+ */
+static bool early_snp_init(struct boot_params *bp)
+{
+ struct cc_blob_sev_info *cc_info;
+
+ if (!bp)
+ return false;
+
+ cc_info = find_cc_blob(bp);
+ if (!cc_info)
+ return false;
+
+ /*
+ * If a SNP-specific Confidential Computing blob is present, then
+ * firmware/bootloader have indicated SNP support. Verifying this
+ * involves CPUID checks which will be more reliable if the SNP
+ * CPUID table is used. See comments over snp_setup_cpuid_table() for
+ * more details.
+ */
+ setup_cpuid_table(cc_info);
+
+ /*
+ * Pass run-time kernel a pointer to CC info via boot_params so EFI
+ * config table doesn't need to be searched again during early startup
+ * phase.
+ */
+ bp->cc_blob_address = (u32)(unsigned long)cc_info;
+
+ return true;
+}
+
/*
* sev_check_cpu_support - Check for SEV support in the CPU capabilities
*
@@ -463,7 +542,7 @@ void sev_enable(struct boot_params *bp)
bp->cc_blob_address = 0;
/*
- * Do an initial SEV capability check before snp_init() which
+ * Do an initial SEV capability check before early_snp_init() which
* loads the CPUID page and the same checks afterwards are done
* without the hypervisor and are trustworthy.
*
@@ -478,7 +557,7 @@ void sev_enable(struct boot_params *bp)
* Setup/preliminary detection of SNP. This will be sanity-checked
* against CPUID/MSR values later.
*/
- snp = snp_init(bp);
+ snp = early_snp_init(bp);
/* Now repeat the checks with the SNP CPUID table. */
@@ -535,85 +614,6 @@ u64 sev_get_status(void)
return m.q;
}
-/* Search for Confidential Computing blob in the EFI config table. */
-static struct cc_blob_sev_info *find_cc_blob_efi(struct boot_params *bp)
-{
- unsigned long cfg_table_pa;
- unsigned int cfg_table_len;
- int ret;
-
- ret = efi_get_conf_table(bp, &cfg_table_pa, &cfg_table_len);
- if (ret)
- return NULL;
-
- return (struct cc_blob_sev_info *)efi_find_vendor_table(bp, cfg_table_pa,
- cfg_table_len,
- EFI_CC_BLOB_GUID);
-}
-
-/*
- * Initial set up of SNP relies on information provided by the
- * Confidential Computing blob, which can be passed to the boot kernel
- * by firmware/bootloader in the following ways:
- *
- * - via an entry in the EFI config table
- * - via a setup_data structure, as defined by the Linux Boot Protocol
- *
- * Scan for the blob in that order.
- */
-static struct cc_blob_sev_info *find_cc_blob(struct boot_params *bp)
-{
- struct cc_blob_sev_info *cc_info;
-
- cc_info = find_cc_blob_efi(bp);
- if (cc_info)
- goto found_cc_info;
-
- cc_info = find_cc_blob_setup_data(bp);
- if (!cc_info)
- return NULL;
-
-found_cc_info:
- if (cc_info->magic != CC_BLOB_SEV_HDR_MAGIC)
- sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
-
- return cc_info;
-}
-
-/*
- * Indicate SNP based on presence of SNP-specific CC blob. Subsequent checks
- * will verify the SNP CPUID/MSR bits.
- */
-bool snp_init(struct boot_params *bp)
-{
- struct cc_blob_sev_info *cc_info;
-
- if (!bp)
- return false;
-
- cc_info = find_cc_blob(bp);
- if (!cc_info)
- return false;
-
- /*
- * If a SNP-specific Confidential Computing blob is present, then
- * firmware/bootloader have indicated SNP support. Verifying this
- * involves CPUID checks which will be more reliable if the SNP
- * CPUID table is used. See comments over snp_setup_cpuid_table() for
- * more details.
- */
- setup_cpuid_table(cc_info);
-
- /*
- * Pass run-time kernel a pointer to CC info via boot_params so EFI
- * config table doesn't need to be searched again during early startup
- * phase.
- */
- bp->cc_blob_address = (u32)(unsigned long)cc_info;
-
- return true;
-}
-
void sev_prep_identity_maps(unsigned long top_level_pgt)
{
/*
--
2.43.2
On Mon, Mar 25, 2024 at 05:26:20PM -0500, Tom Lendacky wrote:
> The snp_init() in boot/compressed/sev.c is local to that file and is not
> called from outside of the file. Change the name so that it is not tied
> to the function definition in arch/x86/include/asm/sev.h.
That part I don't understand: I can rename the function without making
it static and it builds fine, so where is it "tied" to the function
definition in kernel proper?
Don't get me wrong - leaking kernel proper symbols into the decompressor
has been a pet peeve of mine for a while now but this is not the case
here, is it?
And yes, the patch is fine - I'm just asking...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 4/9/24 12:09, Borislav Petkov wrote:
> On Mon, Mar 25, 2024 at 05:26:20PM -0500, Tom Lendacky wrote:
>> The snp_init() in boot/compressed/sev.c is local to that file and is not
>> called from outside of the file. Change the name so that it is not tied
>> to the function definition in arch/x86/include/asm/sev.h.
>
> That part I don't understand: I can rename the function without making
> it static and it builds fine, so where is it "tied" to the function
> definition in kernel proper?
When it's not static and has the name snp_init(), then it has to match
the definition in arch/x86/include/asm/sev.h, which is really intended
for the snp_init() in arch/x86/kernel/sev.c when called from
arch/x86/mm/mem_encrypt_identity.c.
So, yes, changing the name would be enough except then it remains not a
static and you can get a compiler warning about not having a prototype
for it if the -Wmissing-prototypes option is ever applied to that file
(I don't believe it is today because it is in the decompressor code, but
that can change). And since nothing calls the snp_init() in
arch/x86/boot/compressed/sev.c from outside of that file, making it
static was appropriate.
Thanks,
Tom
>
> Don't get me wrong - leaking kernel proper symbols into the decompressor
> has been a pet peeve of mine for a while now but this is not the case
> here, is it?
>
> And yes, the patch is fine - I'm just asking...
>
> Thx.
>
On Tue, Apr 09, 2024 at 12:44:13PM -0500, Tom Lendacky wrote:
> When it's not static and has the name snp_init(), then it has to match the
> definition in arch/x86/include/asm/sev.h, which is really intended for the
> snp_init() in arch/x86/kernel/sev.c when called from
> arch/x86/mm/mem_encrypt_identity.c.
>
> So, yes, changing the name would be enough except then it remains not a
> static and you can get a compiler warning about not having a prototype for
> it if the -Wmissing-prototypes option is ever applied to that file (I don't
> believe it is today because it is in the decompressor code, but that can
> change). And since nothing calls the snp_init() in
> arch/x86/boot/compressed/sev.c from outside of that file, making it static
> was appropriate.
Yes, then please remove all that text about what could potentially
happen from the commit message so that it is not confusing as to what
the situation *currently* is.
The two functions are independent right now. It is enough to say that
you want to differentiate which one is called when, in order to avoid
confusion.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 3/25/2024 11:26 PM, Tom Lendacky wrote:
> The snp_init() in boot/compressed/sev.c is local to that file and is not
> called from outside of the file. Change the name so that it is not tied
> to the function definition in arch/x86/include/asm/sev.h. Move the renamed
> snp_init() and related functions up in the file to avoid having to add a
> forward declaration and make the function static, too.
>
> This will allow the snp_init() function in arch/x86/kernel/sev.c to be
> changed without having to make the same change in boot/compressed/sev.c.
>
> Signed-off-by: Tom Lendacky <[email protected]>
Seems no functional change. Just rename snp_init() & move functions up
in the file.
Reviewed-by: Pankaj Gupta <[email protected]>
> ---
> arch/x86/boot/compressed/sev.c | 162 ++++++++++++++++-----------------
> 1 file changed, 81 insertions(+), 81 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index ec71846d28c9..5ad0ff4664f1 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -413,6 +413,85 @@ void snp_check_features(void)
> }
> }
>
> +/* Search for Confidential Computing blob in the EFI config table. */
> +static struct cc_blob_sev_info *find_cc_blob_efi(struct boot_params *bp)
> +{
> + unsigned long cfg_table_pa;
> + unsigned int cfg_table_len;
> + int ret;
> +
> + ret = efi_get_conf_table(bp, &cfg_table_pa, &cfg_table_len);
> + if (ret)
> + return NULL;
> +
> + return (struct cc_blob_sev_info *)efi_find_vendor_table(bp, cfg_table_pa,
> + cfg_table_len,
> + EFI_CC_BLOB_GUID);
> +}
> +
> +/*
> + * Initial set up of SNP relies on information provided by the
> + * Confidential Computing blob, which can be passed to the boot kernel
> + * by firmware/bootloader in the following ways:
> + *
> + * - via an entry in the EFI config table
> + * - via a setup_data structure, as defined by the Linux Boot Protocol
> + *
> + * Scan for the blob in that order.
> + */
> +static struct cc_blob_sev_info *find_cc_blob(struct boot_params *bp)
> +{
> + struct cc_blob_sev_info *cc_info;
> +
> + cc_info = find_cc_blob_efi(bp);
> + if (cc_info)
> + goto found_cc_info;
> +
> + cc_info = find_cc_blob_setup_data(bp);
> + if (!cc_info)
> + return NULL;
> +
> +found_cc_info:
> + if (cc_info->magic != CC_BLOB_SEV_HDR_MAGIC)
> + sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
> +
> + return cc_info;
> +}
> +
> +/*
> + * Indicate SNP based on presence of SNP-specific CC blob. Subsequent checks
> + * will verify the SNP CPUID/MSR bits.
> + */
> +static bool early_snp_init(struct boot_params *bp)
> +{
> + struct cc_blob_sev_info *cc_info;
> +
> + if (!bp)
> + return false;
> +
> + cc_info = find_cc_blob(bp);
> + if (!cc_info)
> + return false;
> +
> + /*
> + * If a SNP-specific Confidential Computing blob is present, then
> + * firmware/bootloader have indicated SNP support. Verifying this
> + * involves CPUID checks which will be more reliable if the SNP
> + * CPUID table is used. See comments over snp_setup_cpuid_table() for
> + * more details.
> + */
> + setup_cpuid_table(cc_info);
> +
> + /*
> + * Pass run-time kernel a pointer to CC info via boot_params so EFI
> + * config table doesn't need to be searched again during early startup
> + * phase.
> + */
> + bp->cc_blob_address = (u32)(unsigned long)cc_info;
> +
> + return true;
> +}
> +
> /*
> * sev_check_cpu_support - Check for SEV support in the CPU capabilities
> *
> @@ -463,7 +542,7 @@ void sev_enable(struct boot_params *bp)
> bp->cc_blob_address = 0;
>
> /*
> - * Do an initial SEV capability check before snp_init() which
> + * Do an initial SEV capability check before early_snp_init() which
> * loads the CPUID page and the same checks afterwards are done
> * without the hypervisor and are trustworthy.
> *
> @@ -478,7 +557,7 @@ void sev_enable(struct boot_params *bp)
> * Setup/preliminary detection of SNP. This will be sanity-checked
> * against CPUID/MSR values later.
> */
> - snp = snp_init(bp);
> + snp = early_snp_init(bp);
>
> /* Now repeat the checks with the SNP CPUID table. */
>
> @@ -535,85 +614,6 @@ u64 sev_get_status(void)
> return m.q;
> }
>
> -/* Search for Confidential Computing blob in the EFI config table. */
> -static struct cc_blob_sev_info *find_cc_blob_efi(struct boot_params *bp)
> -{
> - unsigned long cfg_table_pa;
> - unsigned int cfg_table_len;
> - int ret;
> -
> - ret = efi_get_conf_table(bp, &cfg_table_pa, &cfg_table_len);
> - if (ret)
> - return NULL;
> -
> - return (struct cc_blob_sev_info *)efi_find_vendor_table(bp, cfg_table_pa,
> - cfg_table_len,
> - EFI_CC_BLOB_GUID);
> -}
> -
> -/*
> - * Initial set up of SNP relies on information provided by the
> - * Confidential Computing blob, which can be passed to the boot kernel
> - * by firmware/bootloader in the following ways:
> - *
> - * - via an entry in the EFI config table
> - * - via a setup_data structure, as defined by the Linux Boot Protocol
> - *
> - * Scan for the blob in that order.
> - */
> -static struct cc_blob_sev_info *find_cc_blob(struct boot_params *bp)
> -{
> - struct cc_blob_sev_info *cc_info;
> -
> - cc_info = find_cc_blob_efi(bp);
> - if (cc_info)
> - goto found_cc_info;
> -
> - cc_info = find_cc_blob_setup_data(bp);
> - if (!cc_info)
> - return NULL;
> -
> -found_cc_info:
> - if (cc_info->magic != CC_BLOB_SEV_HDR_MAGIC)
> - sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
> -
> - return cc_info;
> -}
> -
> -/*
> - * Indicate SNP based on presence of SNP-specific CC blob. Subsequent checks
> - * will verify the SNP CPUID/MSR bits.
> - */
> -bool snp_init(struct boot_params *bp)
> -{
> - struct cc_blob_sev_info *cc_info;
> -
> - if (!bp)
> - return false;
> -
> - cc_info = find_cc_blob(bp);
> - if (!cc_info)
> - return false;
> -
> - /*
> - * If a SNP-specific Confidential Computing blob is present, then
> - * firmware/bootloader have indicated SNP support. Verifying this
> - * involves CPUID checks which will be more reliable if the SNP
> - * CPUID table is used. See comments over snp_setup_cpuid_table() for
> - * more details.
> - */
> - setup_cpuid_table(cc_info);
> -
> - /*
> - * Pass run-time kernel a pointer to CC info via boot_params so EFI
> - * config table doesn't need to be searched again during early startup
> - * phase.
> - */
> - bp->cc_blob_address = (u32)(unsigned long)cc_info;
> -
> - return true;
> -}
> -
> void sev_prep_identity_maps(unsigned long top_level_pgt)
> {
> /*