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

[Dietmar Hahn <dietmar.hahn@xxxxxxxxxxxxxx>]

Am Dienstag 13 März 2012, 14:22:45 schrieb Jan Beulich:
> >>> 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.

You are right. I'am always impressed how you guys looked at the code to see
such things ;-)
OK I think it's remained from my experiments with valid addresses. As in my
tests 0x0 can be used to write to the MSR_IA32_DS_AREA the check of
!msr_content in the if (... can be removed so it's possible to write 0x0 into
the msr.
But this worked only on boxes I was able to test ...
Thanks!

Dietmar.


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

-- 
Company details: http://ts.fujitsu.com/imprint.html

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