diff mbox series

[v7,08/12] KVM: arm64: PMU: Allow userspace to limit PMCR_EL0.N for the guest

Message ID 20231009230858.3444834-9-rananta@google.com (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: PMU: Allow userspace to limit the number of PMCs on vCPU | expand

Commit Message

Raghavendra Rao Ananta Oct. 9, 2023, 11:08 p.m. UTC 
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(-)

Comments

Sebastian Ott Oct. 17, 2023, 3:52 p.m. UTC | #1 
On Mon, 9 Oct 2023, Raghavendra Rao Ananta wrote:
> +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;
> +
> +	mutex_lock(&kvm->arch.config_lock);
> +
> +	/*
> +	 * Make PMCR immutable once the VM has started running, but do
> +	 * not return an error (-EBUSY) to meet the existing expectations.
> +	 */

Why should we mention which error we're _not_ returning?


> +	if (kvm_vm_has_ran_once(vcpu->kvm)) {
> +		mutex_unlock(&kvm->arch.config_lock);
> +		return 0;
> +	}
Raghavendra Rao Ananta Oct. 17, 2023, 4:49 p.m. UTC | #2 
On Tue, Oct 17, 2023 at 8:52 AM Sebastian Ott <sebott@redhat.com> wrote:
>
> On Mon, 9 Oct 2023, Raghavendra Rao Ananta wrote:
> > +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;
> > +
> > +     mutex_lock(&kvm->arch.config_lock);
> > +
> > +     /*
> > +      * Make PMCR immutable once the VM has started running, but do
> > +      * not return an error (-EBUSY) to meet the existing expectations.
> > +      */
>
> Why should we mention which error we're _not_ returning?
>
Oh, it's not to break the existing userspace expectations. Before this
series, any 'write' from userspace was possible. Returning -EBUSY all
of a sudden might tamper with this expectation.

Thank you.
Raghavendra
>
> > +     if (kvm_vm_has_ran_once(vcpu->kvm)) {
> > +             mutex_unlock(&kvm->arch.config_lock);
> > +             return 0;
> > +     }
>
Sebastian Ott Oct. 19, 2023, 10:45 a.m. UTC | #3 
On Tue, 17 Oct 2023, Raghavendra Rao Ananta wrote:
> On Tue, Oct 17, 2023 at 8:52 AM Sebastian Ott <sebott@redhat.com> wrote:
>>
>> On Mon, 9 Oct 2023, Raghavendra Rao Ananta wrote:
>>> +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;
>>> +
>>> +     mutex_lock(&kvm->arch.config_lock);
>>> +
>>> +     /*
>>> +      * Make PMCR immutable once the VM has started running, but do
>>> +      * not return an error (-EBUSY) to meet the existing expectations.
>>> +      */
>>
>> Why should we mention which error we're _not_ returning?
>>
> Oh, it's not to break the existing userspace expectations. Before this
> series, any 'write' from userspace was possible. Returning -EBUSY all
> of a sudden might tamper with this expectation.

Yes I get that part. What I've meant is why specifically mention -EBUSY?
You're also not returning -EFAULT nor -EINVAL.

/*
  * Make PMCR immutable once the VM has started running, but do
  * not return an error to meet the existing expectations.
  */
IMHO provides the same info to the reader and is less confusing

Sebastian
Raghavendra Rao Ananta Oct. 19, 2023, 6:05 p.m. UTC | #4 
On Thu, Oct 19, 2023 at 3:45 AM Sebastian Ott <sebott@redhat.com> wrote:
>
> On Tue, 17 Oct 2023, Raghavendra Rao Ananta wrote:
> > On Tue, Oct 17, 2023 at 8:52 AM Sebastian Ott <sebott@redhat.com> wrote:
> >>
> >> On Mon, 9 Oct 2023, Raghavendra Rao Ananta wrote:
> >>> +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;
> >>> +
> >>> +     mutex_lock(&kvm->arch.config_lock);
> >>> +
> >>> +     /*
> >>> +      * Make PMCR immutable once the VM has started running, but do
> >>> +      * not return an error (-EBUSY) to meet the existing expectations.
> >>> +      */
> >>
> >> Why should we mention which error we're _not_ returning?
> >>
> > Oh, it's not to break the existing userspace expectations. Before this
> > series, any 'write' from userspace was possible. Returning -EBUSY all
> > of a sudden might tamper with this expectation.
>
> Yes I get that part. What I've meant is why specifically mention -EBUSY?
> You're also not returning -EFAULT nor -EINVAL.
>
> /*
>   * Make PMCR immutable once the VM has started running, but do
>   * not return an error to meet the existing expectations.
>   */
> IMHO provides the same info to the reader and is less confusing
>
Sounds good. I'll apply this.

Thank you.
Raghavendra
> Sebastian
diff mbox series

Patch

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index c750722fbe4a..0c8d337b0370 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1087,6 +1087,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;
+
+	mutex_lock(&kvm->arch.config_lock);
+
+	/*
+	 * Make PMCR immutable once the VM has started running, but do
+	 * not return an error (-EBUSY) 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) {
+		u8 pmcr_n_limit = kvm_arm_get_num_counters(kvm);
+
+		/*
+		 * 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)
+			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));
+	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;
+}
+
 /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
 #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
 	{ SYS_DESC(SYS_DBGBVRn_EL1(n)),					\
@@ -2150,8 +2203,8 @@  static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_CTR_EL0), access_ctr },
 	{ SYS_DESC(SYS_SVCR), undef_access },
 
-	{ PMU_SYS_REG(PMCR_EL0), .access = access_pmcr,
-	  .reset = reset_pmcr, .reg = PMCR_EL0, .get_user = get_pmcr },
+	{ PMU_SYS_REG(PMCR_EL0), .access = access_pmcr, .reset = reset_pmcr,
+	  .reg = PMCR_EL0, .get_user = get_pmcr, .set_user = set_pmcr },
 	{ PMU_SYS_REG(PMCNTENSET_EL0),
 	  .access = access_pmcnten, .reg = PMCNTENSET_EL0 },
 	{ PMU_SYS_REG(PMCNTENCLR_EL0),