Re: [PATCH v8 20/40] x86/sev: Use SEV-SNP AP creation to start secondary CPUs

[Brijesh Singh]

On 12/31/21 9:36 AM, Borislav Petkov wrote:
> On Fri, Dec 10, 2021 at 09:43:12AM -0600, Brijesh Singh wrote:
> > diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
> > index 123a96f7dff2..38c14601ae4a 100644
> > --- a/arch/x86/include/asm/sev-common.h
> > +++ b/arch/x86/include/asm/sev-common.h
> > @@ -104,6 +104,7 @@ enum psc_op {
> >   	(((u64)(v) & GENMASK_ULL(63, 12)) >> 12)
> >   
> >   #define GHCB_HV_FT_SNP			BIT_ULL(0)
> > +#define GHCB_HV_FT_SNP_AP_CREATION	(BIT_ULL(1) | GHCB_HV_FT_SNP)
>
> Why is bit 0 ORed in? Because it "Requires SEV-SNP Feature."?

Yes, the SEV-SNP feature is required. Anyway, I will improve a check. We 
will reach to AP creation only after SEV-SNP feature is checked, so, in 
AP creation routine we just need to check for the AP_CREATION specific 
feature flag; I will add comment about it.

> You can still enforce that requirement in the test though.
>
> Or all those SEV features should not be bits but masks -
> GHCB_HV_FT_SNP_AP_CREATION_MASK for example, seeing how the others
> require the previous bits to be set too.

> ...
>
> >   static DEFINE_PER_CPU(struct sev_es_runtime_data*, runtime_data);
> >   DEFINE_STATIC_KEY_FALSE(sev_es_enable_key);
> >   
> > +static DEFINE_PER_CPU(struct sev_es_save_area *, snp_vmsa);
>
> This is what I mean: the struct is called "sev_es... " but the variable
> "snp_...". I.e., it is all sev_<something>.

Sure, I define the variable as sev_vmsa.

> > +
> >   static __always_inline bool on_vc_stack(struct pt_regs *regs)
> >   {
> >   	unsigned long sp = regs->sp;
> > @@ -814,6 +818,231 @@ void snp_set_memory_private(unsigned long vaddr, unsigned int npages)
> >   	pvalidate_pages(vaddr, npages, 1);
> >   }
> >   
> > +static int snp_set_vmsa(void *va, bool vmsa)
> > +{
> > +	u64 attrs;
> > +
> > +	/*
> > +	 * The RMPADJUST instruction is used to set or clear the VMSA bit for
> > +	 * a page. A change to the VMSA bit is only performed when running
> > +	 * at VMPL0 and is ignored at other VMPL levels. If too low of a target
>
> What does "too low" mean here exactly?

I believe its saying that target VMPL is lesser than the current VMPL 
level. Now that we have VMPL0 check enforced in the beginning so will 
work on improving comment.

> The kernel is not at VMPL0 but the specified level is lower? Weird...
>
> > +	 * VMPL level is specified, the instruction can succeed without changing
> > +	 * the VMSA bit should the kernel not be in VMPL0. Using a target VMPL
> > +	 * level of 1 will return a FAIL_PERMISSION error if the kernel is not
> > +	 * at VMPL0, thus ensuring that the VMSA bit has been properly set when
> > +	 * no error is returned.
>
> We do check whether we run at VMPL0 earlier when starting the guest -
> see enforce_vmpl0().
>
> I don't think you need any of that additional verification here - just
> assume you are at VMPL0.

Yep.

> > +	 */
> > +	attrs = 1;
> > +	if (vmsa)
> > +		attrs |= RMPADJUST_VMSA_PAGE_BIT;
> > +
> > +	return rmpadjust((unsigned long)va, RMP_PG_SIZE_4K, attrs);
> > +}
> > +
> > +#define __ATTR_BASE		(SVM_SELECTOR_P_MASK | SVM_SELECTOR_S_MASK)
> > +#define INIT_CS_ATTRIBS		(__ATTR_BASE | SVM_SELECTOR_READ_MASK | SVM_SELECTOR_CODE_MASK)
> > +#define INIT_DS_ATTRIBS		(__ATTR_BASE | SVM_SELECTOR_WRITE_MASK)
> > +
> > +#define INIT_LDTR_ATTRIBS	(SVM_SELECTOR_P_MASK | 2)
> > +#define INIT_TR_ATTRIBS		(SVM_SELECTOR_P_MASK | 3)
> > +
> > +static void *snp_safe_alloc_page(void)
>
> safe?
>
> And you don't need to say "safe" - snp_alloc_vmsa_page() is perfectly fine.

noted.

...

> > +
> > +	/*
> > +	 * A new VMSA is created each time because there is no guarantee that
> > +	 * the current VMSA is the kernels or that the vCPU is not running. If
>
> kernel's.
>
> And if it is not the kernel's, whose it is?

It could be hypervisor's VMSA.

> > +	 * an attempt was done to use the current VMSA with a running vCPU, a
> > +	 * #VMEXIT of that vCPU would wipe out all of the settings being done
> > +	 * here.
>
> I don't understand - this is waking up a CPU, how can it ever be a
> running vCPU which is using the current VMSA?!
>
> There is per_cpu(snp_vmsa, cpu), who else can be using that one currently?

Maybe Tom can expand it bit more?

...

> > +
> > +	if (!ghcb_sw_exit_info_1_is_valid(ghcb) ||
> > +	    lower_32_bits(ghcb->save.sw_exit_info_1)) {
> > +		pr_alert("SNP AP Creation error\n");
>
> alert?

I see that smboot.c is using the pr_err() when failing to wakeup CPU; 
will switch to pr_err(), let me know if you don't agree with it.


thx