lkml.org 
[lkml]   [2021]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH Part1 v5 32/38] x86/sev: enable SEV-SNP-validated CPUID in #VC handlers
    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@amd.com>
    > >
    > > 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));

    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&amp;data=04%7C01%7Cmichael.roth%40amd.com%7C464de4ea70544dc32ba108d9696de67a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637656743008963071%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=wYK2a%2FuHgw0%2FQJsCVCIpoaJxdT0XASMYUfHvmDcowXg%3D&amp;reserved=0

    \
     
     \ /
      Last update: 2021-08-30 17:57    [W:4.382 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site