[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 03/21/2014 06:54 AM, Jan Beulich wrote:
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).

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

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.