diff mbox series

[v5,02/12] KVM: arm64: PMU: Set the default PMU for the guest on vCPU reset

Message ID 20230817003029.3073210-3-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 Aug. 17, 2023, 12:30 a.m. UTC 
From: Reiji Watanabe <reijiw@google.com>

The following patches will use the number of counters information
from the arm_pmu and use this to set the PMCR.N for the guest
during vCPU reset. However, since the guest is not associated
with any arm_pmu until userspace configures the vPMU device
attributes, and a reset can happen before this event, call
kvm_arm_support_pmu_v3() just before doing the reset.

No functional change intended.

Signed-off-by: Reiji Watanabe <reijiw@google.com>
Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 arch/arm64/kvm/pmu-emul.c |  9 +--------
 arch/arm64/kvm/reset.c    | 18 +++++++++++++-----
 include/kvm/arm_pmu.h     |  6 ++++++
 3 files changed, 20 insertions(+), 13 deletions(-)

Comments

kernel test robot Aug. 17, 2023, 5:03 a.m. UTC | #1 
Hi Raghavendra,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 2ccdd1b13c591d306f0401d98dedc4bdcd02b421]

url:    https://github.com/intel-lab-lkp/linux/commits/Raghavendra-Rao-Ananta/KVM-arm64-PMU-Introduce-a-helper-to-set-the-guest-s-PMU/20230817-083353
base:   2ccdd1b13c591d306f0401d98dedc4bdcd02b421
patch link:    https://lore.kernel.org/r/20230817003029.3073210-3-rananta%40google.com
patch subject: [PATCH v5 02/12] KVM: arm64: PMU: Set the default PMU for the guest on vCPU reset
config: arm64-randconfig-r032-20230817 (https://download.01.org/0day-ci/archive/20230817/202308171212.KW8LnRRC-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce: (https://download.01.org/0day-ci/archive/20230817/202308171212.KW8LnRRC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308171212.KW8LnRRC-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from arch/arm64/kernel/asm-offsets.c:16:
   In file included from include/linux/kvm_host.h:45:
   In file included from arch/arm64/include/asm/kvm_host.h:37:
>> include/kvm/arm_pmu.h:176:62: warning: declaration of 'struct arm_pmu' will not be visible outside of this function [-Wvisibility]
     176 | static inline int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
         |                                                              ^
   1 warning generated.
--
   In file included from arch/arm64/kernel/asm-offsets.c:16:
   In file included from include/linux/kvm_host.h:45:
   In file included from arch/arm64/include/asm/kvm_host.h:37:
>> include/kvm/arm_pmu.h:176:62: warning: declaration of 'struct arm_pmu' will not be visible outside of this function [-Wvisibility]
     176 | static inline int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
         |                                                              ^
   1 warning generated.


vim +176 include/kvm/arm_pmu.h

   175	
 > 176	static inline int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
   177	{
   178		return -ENODEV;
   179	}
   180
kernel test robot Aug. 17, 2023, 7:54 a.m. UTC | #2 
Hi Raghavendra,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 2ccdd1b13c591d306f0401d98dedc4bdcd02b421]

url:    https://github.com/intel-lab-lkp/linux/commits/Raghavendra-Rao-Ananta/KVM-arm64-PMU-Introduce-a-helper-to-set-the-guest-s-PMU/20230817-083353
base:   2ccdd1b13c591d306f0401d98dedc4bdcd02b421
patch link:    https://lore.kernel.org/r/20230817003029.3073210-3-rananta%40google.com
patch subject: [PATCH v5 02/12] KVM: arm64: PMU: Set the default PMU for the guest on vCPU reset
config: arm64-randconfig-r024-20230817 (https://download.01.org/0day-ci/archive/20230817/202308171559.K5QeXXZk-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230817/202308171559.K5QeXXZk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308171559.K5QeXXZk-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from arch/arm64/include/asm/kvm_host.h:37,
                    from include/linux/kvm_host.h:45,
                    from arch/arm64/kernel/asm-offsets.c:16:
>> include/kvm/arm_pmu.h:176:62: warning: 'struct arm_pmu' declared inside parameter list will not be visible outside of this definition or declaration
     176 | static inline int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
         |                                                              ^~~~~~~
--
   In file included from arch/arm64/include/asm/kvm_host.h:37,
                    from include/linux/kvm_host.h:45,
                    from arch/arm64/kernel/asm-offsets.c:16:
>> include/kvm/arm_pmu.h:176:62: warning: 'struct arm_pmu' declared inside parameter list will not be visible outside of this definition or declaration
     176 | static inline int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
         |                                                              ^~~~~~~


vim +176 include/kvm/arm_pmu.h

   175	
 > 176	static inline int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
   177	{
   178		return -ENODEV;
   179	}
   180
Oliver Upton Sept. 15, 2023, 7:33 p.m. UTC | #3 
On Thu, Aug 17, 2023 at 12:30:19AM +0000, Raghavendra Rao Ananta wrote:
> From: Reiji Watanabe <reijiw@google.com>
> 
> The following patches will use the number of counters information
> from the arm_pmu and use this to set the PMCR.N for the guest
> during vCPU reset. However, since the guest is not associated
> with any arm_pmu until userspace configures the vPMU device
> attributes, and a reset can happen before this event, call
> kvm_arm_support_pmu_v3() just before doing the reset.
> 
> No functional change intended.

But there absolutely is a functional change here, and user visible at
that. KVM_ARM_VCPU_INIT ioctls can now fail with -ENODEV, which is not
part of the documented errors for the interface.

> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  arch/arm64/kvm/pmu-emul.c |  9 +--------
>  arch/arm64/kvm/reset.c    | 18 +++++++++++++-----
>  include/kvm/arm_pmu.h     |  6 ++++++
>  3 files changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 0ffd1efa90c07..b87822024828a 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -865,7 +865,7 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
>  	return true;
>  }
>  
> -static int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
> +int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
>  {
>  	lockdep_assert_held(&kvm->arch.config_lock);
>  
> @@ -937,13 +937,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  	if (vcpu->arch.pmu.created)
>  		return -EBUSY;
>  
> -	if (!kvm->arch.arm_pmu) {
> -		int ret = kvm_arm_set_vm_pmu(kvm, NULL);
> -
> -		if (ret)
> -			return ret;
> -	}
> -
>  	switch (attr->attr) {
>  	case KVM_ARM_VCPU_PMU_V3_IRQ: {
>  		int __user *uaddr = (int __user *)(long)attr->addr;
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index bc8556b6f4590..4c20f1ccd0789 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -206,6 +206,7 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
>   */
>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  {
> +	struct kvm *kvm = vcpu->kvm;
>  	struct vcpu_reset_state reset_state;
>  	int ret;
>  	bool loaded;
> @@ -216,6 +217,18 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  	vcpu->arch.reset_state.reset = false;
>  	spin_unlock(&vcpu->arch.mp_state_lock);
>  
> +	/*
> +	 * When the vCPU has a PMU, but no PMU is set for the guest
> +	 * yet, set the default one.
> +	 */
> +	if (kvm_vcpu_has_pmu(vcpu) && unlikely(!kvm->arch.arm_pmu)) {
> +		ret = -EINVAL;
> +		if (kvm_arm_support_pmu_v3())
> +			ret = kvm_arm_set_vm_pmu(kvm, NULL);
> +		if (ret)
> +			return ret;
> +	}
> +

On top of my prior suggestion w.r.t. the default PMU helper, I'd rather
see this block look like:

	if (kvm_vcpu_has_pmu(vcpu)) {
		if (!kvm_arm_support_pmu_v3())
			return -EINVAL;
		/*
		 * When the vCPU has a PMU but no PMU is set for the
		 * guest yet, set the default one.
		 */
		if (unlikely(!kvm->arch.arm_pmu) && kvm_set_default_pmu(kvm))
			return -EINVAL;
	}

This would eliminate the possibility of returning ENODEV to userspace
where we shouldn't.
Raghavendra Rao Ananta Sept. 18, 2023, 4:41 p.m. UTC | #4 
On Fri, Sep 15, 2023 at 12:33 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Thu, Aug 17, 2023 at 12:30:19AM +0000, Raghavendra Rao Ananta wrote:
> > From: Reiji Watanabe <reijiw@google.com>
> >
> > The following patches will use the number of counters information
> > from the arm_pmu and use this to set the PMCR.N for the guest
> > during vCPU reset. However, since the guest is not associated
> > with any arm_pmu until userspace configures the vPMU device
> > attributes, and a reset can happen before this event, call
> > kvm_arm_support_pmu_v3() just before doing the reset.
> >
> > No functional change intended.
>
> But there absolutely is a functional change here, and user visible at
> that. KVM_ARM_VCPU_INIT ioctls can now fail with -ENODEV, which is not
> part of the documented errors for the interface.
>
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > ---
> >  arch/arm64/kvm/pmu-emul.c |  9 +--------
> >  arch/arm64/kvm/reset.c    | 18 +++++++++++++-----
> >  include/kvm/arm_pmu.h     |  6 ++++++
> >  3 files changed, 20 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > index 0ffd1efa90c07..b87822024828a 100644
> > --- a/arch/arm64/kvm/pmu-emul.c
> > +++ b/arch/arm64/kvm/pmu-emul.c
> > @@ -865,7 +865,7 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
> >       return true;
> >  }
> >
> > -static int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
> > +int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
> >  {
> >       lockdep_assert_held(&kvm->arch.config_lock);
> >
> > @@ -937,13 +937,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> >       if (vcpu->arch.pmu.created)
> >               return -EBUSY;
> >
> > -     if (!kvm->arch.arm_pmu) {
> > -             int ret = kvm_arm_set_vm_pmu(kvm, NULL);
> > -
> > -             if (ret)
> > -                     return ret;
> > -     }
> > -
> >       switch (attr->attr) {
> >       case KVM_ARM_VCPU_PMU_V3_IRQ: {
> >               int __user *uaddr = (int __user *)(long)attr->addr;
> > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > index bc8556b6f4590..4c20f1ccd0789 100644
> > --- a/arch/arm64/kvm/reset.c
> > +++ b/arch/arm64/kvm/reset.c
> > @@ -206,6 +206,7 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
> >   */
> >  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >  {
> > +     struct kvm *kvm = vcpu->kvm;
> >       struct vcpu_reset_state reset_state;
> >       int ret;
> >       bool loaded;
> > @@ -216,6 +217,18 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >       vcpu->arch.reset_state.reset = false;
> >       spin_unlock(&vcpu->arch.mp_state_lock);
> >
> > +     /*
> > +      * When the vCPU has a PMU, but no PMU is set for the guest
> > +      * yet, set the default one.
> > +      */
> > +     if (kvm_vcpu_has_pmu(vcpu) && unlikely(!kvm->arch.arm_pmu)) {
> > +             ret = -EINVAL;
> > +             if (kvm_arm_support_pmu_v3())
> > +                     ret = kvm_arm_set_vm_pmu(kvm, NULL);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
>
> On top of my prior suggestion w.r.t. the default PMU helper, I'd rather
> see this block look like:
>
>         if (kvm_vcpu_has_pmu(vcpu)) {
>                 if (!kvm_arm_support_pmu_v3())
>                         return -EINVAL;
>                 /*
>                  * When the vCPU has a PMU but no PMU is set for the
>                  * guest yet, set the default one.
>                  */
>                 if (unlikely(!kvm->arch.arm_pmu) && kvm_set_default_pmu(kvm))
>                         return -EINVAL;
>         }
>
> This would eliminate the possibility of returning ENODEV to userspace
> where we shouldn't.
>
I understand that we'll be breaking the API contract and userspace may
have to adapt to this change, but is it not acceptable to document and
return ENODEV, since ENODEV may offer more clarity to userspace as to
why the ioctl failed? In general, do we never extend the APIs?

Thank you.
Raghavendra
> --
> Thanks,
> Oliver
Oliver Upton Sept. 18, 2023, 4:47 p.m. UTC | #5 
On Mon, Sep 18, 2023 at 09:41:02AM -0700, Raghavendra Rao Ananta wrote:
> On Fri, Sep 15, 2023 at 12:33 PM Oliver Upton <oliver.upton@linux.dev> wrote:

[...]

> > This would eliminate the possibility of returning ENODEV to userspace
> > where we shouldn't.
> >
> I understand that we'll be breaking the API contract and userspace may
> have to adapt to this change, but is it not acceptable to document and
> return ENODEV, since ENODEV may offer more clarity to userspace as to
> why the ioctl failed? In general, do we never extend the APIs?

Yes, we extend the existing interfaces all the time, but we almost
always require user opt in for user-visible changes in behavior. Look at
the way arm64_check_features() is handled -- we hide the 'detailed'
error and return EINVAL due to UAPI.

--
Thanks,
Oliver
Raghavendra Rao Ananta Sept. 18, 2023, 4:58 p.m. UTC | #6 
On Mon, Sep 18, 2023 at 9:47 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Mon, Sep 18, 2023 at 09:41:02AM -0700, Raghavendra Rao Ananta wrote:
> > On Fri, Sep 15, 2023 at 12:33 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> [...]
>
> > > This would eliminate the possibility of returning ENODEV to userspace
> > > where we shouldn't.
> > >
> > I understand that we'll be breaking the API contract and userspace may
> > have to adapt to this change, but is it not acceptable to document and
> > return ENODEV, since ENODEV may offer more clarity to userspace as to
> > why the ioctl failed? In general, do we never extend the APIs?
>
> Yes, we extend the existing interfaces all the time, but we almost
> always require user opt in for user-visible changes in behavior. Look at
> the way arm64_check_features() is handled -- we hide the 'detailed'
> error and return EINVAL due to UAPI.
>
Got it. Let's return EINVAL then. Thanks!

- Raghavendra
> --
> Thanks,
> Oliver
diff mbox series

Patch

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 0ffd1efa90c07..b87822024828a 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -865,7 +865,7 @@  static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
 	return true;
 }
 
-static int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
+int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
 {
 	lockdep_assert_held(&kvm->arch.config_lock);
 
@@ -937,13 +937,6 @@  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 	if (vcpu->arch.pmu.created)
 		return -EBUSY;
 
-	if (!kvm->arch.arm_pmu) {
-		int ret = kvm_arm_set_vm_pmu(kvm, NULL);
-
-		if (ret)
-			return ret;
-	}
-
 	switch (attr->attr) {
 	case KVM_ARM_VCPU_PMU_V3_IRQ: {
 		int __user *uaddr = (int __user *)(long)attr->addr;
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index bc8556b6f4590..4c20f1ccd0789 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -206,6 +206,7 @@  static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
  */
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 {
+	struct kvm *kvm = vcpu->kvm;
 	struct vcpu_reset_state reset_state;
 	int ret;
 	bool loaded;
@@ -216,6 +217,18 @@  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 	vcpu->arch.reset_state.reset = false;
 	spin_unlock(&vcpu->arch.mp_state_lock);
 
+	/*
+	 * When the vCPU has a PMU, but no PMU is set for the guest
+	 * yet, set the default one.
+	 */
+	if (kvm_vcpu_has_pmu(vcpu) && unlikely(!kvm->arch.arm_pmu)) {
+		ret = -EINVAL;
+		if (kvm_arm_support_pmu_v3())
+			ret = kvm_arm_set_vm_pmu(kvm, NULL);
+		if (ret)
+			return ret;
+	}
+
 	/* Reset PMU outside of the non-preemptible section */
 	kvm_pmu_vcpu_reset(vcpu);
 
@@ -257,11 +270,6 @@  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 		} else {
 			pstate = VCPU_RESET_PSTATE_EL1;
 		}
-
-		if (kvm_vcpu_has_pmu(vcpu) && !kvm_arm_support_pmu_v3()) {
-			ret = -EINVAL;
-			goto out;
-		}
 		break;
 	}
 
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 847da6fc27139..66a2f8477641e 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -100,6 +100,7 @@  void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu);
 })
 
 u8 kvm_arm_pmu_get_pmuver_limit(void);
+int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu);
 
 #else
 struct kvm_pmu {
@@ -172,6 +173,11 @@  static inline u8 kvm_arm_pmu_get_pmuver_limit(void)
 	return 0;
 }
 
+static inline int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
+{
+	return -ENODEV;
+}
+
 #endif
 
 #endif