Messages in this thread | | | Date | Mon, 8 Aug 2022 17:18:54 -0500 | Subject | Re: [PATCH v2 1/2] x86/sev: Put PSC struct on the stack in prep for unaccepted memory support | From | Tom Lendacky <> |
| |
On 8/8/22 16:43, Dave Hansen wrote: > On 8/8/22 10:16, Tom Lendacky wrote: > ... >> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h >> index b8357d6ecd47..6f7268a817fc 100644 >> --- a/arch/x86/include/asm/sev-common.h >> +++ b/arch/x86/include/asm/sev-common.h >> @@ -107,7 +107,7 @@ enum psc_op { >> #define GHCB_HV_FT_SNP_AP_CREATION BIT_ULL(1) >> >> /* SNP Page State Change NAE event */ >> -#define VMGEXIT_PSC_MAX_ENTRY 253 >> +#define VMGEXIT_PSC_MAX_ENTRY 64 > > In general, the stack-based allocation looks fine. It might be worth a > comment in there to make it clear that this can consume stack space.
I'll add that.
> >> struct psc_hdr { >> u16 cur_entry; >> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c >> index c05f0124c410..275aa890611f 100644 >> --- a/arch/x86/kernel/sev.c >> +++ b/arch/x86/kernel/sev.c >> @@ -66,6 +66,9 @@ static struct ghcb boot_ghcb_page __bss_decrypted __aligned(PAGE_SIZE); >> */ >> static struct ghcb *boot_ghcb __section(".data"); >> >> +/* Flag to indicate when the first per-CPU GHCB is registered */ >> +static bool ghcb_percpu_ready __section(".data"); > > So, there's a code path that can't be entered until this is set? Seems > like the least we can do it annotate that path with a > WARN_ON_ONCE(!ghcb_percpu_ready).
Sure, that can be added. Right now the only function that calls vmgexit_psc() is covered (set_pages_state()/__set_pages_state()) and is doing the right thing. But I guess if anything is added in the future, that will provide details on what happened.
> > Also, how does having _one_ global variable work for indicating the > state of multiple per-cpu structures? The code doesn't seem to delay > setting this variable until after _all_ of the per-cpu state is ready.
All of the per-CPU GHCBs are allocated during the BSP boot, before any AP is started. The APs only ever run the kernel_exc_vmm_communication() #VC handler and only ever use the per-CPU version of the GHCB, never the early boot version. This is based on the initial_vc_handler being switched to the runtime #VC handler, kernel_exc_vmm_communication.
The trigger for the switch over for the BSP from the early boot GHCB to the per-CPU GHCB is during setup_ghcb() after the initial_vc_handler has been switched to kernel_exc_vmm_communication, which is just after the per-CPU allocations. By putting the setting of the ghcb_percpu_ready in setup_ghcb(), it indicates that the BSP per-CPU GHCB has been registered and can be used.
> >> /* Bitmap of SEV features supported by the hypervisor */ >> static u64 sev_hv_features __ro_after_init; >> >> @@ -660,7 +663,7 @@ static void pvalidate_pages(unsigned long vaddr, unsigned int npages, bool valid >> } >> } >> >> -static void __init early_set_pages_state(unsigned long paddr, unsigned int npages, enum psc_op op) >> +static void early_set_pages_state(unsigned long paddr, unsigned int npages, enum psc_op op) >> { >> unsigned long paddr_end; >> u64 val; >> @@ -868,11 +871,16 @@ static void __set_pages_state(struct snp_psc_desc *data, unsigned long vaddr, >> static void set_pages_state(unsigned long vaddr, unsigned int npages, int op) >> { >> unsigned long vaddr_end, next_vaddr; >> - struct snp_psc_desc *desc; >> + struct snp_psc_desc desc; >> + >> + /* >> + * Use the MSR protocol when the per-CPU GHCBs are not yet registered, >> + * since vmgexit_psc() uses the per-CPU GHCB. >> + */ >> + if (!ghcb_percpu_ready) >> + return early_set_pages_state(__pa(vaddr), npages, op); >> >> - desc = kmalloc(sizeof(*desc), GFP_KERNEL_ACCOUNT); >> - if (!desc) >> - panic("SNP: failed to allocate memory for PSC descriptor\n"); >> + memset(&desc, 0, sizeof(desc)); > > Why is this using memset()? The compiler should be smart enough to > delay initializing 'desc' until after the return with this kind of > construct: > > struct snp_psc_desc desc = {}; > if (foo) > return; > // use 'desc' here > > The compiler *knows* there is no access to 'desc' before the if().
Yup, I can change that.
Thanks, Tom
> > >> vaddr = vaddr & PAGE_MASK; >> vaddr_end = vaddr + (npages << PAGE_SHIFT); >> @@ -882,12 +890,10 @@ static void set_pages_state(unsigned long vaddr, unsigned int npages, int op) >> next_vaddr = min_t(unsigned long, vaddr_end, >> (VMGEXIT_PSC_MAX_ENTRY * PAGE_SIZE) + vaddr); >> >> - __set_pages_state(desc, vaddr, next_vaddr, op); >> + __set_pages_state(&desc, vaddr, next_vaddr, op); >> >> vaddr = next_vaddr; >> } >> - >> - kfree(desc); >> } >> >> void snp_set_memory_shared(unsigned long vaddr, unsigned int npages) >> @@ -1254,6 +1260,8 @@ void setup_ghcb(void) >> if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) >> snp_register_per_cpu_ghcb(); >> >> + ghcb_percpu_ready = true; >> + >> return; >> } >> >
| |