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.
