Re: [PATCH v1 7/7] x86/vmx: switch IPT MSRs on vmentry/vmexit

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 16/06/2020 16:24, Michał Leszczyński wrote:
> Enable IPT when entering the VM and disable it on vmexit.
> Register state is persisted using vCPU ipt_state structure.
>
> Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl>
> ---
>  xen/arch/x86/hvm/vmx/vmx.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 97104c319e..01d9a7b584 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3698,6 +3698,15 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>      __vmread(GUEST_RSP,    &regs->rsp);
>      __vmread(GUEST_RFLAGS, &regs->rflags);
>  
> +    if ( unlikely(v->arch.hvm.vmx.ipt_state) )
> +    {
> +        wrmsrl(MSR_IA32_RTIT_CTL, 0);
> +        smp_rmb();
> +
> +        rdmsrl(MSR_IA32_RTIT_STATUS, v->arch.hvm.vmx.ipt_state->status);
> +        rdmsrl(MSR_IA32_RTIT_OUTPUT_MASK, v->arch.hvm.vmx.ipt_state->output_mask);
> +    }
> +
>      hvm_invalidate_regs_fields(regs);
>  
>      if ( paging_mode_hap(v->domain) )
> @@ -4497,6 +4506,23 @@ bool vmx_vmenter_helper(const struct cpu_user_regs *regs)
>      }
>  
>   out:
> +    if ( unlikely(curr->arch.hvm.vmx.ipt_state) )
> +    {
> +        wrmsrl(MSR_IA32_RTIT_CTL, 0);
> +
> +        if (curr->arch.hvm.vmx.ipt_state->ctl)
> +        {
> +            wrmsrl(MSR_IA32_RTIT_OUTPUT_BASE, curr->arch.hvm.vmx.ipt_state->output_base);
> +            wrmsrl(MSR_IA32_RTIT_OUTPUT_MASK, curr->arch.hvm.vmx.ipt_state->output_mask);
> +            wrmsrl(MSR_IA32_RTIT_STATUS, curr->arch.hvm.vmx.ipt_state->status);
> +
> +            // MSR_IA32_RTIT_CTL is context-switched manually instead of being
> +            // stored inside VMCS, as of Q2'20 only the most recent processors
> +            // support such field in VMCS
> +            wrmsrl(MSR_IA32_RTIT_CTL, curr->arch.hvm.vmx.ipt_state->ctl);
> +        }
> +    }
> +

Some notes to help with v2.

RTIT_CTL wants managing by MSR load/save list.  See how
vmx_update_guest_efer() manages MSR_EFER for the Gen1 hardware case,
because RTIT_CTL is very similar until we get to IceLake hardware and
have a GUEST_RTIT_CTRL field.

With RTIT_CTL handled by MSR load/save list, we are now certain that
TraceEn is always clear in hypervisor context, so there's no need to
explicitly zero it before playing with other MSRs.


You don't need to save/restore the values in vmentry/exit, because that
is very expensive an unnecessary.  Instead, you can use
vmx_ctxt_switch_{from,to}() which is based on when the vcpu is switched
in/out of context.

Specifically, from your current code, it looks to be safe to leave
RTIT_STATUS/OUTPUT_MASK dirty in hardware across multiple
vmentries/exits while the vcpu is in context.

~Andrew