| Message ID | 6189fed4-2aac-8ca3-90f6-7a750a8993dd@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/SVM: restrict hardware SSBD update upon guest VIRT_SPEC_CTRL write | expand |
On Thu, Dec 08, 2022 at 12:24:54PM +0100, Jan Beulich wrote: > core_set_legacy_ssbd() counts the number of times SSBD is being enabled > via LS_CFG on a core. This assumes that calls there only occur if the > state actually changes. While svm_ctxt_switch_{to,from}() conform to > this, guest_wrmsr() doesn't: It also calls the function when the bit > doesn't actually change. Extend the conditional there accordingly. > > Fixes: b2030e6730a2 ("amd/virt_ssbd: set SSBD at vCPU context switch") > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > This is the less intrusive but more fragile variant of a fix. The > alternative would be to have core_set_legacy_ssbd() record per-thread > state, such that the necessary checking can be done there. Hm, yes, it's going to take a bit more of memory to keep track of this. > This wants properly testing on affected hardware. From Andrew's > description it's also not clear whether this really is addressing that > problem, or yet another one in this same area. > > --- a/xen/arch/x86/msr.c > +++ b/xen/arch/x86/msr.c > @@ -699,12 +699,16 @@ int guest_wrmsr(struct vcpu *v, uint32_t > } > else I think you could turn this into an `else if` and check if the new value and the current one differ on the SSBD bit? Provided it fixes the issue: Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On 09.12.2022 10:59, Roger Pau Monné wrote: > On Thu, Dec 08, 2022 at 12:24:54PM +0100, Jan Beulich wrote: >> --- a/xen/arch/x86/msr.c >> +++ b/xen/arch/x86/msr.c >> @@ -699,12 +699,16 @@ int guest_wrmsr(struct vcpu *v, uint32_t >> } >> else > > I think you could turn this into an `else if` and check if the new > value and the current one differ on the SSBD bit? I'd prefer not to: Keeping it as I have it will likely reduce code churn if a 2nd bit wants supporting in that MSR. > Provided it fixes the issue: > > Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, but I'm a little puzzled by the constraint: Imo even if this doesn't address the observed issue, it still fixes one aspect of wrong behavior here. The sole difference then would be that the Reported-by: would go away. Jan
On 09/12/2022 09:59, Roger Pau Monné wrote: > On Thu, Dec 08, 2022 at 12:24:54PM +0100, Jan Beulich wrote: >> core_set_legacy_ssbd() counts the number of times SSBD is being enabled >> via LS_CFG on a core. This assumes that calls there only occur if the >> state actually changes. While svm_ctxt_switch_{to,from}() conform to >> this, guest_wrmsr() doesn't: It also calls the function when the bit >> doesn't actually change. Extend the conditional there accordingly. >> >> Fixes: b2030e6730a2 ("amd/virt_ssbd: set SSBD at vCPU context switch") >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> This is the less intrusive but more fragile variant of a fix. The >> alternative would be to have core_set_legacy_ssbd() record per-thread >> state, such that the necessary checking can be done there. > Hm, yes, it's going to take a bit more of memory to keep track of > this. It shouldn't. Turn the count field into a bitmap of thread_ids. The interface to this functionality should be WRMSR-like, and should support repeated writes of the same value. Anything else is a timebomb which has already gone off once... I'll have a play with this while looking into the repro I've got. ~Andrew
On Fri, Dec 09, 2022 at 11:11:29AM +0100, Jan Beulich wrote: > On 09.12.2022 10:59, Roger Pau Monné wrote: > > On Thu, Dec 08, 2022 at 12:24:54PM +0100, Jan Beulich wrote: > >> --- a/xen/arch/x86/msr.c > >> +++ b/xen/arch/x86/msr.c > >> @@ -699,12 +699,16 @@ int guest_wrmsr(struct vcpu *v, uint32_t > >> } > >> else > > > > I think you could turn this into an `else if` and check if the new > > value and the current one differ on the SSBD bit? > > I'd prefer not to: Keeping it as I have it will likely reduce code churn > if a 2nd bit wants supporting in that MSR. > > > Provided it fixes the issue: > > > > Acked-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks, but I'm a little puzzled by the constraint: Imo even if this > doesn't address the observed issue, it still fixes one aspect of wrong > behavior here. The sole difference then would be that the Reported-by: > would go away. Just wanted to make sure whether there was a further issue linked with this, in a way that we might need to change the fix. Maybe do the accounting in amd_set_legacy_ssbd() and keep track of each thread status. Thanks, Roger.
--- a/xen/arch/x86/msr.c +++ b/xen/arch/x86/msr.c @@ -699,12 +699,16 @@ int guest_wrmsr(struct vcpu *v, uint32_t } else { + uint64_t orig = msrs->virt_spec_ctrl.raw; + msrs->virt_spec_ctrl.raw = val & SPEC_CTRL_SSBD; - if ( v == curr ) - /* - * Propagate the value to hardware, as it won't be set on guest - * resume path. - */ + if ( v == curr && + /* + * Propagate the value to hardware, as it won't be set on guest + * resume path. But only do so if the bit actually changed, to + * avoid issues with core_set_legacy_ssbd()'s refcounting. + */ + ((val ^ orig) & SPEC_CTRL_SSBD) ) amd_set_legacy_ssbd(val & SPEC_CTRL_SSBD); } break;
core_set_legacy_ssbd() counts the number of times SSBD is being enabled via LS_CFG on a core. This assumes that calls there only occur if the state actually changes. While svm_ctxt_switch_{to,from}() conform to this, guest_wrmsr() doesn't: It also calls the function when the bit doesn't actually change. Extend the conditional there accordingly. Fixes: b2030e6730a2 ("amd/virt_ssbd: set SSBD at vCPU context switch") Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- This is the less intrusive but more fragile variant of a fix. The alternative would be to have core_set_legacy_ssbd() record per-thread state, such that the necessary checking can be done there. This wants properly testing on affected hardware. From Andrew's description it's also not clear whether this really is addressing that problem, or yet another one in this same area.