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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.17 at 18:37, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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:
>>>>> +    /* ... 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.

The tsc_set_info() would likely be in x86 specific code, but the
set_disable_migrate would, as you say, presumably want handling
in/from common code. So unless this would turn out to be a rather
costly change, I'd indeed prefer if you adjusted these.

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

"we don't guarantee" != "we guarantee not to"

But my main point here is that a domain's cpuid= may specify a
higher than default max leaf, and I think going forward we ought
to still return all zero for those leaves in that case, or else the
overall spirit of white listing would get violated.

Jan


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