Received-SPF: pass (google.com: domain of linux-kernel+bounces-40921-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <cover.1706307364.git.thomas.lendacky@amd.com> <98db05934aa2d12e7df4e34ca33309b1aa8c0284.1706307364.git.thomas.lendacky@amd.com>
In-Reply-To: <98db05934aa2d12e7df4e34ca33309b1aa8c0284.1706307364.git.thomas.lendacky@amd.com>
From: Dionna Amalie Glaze <dionnaglaze@google.com>
Date: Fri, 26 Jan 2024 16:05:00 -0800
Message-ID: <CAAH4kHY-K09tMkqDxnrgbH7vW3uvdtZTRfVH0Bysu77YV36AYw@mail.gmail.com>
Subject: Re: [PATCH 01/11] x86/sev: Rename snp_init() in the
 boot/compressed/sev.c file
To: Tom Lendacky <thomas.lendacky@amd.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org, 
	Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, 
	Dave Hansen <dave.hansen@linux.intel.com>, "H. Peter Anvin" <hpa@zytor.com>, 
	Andy Lutomirski <luto@kernel.org>, Peter Zijlstra <peterz@infradead.org>, 
	Dan Williams <dan.j.williams@intel.com>, Michael Roth <michael.roth@amd.com>, 
	Ashish Kalra <ashish.kalra@amd.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jan 26, 2024 at 2:16=E2=80=AFPM Tom Lendacky <thomas.lendacky@amd.c=
om> wrote:
>
> The snp_init() function is local to the boot/compressed/sev.c 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 t=
o
> 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 <thomas.lendacky@amd.com>
> ---
>  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/se=
v.c
> index 454acd7a2daf..c3030cfb6484 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -405,6 +405,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 =3D 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_t=
able_pa,
> +                                                               cfg_table=
_len,
> +                                                               EFI_CC_BL=
OB_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 =3D find_cc_blob_efi(bp);
> +       if (cc_info)
> +               goto found_cc_info;
> +
> +       cc_info =3D find_cc_blob_setup_data(bp);
> +       if (!cc_info)
> +               return NULL;
> +
> +found_cc_info:
> +       if (cc_info->magic !=3D 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 ch=
ecks
> + * will verify the SNP CPUID/MSR bits.
> + */
> +static bool snp_setup(struct boot_params *bp)
> +{
> +       struct cc_blob_sev_info *cc_info;
> +
> +       if (!bp)
> +               return false;
> +
> +       cc_info =3D 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 E=
FI
> +        * config table doesn't need to be searched again during early st=
artup
> +        * phase.
> +        */
> +       bp->cc_blob_address =3D (u32)(unsigned long)cc_info;
> +
> +       return true;
> +}
> +
>  /*
>   * sev_check_cpu_support - Check for SEV support in the CPU capabilities
>   *
> @@ -455,7 +534,7 @@ void sev_enable(struct boot_params *bp)
>                 bp->cc_blob_address =3D 0;
>
>         /*
> -        * Do an initial SEV capability check before snp_init() which
> +        * Do an initial SEV capability check before snp_setup() which
>          * loads the CPUID page and the same checks afterwards are done
>          * without the hypervisor and are trustworthy.
>          *
> @@ -470,7 +549,7 @@ void sev_enable(struct boot_params *bp)
>          * Setup/preliminary detection of SNP. This will be sanity-checke=
d
>          * against CPUID/MSR values later.
>          */
> -       snp =3D snp_init(bp);
> +       snp =3D snp_setup(bp);
>
>         /* Now repeat the checks with the SNP CPUID table. */
>
> @@ -527,85 +606,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 =3D 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_t=
able_pa,
> -                                                               cfg_table=
_len,
> -                                                               EFI_CC_BL=
OB_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 =3D find_cc_blob_efi(bp);
> -       if (cc_info)
> -               goto found_cc_info;
> -
> -       cc_info =3D find_cc_blob_setup_data(bp);
> -       if (!cc_info)
> -               return NULL;
> -
> -found_cc_info:
> -       if (cc_info->magic !=3D 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 ch=
ecks
> - * 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 =3D 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 E=
FI
> -        * config table doesn't need to be searched again during early st=
artup
> -        * phase.
> -        */
> -       bp->cc_blob_address =3D (u32)(unsigned long)cc_info;
> -
> -       return true;
> -}
> -

Did some kind of whitespace replacement happen accidentally? There's a
lot that isn't changed but the diff is quite big.

>  void sev_prep_identity_maps(unsigned long top_level_pgt)
>  {
>         /*
> --
> 2.42.0
>
>


--=20
-Dionna Glaze, PhD (she/her)