Re: [PATCH v3 8/8] KVM:X86: Add user-space read/write interface for CET MSRs.

[Sean Christopherson]

On Mon, Feb 25, 2019 at 09:27:16PM +0800, Yang Weijiang wrote:
> The Guest MSRs are stored in fpu storage area, they are
> operated by XSAVES/XRSTORS, so use kvm_load_guest_fpu
> to restore them is a convenient way to let KVM access
> them. After finish operation, need to restore Host MSR
> contents by kvm_put_guest_fpu.
> 
> Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>
> ---
>  arch/x86/kvm/x86.c | 46 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a0f8b71b2132..a4bdbef3a712 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -75,6 +75,8 @@
>  
>  #define MAX_IO_MSRS 256
>  #define KVM_MAX_MCE_BANKS 32
> +#define MAX_GUEST_CET_MSRS 5
> +
>  u64 __read_mostly kvm_mce_cap_supported = MCG_CTL_P | MCG_SER_P;
>  EXPORT_SYMBOL_GPL(kvm_mce_cap_supported);
>  
> @@ -214,6 +216,8 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
>  u64 __read_mostly host_xcr0;
>  
>  static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt);
> +static void kvm_load_guest_fpu(struct kvm_vcpu *vcpu);
> +static void kvm_put_guest_fpu(struct kvm_vcpu *vcpu);
>  
>  static inline void kvm_async_pf_hash_reset(struct kvm_vcpu *vcpu)
>  {
> @@ -2889,21 +2893,57 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  }
>  EXPORT_SYMBOL_GPL(kvm_get_msr_common);
>  
> +static int do_cet_msrs(struct kvm_vcpu *vcpu, int entry_num,
> +		       struct kvm_msr_entry *entries, bool read)
> +{
> +	int i = entry_num;
> +	int j = MAX_GUEST_CET_MSRS;
> +	bool has_cet;
> +
> +	has_cet = guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) |
> +		  guest_cpuid_has(vcpu, X86_FEATURE_IBT);
> +	/*
> +	 * Guest CET MSRs are saved by XSAVES, so need to restore
> +	 * them first, then read out or update the contents and
> +	 * restore Host ones.
> +	 */
> +	if (has_cet) {
> +		kvm_load_guest_fpu(vcpu);
> +
> +		if (read) {
> +			for (j = 0; j < MAX_GUEST_CET_MSRS; j++, i++)
> +				rdmsrl(entries[i].index, entries[i].data);
> +		} else {
> +			for (j = 0; j < MAX_GUEST_CET_MSRS; j++, i++)
> +				wrmsrl(entries[i].index, entries[i].data);
> +		}
> +
> +		kvm_put_guest_fpu(vcpu);
> +	}
> +	return j;
> +}
>  /*
>   * Read or write a bunch of msrs. All parameters are kernel addresses.
>   *
>   * @return number of msrs set successfully.
>   */
>  static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs,
> -		    struct kvm_msr_entry *entries,
> +		    struct kvm_msr_entry *entries, bool read,
>  		    int (*do_msr)(struct kvm_vcpu *vcpu,
>  				  unsigned index, u64 *data))
>  {
>  	int i;
>  
> -	for (i = 0; i < msrs->nmsrs; ++i)
> +	for (i = 0; i < msrs->nmsrs; ++i) {
> +		/* If it comes to CET related MSRs, read them together. */
> +		if (entries[i].index == MSR_IA32_U_CET) {
> +			i += do_cet_msrs(vcpu, i, entries, read) - 1;

This wrong, not to mention horribly buggy.  The correct way to handle
MSRs that are switched via the VMCS is to read/write the VMCS in
vmx_{get,set}_msr() as needed, e.g. vmcs_writel(GUEST_GS_BASE, data).

> +			continue;
> +		}
> +
>  		if (do_msr(vcpu, entries[i].index, &entries[i].data))
>  			break;
> +	}
>  
>  	return i;
>  }
> @@ -2938,7 +2978,7 @@ static int msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs __user *user_msrs,
>  		goto out;
>  	}
>  
> -	r = n = __msr_io(vcpu, &msrs, entries, do_msr);
> +	r = n = __msr_io(vcpu, &msrs, entries, !!writeback, do_msr);
>  	if (r < 0)
>  		goto out_free;
>  
> -- 
> 2.17.1
>