| Message ID | 1446186123-11548-6-git-send-email-zhaoshenglong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/30/2015 02:21 AM, Shannon Zhao wrote: > From: Shannon Zhao <shannon.zhao@linaro.org> > > Since the reset value of PMSELR_EL0 is UNKNOWN, use reset_unknown for > its reset handler. As it doesn't need to deal with the acsessing action Nit: accessing > specially, it uses default case to emulate writing and reading PMSELR > register. > > Add a helper for CP15 registers reset to UNKNOWN. > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
On Fri, 30 Oct 2015 14:21:47 +0800 Shannon Zhao <zhaoshenglong@huawei.com> wrote: > From: Shannon Zhao <shannon.zhao@linaro.org> > > Since the reset value of PMSELR_EL0 is UNKNOWN, use reset_unknown for > its reset handler. As it doesn't need to deal with the acsessing action > specially, it uses default case to emulate writing and reading PMSELR > register. > > Add a helper for CP15 registers reset to UNKNOWN. > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > --- > arch/arm64/kvm/sys_regs.c | 5 +++-- > arch/arm64/kvm/sys_regs.h | 8 ++++++++ > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 5b591d6..35d232e 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -707,7 +707,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { > trap_raz_wi }, > /* PMSELR_EL0 */ > { Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1100), Op2(0b101), > - trap_raz_wi }, > + access_pmu_regs, reset_unknown, PMSELR_EL0 }, > /* PMCEID0_EL0 */ > { Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1100), Op2(0b110), > trap_raz_wi }, > @@ -998,7 +998,8 @@ static const struct sys_reg_desc cp15_regs[] = { > { Op1( 0), CRn( 9), CRm(12), Op2( 1), trap_raz_wi }, > { 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), trap_raz_wi }, > + { Op1( 0), CRn( 9), CRm(12), Op2( 5), access_pmu_cp15_regs, > + reset_unknown_cp15, c9_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(13), Op2( 0), trap_raz_wi }, > diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h > index eaa324e..8afeff7 100644 > --- a/arch/arm64/kvm/sys_regs.h > +++ b/arch/arm64/kvm/sys_regs.h > @@ -110,6 +110,14 @@ static inline void reset_unknown(struct kvm_vcpu *vcpu, > vcpu_sys_reg(vcpu, r->reg) = 0x1de7ec7edbadc0deULL; > } > > +static inline void reset_unknown_cp15(struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *r) > +{ > + BUG_ON(!r->reg); > + BUG_ON(r->reg >= NR_COPRO_REGS); > + vcpu_cp15(vcpu, r->reg) = 0xdecafbad; > +} > + > static inline void reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > { > BUG_ON(!r->reg); Same remark here as the one I made earlier. I'm pretty sure we don't call any CP15 reset because they are all shared with their 64bit counterparts. The same thing goes for the whole series. Thanks, M.
Hi Marc, On 2015/12/1 1:56, Marc Zyngier wrote: > Same remark here as the one I made earlier. I'm pretty sure we don't > call any CP15 reset because they are all shared with their 64bit > counterparts. The same thing goes for the whole series. Ok, I see. But within the 64bit reset function, it needs to update the 32bit register value, right? Since when accessing these 32bit registers, it uses the offset c9_PMXXXX. Thanks,
On 01/12/15 01:51, Shannon Zhao wrote: > Hi Marc, > > On 2015/12/1 1:56, Marc Zyngier wrote: >> Same remark here as the one I made earlier. I'm pretty sure we don't >> call any CP15 reset because they are all shared with their 64bit >> counterparts. The same thing goes for the whole series. > Ok, I see. But within the 64bit reset function, it needs to update the > 32bit register value, right? Since when accessing these 32bit registers, > it uses the offset c9_PMXXXX. It shouldn't, because the 64bit and 32bit share the same storage. From your own patch: +/* Performance Monitors*/ +#define c9_PMCR (PMCR_EL0 * 2) +#define c9_PMOVSSET (PMOVSSET_EL0 * 2) +#define c9_PMOVSCLR (PMOVSCLR_EL0 * 2) +#define c9_PMCCNTR (PMCCNTR_EL0 * 2) +#define c9_PMSELR (PMSELR_EL0 * 2) +#define c9_PMCEID0 (PMCEID0_EL0 * 2) +#define c9_PMCEID1 (PMCEID1_EL0 * 2) +#define c9_PMXEVCNTR (PMXEVCNTR_EL0 * 2) +#define c9_PMXEVTYPER (PMXEVTYPER_EL0 * 2) +#define c9_PMCNTENSET (PMCNTENSET_EL0 * 2) +#define c9_PMCNTENCLR (PMCNTENCLR_EL0 * 2) +#define c9_PMINTENSET (PMINTENSET_EL1 * 2) +#define c9_PMINTENCLR (PMINTENCLR_EL1 * 2) +#define c9_PMUSERENR (PMUSERENR_EL0 * 2) +#define c9_PMSWINC (PMSWINC_EL0 * 2) These are indexes in the copro array: struct kvm_cpu_context { struct kvm_regs gp_regs; union { u64 sys_regs[NR_SYS_REGS]; u32 copro[NR_COPRO_REGS]; }; }; which is in a union with the sys_reg array. So anything that affects one affects the other because: - there is only one state in the physical CPU, no matter which mode you're in, - the guest EL1 is either 32bit or 64bit, and never changes over time. Hope this helps, M.
On 2015/12/1 16:49, Marc Zyngier wrote: > On 01/12/15 01:51, Shannon Zhao wrote: >> Hi Marc, >> >> On 2015/12/1 1:56, Marc Zyngier wrote: >>> Same remark here as the one I made earlier. I'm pretty sure we don't >>> call any CP15 reset because they are all shared with their 64bit >>> counterparts. The same thing goes for the whole series. >> Ok, I see. But within the 64bit reset function, it needs to update the >> 32bit register value, right? Since when accessing these 32bit registers, >> it uses the offset c9_PMXXXX. > > It shouldn't, because the 64bit and 32bit share the same storage. From > your own patch: > > +/* Performance Monitors*/ > +#define c9_PMCR (PMCR_EL0 * 2) > +#define c9_PMOVSSET (PMOVSSET_EL0 * 2) > +#define c9_PMOVSCLR (PMOVSCLR_EL0 * 2) > +#define c9_PMCCNTR (PMCCNTR_EL0 * 2) > +#define c9_PMSELR (PMSELR_EL0 * 2) > +#define c9_PMCEID0 (PMCEID0_EL0 * 2) > +#define c9_PMCEID1 (PMCEID1_EL0 * 2) > +#define c9_PMXEVCNTR (PMXEVCNTR_EL0 * 2) > +#define c9_PMXEVTYPER (PMXEVTYPER_EL0 * 2) > +#define c9_PMCNTENSET (PMCNTENSET_EL0 * 2) > +#define c9_PMCNTENCLR (PMCNTENCLR_EL0 * 2) > +#define c9_PMINTENSET (PMINTENSET_EL1 * 2) > +#define c9_PMINTENCLR (PMINTENCLR_EL1 * 2) > +#define c9_PMUSERENR (PMUSERENR_EL0 * 2) > +#define c9_PMSWINC (PMSWINC_EL0 * 2) > > These are indexes in the copro array: > > struct kvm_cpu_context { > struct kvm_regs gp_regs; > union { > u64 sys_regs[NR_SYS_REGS]; > u32 copro[NR_COPRO_REGS]; > }; > }; > > which is in a union with the sys_reg array. So anything that affects one > affects the other because: > - there is only one state in the physical CPU, no matter which mode > you're in, > - the guest EL1 is either 32bit or 64bit, and never changes over time. > > Hope this helps, > Ok, I see. Thanks for the explanation. :)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 5b591d6..35d232e 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -707,7 +707,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { trap_raz_wi }, /* PMSELR_EL0 */ { Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1100), Op2(0b101), - trap_raz_wi }, + access_pmu_regs, reset_unknown, PMSELR_EL0 }, /* PMCEID0_EL0 */ { Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1100), Op2(0b110), trap_raz_wi }, @@ -998,7 +998,8 @@ static const struct sys_reg_desc cp15_regs[] = { { Op1( 0), CRn( 9), CRm(12), Op2( 1), trap_raz_wi }, { 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), trap_raz_wi }, + { Op1( 0), CRn( 9), CRm(12), Op2( 5), access_pmu_cp15_regs, + reset_unknown_cp15, c9_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(13), Op2( 0), trap_raz_wi }, diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h index eaa324e..8afeff7 100644 --- a/arch/arm64/kvm/sys_regs.h +++ b/arch/arm64/kvm/sys_regs.h @@ -110,6 +110,14 @@ static inline void reset_unknown(struct kvm_vcpu *vcpu, vcpu_sys_reg(vcpu, r->reg) = 0x1de7ec7edbadc0deULL; } +static inline void reset_unknown_cp15(struct kvm_vcpu *vcpu, + const struct sys_reg_desc *r) +{ + BUG_ON(!r->reg); + BUG_ON(r->reg >= NR_COPRO_REGS); + vcpu_cp15(vcpu, r->reg) = 0xdecafbad; +} + static inline void reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { BUG_ON(!r->reg);