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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/11/17 09:53, Jan Beulich wrote:
>>>> 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.

Ok.

>  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.

Whether an MSR should be migrated depends on whether the guest is able
to use it.

I'm specifically looking to remove the concept of optional MSRs to avoid
duplicating the MSR policy logic, and risking it being different.  In
reality, what this means is that the migration code will be expected to
cope with the union of all possible MSRs, and only actually get a subset
to put into the stream.

Also, this MSR specifically is about to become common, but that is
post-4.10 at this point.

>
>> +                };
>> +                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.

Oh, indeed.  I hadn't even noticed this one here.

~Andrew

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