Re: [Xen-devel] [PATCH 2 of 2] V2 vpmu: Add the BTS extension

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.03.12 at 14:13, Dietmar Hahn <dietmar.hahn@xxxxxxxxxxxxxx> wrote:
> Add the BTS functionality to the existing vpmu implementation for Intel 
> cpus.
> 
> Signed-off-by: Dietmar Hahn <dietmar.hahn@xxxxxxxxxxxxxx>

I was about to get this committed, but I noticed some inconsistency:

> @@ -425,8 +449,26 @@ static int core2_vpmu_do_wrmsr(unsigned 
>                       "which is not supported.\n");
>          return 1;
>      case MSR_IA32_DS_AREA:
> -        gdprintk(XENLOG_WARNING, "Guest setting of DTS is ignored.\n");
> -        return 1;
> +        if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) )
> +        {
> +            if (!msr_content || !is_canonical_address(msr_content))
> +            {
> +                gdprintk(XENLOG_WARNING, "Illegal address for IA32_DS_AREA: 
> 0x%lx\n",
> +                                                            msr_content);
> +                vmx_inject_hw_exception(TRAP_gp_fault, 0);
> +                return 1;
> +            }
> +            else
> +            {
> +                core2_vpmu_cxt->pmu_enable->ds_area_enable = msr_content ? 1 
> : 0;

In the if() above you already exclude msr_content == 0, so why
the conditional here? Or is it the other way around - the conditional
here is correct, and the respective part of the if() clause above is
wrong?

As I did already make a few other cleanup adjustments to your patch,
please just let me know.

Jan

> +                break;
> +            }
> +        }
> +        else
> +        {
> +            gdprintk(XENLOG_WARNING, "Guest setting of DTS is ignored.\n");
> +            return 1;
> +        }
>      case MSR_CORE_PERF_GLOBAL_CTRL:
>          global_ctrl = msr_content;
>          for ( i = 0; i < core2_get_pmc_count(); i++ )



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