| Message ID | 1441961715-11688-5-git-send-email-zhaoshenglong@huawei.com |
|---|---|
| State | New |
| Headers | show |
On 11/09/15 09:54, Shannon Zhao wrote: > From: Shannon Zhao <shannon.zhao@linaro.org> > > Add reset handler which gets host value of PMCR_EL0 and make writable > bits architecturally UNKNOWN. Add a common access handler for PMU > registers which emulates writing and reading register and add emulation > for PMCR. > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > --- > arch/arm64/kvm/sys_regs.c | 76 +++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 74 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index c370b40..db1be44 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -33,6 +33,7 @@ > #include <asm/kvm_emulate.h> > #include <asm/kvm_host.h> > #include <asm/kvm_mmu.h> > +#include <asm/pmu.h> > > #include <trace/events/kvm.h> > > @@ -236,6 +237,48 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > vcpu_sys_reg(vcpu, MPIDR_EL1) = (1ULL << 31) | mpidr; > } > > +static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > +{ > + u32 pmcr; > + > + asm volatile("mrs %0, pmcr_el0\n" : "=r" (pmcr)); > + /* Writable bits of PMCR_EL0 (ARMV8_PMCR_MASK) is reset to UNKNOWN*/ > + if (!vcpu_mode_is_32bit(vcpu)) > + vcpu_sys_reg(vcpu, r->reg) = (pmcr & ~ARMV8_PMCR_MASK) > + | (ARMV8_PMCR_MASK & 0xdecafbad); > + else > + vcpu_cp15(vcpu, r->reg) = (pmcr & ~ARMV8_PMCR_MASK) > + | (ARMV8_PMCR_MASK & 0xdecafbad); I have some concerns about blindly reusing the top bits of the host's PMCR_EL0 register, specially when it comes to the PMCR_EL0.N. Given that we're fully emulating the PMU, shouldn't we simply define how many counters we're emulating? > +} > + > +/* PMU registers accessor. */ > +static bool access_pmu_regs(struct kvm_vcpu *vcpu, > + const struct sys_reg_params *p, > + const struct sys_reg_desc *r) > +{ > + unsigned long val; > + > + if (p->is_write) { > + switch (r->reg) { > + case PMCR_EL0: { > + /* Only update writeable bits of PMCR */ > + val = vcpu_sys_reg(vcpu, r->reg); > + val &= ~ARMV8_PMCR_MASK; > + val |= *vcpu_reg(vcpu, p->Rt) & ARMV8_PMCR_MASK; > + vcpu_sys_reg(vcpu, r->reg) = val; > + break; > + } > + default: > + vcpu_sys_reg(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt); > + break; > + } > + } else { > + *vcpu_reg(vcpu, p->Rt) = vcpu_sys_reg(vcpu, r->reg); > + } > + > + 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 */ \ > @@ -427,7 +470,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { > > /* PMCR_EL0 */ > { Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1100), Op2(0b000), > - trap_raz_wi }, > + access_pmu_regs, reset_pmcr, PMCR_EL0, }, > /* PMCNTENSET_EL0 */ > { Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1100), Op2(0b001), > trap_raz_wi }, > @@ -632,6 +675,34 @@ static const struct sys_reg_desc cp14_64_regs[] = { > { Op1( 0), CRm( 2), .access = trap_raz_wi }, > }; > > +/* PMU CP15 registers accessor. */ > +static bool access_pmu_cp15_regs(struct kvm_vcpu *vcpu, > + const struct sys_reg_params *p, > + const struct sys_reg_desc *r) > +{ > + unsigned long val; > + > + if (p->is_write) { > + switch (r->reg) { > + case c9_PMCR: { > + /* Only update writeable bits of PMCR */ > + val = vcpu_cp15(vcpu, r->reg); > + val &= ~ARMV8_PMCR_MASK; > + val |= *vcpu_reg(vcpu, p->Rt) & ARMV8_PMCR_MASK; > + vcpu_cp15(vcpu, r->reg) = val; > + break; > + } > + default: > + vcpu_cp15(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt); > + break; > + } > + } else { > + *vcpu_reg(vcpu, p->Rt) = vcpu_cp15(vcpu, r->reg); > + } > + > + return true; > +} > + > /* > * Trapped cp15 registers. TTBR0/TTBR1 get a double encoding, > * depending on the way they are accessed (as a 32bit or a 64bit > @@ -660,7 +731,8 @@ static const struct sys_reg_desc cp15_regs[] = { > { Op1( 0), CRn( 7), CRm(14), Op2( 2), access_dcsw }, > > /* PMU */ > - { Op1( 0), CRn( 9), CRm(12), Op2( 0), trap_raz_wi }, > + { Op1( 0), CRn( 9), CRm(12), Op2( 0), access_pmu_cp15_regs, > + reset_pmcr, c9_PMCR }, > { 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 }, >
On 2015/9/11 18:07, Marc Zyngier wrote: > On 11/09/15 09:54, Shannon Zhao wrote: >> > From: Shannon Zhao <shannon.zhao@linaro.org> >> > >> > Add reset handler which gets host value of PMCR_EL0 and make writable >> > bits architecturally UNKNOWN. Add a common access handler for PMU >> > registers which emulates writing and reading register and add emulation >> > for PMCR. >> > >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> >> > --- >> > arch/arm64/kvm/sys_regs.c | 76 +++++++++++++++++++++++++++++++++++++++++++++-- >> > 1 file changed, 74 insertions(+), 2 deletions(-) >> > >> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >> > index c370b40..db1be44 100644 >> > --- a/arch/arm64/kvm/sys_regs.c >> > +++ b/arch/arm64/kvm/sys_regs.c >> > @@ -33,6 +33,7 @@ >> > #include <asm/kvm_emulate.h> >> > #include <asm/kvm_host.h> >> > #include <asm/kvm_mmu.h> >> > +#include <asm/pmu.h> >> > >> > #include <trace/events/kvm.h> >> > >> > @@ -236,6 +237,48 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) >> > vcpu_sys_reg(vcpu, MPIDR_EL1) = (1ULL << 31) | mpidr; >> > } >> > >> > +static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) >> > +{ >> > + u32 pmcr; >> > + >> > + asm volatile("mrs %0, pmcr_el0\n" : "=r" (pmcr)); >> > + /* Writable bits of PMCR_EL0 (ARMV8_PMCR_MASK) is reset to UNKNOWN*/ >> > + if (!vcpu_mode_is_32bit(vcpu)) >> > + vcpu_sys_reg(vcpu, r->reg) = (pmcr & ~ARMV8_PMCR_MASK) >> > + | (ARMV8_PMCR_MASK & 0xdecafbad); >> > + else >> > + vcpu_cp15(vcpu, r->reg) = (pmcr & ~ARMV8_PMCR_MASK) >> > + | (ARMV8_PMCR_MASK & 0xdecafbad); > I have some concerns about blindly reusing the top bits of the host's > PMCR_EL0 register, specially when it comes to the PMCR_EL0.N. Given that > we're fully emulating the PMU, shouldn't we simply define how many > counters we're emulating? > But how many counters should we define? And what does this definition based on? The only gist I think is the number of counters on host. And what's the reason to define less or more than PMCR_EL0.N? I didn't find one. So I choose to be consistent with host. Thanks,
On 14/09/15 04:14, Shannon Zhao wrote: > > > On 2015/9/11 18:07, Marc Zyngier wrote: >> On 11/09/15 09:54, Shannon Zhao wrote: >>>> From: Shannon Zhao <shannon.zhao@linaro.org> >>>> >>>> Add reset handler which gets host value of PMCR_EL0 and make writable >>>> bits architecturally UNKNOWN. Add a common access handler for PMU >>>> registers which emulates writing and reading register and add emulation >>>> for PMCR. >>>> >>>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> >>>> --- >>>> arch/arm64/kvm/sys_regs.c | 76 +++++++++++++++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 74 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >>>> index c370b40..db1be44 100644 >>>> --- a/arch/arm64/kvm/sys_regs.c >>>> +++ b/arch/arm64/kvm/sys_regs.c >>>> @@ -33,6 +33,7 @@ >>>> #include <asm/kvm_emulate.h> >>>> #include <asm/kvm_host.h> >>>> #include <asm/kvm_mmu.h> >>>> +#include <asm/pmu.h> >>>> >>>> #include <trace/events/kvm.h> >>>> >>>> @@ -236,6 +237,48 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) >>>> vcpu_sys_reg(vcpu, MPIDR_EL1) = (1ULL << 31) | mpidr; >>>> } >>>> >>>> +static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) >>>> +{ >>>> + u32 pmcr; >>>> + >>>> + asm volatile("mrs %0, pmcr_el0\n" : "=r" (pmcr)); >>>> + /* Writable bits of PMCR_EL0 (ARMV8_PMCR_MASK) is reset to UNKNOWN*/ >>>> + if (!vcpu_mode_is_32bit(vcpu)) >>>> + vcpu_sys_reg(vcpu, r->reg) = (pmcr & ~ARMV8_PMCR_MASK) >>>> + | (ARMV8_PMCR_MASK & 0xdecafbad); >>>> + else >>>> + vcpu_cp15(vcpu, r->reg) = (pmcr & ~ARMV8_PMCR_MASK) >>>> + | (ARMV8_PMCR_MASK & 0xdecafbad); >> I have some concerns about blindly reusing the top bits of the host's >> PMCR_EL0 register, specially when it comes to the PMCR_EL0.N. Given that >> we're fully emulating the PMU, shouldn't we simply define how many >> counters we're emulating? >> > > But how many counters should we define? And what does this definition > based on? The only gist I think is the number of counters on host. And > what's the reason to define less or more than PMCR_EL0.N? I didn't find > one. So I choose to be consistent with host. The problem is that choosing the host value may be just the wrong thing once you migrate it. It is not a big deal, but it is worth keeping it in mind. it should anyway be possible to size the PMU from userspace, overriding the value you've selected at reset. Thanks, M.
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index c370b40..db1be44 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -33,6 +33,7 @@ #include <asm/kvm_emulate.h> #include <asm/kvm_host.h> #include <asm/kvm_mmu.h> +#include <asm/pmu.h> #include <trace/events/kvm.h> @@ -236,6 +237,48 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) vcpu_sys_reg(vcpu, MPIDR_EL1) = (1ULL << 31) | mpidr; } +static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) +{ + u32 pmcr; + + asm volatile("mrs %0, pmcr_el0\n" : "=r" (pmcr)); + /* Writable bits of PMCR_EL0 (ARMV8_PMCR_MASK) is reset to UNKNOWN*/ + if (!vcpu_mode_is_32bit(vcpu)) + vcpu_sys_reg(vcpu, r->reg) = (pmcr & ~ARMV8_PMCR_MASK) + | (ARMV8_PMCR_MASK & 0xdecafbad); + else + vcpu_cp15(vcpu, r->reg) = (pmcr & ~ARMV8_PMCR_MASK) + | (ARMV8_PMCR_MASK & 0xdecafbad); +} + +/* PMU registers accessor. */ +static bool access_pmu_regs(struct kvm_vcpu *vcpu, + const struct sys_reg_params *p, + const struct sys_reg_desc *r) +{ + unsigned long val; + + if (p->is_write) { + switch (r->reg) { + case PMCR_EL0: { + /* Only update writeable bits of PMCR */ + val = vcpu_sys_reg(vcpu, r->reg); + val &= ~ARMV8_PMCR_MASK; + val |= *vcpu_reg(vcpu, p->Rt) & ARMV8_PMCR_MASK; + vcpu_sys_reg(vcpu, r->reg) = val; + break; + } + default: + vcpu_sys_reg(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt); + break; + } + } else { + *vcpu_reg(vcpu, p->Rt) = vcpu_sys_reg(vcpu, r->reg); + } + + 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 */ \ @@ -427,7 +470,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { /* PMCR_EL0 */ { Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1100), Op2(0b000), - trap_raz_wi }, + access_pmu_regs, reset_pmcr, PMCR_EL0, }, /* PMCNTENSET_EL0 */ { Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1100), Op2(0b001), trap_raz_wi }, @@ -632,6 +675,34 @@ static const struct sys_reg_desc cp14_64_regs[] = { { Op1( 0), CRm( 2), .access = trap_raz_wi }, }; +/* PMU CP15 registers accessor. */ +static bool access_pmu_cp15_regs(struct kvm_vcpu *vcpu, + const struct sys_reg_params *p, + const struct sys_reg_desc *r) +{ + unsigned long val; + + if (p->is_write) { + switch (r->reg) { + case c9_PMCR: { + /* Only update writeable bits of PMCR */ + val = vcpu_cp15(vcpu, r->reg); + val &= ~ARMV8_PMCR_MASK; + val |= *vcpu_reg(vcpu, p->Rt) & ARMV8_PMCR_MASK; + vcpu_cp15(vcpu, r->reg) = val; + break; + } + default: + vcpu_cp15(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt); + break; + } + } else { + *vcpu_reg(vcpu, p->Rt) = vcpu_cp15(vcpu, r->reg); + } + + return true; +} + /* * Trapped cp15 registers. TTBR0/TTBR1 get a double encoding, * depending on the way they are accessed (as a 32bit or a 64bit @@ -660,7 +731,8 @@ static const struct sys_reg_desc cp15_regs[] = { { Op1( 0), CRn( 7), CRm(14), Op2( 2), access_dcsw }, /* PMU */ - { Op1( 0), CRn( 9), CRm(12), Op2( 0), trap_raz_wi }, + { Op1( 0), CRn( 9), CRm(12), Op2( 0), access_pmu_cp15_regs, + reset_pmcr, c9_PMCR }, { 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 },