Re: [Xen-devel] [PATCH for-4.10] x86: Avoid corruption on migrate for vcpus using CPUID Faulting

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.11.17 at 19:15, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1286,7 +1286,7 @@ long arch_do_domctl(
>          struct xen_domctl_vcpu_msrs *vmsrs = &domctl->u.vcpu_msrs;
>          struct xen_domctl_vcpu_msr msr;
>          struct vcpu *v;
> -        uint32_t nr_msrs = 0;
> +        uint32_t nr_msrs = 1;

I think this ought to be ARRAY_SIZE(<whatever>), to avoid a
possible update of one but not the other. The price to pay (moving
the array outwards) is reasonable imo. However, ...

> @@ -1311,10 +1311,49 @@ long arch_do_domctl(
>                  vmsrs->msr_count = nr_msrs;
>              else
>              {
> +                static const uint32_t msrs[] = {
> +                    MSR_INTEL_MISC_FEATURES_ENABLES,

... is this really a non-optional MSR? In particular,
calculate_hvm_max_policy() ties it to INTEL_PLATFORM_INFO,
which in turn is being tied to running on an Intel CPU.
calculate_pv_max_policy() is even more picky. I think you want
to introduce a function in msr.c complementing guest_rdmsr() /
guest_wrmsr(), similar to HVM's .init_msr() hook.

> +                };
> +                unsigned int j;
> +
>                  i = 0;
>  
>                  vcpu_pause(v);
>  
> +                for ( j = 0; j < ARRAY_SIZE(msrs); ++j )
> +                {
> +                    uint64_t val;
> +                    int rc = guest_rdmsr(v, msrs[j], &val);
> +
> +                    /*
> +                     * It is the programmers responsibility to ensure that
> +                     * msrs[] contain generally-readable MSRs.
> +                     * X86EMUL_EXCEPTION here implies a missing feature.
> +                     */
> +                    if ( rc == X86EMUL_EXCEPTION )
> +                        continue;
> +
> +                    if ( rc != X86EMUL_OKAY )
> +                    {
> +                        ASSERT_UNREACHABLE();
> +                        ret = -ENXIO;
> +                        break;
> +                    }
> +
> +                    if ( !val )
> +                        continue; /* Skip empty MSRs. */
> +
> +                    if ( i < vmsrs->msr_count && !ret )
> +                    {
> +                        msr.index = msrs[j];
> +                        msr.reserved = 0;
> +                        msr.value = val;
> +                        if ( copy_to_guest_offset(vmsrs->msrs, i, &msr, 1) )
> +                            ret = -EFAULT;
> +                    }
> +                    ++i;
> +                }
> +
>                  if ( boot_cpu_has(X86_FEATURE_DBEXT) )
>                  {
>                      unsigned int j;

With the j you introduce in the next outer scope, I think this one
should  be removed.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1322,11 +1322,14 @@ static int hvm_load_cpu_xsave_states(struct domain 
> *d, hvm_domain_context_t *h)
>  }
>  
>  #define HVM_CPU_MSR_SIZE(cnt) offsetof(struct hvm_msr, msr[cnt])
> -static unsigned int __read_mostly msr_count_max;
> +static unsigned int __read_mostly msr_count_max = 1;
>  
>  static int hvm_save_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
>  {
>      struct vcpu *v;
> +    static const uint32_t msrs[] = {
> +        MSR_INTEL_MISC_FEATURES_ENABLES,
> +    };

Same comments as above.

Jan


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