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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.17 at 13:39, <andrew.cooper3@xxxxxxxxxx> 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.

> --- 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).

> @@ -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.

In any event
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


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