On Wed, Jan 27, 2016 at 11:51:43AM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
>
> This register resets as unknown in 64bit mode while it resets as zero
> in 32bit mode. Here we choose to reset it as zero for consistency.
>
> PMUSERENR_EL0 holds some bits which decide whether PMU registers can be
> accessed from EL0. Add some check helpers to handle the access from EL0.
>
> When these bits are zero, only reading PMUSERENR will trap to EL2 and
> writing PMUSERENR or reading/writing other PMU registers will trap to
> EL1 other than EL2 when HCR.TGE==0. To current KVM configuration
> (HCR.TGE==0) there is no way to get these traps. Here we write 0xf to
> physical PMUSERENR register on VM entry, so that it will trap PMU access
> from EL0 to EL2. Within the register access handler we check the real
> value of guest PMUSERENR register to decide whether this access is
> allowed. If not allowed, return false to inject UND to guest.
>
> Signed-off-by: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
> ---
> arch/arm64/include/asm/pmu.h | 9 ++++
> arch/arm64/kvm/hyp/hyp.h | 1 +
> arch/arm64/kvm/hyp/switch.c | 3 ++
> arch/arm64/kvm/sys_regs.c | 100 ++++++++++++++++++++++++++++++++++++++++---
> 4 files changed, 107 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/include/asm/pmu.h b/arch/arm64/include/asm/pmu.h
> index 6f14a01..eb3dc88 100644
> --- a/arch/arm64/include/asm/pmu.h
> +++ b/arch/arm64/include/asm/pmu.h
> @@ -69,4 +69,13 @@
> #define ARMV8_EXCLUDE_EL0 (1 << 30)
> #define ARMV8_INCLUDE_EL2 (1 << 27)
>
> +/*
> + * PMUSERENR: user enable reg
> + */
> +#define ARMV8_USERENR_MASK 0xf /* Mask for writable bits */
> +#define ARMV8_USERENR_EN (1 << 0) /* PMU regs can be accessed at EL0 */
> +#define ARMV8_USERENR_SW (1 << 1) /* PMSWINC can be written at EL0 */
> +#define ARMV8_USERENR_CR (1 << 2) /* Cycle counter can be read at EL0 */
> +#define ARMV8_USERENR_ER (1 << 3) /* Event counter can be read at EL0 */
> +
> #endif /* __ASM_PMU_H */
> diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h
> index fb27517..9a28b7bd8 100644
> --- a/arch/arm64/kvm/hyp/hyp.h
> +++ b/arch/arm64/kvm/hyp/hyp.h
> @@ -22,6 +22,7 @@
> #include <linux/kvm_host.h>
> #include <asm/kvm_mmu.h>
> #include <asm/sysreg.h>
> +#include <asm/pmu.h>
>
> #define __hyp_text __section(.hyp.text) notrace
>
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index ca8f5a5..1a7d679 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -37,6 +37,8 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
> /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */
> write_sysreg(1 << 15, hstr_el2);
> write_sysreg(CPTR_EL2_TTA | CPTR_EL2_TFP, cptr_el2);
> + /* Make sure we trap PMU access from EL0 to EL2 */
> + write_sysreg(ARMV8_USERENR_MASK, pmuserenr_el0);
> write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
> }
>
> @@ -45,6 +47,7 @@ static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu)
> write_sysreg(HCR_RW, hcr_el2);
> write_sysreg(0, hstr_el2);
> write_sysreg(read_sysreg(mdcr_el2) & MDCR_EL2_HPMN_MASK, mdcr_el2);
> + write_sysreg(0, pmuserenr_el0);
> write_sysreg(0, cptr_el2);
> }
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index eefc60a..084e527 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -453,6 +453,37 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> vcpu_sys_reg(vcpu, PMCR_EL0) = val;
> }
>
> +static bool pmu_access_el0_disabled(struct kvm_vcpu *vcpu)
> +{
> + u64 reg = vcpu_sys_reg(vcpu, PMUSERENR_EL0);
> +
> + return !((reg & ARMV8_USERENR_EN) || vcpu_mode_priv(vcpu));
> +}
> +
> +static bool pmu_write_swinc_el0_disabled(struct kvm_vcpu *vcpu)
> +{
> + u64 reg = vcpu_sys_reg(vcpu, PMUSERENR_EL0);
> +
> + return !((reg & (ARMV8_USERENR_SW | ARMV8_USERENR_EN))
> + || vcpu_mode_priv(vcpu));
> +}
> +
> +static bool pmu_access_cycle_counter_el0_disabled(struct kvm_vcpu *vcpu)
> +{
> + u64 reg = vcpu_sys_reg(vcpu, PMUSERENR_EL0);
> +
> + return !((reg & (ARMV8_USERENR_CR | ARMV8_USERENR_EN))
> + || vcpu_mode_priv(vcpu));
> +}
> +
> +static bool pmu_access_event_counter_el0_disabled(struct kvm_vcpu *vcpu)
> +{
> + u64 reg = vcpu_sys_reg(vcpu, PMUSERENR_EL0);
> +
> + return !((reg & (ARMV8_USERENR_ER | ARMV8_USERENR_EN))
> + || vcpu_mode_priv(vcpu));
> +}
> +
> static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> const struct sys_reg_desc *r)
> {
> @@ -461,6 +492,9 @@ static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> if (!kvm_arm_pmu_v3_ready(vcpu))
> return trap_raz_wi(vcpu, p, r);
>
> + if (pmu_access_el0_disabled(vcpu))
> + return false;
Based on the function name I'm not sure I like embedding vcpu_mode_priv.
It seems a condition like
if (!vcpu_mode_priv(vcpu) && !pmu_access_el0_enabled(vcpu))
return false;
would be more clear here and the other callsites below. (I also prefer
checking for enabled vs. disabled)
> +
> if (p->is_write) {
> /* Only update writeable bits of PMCR */
> val = vcpu_sys_reg(vcpu, PMCR_EL0);
> @@ -484,6 +518,9 @@ static bool access_pmselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> if (!kvm_arm_pmu_v3_ready(vcpu))
> return trap_raz_wi(vcpu, p, r);
>
> + if (pmu_access_event_counter_el0_disabled(vcpu))
> + return false;
> +
> if (p->is_write)
> vcpu_sys_reg(vcpu, PMSELR_EL0) = p->regval;
> else
> @@ -501,7 +538,7 @@ static bool access_pmceid(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> if (!kvm_arm_pmu_v3_ready(vcpu))
> return trap_raz_wi(vcpu, p, r);
>
> - if (p->is_write)
> + if (p->is_write || pmu_access_el0_disabled(vcpu))
> return false;
>
> if (!(p->Op2 & 1))
> @@ -534,6 +571,9 @@ static bool access_pmu_evtyper(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> if (!kvm_arm_pmu_v3_ready(vcpu))
> return trap_raz_wi(vcpu, p, r);
>
> + if (pmu_access_el0_disabled(vcpu))
> + return false;
> +
> if (r->CRn == 9 && r->CRm == 13 && r->Op2 == 1) {
> /* PMXEVTYPER_EL0 */
> idx = vcpu_sys_reg(vcpu, PMSELR_EL0) & ARMV8_COUNTER_MASK;
> @@ -574,11 +614,17 @@ static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
> if (r->CRn == 9 && r->CRm == 13) {
> if (r->Op2 == 2) {
> /* PMXEVCNTR_EL0 */
> + if (pmu_access_event_counter_el0_disabled(vcpu))
> + return false;
> +
> idx = vcpu_sys_reg(vcpu, PMSELR_EL0)
> & ARMV8_COUNTER_MASK;
> reg = PMEVCNTR0_EL0 + idx;
> } else if (r->Op2 == 0) {
> /* PMCCNTR_EL0 */
> + if (pmu_access_cycle_counter_el0_disabled(vcpu))
> + return false;
> +
> idx = ARMV8_CYCLE_IDX;
> reg = PMCCNTR_EL0;
> } else {
> @@ -586,6 +632,9 @@ static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
> }
> } else if (r->CRn == 14 && (r->CRm & 12) == 8) {
> /* PMEVCNTRn_EL0 */
> + if (pmu_access_event_counter_el0_disabled(vcpu))
> + return false;
> +
> idx = ((r->CRm & 3) << 3) | (r->Op2 & 7);
> reg = PMEVCNTR0_EL0 + idx;
> } else {
> @@ -596,10 +645,14 @@ static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
> return false;
>
> val = kvm_pmu_get_counter_value(vcpu, idx);
> - if (p->is_write)
> + if (p->is_write) {
> + if (pmu_access_el0_disabled(vcpu))
> + return false;
> +
This check isn't necessary because at this point we've either already
checked ARMV8_USERENR_EN with one of the other tests, or we've BUGed.
> vcpu_sys_reg(vcpu, reg) += (s64)p->regval - val;
> - else
> + } else {
> p->regval = val;
> + }
It's nasty to have to add 3 checks to access_pmu_evcntr. Can we instead
just have another helper that takes a reg_idx argument, i.e.
static bool pmu_reg_access_el0_disabled(struct kvm_vcpu *vcpu, u64 idx)
{
if (idx == PMCCNTR_EL0)
return pmu_access_cycle_counter_el0_disabled
if (idx >= PMEVCNTR0_EL0 && idx <= PMEVCNTR30_EL0)
return pmu_access_event_counter_el0_disabled
...
and call it once after the pmu_counter_idx_valid check?
>
> return true;
> }
> @@ -612,6 +665,9 @@ static bool access_pmcnten(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> if (!kvm_arm_pmu_v3_ready(vcpu))
> return trap_raz_wi(vcpu, p, r);
>
> + if (pmu_access_el0_disabled(vcpu))
> + return false;
> +
> mask = kvm_pmu_valid_counter_mask(vcpu);
> if (p->is_write) {
> val = p->regval & mask;
> @@ -639,6 +695,9 @@ static bool access_pminten(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> if (!kvm_arm_pmu_v3_ready(vcpu))
> return trap_raz_wi(vcpu, p, r);
>
> + if (!vcpu_mode_priv(vcpu))
> + return false;
> +
> if (p->is_write) {
> if (r->Op2 & 0x1)
> /* accessing PMINTENSET_EL1 */
> @@ -663,6 +722,9 @@ static bool access_pmovs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> if (!kvm_arm_pmu_v3_ready(vcpu))
> return trap_raz_wi(vcpu, p, r);
>
> + if (pmu_access_el0_disabled(vcpu))
> + return false;
> +
> if (p->is_write) {
> if (r->CRm & 0x2)
> /* accessing PMOVSSET_EL0 */
> @@ -685,6 +747,9 @@ static bool access_pmswinc(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> if (!kvm_arm_pmu_v3_ready(vcpu))
> return trap_raz_wi(vcpu, p, r);
>
> + if (pmu_write_swinc_el0_disabled(vcpu))
> + return false;
> +
> if (p->is_write) {
> mask = kvm_pmu_valid_counter_mask(vcpu);
> kvm_pmu_software_increment(vcpu, p->regval & mask);
> @@ -694,6 +759,26 @@ static bool access_pmswinc(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> return false;
> }
>
> +static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + if (!kvm_arm_pmu_v3_ready(vcpu))
> + return trap_raz_wi(vcpu, p, r);
> +
> + if (p->is_write) {
> + if (!vcpu_mode_priv(vcpu))
> + return false;
> +
> + vcpu_sys_reg(vcpu, PMUSERENR_EL0) = p->regval
> + & ARMV8_USERENR_MASK;
> + } else {
> + p->regval = vcpu_sys_reg(vcpu, PMUSERENR_EL0)
> + & ARMV8_USERENR_MASK;
> + }
> +
> + 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 */ \
> @@ -923,9 +1008,12 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> /* PMXEVCNTR_EL0 */
> { Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1101), Op2(0b010),
> access_pmu_evcntr },
> - /* PMUSERENR_EL0 */
> + /* PMUSERENR_EL0
> + * This register resets as unknown in 64bit mode while it resets as zero
> + * in 32bit mode. Here we choose to reset it as zero for consistency.
> + */
> { Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1110), Op2(0b000),
> - trap_raz_wi },
> + access_pmuserenr, reset_val, PMUSERENR_EL0, 0 },
> /* PMOVSSET_EL0 */
> { Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1110), Op2(0b011),
> access_pmovs, reset_unknown, PMOVSSET_EL0 },
> @@ -1250,7 +1338,7 @@ static const struct sys_reg_desc cp15_regs[] = {
> { Op1( 0), CRn( 9), CRm(13), Op2( 0), access_pmu_evcntr },
> { Op1( 0), CRn( 9), CRm(13), Op2( 1), access_pmu_evtyper },
> { Op1( 0), CRn( 9), CRm(13), Op2( 2), access_pmu_evcntr },
> - { Op1( 0), CRn( 9), CRm(14), Op2( 0), trap_raz_wi },
> + { Op1( 0), CRn( 9), CRm(14), Op2( 0), access_pmuserenr },
> { Op1( 0), CRn( 9), CRm(14), Op2( 1), access_pminten },
> { Op1( 0), CRn( 9), CRm(14), Op2( 2), access_pminten },
> { Op1( 0), CRn( 9), CRm(14), Op2( 3), access_pmovs },
> --
> 2.0.4
>
>
> --
> 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
--
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
