Re: [PATCH 27/27] x86/cpuid: Alter the legacy-path prototypes to match guest_cpuid()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 05/01/17 14:19, Jan Beulich wrote:
>>>> On 04.01.17 at 13:39, <andrew.cooper3@citrix.com> wrote:
>> Here and elsewhere, it is becomes very obvious that the PVH path using
>> pv_cpuid() is broken, as the guest_kernel_mode() check using
>> guest_cpu_user_regs() is erroneous.  I am tempted to just switch PVH onto the
>> HVM path, which won't make it any more broken than it currently is.
> Are you sure? There was a reason it had been done this way back then.

Oh yes, the same problem Roger is having with PVHv2.  Only the
pv_cpuid() path has logic to read from native in the case of the control
domain, for whom no policy is constructed.

This series lays a lot of groundwork to fixing the dom0 policy problem,
but wont be fully working for PVHv2 until I remove all of the legacy
path. (and that is at least the same quantity of work again, I reckon).

>
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -337,30 +337,26 @@ int init_domain_cpuid_policy(struct domain *d)
>>      return 0;
>>  }
>>  
>> -static void pv_cpuid(struct cpu_user_regs *regs)
>> +static void pv_cpuid(unsigned int leaf, unsigned int subleaf,
>> +                     struct cpuid_leaf *res)
>>  {
>> -    uint32_t leaf, subleaf, a, b, c, d;
>> +    const struct cpu_user_regs *regs = guest_cpu_user_regs();
> Please consider moving this into the !is_pvh_domain() scope,
> open coding the one access outside of that.
>
>> @@ -538,33 +534,33 @@ static void pv_cpuid(struct cpu_user_regs *regs)
>>                                    xstate_sizes[_XSTATE_HI_ZMM]);
>>              }
>>  
>> -            a = (uint32_t)xfeature_mask;
>> -            d = (uint32_t)(xfeature_mask >> 32);
>> -            c = xstate_size;
>> +            res->a = (uint32_t)xfeature_mask;
>> +            res->d = (uint32_t)(xfeature_mask >> 32);
>> +            res->c = xstate_size;
> Please consider at once dropping these pointless casts (also on the
> HVM side then).

I can do, but after this patch, I only ever expect to delete code from
these functions as more leaves move over to the new infrastructure.

>
>> @@ -945,27 +927,7 @@ void guest_cpuid(const struct vcpu *v, unsigned int leaf,
>>   legacy:
>>      /* {pv,hvm}_cpuid() have this expectation. */
>>      ASSERT(v == curr);
>> -
>> -    if ( is_pv_vcpu(v) || is_pvh_vcpu(v) )
>> -    {
>> -        struct cpu_user_regs regs = *guest_cpu_user_regs();
>> -
>> -        regs.rax = leaf;
>> -        regs.rcx = subleaf;
>> -
>> -        pv_cpuid(&regs);
>> -
>> -        res->a = regs._eax;
>> -        res->b = regs._ebx;
>> -        res->c = regs._ecx;
>> -        res->d = regs._edx;
>> -    }
>> -    else
>> -    {
>> -        res->c = subleaf;
>> -
>> -        hvm_cpuid(leaf, &res->a, &res->b, &res->c, &res->d);
>> -    }
>> +    (is_pv_vcpu(v) || is_pvh_vcpu(v) ? pv_cpuid : hvm_cpuid)(leaf, subleaf, res);
> Afaics as of patch 8 you have v->domain already latched into a
> local variable, so please use is_*_domain() here.

Actually, I will switch it around to is_hvm_domain() which is shorter,
and will require no modification when PVHv1 finally gets excised.

~Andrew

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