On Mon, 14 May 2012 18:49:40 -0400, Christoffer Dall <c.dall at virtualopensystems.com> wrote:
> On Thu, Mar 29, 2012 at 1:17 AM, Rusty Russell <rusty.russell at 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 at 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.
