Re: [PATCH 2/3] ARM: KVM: Fake up performance counters a little more precisely.

[Rusty Russell <rusty@rustcorp.com.au>]

On Mon, 14 May 2012 18:49:40 -0400, Christoffer Dall <c.dall@virtualopensystems.com> wrote:
> On Thu, Mar 29, 2012 at 1:17 AM, Rusty Russell <rusty.russell@linaro.org> wrote:
> > Rather than just making all of c9 read-zero/write-discard, this changes
> > it to the explicit profiling registers we need.  This is a start for the
> > future implementation were we actually implement performance monitoring,
> > and makes sure we're not discarding important things.
> >
> > Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
> > ---
> >  arch/arm/kvm/emulate.c |   67 +++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 65 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
> > index aec1b6e..4bdab8f 100644
> > --- a/arch/arm/kvm/emulate.c
> > +++ b/arch/arm/kvm/emulate.c
> > @@ -238,6 +238,64 @@ static bool read_l2ctlr(struct kvm_vcpu *vcpu,
> >        return true;
> >  }
> >
> > +static bool read_pmcr(struct kvm_vcpu *vcpu,
> > +                     const struct coproc_params *p,
> > +                     unsigned long arg)
> > +{
> > +       u32 imp, idcode, num;
> > +
> > +       imp = (vcpu->arch.cp15[c0_MIDR] & 0xFF000000) >> 24;
> > +       idcode = (vcpu->arch.cp15[c0_MIDR] & 0x00000FF0) >> 4;
> 
> where does it say that idcode is always te same as MIDR?

Good point, it doesn't.  It is true for the Cortex A-15, at least.  This
would be more generically a switch statement on "what CPU are we" ioctl,
as I mentioned in the previous mail.

> > +       /* No counters. */
> > +       num = 0;
> > +
> > +       /* Other bits are at reset value. */
> 
> what's the point of writing anything below then? I would assume that
> you can have really weird behavior if you read something different
> from what you once wrote, shouldn't you read num from the vcpu value
> with the bit-filter below?

I think I meant that the other bits are 0 at reset.  But yes, some
should be read back as written.

This would mean saving what they actually wrote, which is not a bad
idea.

> > +/* FIXME: We ignore them enabling performance monitoring. */
> 
> if this is a FIXME, then how is it eventually going to be fixed?

I was thinking that eventually we implement performance monitoring?

> > +static bool write_pmcr(struct kvm_vcpu *vcpu,
> > +                      const struct coproc_params *p,
> > +                      unsigned long arg)
> > +{
> > +       u32 val = *vcpu_reg(vcpu, p->Rt1);
> > +
> > +       kvm_debug("pmcr write:%s%s%s%s%s%s\n",
> > +                 val & (1 << 5) ? " DP" : "",
> > +                 val & (1 << 4) ? " X" : "",
> > +                 val & (1 << 3) ? " D" : "",
> > +                 val & (1 << 2) ? " C" : "",
> > +                 val & (1 << 1) ? " P" : "",
> > +                 val & (1 << 0) ? " E" : "");
> > +       return true;
> > +}
> > +
> > +static bool read_pmcntenclr(struct kvm_vcpu *vcpu,
> > +                           const struct coproc_params *p,
> > +                           unsigned long arg)
> > +{
> > +       /* Cycle counter is off, there are no others. */
> > +       *vcpu_reg(vcpu, p->Rt1) = 0;
> > +       return true;
> > +}
> > +
> > +static bool write_pmcntenclr(struct kvm_vcpu *vcpu,
> > +                           const struct coproc_params *p,
> > +                           unsigned long arg)
> > +{
> > +       /* Writing a 1 means disable a counter.  That's cool. */
> > +       return true;
> > +}
> > +
> > +static bool write_pmintenclr(struct kvm_vcpu *vcpu,
> > +                            const struct coproc_params *p,
> > +                            unsigned long arg)
> > +{
> > +       /* Writing a 1 means disable an overflow interrupt.  That's cool. */
> > +       return true;
> > +}
> > +
> >  static bool access_cp15_reg(struct kvm_vcpu *vcpu,
> >                            const struct coproc_params *p,
> >                            unsigned long cp15_reg)
> > @@ -279,13 +337,18 @@ struct coproc_emulate {
> >  static const struct coproc_emulate coproc_emulate[] = {
> >        /*
> >         * L2CTLR access (guest wants to know #CPUs).
> > -        *
> > -        * FIXME: Hack Alert: Read zero as default case.
> >         */
> >        { CRn( 9), CRm( 0), Op1( 1), Op2( 2), is32,  READ,  read_l2ctlr},
> >        { CRn( 9), CRm(DF), Op1(DF), Op2(DF), is32,  WRITE, ignore_write},
> >        { CRn( 9), CRm(DF), Op1(DF), Op2(DF), is32,  READ,  read_zero},
> >
> > +       /* Guest reads/writes PMU, assuming there will be one. */
> > +       { CRn( 9), CRm(12), Op1( 0), Op2( 0), is32,  READ,  read_pmcr},
> > +       { CRn( 9), CRm(12), Op1( 0), Op2( 0), is32,  WRITE, write_pmcr},
> > +       { CRn( 9), CRm(12), Op1( 0), Op2( 2), is32,  READ,  read_pmcntenclr},
> > +       { CRn( 9), CRm(12), Op1( 0), Op2( 2), is32,  WRITE, write_pmcntenclr},
> > +       { CRn( 9), CRm(14), Op1( 0), Op2( 2), is32,  WRITE, write_pmintenclr},
> > +
> 
> won't all the DF versions above "eat" these calls? Should they just go
> away or are there still default cases to catch in which case the
> comment about reading zero should perhaps be modified to specifically
> mention these cases?

Posted in too much of a hurry.  Yes, the DFs to get removed.

I'll re-spin this, and re-test.

Thanks,
Rusty.