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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.03.14 at 04:51, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 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).

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

Jan

> +        /*
> +         * 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;
>  
>      if ( idx > limit ) 
>          return 0;



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