Re: [PATCH v3 1/4] xen/libxc: Allow changes to hypervisor CPUID leaf from config file

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

On 03/14/2014 04:09 AM, Jan Beulich wrote:
>>>> On 13.03.14 at 19:08, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> @@ -43,22 +45,29 @@ static int hypervisor_is_64bit(xc_interface *xch)
>>   static void cpuid(const unsigned int *input, unsigned int *regs)
>>   {
>>       unsigned int count = (input[1] == XEN_CPUID_INPUT_UNUSED) ? 0 : input[1];
>> +    uint8_t is_hyp = IS_HYPERVISOR_LEAF(input[0]);
>>   #ifdef __i386__
>>       /* Use the stack to avoid reg constraint failures with some gcc flags */
>>       asm (
>>           "push %%ebx; push %%edx\n\t"
>> -        "cpuid\n\t"
>> +        "testb $0xff,%5\n\t"
>> +        "jz 1f\n\t"
>> +        XEN_EMULATE_PREFIX
>> +        "1: cpuid\n\t"
>>           "mov %%ebx,4(%4)\n\t"
>>           "mov %%edx,12(%4)\n\t"
>>           "pop %%edx; pop %%ebx\n\t"
>>           : "=a" (regs[0]), "=c" (regs[2])
>> -        : "0" (input[0]), "1" (count), "S" (regs)
>> +        : "0" (input[0]), "1" (count), "S" (regs), "m" (is_hyp)
> All inputs must be in registers here, since memory references might
> use %esp and hence be off by 2 stack slots due to the pushes/pops
> surrounding the actual operation. Since you evaluate the flag prior
> to the CPUID, using "db" as constraint would seem possible here.

gcc for some reason rejects "b" as inconsistent (but "d" works fine).


>> @@ -555,6 +564,15 @@ static int xc_cpuid_policy(
>>   {
>>       xc_dominfo_t        info;
>>   
>> +    if ( IS_HYPERVISOR_LEAF(input[0]) )
>> +    {
>> +        /* Only leaf 1 can be modified */
>> +        if ( input[0] == 0x40000000 )
>> +            return 0;
>> +        else
>> +            return -EACCES;
> And I'm still worried about altering this in uncontrolled ways: No
> good can result from allowing ecx/edx/ebx to be modified, and
> improperly modifying the eax value (namely putting in place
> wrong upper bits, the more that those aren't statically
> determined) won't help much either. IOW it should really only
> be the low 8 bits of eax that can be overridden, and it shouldn't
> be possible to clear these 8 bits.

I'll add mask array to xc_cpuid_policy (and probably other policy
routines that it calls) that will specify bits that are permitted to be
overwritten.

-boris