Re: [PATCH v4] KVM: VMX: do not disable interception for MSR_IA32_SPEC_CTRL on eIBRS

[Jim Mattson]

On Wed, May 31, 2023 at 11:17 AM Jon Kohler <jon@xxxxxxxxxxx> wrote:
>
>
>
> > On May 31, 2023, at 2:08 PM, Jim Mattson <jmattson@xxxxxxxxxx> wrote:
> >
> > On Wed, May 31, 2023 at 10:22 AM Waiman Long <longman@xxxxxxxxxx> wrote:
> >>
> >> On 5/31/23 13:13, Jon Kohler wrote:
> >>>
> >>>> On May 31, 2023, at 1:02 PM, Waiman Long <longman@xxxxxxxxxx> wrote:
> >>>>
> >>>> On 5/31/23 10:41, Jon Kohler wrote:
> >>>>> Avoid expensive rdmsr on every VM Exit for MSR_IA32_SPEC_CTRL on
> >>>>> eIBRS enabled systems iff the guest only sets IA32_SPEC_CTRL[0] (IBRS)
> >>>>> and not [1] (STIBP) or [2] (SSBD) by not disabling interception in
> >>>>> the MSR bitmap. Note: this logic is only for eIBRS, as Intel's guidance
> >>>>> has long been that eIBRS only needs to be set once, so most guests with
> >>>>> eIBRS awareness should behave nicely. We would not want to accidentally
> >>>>> regress misbehaving guests on pre-eIBRS systems, who might be spamming
> >>>>> IBRS MSR without the hypervisor being able to see it today.
> >>>>>
> >>>>> eIBRS enabled guests using just IBRS will only write SPEC_CTRL MSR
> >>>>> once or twice per vCPU on boot, so it is far better to take those
> >>>>> VM exits on boot than having to read and save this msr on every
> >>>>> single VM exit forever. This outcome was suggested on Andrea's commit
> >>>>> 2f46993d83ff ("x86: change default to spec_store_bypass_disable=prctl spectre_v2_user=prctl")
> >>>>> however, since interception is still unilaterally disabled, the rdmsr
> >>>>> tax is still there even after that commit.
> >>>>>
> >>>>> This is a significant win for eIBRS enabled systems as this rdmsr
> >>>>> accounts for roughly ~50% of time for vmx_vcpu_run() as observed
> >>>>> by perf top disassembly, and is in the critical path for all
> >>>>> VM-Exits, including fastpath exits.
> >>>>>
> >>>>> Opportunistically update comments for both MSR_IA32_SPEC_CTRL and
> >>>>> MSR_IA32_PRED_CMD to make it clear how L1 vs L2 handling works.
> >>>>>
> >>>>> Fixes: 2f46993d83ff ("x86: change default to spec_store_bypass_disable=prctl spectre_v2_user=prctl")
> >>>>> Signed-off-by: Jon Kohler <jon@xxxxxxxxxxx>
> >>>>> Cc: Sean Christopherson <seanjc@xxxxxxxxxx>
> >>>>> Cc: Andrea Arcangeli <aarcange@xxxxxxxxxx>
> >>>>> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> >>>>> Cc: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> >>>>> Cc: Waiman Long <longman@xxxxxxxxxx>
> >>>>> ---
> >>>>> v1
> >>>>>  - https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_20220512174427.3608-2D1-2Djon-40nutanix.com_&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=NGPRGGo37mQiSXgHKm5rCQ&m=PT1QjB8Lk_a3baDOwHBfedQG67HsVDmOdmcWHlr5PrT8WyuS9e6PfHF5JxLxD0zw&s=jNnloZQgh0KG-n36uwVC0dJTmokvqsQdYQCWYI8hVvM&e= v1 -> v2:
> >>>>>  - https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_20220520195303.58692-2D1-2Djon-40nutanix.com_&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=NGPRGGo37mQiSXgHKm5rCQ&m=PT1QjB8Lk_a3baDOwHBfedQG67HsVDmOdmcWHlr5PrT8WyuS9e6PfHF5JxLxD0zw&s=Rwi5NoHwaezlmzzLiGGCuI6QHuGQZ1BVK2hs6-SZvzU&e=   - Addressed comments on approach from Sean.
> >>>>> v2 -> v3:
> >>>>>  - https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_kvm_20220520204115.67580-2D1-2Djon-40nutanix.com_&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=NGPRGGo37mQiSXgHKm5rCQ&m=PT1QjB8Lk_a3baDOwHBfedQG67HsVDmOdmcWHlr5PrT8WyuS9e6PfHF5JxLxD0zw&s=R2Ykxdv-DyeVGLWd8_pLpu43zEsnWzpyvvBPEZ9lz-Y&e=   - Addressed comments on approach from Sean.
> >>>>> v3 -> v4:
> >>>>>  - Fixed inline code comments from Sean.
> >>>>>
> >>>>>  arch/x86/kvm/vmx/vmx.c | 35 ++++++++++++++++++++++++-----------
> >>>>>  1 file changed, 24 insertions(+), 11 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> >>>>> index 44fb619803b8..5e643ac897bc 100644
> >>>>> --- a/arch/x86/kvm/vmx/vmx.c
> >>>>> +++ b/arch/x86/kvm/vmx/vmx.c
> >>>>> @@ -2260,20 +2260,33 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >>>>>                    return 1;
> >>>>>
> >>>>>            vmx->spec_ctrl = data;
> >>>>> -           if (!data)
> >>>>> +
> >>>>> +           /*
> >>>>> +            * Disable interception on the first non-zero write, except if
> >>>>> +            * eIBRS is advertised to the guest and the guest is enabling
> >>>>> +            * _only_ IBRS.  On eIBRS systems, kernels typically set IBRS
> >>>>> +            * once at boot and never touch it post-boot.  All other bits,
> >>>>> +            * and IBRS on non-eIBRS systems, are often set on a per-task
> >>>>> +            * basis, i.e. change frequently, so the benefit of avoiding
> >>>>> +            * VM-exits during guest context switches outweighs the cost of
> >>>>> +            * RDMSR on every VM-Exit to save the guest's value.
> >>>>> +            */
> >>>>> +           if (!data ||
> >>>>> +               (data == SPEC_CTRL_IBRS &&
> >>>>> +                (vcpu->arch.arch_capabilities & ARCH_CAP_IBRS_ALL)))
> >>>>>                    break;
> >>>>>
> >>>>>            /*
> >>>>> -            * For non-nested:
> >>>>> -            * When it's written (to non-zero) for the first time, pass
> >>>>> -            * it through.
> >>>>> -            *
> >>>>> -            * For nested:
> >>>>> -            * The handling of the MSR bitmap for L2 guests is done in
> >>>>> -            * nested_vmx_prepare_msr_bitmap. We should not touch the
> >>>>> -            * vmcs02.msr_bitmap here since it gets completely overwritten
> >>>>> -            * in the merging. We update the vmcs01 here for L1 as well
> >>>>> -            * since it will end up touching the MSR anyway now.
> >>>>> +            * Update vmcs01.msr_bitmap even if L2 is active, i.e. disable
> >>>>> +            * interception for the vCPU on the first write regardless of
> >>>>> +            * whether the WRMSR came from L1 or L2.  vmcs02's bitmap is a
> >>>>> +            * combination of vmcs01 and vmcs12 bitmaps, and will be
> >>>>> +            * recomputed by nested_vmx_prepare_msr_bitmap() on the next
> >>>>> +            * nested VM-Enter.  Note, this does mean that future WRMSRs
> >>>>> +            * from L2 will be intercepted until the next nested VM-Exit if
> >>>>> +            * L2 was the first to write, but L1 exposing the MSR to L2
> >>>>> +            * without first writing it is unlikely and not worth the
> >>>>> +            * extra bit of complexity.
> >>>>>             */
> >>>>>            vmx_disable_intercept_for_msr(vcpu,
> >>>>>                                          MSR_IA32_SPEC_CTRL,
> >>>> I have 2 comments.
> >>>>
> >>>> 1) Besides the IBRS, STIBP & SSBD bits, the SPEC_CTRL MSR may have the RRSBA_DIS_S bit set in the future. I am not aware of any current Intel processors having this capability yet, but a future Intel processor may have this and the above patch will have to be modified accordingly. It looks like that the RRSBA_DIS_S bit will be set once.
> >>> Agreed. Once that becomes pubic with future processors, this code can be fixed up in a fairly trivial manner. I don’t have any access to said future processors, so I’d like to keep it as-is today rather than project it out too far. Is that ok?
> >> That is certainly OK. I am just raising a question here.
> >
> > How difficult would it be to do a back of the envelope cost/benefit
> > analysis, rather than relying on heuristics based on today's typical
> > guest behavior?
> >
> > Say that it's a minimum of 1000 cycles to intercept this WRMSR. The
> > tradeoff is the cost of a RDMSR on every VM-exit. How long does a
> > RDMSR take these days? On the order of 50 cycles? So, if the guest
> > consistently writes IA32_SPEC_CTRL more often than once every 20
> > VM-exits, it's better not to intercept it.
> >
> > Most of the bookkeeping could be handled in the WRMSR(IA32_SPEC_CTRL)
> > intercept. The only overhead we'd incur on every VM-exit would be the
> > cost of incrementing a VM-exit counter.
> >
> > It's a bit more complicated, but it directly addresses the issue, and
> > it's more future-proof.
>
> Yea, I thought about it. One one hand, simplicity is king and on the other
> hand, not having to think about this again is nice too.
>
> The challenge in my mind is that on setups where this truly is static, we’d
> be taking some incremental amount of memory to keep the counter around,
> just to have the same outcome each time. Doesn’t feel right (to me) unless that is
> also used for “other” stuff as some sort of general purpose/common counter.

If you're feeling mean, there's plenty of wasted space where you could
put the counter. For instance, we still allocate an entire page for
every VMCS, don't we?

> RE Cost: I can’t put my finger on it, but I swear that RDMSR for *this*
> specific MSR is more expensive than any other RDMSR I’ve come across
> for run-of-the-mill random MSRs. I flipped thru the SDM and the mitigations
> documentation, and it only ever mentions that there is a notable cost to
> do WRMSR IA32_SPEC_CTRL, but nothing about the RDMSR side.
>
> If anyone happens to know from an Intel-internals perspective, I’d be quite
> interested to know why it just “feels” so darn costly. i.e. is the proc also doing
> special things under the covers, similar to what the processor does on
> writes to this one?

What do you mean by "feels"? Have you measured it?