Re: [Xen-devel] [PATCH 1/2] x86/xstate: Use the CPUID policy in valid_xcr0(), rather than allowing all features

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.07.18 at 18:36, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 18/07/18 17:16, Jan Beulich wrote:
>>>>> On 18.07.18 at 18:05, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 18/07/18 15:48, Jan Beulich wrote:
>>>>>>> On 18.07.18 at 14:30, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> Backporting notes: This is safe in the restore case, but only back as far 
>>>>> as
>>>>> the introduction of cpuid_policy infrastructure.  Before then, a restore
>>>>> boolean needs to be pumbed down as well, and used to select between the
>>>>> hardware maximum value and calls to {hvm,pv}_cpuid() to find the
>>>>> toolstack-chosen level.
>>>> So can I take this to mean that for ...
>>>>
>>>>> --- a/xen/arch/x86/domctl.c
>>>>> +++ b/xen/arch/x86/domctl.c
>>>>> @@ -1170,7 +1170,7 @@ long arch_do_domctl(
>>>>>              if ( _xcr0_accum )
>>>>>              {
>>>>>                  if ( evc->size >= PV_XSAVE_HDR_SIZE + 
>>>>> XSTATE_AREA_MIN_SIZE )
>>>>> -                    ret = validate_xstate(_xcr0, _xcr0_accum,
>>>>> +                    ret = validate_xstate(d, _xcr0, _xcr0_accum,
>>>> ... this and ...
>>>>
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -1254,7 +1254,7 @@ static int hvm_load_cpu_xsave_states(struct domain 
>>>>> *d, 
> 
>>> hvm_domain_context_t *h)
>>>>>      ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur];
>>>>>      h->cur += desc->length;
>>>>>  
>>>>> -    err = validate_xstate(ctxt->xcr0, ctxt->xcr0_accum,
>>>>> +    err = validate_xstate(d, ctxt->xcr0, ctxt->xcr0_accum,
>>>> ... this there's no ordering issue on master (i.e. policy arrives before
>>>> register state)?
>>> You are correct.  In the case that the policy hasn't arrived on migrate,
>>> d->arch.cpuid is the applicable max policy.
>> Hmm, you say max policy, but the plan is to have max >= default.
>> Isn't it the default policy that's active before the respective migration
>> data arrives? In which case - won't this go wrong for domains with
>> a policy allowing more than default? Or are you/we assuming that
>> for the particular features here there's not going to be any way to
>> expose more than the default (IOW default == max for all of
>> XSTATE) for the foreseeable future (in which case this assumption
>> should be spelled out in the description I think)?
> 
> The dependency chain for fixing this is:
> 
> 1) Get get/set policy working (still partially blocked behind getting
> the marshalling series in.  Will also contain some necessary gross
> hacks).  max == default at this point.
> 2) Rearrange the order of data in the migration stream (requires
> get/serialise on the source side, set or fabricate on the destination side).
> 3) Properly split default <= max in Xen.
> 4) Teach the toolstack to be able explicitly opt in to
> nested-virt/itsc/vpmu/(whatever else I've forgotten) by feeding them in
> from the max policy during construction (addresses the gross hacks in
> point 1).
> 5) Switch Xen to dup()ing the default policy rather than the max.

Okay, so the above is merely one detail to keep in mind for this
entire process (at or before step 3 in particular).

Jan



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