Re: [Xen-devel] [PATCH v1 2/5] x86/msr: introduce struct msr_vcpu_policy

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.08.17 at 12:34, <sergey.dyasli@xxxxxxxxxx> wrote:
> @@ -40,11 +44,15 @@ static void __init calculate_hvm_max_policy(void)
>          dp->plaform_info.available = true;
>          dp->plaform_info.cpuid_faulting = true;
>      }
> +
> +    /* 0x00000140  MSR_INTEL_MISC_FEATURES_ENABLES */
> +    vp->misc_features_enables.available = dp->plaform_info.available;
>  }
>  
>  static void __init calculate_pv_max_policy(void)
>  {
>      struct msr_domain_policy *dp = &pv_max_msr_domain_policy;
> +    struct msr_vcpu_policy *vp = &pv_max_msr_vcpu_policy;
>  
>      /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
>      if ( cpu_has_cpuid_faulting )
> @@ -52,6 +60,9 @@ static void __init calculate_pv_max_policy(void)
>          dp->plaform_info.available = true;
>          dp->plaform_info.cpuid_faulting = true;
>      }
> +
> +    /* 0x00000140  MSR_INTEL_MISC_FEATURES_ENABLES */
> +    vp->misc_features_enables.available = dp->plaform_info.available;
>  }

The similarity of the two changes makes me wonder whether
down the road it wouldn't be better to have a function doing
things which are uniform between HVM and PV.

> @@ -84,6 +95,28 @@ int init_domain_msr_policy(struct domain *d)
>      return 0;
>  }
>  
> +int init_vcpu_msr_policy(struct vcpu *v)
> +{
> +    struct domain *d = v->domain;
> +    struct msr_vcpu_policy *vp;
> +
> +    vp = xmalloc(struct msr_vcpu_policy);
> +
> +    if ( !vp )
> +        return -ENOMEM;
> +
> +    *vp = is_pv_domain(d) ? pv_max_msr_vcpu_policy :
> +                            hvm_max_msr_vcpu_policy;
> +
> +    /* See comment in intel_ctxt_switch_levelling() */
> +    if ( is_control_domain(d) )
> +        vp->misc_features_enables.available = false;

It's CPUID faulting that's meant to be suppressed, not the MSR's
presence.

> --- a/xen/include/asm-x86/msr.h
> +++ b/xen/include/asm-x86/msr.h
> @@ -212,8 +212,19 @@ struct msr_domain_policy
>      } plaform_info;
>  };
>  
> +/* MSR policy object for per-vCPU MSRs */
> +struct msr_vcpu_policy
> +{
> +    /* 0x00000140  MSR_INTEL_MISC_FEATURES_ENABLES */
> +    struct {
> +        bool available; /* This MSR is non-architectural */
> +        bool cpuid_faulting;

Here as well as in patch 1 I think down the road we want these
to be single-bit bitfields, to limit the growth of the structure.

In any event, none of the comments are meant to prevent the
series from going in as-is - in fact I'm preparing to commit it as
I'm going over the patches. Follow-up changes for some of the
items pointed out would be nice post-4.10.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel