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

[Jim Mattson]

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.