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

Re: [Xen-devel] [PATCH v6 12/19] x86/VPMU: Add support for PMU register handling on PV guests

On 05/22/2014 10:50 AM, Jan Beulich wrote:

@@ -868,8 +869,10 @@ void pv_cpuid(struct cpu_user_regs *regs)
          __clear_bit(X86_FEATURE_TOPOEXT % 32, &c);
+ case 0x0000000a: /* Architectural Performance Monitor Features (Intel) */
+        break;
      case 0x00000005: /* MONITOR/MWAIT */
-    case 0x0000000a: /* Architectural Performance Monitor Features */
Is there actually anything wrong with leaving this as it was, i.e.
clearing a, b, c, and d?

Since AMD's PMU-related CPUID is 0x80000001 (and is not used currently anyway as there is no do_cpuid op in AMD's vpmu) I think I'll just move vpmu_do_cpuid() into 0x0000000a case.

@@ -885,6 +888,8 @@ void pv_cpuid(struct cpu_user_regs *regs)
+    vpmu_do_cpuid(regs->eax, &a, &b, &c, &d);
This seems incomplete without passing in regs->ecx. And without at
least a brief comment it also looks misplaced at the first glance.

vpmu_cpuid() doesn't use indexed CPUIDs (but I can see how ecx could have been added for consistency if I kept the call where it is now.)

@@ -2515,7 +2520,14 @@ static int emulate_privileged_op(struct cpu_user_regs 
              if ( v->arch.debugreg[7] & DR7_ACTIVE_MASK )
                  wrmsrl(regs->_ecx, msr_content);
+        case MSR_P6_PERFCTR0...MSR_P6_PERFCTR1:
+        case MSR_P6_EVNTSEL0...MSR_P6_EVNTSEL1:
+            if ( !vpmu_do_wrmsr(regs->ecx, msr_content) )
+                goto invalid;
+            break;
Can you really handle both Intel and AMD ones as a group here,
without consideration whose CPU you're actually running on? I
think for forward compatibility you should be making the call only
for Intel MSRs on Intel CPUs, and respectively for AMD.

The vendor-specific paths are taken in vpmu_do_wrmsr() (and rdmsr). Not sure if splitting this into two cases would be better but if you feel it adds to clarity I can do this.


Xen-devel mailing list



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