Re: [Xen-devel] [PATCH 07/27] x86/cpuid: Recalculate a domains CPUID policy when appropriate

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/01/17 16:04, Jan Beulich wrote:
>>>> On 04.01.17 at 16:33, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 04/01/17 15:01, Jan Beulich wrote:
>>>>>> On 04.01.17 at 13:39, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> +void recalculate_cpuid_policy(struct domain *d)
>>>> +{
>>>> +    struct cpuid_policy *p = d->arch.cpuid;
>>>> +    const struct cpuid_policy *max =
>>>> +        is_pv_domain(d) ? &pv_max_policy : &hvm_max_policy;
>>>> +    uint32_t fs[FSCAPINTS], max_fs[FSCAPINTS];
>>>> +    unsigned int i;
>>>> +
>>>> +    cpuid_policy_to_featureset(p, fs);
>>>> +    memcpy(max_fs, max->fs, sizeof(max_fs));
>>>> +
>>>> +    /* Allow a toolstack to possibly select ITSC... */
>>>> +    if ( cpu_has_itsc )
>>>> +        __set_bit(X86_FEATURE_ITSC, max_fs);
>>> This special casing calls for some explanation in the commit message
>>> (or the comment here).
>> Ah - this logic is all copied from the current dynamic adjustment we
>> make in {pv,hvm}_cpuid().  This ITSC one is expressed differently, but
>> it should hopefully be obvious in the context of patches 18 and 19.
> Well, before making the comment I did check. The clearing of
> the flag is a copy of existing code, but the setting of it here
> isn't afaics.

The relevant hunk is:

     case 0x80000007:
-        d &= (pv_featureset[FEATURESET_e7d] |
-              (host_featureset[FEATURESET_e7d] &
cpufeat_mask(X86_FEATURE_ITSC)));
+        d = p->extd.e7d;
         break;

which was introduced as an ITSC bugfix in c/s a5adcee740d.  ITSC is by
default hidden in the featureset offered to toolstacks at the moment,
but we need to cope with the toolstack explicitly setting ITSC after
setting nomigrate.

This will eventually be fixed by having ITSC set in the max_policy, but
clear in the default_policy, but that is also mixed in with the
toolstack changes.

>
>>>> +    /* ... but hide ITSC in the common case. */
>>>> +    if ( !d->disable_migrate && !d->arch.vtsc )
>>>> +        __clear_bit(X86_FEATURE_ITSC, fs);
>>> The 32-bit PV logic could easily move below here afaics, reducing
>>> the distance between the two parts of the comment.
>>>
>>> Also this requires adjustment of the policy by (the caller of)
>>> tsc_set_info().
>> And also XEN_DOMCTL_set_disable_migrate.
>>
>> Currently the various toolstacks issues these hypercalls in the correct
>> order, so I was planning to ignore these edge cases until the toolstack
>> side work (see below).
> Let's not do that - it'll be some time until that other work lands,
> I assume, and introducing (further) dependencies on tool stacks
> to do things in the right order is quite bad imo.

This is code which hasn't changed in years.  But if you insist, then I
will see about best to do an x86-only change to the common code.

>
>>>>  static void update_domain_cpuid_info(struct domain *d,
>>>>                                       const xen_domctl_cpuid_t *ctl)
>>>>  {
>>>> +    struct cpuid_policy *p = d->arch.cpuid;
>>>> +    struct cpuid_leaf leaf = { ctl->eax, ctl->ebx, ctl->ecx, ctl->edx };
>>>> +
>>>> +    if ( ctl->input[0] < ARRAY_SIZE(p->basic.raw) )
>>>> +    {
>>>> +        if ( ctl->input[0] == 7 )
>>>> +        {
>>>> +            if ( ctl->input[1] < ARRAY_SIZE(p->feat.raw) )
>>>> +                p->feat.raw[ctl->input[1]] = leaf;
>>>> +        }
>>>> +        else if ( ctl->input[0] == 0xd )
>>>> +        {
>>>> +            if ( ctl->input[1] < ARRAY_SIZE(p->xstate.raw) )
>>>> +                p->xstate.raw[ctl->input[1]] = leaf;
>>>> +        }
>>>> +        else
>>>> +            p->basic.raw[ctl->input[0]] = leaf;
>>>> +    }
>>>> +    else if ( (ctl->input[0] - 0x80000000) < ARRAY_SIZE(p->extd.raw) )
>>>> +        p->extd.raw[ctl->input[0] - 0x80000000] = leaf;
>>> These checks against ARRAY_SIZE() worry me - wouldn't we better
>>> refuse any attempts to set values not representable in the policy?
>> We can't do that yet, without toolstack side changes.  Currently the
>> toolstack can lodge any values it wishes, and all we do is ignore them,
>> which can be arbitrary information from a cpuid= clause.
> Hmm, do we really _ignore_ them in all cases (rather than handing
> them through to guests)? If so, that should indeed be good enough
> for now.

Any arbitrary values get can get inserted into the cpuids[] array but,
given your fairly-recent change to check max_leaf, we don't guarantee to
hand the values to a guest.

~Andrew

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