[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/3] amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests
On 01.02.2022 17:46, Roger Pau Monne wrote: > Allow HVM guests untrapped access to MSR_VIRT_SPEC_CTRL if the > hardware has support for it. This requires adding logic in the > vm{entry,exit} paths for SVM in order to context switch between the > hypervisor value and the guest one. The added handlers for context > switch will also be used for the legacy SSBD support. So by "hardware" you mean virtual hardware here, when we run virtualized ourselves? While the wording in AMD's whitepaper suggests hardware could exist with both MSRs implemented, so far it was my understanding that VIRT_SPEC_CTRL was rather left for hypervisors to implement. Maybe I'm wrong with this, in which case some of the further comments may also be wrong. > --- a/xen/arch/x86/cpu/amd.c > +++ b/xen/arch/x86/cpu/amd.c > @@ -687,6 +687,7 @@ void amd_init_lfence(struct cpuinfo_x86 *c) > */ > void amd_init_ssbd(const struct cpuinfo_x86 *c) > { > + struct cpu_info *info = get_cpu_info(); > int bit = -1; > > if (cpu_has_ssb_no) > @@ -699,7 +700,7 @@ void amd_init_ssbd(const struct cpuinfo_x86 *c) > > if (cpu_has_virt_ssbd) { > wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0); > - return; > + goto out; > } > > switch (c->x86) { > @@ -729,6 +730,10 @@ void amd_init_ssbd(const struct cpuinfo_x86 *c) > > if (bit < 0) > printk_once(XENLOG_ERR "No SSBD controls available\n"); > + > + out: > + info->last_spec_ctrl = info->xen_spec_ctrl = opt_ssbd ? SPEC_CTRL_SSBD > + : 0; > } Besides me being uncertain about the placement of these (preferably the writes would be where the other similar writes are), this re-use of the values suggests that you mean to prefer VIRT_SPEC_CTRL use over that of SPEC_CTRL (see below). Additionally - the value you store isn't necessarily the value you wrote to the MSR. It only is if you cam here via the "goto out". > --- a/xen/arch/x86/hvm/svm/entry.S > +++ b/xen/arch/x86/hvm/svm/entry.S > @@ -71,7 +71,9 @@ __UNLIKELY_END(nsvm_hap) > mov %al, CPUINFO_last_spec_ctrl(%rsp) > 1: /* No Spectre v1 concerns. Execution will hit VMRUN imminently. > */ > .endm > - ALTERNATIVE "", svm_vmentry_spec_ctrl, X86_FEATURE_SC_MSR_HVM > + ALTERNATIVE_2 "", STR(call vmentry_virt_spec_ctrl), \ I'm afraid this violates the "ret" part of the warning a few lines up, while ... > + X86_FEATURE_VIRT_SC_MSR_HVM, \ > + svm_vmentry_spec_ctrl, X86_FEATURE_SC_MSR_HVM > > pop %r15 > pop %r14 > @@ -111,7 +113,9 @@ __UNLIKELY_END(nsvm_hap) > wrmsr > mov %al, CPUINFO_last_spec_ctrl(%rsp) > .endm > - ALTERNATIVE "", svm_vmexit_spec_ctrl, X86_FEATURE_SC_MSR_HVM > + ALTERNATIVE_2 "", STR(call vmexit_virt_spec_ctrl), \ ... this violates ... > + X86_FEATURE_VIRT_SC_MSR_HVM, \ > + svm_vmexit_spec_ctrl, X86_FEATURE_SC_MSR_HVM > /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */ ... the "ret" part of this warning. Furthermore, opposite to what the change to amd_init_ssbd() suggests, the ordering of the alternatives here means you prefer SPEC_CTRL over VIRT_SPEC_CTRL; see the comment near the top of _apply_alternatives(). Unless I've missed logic guaranteeing that both of the keyed to features can't be active at the same time. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |