On Mon, 14 May 2012 18:49:40 -0400, Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Mar 29, 2012 at 1:17 AM, Rusty Russell <rusty.russell@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
