| Message ID | 87r4wcx9yj.fsf@rustcorp.com.au |
|---|---|
| State | New |
| Headers | show |
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? > + /* 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? > + *vcpu_reg(vcpu, p->Rt1) = (imp << 24) | (idcode << 16) | (num << 11); > + return true; > +} > + > +/* FIXME: We ignore them enabling performance monitoring. */ if this is a FIXME, then how is it eventually going to be fixed? > +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? > /* > * These CRn == 10 entries may not need to exist - if we can > * ignore guest attempts to tamper with TLB lockdowns then it Thanks, Christoffer
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.
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; + /* No counters. */ + num = 0; + + /* Other bits are at reset value. */ + *vcpu_reg(vcpu, p->Rt1) = (imp << 24) | (idcode << 16) | (num << 11); + return true; +} + +/* FIXME: We ignore them enabling 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}, + /* * These CRn == 10 entries may not need to exist - if we can * ignore guest attempts to tamper with TLB lockdowns then it
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(-)