| Message ID | 1454656456-11640-7-git-send-email-zhaoshenglong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 05, 2016 at 03:14:01PM +0800, Shannon Zhao wrote: > From: Shannon Zhao <shannon.zhao@linaro.org> > > Add access handler which gets host value of PMCEID0 or PMCEID1 when > guest access these registers. Writing action to PMCEID0 or PMCEID1 is > UNDEFINED. > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > --- > arch/arm64/kvm/sys_regs.c | 29 +++++++++++++++++++++++++---- > 1 file changed, 25 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index fc60041..06257e2 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -492,6 +492,27 @@ static bool access_pmselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > return true; > } > > +static bool access_pmceid(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > + const struct sys_reg_desc *r) > +{ > + u64 pmceid; > + > + if (!kvm_arm_pmu_v3_ready(vcpu)) > + return trap_raz_wi(vcpu, p, r); > + > + if (p->is_write) > + return false; Isn't it really a BUG_ON(p->is_write) ? Presumably a guest write to these registers will raise an undefined exception in EL0/1 and we don't get here by any other path than the trap handler, do we? Otherwise looks good to me. Thanks, -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016/2/8 20:09, Christoffer Dall wrote: > On Fri, Feb 05, 2016 at 03:14:01PM +0800, Shannon Zhao wrote: >> >From: Shannon Zhao<shannon.zhao@linaro.org> >> > >> >Add access handler which gets host value of PMCEID0 or PMCEID1 when >> >guest access these registers. Writing action to PMCEID0 or PMCEID1 is >> >UNDEFINED. >> > >> >Signed-off-by: Shannon Zhao<shannon.zhao@linaro.org> >> >--- >> > arch/arm64/kvm/sys_regs.c | 29 +++++++++++++++++++++++++---- >> > 1 file changed, 25 insertions(+), 4 deletions(-) >> > >> >diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >> >index fc60041..06257e2 100644 >> >--- a/arch/arm64/kvm/sys_regs.c >> >+++ b/arch/arm64/kvm/sys_regs.c >> >@@ -492,6 +492,27 @@ static bool access_pmselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, >> > return true; >> > } >> > >> >+static bool access_pmceid(struct kvm_vcpu *vcpu, struct sys_reg_params *p, >> >+ const struct sys_reg_desc *r) >> >+{ >> >+ u64 pmceid; >> >+ >> >+ if (!kvm_arm_pmu_v3_ready(vcpu)) >> >+ return trap_raz_wi(vcpu, p, r); >> >+ >> >+ if (p->is_write) >> >+ return false; > Isn't it really a BUG_ON(p->is_write) ? > > Presumably a guest write to these registers will raise an undefined > exception in EL0/1 and we don't get here by any other path than the trap > handler, do we? Yeah, for EL1, it shouldn't get here. But for EL0, to support the function of PMUSERENR, we firstly trap the access to EL2, then according to the real value of PMUSERENR to decide whether inject an UND to EL1. Thanks,
On 20 February 2016 at 13:15, Shannon Zhao <shannon.zhao@linaro.org> wrote: > > > On 2016/2/8 20:09, Christoffer Dall wrote: >> Isn't it really a BUG_ON(p->is_write) ? >> >> Presumably a guest write to these registers will raise an undefined >> exception in EL0/1 and we don't get here by any other path than the trap >> handler, do we? > > Yeah, for EL1, it shouldn't get here. But for EL0, to support the function > of PMUSERENR, we firstly trap the access to EL2, then according to the real > value of PMUSERENR to decide whether inject an UND to EL1. I thought the PMUSERENR check took priority over trap-to-EL2 checks in the architecture; so if this is an EL0 access and you get to EL2 at all then you know that PMUSERENR is set to permit EL0 reads. Similarly, UNDEF because of write-to-always-RO register has priority over trap-to-EL2, so I think Christoffer is right and you should never be able to get here for a write. (This should be easy to write a test program for if you're unsure.) thanks -- PMM -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016/2/20 21:30, Peter Maydell wrote: > On 20 February 2016 at 13:15, Shannon Zhao<shannon.zhao@linaro.org> wrote: >> > >> > >> >On 2016/2/8 20:09, Christoffer Dall wrote: >>> >>Isn't it really a BUG_ON(p->is_write) ? >>> >> >>> >>Presumably a guest write to these registers will raise an undefined >>> >>exception in EL0/1 and we don't get here by any other path than the trap >>> >>handler, do we? >> > >> >Yeah, for EL1, it shouldn't get here. But for EL0, to support the function >> >of PMUSERENR, we firstly trap the access to EL2, then according to the real >> >value of PMUSERENR to decide whether inject an UND to EL1. > I thought the PMUSERENR check took priority over trap-to-EL2 checks > in the architecture; so if this is an EL0 access and you get to EL2 > at all then you know that PMUSERENR is set to permit EL0 reads. > > Similarly, UNDEF because of write-to-always-RO register has > priority over trap-to-EL2, so I think Christoffer is right > and you should never be able to get here for a write. > (This should be easy to write a test program for if you're unsure.) Thanks, I'll check this.
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index fc60041..06257e2 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -492,6 +492,27 @@ static bool access_pmselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, return true; } +static bool access_pmceid(struct kvm_vcpu *vcpu, struct sys_reg_params *p, + const struct sys_reg_desc *r) +{ + u64 pmceid; + + if (!kvm_arm_pmu_v3_ready(vcpu)) + return trap_raz_wi(vcpu, p, r); + + if (p->is_write) + return false; + + if (!(p->Op2 & 1)) + asm volatile("mrs %0, pmceid0_el0\n" : "=r" (pmceid)); + else + asm volatile("mrs %0, pmceid1_el0\n" : "=r" (pmceid)); + + p->regval = pmceid; + + return true; +} + /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */ #define DBG_BCR_BVR_WCR_WVR_EL1(n) \ /* DBGBVRn_EL1 */ \ @@ -694,10 +715,10 @@ static const struct sys_reg_desc sys_reg_descs[] = { access_pmselr, reset_unknown, PMSELR_EL0 }, /* PMCEID0_EL0 */ { Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1100), Op2(0b110), - trap_raz_wi }, + access_pmceid }, /* PMCEID1_EL0 */ { Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1100), Op2(0b111), - trap_raz_wi }, + access_pmceid }, /* PMCCNTR_EL0 */ { Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1101), Op2(0b000), trap_raz_wi }, @@ -943,8 +964,8 @@ static const struct sys_reg_desc cp15_regs[] = { { Op1( 0), CRn( 9), CRm(12), Op2( 2), trap_raz_wi }, { Op1( 0), CRn( 9), CRm(12), Op2( 3), trap_raz_wi }, { Op1( 0), CRn( 9), CRm(12), Op2( 5), access_pmselr }, - { Op1( 0), CRn( 9), CRm(12), Op2( 6), trap_raz_wi }, - { Op1( 0), CRn( 9), CRm(12), Op2( 7), trap_raz_wi }, + { Op1( 0), CRn( 9), CRm(12), Op2( 6), access_pmceid }, + { Op1( 0), CRn( 9), CRm(12), Op2( 7), access_pmceid }, { Op1( 0), CRn( 9), CRm(13), Op2( 0), trap_raz_wi }, { Op1( 0), CRn( 9), CRm(13), Op2( 1), trap_raz_wi }, { Op1( 0), CRn( 9), CRm(13), Op2( 2), trap_raz_wi },