Messages in this thread Patch in this message | | | Date | Mon, 8 Aug 2022 14:43:52 -0700 | Subject | Re: [PATCH v2 1/2] x86/sev: Put PSC struct on the stack in prep for unaccepted memory support | From | Dave Hansen <> |
| |
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.
> 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).
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.
> /* 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().
> 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; > } >
| |