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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/01/17 08:24, Jan Beulich wrote:
>>>> 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.

Does this concern still stand in light of max_leaf handling in patches
21 and 22?

~Andrew

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