Messages in this thread |  | | Subject | Re: [PATCH v8 20/40] x86/sev: Use SEV-SNP AP creation to start secondary CPUs | From | Brijesh Singh <> | Date | Wed, 12 Jan 2022 10:33:40 -0600 |
| |
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
|  |