Re: [PATCH Part1 v5 32/38] x86/sev: enable SEV-SNP-validated CPUID in #VC handlers

[Michael Roth]

On Fri, Aug 27, 2021 at 01:32:40PM -0500, Michael Roth wrote:
> On Fri, Aug 27, 2021 at 05:18:49PM +0200, Borislav Petkov wrote:
> > On Fri, Aug 20, 2021 at 10:19:27AM -0500, Brijesh Singh wrote:
> > > From: Michael Roth <michael.roth@xxxxxxx>
> > > 
> > > This adds support for utilizing the SEV-SNP-validated CPUID table in
> > 
> > s/This adds support for utilizing/Utilize/
> > 
> > Yap, it can really be that simple. :)
> > 
> > > the various #VC handler routines used throughout boot/run-time. Mostly
> > > this is handled by re-using the CPUID lookup code introduced earlier
> > > for the boot/compressed kernel, but at various stages of boot some work
> > > needs to be done to ensure the CPUID table is set up and remains
> > > accessible throughout. The following init routines are introduced to
> > > handle this:
> > 
> > Do not talk about what your patch does - that should hopefully be
> > visible in the diff itself. Rather, talk about *why* you're doing what
> > you're doing.
> 
> I'll get this cleaned up.
> 
> > 
> > > sev_snp_cpuid_init():
> > 
> > This one is not really introduced - it is already there.
> > 
> > <snip all the complex rest>
> > 
> > So this patch is making my head spin. It seems we're dancing a lot of
> > dance just to have our CPUID page present at all times. Which begs the
> > question: do we need it during the whole lifetime of the guest?
> > 
> > Regardless, I think this can be simplified by orders of
> > magnitude if we allocated statically 4K for that CPUID page in
> > arch/x86/boot/compressed/mem_encrypt.S, copied the supplied CPUID page
> > from the firmware to it and from now on, work with our own copy.
> 
> That makes sense. I was thinking it was safer to work with the FW page
> since it would be less susceptible to something like a buffer overflow
> modifying the CPUID table, but __ro_after_init seems like it would
> provide similar protections. And yes, would definitely be great to avoid
> the need for so many [re-]init routines.
> 
> > 
> > You probably would need to still remap it for kernel proper but it would
> > get rid of all that crazy in this patch here.
> > 
> > Hmmm?
> 
> If the memory is allocated in boot/compressed/mem_encrypt.S, wouldn't
> kernel proper still need to create a static buffer for its copy? And if
> not, wouldn't boot compressed still need a way to pass the PA of this
> buffer? That seems like it would need to be done via boot_params. It
> seems like it would also need to be marked as reserved as well since
> kernel proper could no longer rely on the EFI map to handle it.
> 
> I've been testing a similar approach based on your suggestion that seems
> to work out pretty well, but there's still some ugliness due to the
> fixup_pointer() stuff that's needed early during snp_cpuid_init() in
> kernel proper, which results in the need for 2 init routines there. Not
> sure if there's a better way to handle it, but it's a lot better than 4
> init routines at least, and with this there is no longer any need to
> store the address/size of the FW page:
> 
> in arch/x86/kernel/sev-shared.c:
> 
> /* Firmware-enforced limit on CPUID table entries */
> #define SNP_CPUID_COUNT_MAX 64
> 
> struct sev_snp_cpuid_info {
>         u32 count;
>         u32 __reserved1;
>         u64 __reserved2;
>         struct sev_snp_cpuid_fn fn[SNP_CPUID_COUNT_MAX];
> } __packed;
> 
> static struct snp_cpuid_info cpuid_info_copy __ro_after_init;
> static const struct snp_cpuid_info *cpuid_info __ro_after_init;
> static int sev_snp_cpuid_enabled __ro_after_init;
> 
> /*
>  * Initial set up of CPUID table when running identity-mapped.
>  */
> #ifdef __BOOT_COMPRESSED
> void sev_snp_cpuid_init(struct boot_params *bp)
> #else
> void __init sev_snp_cpuid_init(struct boot_params *bp, unsigned long physaddr)
> #endif
> {
>         const struct sev_snp_cpuid_info *cpuid_info_fw;
> 
>         cpuid_info_fw = snp_probe_cpuid_info(bp);
>         if (!cpuid_info_fw)
>                 return;
> 
> #ifdef __BOOT_COMPRESSED
>         cpuid_info2 = &cpuid_info_copy;
> #else
>         /* Kernel proper calls this while pointer fixups are still needed. */
>         cpuid_info2 = (const struct sev_snp_cpuid_info *)
>                        ((void *)&cpuid_info_copy - (void *)_text + physaddr);
> #endif
>         memcpy((struct sev_snp_cpuid_info *)cpuid_info2, cpuid_info_fw,
>                sizeof(*cpuid_info2));

These should be cpuid_info, not cpuid_info2.