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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.14 at 17:53, <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -540,7 +570,8 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t 
> msr_content)
>          }
>      }
>  
> -    if ((global_ctrl & *enabled_cntrs) || (core2_vpmu_cxt->ds_area != 0)  )
> +    if ((core2_vpmu_cxt->global_ctrl & *enabled_cntrs) ||
> +        (core2_vpmu_cxt->ds_area != 0)  )

Please fix coding style issues in code that you touch anyway ...

> @@ -570,13 +601,19 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, 
> uint64_t msr_content)
>                  inject_gp = 1;
>              break;
>          }
> -        if (inject_gp)
> -            hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +
> +        if (inject_gp) 
> +            inject_trap(v, TRAP_gp_fault);

... and don't make coding style issues even worse (trailing blank
being added here).

> @@ -868,8 +869,10 @@ void pv_cpuid(struct cpu_user_regs *regs)
>          __clear_bit(X86_FEATURE_TOPOEXT % 32, &c);
>          break;
>  
> +    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?

> @@ -885,6 +888,8 @@ void pv_cpuid(struct cpu_user_regs *regs)
>      }
>  
>   out:
> +    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.

> @@ -2515,7 +2520,14 @@ static int emulate_privileged_op(struct cpu_user_regs 
> *regs)
>              if ( v->arch.debugreg[7] & DR7_ACTIVE_MASK )
>                  wrmsrl(regs->_ecx, msr_content);
>              break;
> -
> +        case MSR_P6_PERFCTR0...MSR_P6_PERFCTR1:
> +        case MSR_P6_EVNTSEL0...MSR_P6_EVNTSEL1:
> +        case MSR_CORE_PERF_FIXED_CTR0...MSR_CORE_PERF_FIXED_CTR2:
> +        case MSR_CORE_PERF_FIXED_CTR_CTRL...MSR_CORE_PERF_GLOBAL_OVF_CTRL:
> +        case MSR_AMD_FAM15H_EVNTSEL0...MSR_AMD_FAM15H_PERFCTR5:
> +            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.

Jan


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