Re: [Xen-devel] [PATCH v2 04/14] x86/cpuid: Handle more simple Intel leaves in guest_cpuid()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.01.17 at 12:35, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 24/01/17 11:20, Jan Beulich wrote:
>>>>> On 23.01.17 at 15:39, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> @@ -153,21 +158,31 @@ static void recalculate_xstate(struct cpuid_policy *p)
>>>  
>>>  /*
>>>   * Misc adjustments to the policy.  Mostly clobbering reserved fields and
>>> - * duplicating shared fields.
>>> + * duplicating shared fields.  Intentionally hidden fields are annotated.
>>>   */
>>>  static void recalculate_misc(struct cpuid_policy *p)
>>>  {
>>> +    p->basic.raw[0x8] = EMPTY_LEAF;
>>> +    p->basic.raw[0xc] = EMPTY_LEAF;
>>> +
>>>      p->extd.e1d &= ~CPUID_COMMON_1D_FEATURES;
>>>  
>>>      switch ( p->x86_vendor )
>>>      {
>>>      case X86_VENDOR_INTEL:
>>> +        p->basic.l2_nr_queries = 1; /* Fixed to 1 query. */
>>> +        p->basic.raw[0x3] = EMPTY_LEAF; /* PSN - always hidden. */
>>> +        p->basic.raw[0x9] = EMPTY_LEAF; /* DCA - always hidden. */
>> Hmm, for one this isn't very useful without also faking zero output
>> for the respective MSR read. And then I think this might still be
>> useful for pinned domains, so I'd view this as temporary state only
>> (same for the un-exposed CPUID bit), yet the comment makes me
>> assume this is intended to be permanent.
> 
> Well, thus far the toolstack has always explicitly zeroed this leaf. 
> From that point of view, it is no change.
> 
> The MSR side of things is a separate can of worms which is following
> this cpuid work on my todo list.
> 
> As for potential exposure, it should be fine to expose to
> non-migrateable domains, but I'd consider that a new feature (and far
> easier to implement once the default vs max cpuid_policy infrastructure
> is in place).
> 
>>
>>> @@ -694,8 +709,9 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, 
>>> struct cpuid_leaf *res)
>>>          break;
>>>  
>>>      case 0x0:
>>> -    case 0x7:
>>> -    case XSTATE_CPUID:
>>> +    case 0x2 ... 0x3:
>>> +    case 0x7 ... 0x9:
>>> +    case 0xc ... XSTATE_CPUID:
>> I can see how using a mix of literal numbers and constants ends up
>> ugly here. I think we have two options: Either use only numbers,
>> but then preferably include the constant(s) covered in a comment.
>> Or only use ranges both ends of which are literal numbers.
> 
> These lists are just temporary (and this one specifically is only to
> check I don't have duplicate case statements).  When the legacy path
> disappears (i.e. I fill in all the holes in that numbering),
> {pv,hvm}_cpuid() will disappear, and the similar list in guest_cpuid()
> will be dropped and become the default case.
> 
> I don't mind them staying as they are.  They won't be around for long.

Okay then.

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


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