Re: [PATCH v6 1/4] xen/libxc: Allow changing max number of hypervisor cpuid leaves

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 03/21/2014 06:54 AM, Jan Beulich wrote:
>>>> On 21.03.14 at 04:51, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> --- a/tools/libxc/xc_cpuid_x86.c
>> +++ b/tools/libxc/xc_cpuid_x86.c
>> @@ -555,6 +555,17 @@ static int xc_cpuid_policy(
>>   {
>>       xc_dominfo_t        info;
>>   
>> +    /*
>> +     * For hypervisor leaves (0x4000XXXX) only 0x40000x00.EAX[7:0] bits (max
> 0x4000xx00.EAX[7:0] (also in the description).
>
>> +     * number of leaves) can be set by user. Hypervisor will enforce this so
>> +     * all other bits are don't-care and we can set them to zero.
>> +     */
>> +    if ( (input[0] & 0xffff0000) == 0x40000000 )
>> +    {
>> +        regs[0] = regs[1] = regs[2] = regs[3] = 0;
>> +        return 0;
>> +    }
> Except that x in the config won't work anymore (not that I consider
> this particularly useful here, but you never know what people come
> up with - read: I'm not sure this needs dealing with).

Yes, I did think about 'x' but didn't feel that it's worth adding back
pv cpuid (which would address this issue) since specifying 'x' only
for some bits in the number of leaves field seemed somewhat strange.

>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -677,15 +677,21 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
>>       struct domain *d = current->domain;
>>       /* Optionally shift out of the way of Viridian architectural leaves. */
>>       uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
>> -    uint32_t limit;
>> +    uint32_t limit, dummy;
>>   
>>       idx -= base;
>> +    if ( idx > 3 )
>> +        return 0; /* Avoid unnecessary pass through domain_cpuid() */
>>   
>> -    /*
>> -     * Some Solaris PV drivers fail if max > base + 2. Help them out by
>> -     * hiding the PVRDTSCP leaf if PVRDTSCP is disabled.
>> -     */
>> -    limit = (d->arch.tsc_mode < TSC_MODE_PVRDTSCP) ? 2 : 3;
>> +    /* Number of leaves may be user-specified */
>> +    domain_cpuid(d, base, 0, &limit, &dummy, &dummy, &dummy);
>> +    limit &= 0xff;
>> +    if ( (limit < 2) || (limit > 3) )
> So all of the sudden you also enforce a lower limit? That doesn't
> seem to belong here (and at a first glance it doesn't seem to be
> right either).
>
> There are two things you want enforce
> - limit never becoming zero due to user override (at once taking
>    care of the no override case)

You think not having leaf 1 is something we should allow? I can see why
2 perhaps may be omitted.

> - limit never getting set higher than the hypervisor internal limit
> but that's unrelated to the Solaris specifics here, so should be
> handled separately (and in fact there's no point in having the
> fake revert in the next patch then: do the right thing here right
> away, as what the next patch does isn't really a revert anymore
> with the changes here in place).

Yes, I thought that reverting Solaris fix as a standalone patch may be too
much work for no benefit. I'll merge it into the first patch.

-boris