Re: [Xen-devel] [PATCH v2 1/5] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.03.18 at 19:58, <andrew.cooper3@xxxxxxxxxx> wrote:
> @@ -554,13 +551,11 @@ static void update_reference_tsc(struct domain *d, 
> bool_t initialize)
>      put_page_and_type(page);
>  }
>  
> -int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
> +int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
>  {
> -    struct vcpu *v = current;
>      struct domain *d = v->domain;
>  
> -    if ( !is_viridian_domain(d) )
> -        return 0;
> +    ASSERT(is_viridian_domain(d));
>  
>      switch ( idx )
>      {
> @@ -615,7 +610,7 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
>  
>      case HV_X64_MSR_REFERENCE_TSC:
>          if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
> -            return 0;
> +            goto gp_fault;
>  
>          perfc_incr(mshv_wrmsr_tsc_msr);
>          d->arch.hvm_domain.viridian.reference_tsc.raw = val;
> @@ -655,14 +650,15 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
>      }
>  
>      default:
> -        if ( idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX )
> -            gprintk(XENLOG_WARNING, "write to unimplemented MSR %#x\n",
> -                    idx);
> -
> -        return 0;
> +        gdprintk(XENLOG_WARNING,
> +                 "Write %016"PRIx64" to unimplemented MSR %#x\n", val, idx);
> +        goto gp_fault;
>      }
>  
> -    return 1;
> +    return X86EMUL_OKAY;
> +
> + gp_fault:
> +    return X86EMUL_EXCEPTION;
>  }

Still these ugly goto-s to just a single return statement. But well,
Paul is happy with them ...

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -141,9 +141,11 @@ int init_vcpu_msr_policy(struct vcpu *v)
>  
>  int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>  {
> -    const struct cpuid_policy *cp = v->domain->arch.cpuid;
> -    const struct msr_domain_policy *dp = v->domain->arch.msr;
> +    const struct domain *d = v->domain;
> +    const struct cpuid_policy *cp = d->arch.cpuid;
> +    const struct msr_domain_policy *dp = d->arch.msr;
>      const struct msr_vcpu_policy *vp = v->arch.msr;
> +    int ret = X86EMUL_OKAY;
>  
>      switch ( msr )
>      {
> @@ -175,11 +177,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>          break;
>  
> +    case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 
> 1:

The "HYPERVISOR" in here starts to make sense in the next patch,
but its combination with "VIRIDIAN" is still suspicious. I'll comment
on this further for the next patch.

> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -540,4 +540,8 @@
>  #define MSR_PKGC9_IRTL                       0x00000634
>  #define MSR_PKGC10_IRTL                      0x00000635
>  
> +/* Hypervisor leaves in the 0x4xxxxxxx range. */
> +#define MSR_HYPERVISOR_START            0x40000000
> +#define NR_VIRIDIAN_MSRS                0x00000200

Is "leaves" really an appropriate term for MSRs?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel