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:

quoted

From: Michael Roth <redacted>

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. :)

quoted

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.

quoted

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));

        sev_snp_cpuid_enabled = 1;
}

#ifndef __BOOT_COMPRESSED
/*
 * This is called after the switch to virtual kernel addresses. At this
 * point pointer fixups are no longer needed, and the virtual address of
 * the CPUID info buffer has changed, so re-initialize the pointer.
 */
void __init sev_snp_cpuid_init_virtual(void)
{
        /*
         * sev_snp_cpuid_init() already did the initial parsing of bootparams
         * and initial setup. If that didn't enable the feature then don't try
         * to enable it here.
         */
        if (!sev_snp_cpuid_active())
                return;

        /*
         * Either boot_params/EFI advertised the feature even though SNP isn't
         * enabled, or something else went wrong. Bail out.
         */
        if (!sev_feature_enabled(SEV_SNP))
                sev_es_terminate(1, GHCB_TERM_CPUID);

        cpuid_info = &cpuid_info_copy;
}
#endif

Then the rest of the code just accesses cpuid_info directly as it does now.

Would that be a reasonable approach for v6?

-- 
Regards/Gruss,
    Boris.

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=04%7C01%7Cmichael.roth%40amd.com%7C464de4ea70544dc32ba108d9696de67a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637656743008963071%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=wYK2a%2FuHgw0%2FQJsCVCIpoaJxdT0XASMYUfHvmDcowXg%3D&reserved=0