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

Brijesh Singh
2022-01-12T16:33:52Z

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