On Fri, 20 Oct 2023 22:40:47 +0100,
Raghavendra Rao Ananta <rananta@xxxxxxxxxx> wrote:
>
> From: Reiji Watanabe <reijiw@xxxxxxxxxx>
>
> KVM does not yet support userspace modifying PMCR_EL0.N (With
> the previous patch, KVM ignores what is written by userspace).
> Add support userspace limiting PMCR_EL0.N.
>
> Disallow userspace to set PMCR_EL0.N to a value that is greater
> than the host value as KVM doesn't support more event counters
> than what the host HW implements. Also, make this register
> immutable after the VM has started running. To maintain the
> existing expectations, instead of returning an error, KVM
> returns a success for these two cases.
>
> Finally, ignore writes to read-only bits that are cleared on
> vCPU reset, and RES{0,1} bits (including writable bits that
> KVM doesn't support yet), as those bits shouldn't be modified
> (at least with the current KVM).
>
> Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
> Signed-off-by: Raghavendra Rao Ananta <rananta@xxxxxxxxxx>
> ---
> arch/arm64/kvm/sys_regs.c | 57 +++++++++++++++++++++++++++++++++++++--
> 1 file changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 2e5d497596ef8..a2c5f210b3d6b 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1176,6 +1176,59 @@ static int get_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> return 0;
> }
>
> +static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> + u64 val)
> +{
> + struct kvm *kvm = vcpu->kvm;
> + u64 new_n, mutable_mask;
Really, this lacks consistency. Either you make N a u8 everywhere, or
a u64 everywhere. I don't mind either, but the type confusion is not
great.
> +
> + mutex_lock(&kvm->arch.config_lock);
> +
> + /*
> + * Make PMCR immutable once the VM has started running, but
> + * do not return an error to meet the existing expectations.
> + */
> + if (kvm_vm_has_ran_once(vcpu->kvm)) {
> + mutex_unlock(&kvm->arch.config_lock);
> + return 0;
> + }
> +
> + new_n = (val >> ARMV8_PMU_PMCR_N_SHIFT) & ARMV8_PMU_PMCR_N_MASK;
> + if (new_n != kvm->arch.pmcr_n) {
Why do we need to check this?
> + u8 pmcr_n_limit = kvm_arm_pmu_get_max_counters(kvm);
Can you see why I'm annoyed?
> +
> + /*
> + * The vCPU can't have more counters than the PMU hardware
> + * implements. Ignore this error to maintain compatibility
> + * with the existing KVM behavior.
> + */
> + if (new_n <= pmcr_n_limit)
Isn't this the only thing that actually matters?
> + kvm->arch.pmcr_n = new_n;
> + }
> + mutex_unlock(&kvm->arch.config_lock);
> +
> + /*
> + * Ignore writes to RES0 bits, read only bits that are cleared on
> + * vCPU reset, and writable bits that KVM doesn't support yet.
> + * (i.e. only PMCR.N and bits [7:0] are mutable from userspace)
> + * The LP bit is RES0 when FEAT_PMUv3p5 is not supported on the vCPU.
> + * But, we leave the bit as it is here, as the vCPU's PMUver might
> + * be changed later (NOTE: the bit will be cleared on first vCPU run
> + * if necessary).
> + */
> + mutable_mask = (ARMV8_PMU_PMCR_MASK |
> + (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT));
Why is N part of the 'mutable' mask? The only bits that should make it
into the register are ARMV8_PMU_PMCR_MASK.
> + val &= mutable_mask;
> + val |= (__vcpu_sys_reg(vcpu, r->reg) & ~mutable_mask);
> +
> + /* The LC bit is RES1 when AArch32 is not supported */
> + if (!kvm_supports_32bit_el0())
> + val |= ARMV8_PMU_PMCR_LC;
> +
> + __vcpu_sys_reg(vcpu, r->reg) = val;
> + return 0;
I think this should be rewritten as:
val &= ARMV8_PMU_PMCR_MASK;
/* The LC bit is RES1 when AArch32 is not supported */
if (!kvm_supports_32bit_el0())
val |= ARMV8_PMU_PMCR_LC;
__vcpu_sys_reg(vcpu, r->reg) = val;
return 0;
And that's it. Drop this 'mutable_mask' nonsense, as we should be
getting the correct value (merge of the per-vcpu register and VM-wide
N) since patch 4.
M.
--
Without deviation from the norm, progress is not possible.
