Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Mon, 23 Oct 2023 14:00:18 +0100
Message-ID: <86wmvd4hp9.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Raghavendra Rao Ananta <rananta@google.com>
Cc:     Oliver Upton <oliver.upton@linux.dev>,
        Alexandru Elisei <alexandru.elisei@arm.com>,
        James Morse <james.morse@arm.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Zenghui Yu <yuzenghui@huawei.com>,
        Shaoqin Huang <shahuang@redhat.com>,
        Jing Zhang <jingzhangos@google.com>,
        Reiji Watanabe <reijiw@google.com>,
        Colton Lewis <coltonlewis@google.com>,
        linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH v8 07/13] KVM: arm64: PMU: Allow userspace to limit PMCR_EL0.N for the guest
In-Reply-To: <20231020214053.2144305-8-rananta@google.com>
References: <20231020214053.2144305-1-rananta@google.com>
        <20231020214053.2144305-8-rananta@google.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.1
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Fri, 20 Oct 2023 22:40:47 +0100,
Raghavendra Rao Ananta <rananta@google.com> wrote:
> 
> From: Reiji Watanabe <reijiw@google.com>
> 
> 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@google.com>
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  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.