[PATCH] KVM: arm64: Fix smp_processor_id() call in preemptible context

[PATCH] KVM: arm64: Fix smp_processor_id() call in preemptible context

[Sebastian Ott]

Commit 1c913a1c35aa ("KVM: arm64: Iterate arm_pmus list to probe for
default PMU") introduced a smp_processor_id() call in preemtible context:

[70506.110187] BUG: using smp_processor_id() in preemptible [00000000] code: qemu-system-aar/3078242
[70506.119077] caller is debug_smp_processor_id+0x20/0x30
[70506.124229] CPU: 129 PID: 3078242 Comm: qemu-system-aar Tainted: G        W          6.4.0-rc5 #25
[70506.133176] Hardware name: GIGABYTE R181-T92-00/MT91-FS4-00, BIOS F34 08/13/2020
[70506.140559] Call trace:
[70506.142993]  dump_backtrace+0xa4/0x130
[70506.146737]  show_stack+0x20/0x38
[70506.150040]  dump_stack_lvl+0x48/0x60
[70506.153704]  dump_stack+0x18/0x28
[70506.157007]  check_preemption_disabled+0xe4/0x108
[70506.161701]  debug_smp_processor_id+0x20/0x30
[70506.166046]  kvm_arm_pmu_v3_set_attr+0x460/0x628
[70506.170662]  kvm_arm_vcpu_arch_set_attr+0x88/0xd8
[70506.175363]  kvm_arch_vcpu_ioctl+0x258/0x4a8
[70506.179632]  kvm_vcpu_ioctl+0x32c/0x6b8
[70506.183465]  __arm64_sys_ioctl+0xb4/0x100
[70506.187467]  invoke_syscall+0x78/0x108
[70506.191205]  el0_svc_common.constprop.0+0x4c/0x100
[70506.195984]  do_el0_svc+0x34/0x50
[70506.199287]  el0_svc+0x34/0x108
[70506.202416]  el0t_64_sync_handler+0xf4/0x120
[70506.206674]  el0t_64_sync+0x194/0x198

Just disable preemption for this section.

Fixes: 1c913a1c35aa ("KVM: arm64: Iterate arm_pmus list to probe for default PMU")
Signed-off-by: Sebastian Ott <sebott@redhat.com>
---
  arch/arm64/kvm/pmu-emul.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 491ca7eb2a4c..f9e4e4334875 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -700,6 +700,7 @@ static struct arm_pmu *kvm_pmu_probe_armpmu(void)

  	mutex_lock(&arm_pmus_lock);

+	preempt_disable();
  	cpu = smp_processor_id();
  	list_for_each_entry(entry, &arm_pmus, entry) {
  		tmp = entry->arm_pmu;
@@ -709,7 +710,7 @@ static struct arm_pmu *kvm_pmu_probe_armpmu(void)
  			break;
  		}
  	}
-
+	preempt_enable();
  	mutex_unlock(&arm_pmus_lock);

  	return pmu;
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Fix smp_processor_id() call in preemptible context

[Sean Christopherson]

On Tue, Jun 06, 2023, Sebastian Ott wrote:
> Fixes: 1c913a1c35aa ("KVM: arm64: Iterate arm_pmus list to probe for default PMU")
> Signed-off-by: Sebastian Ott <sebott@redhat.com>
> ---
>  arch/arm64/kvm/pmu-emul.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 491ca7eb2a4c..f9e4e4334875 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -700,6 +700,7 @@ static struct arm_pmu *kvm_pmu_probe_armpmu(void)
> 
>  	mutex_lock(&arm_pmus_lock);
> 
> +	preempt_disable();

get_cpu() + put_cpu() would be more succinct and self-documenting.

>  	cpu = smp_processor_id();
>  	list_for_each_entry(entry, &arm_pmus, entry) {
>  		tmp = entry->arm_pmu;
> @@ -709,7 +710,7 @@ static struct arm_pmu *kvm_pmu_probe_armpmu(void)
>  			break;
>  		}
>  	}
> -
> +	preempt_enable();
>  	mutex_unlock(&arm_pmus_lock);
> 
>  	return pmu;
> -- 
> 2.40.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Fix smp_processor_id() call in preemptible context

[Oliver Upton]

Hi Sebastian,

On Tue, Jun 06, 2023 at 12:37:30PM +0200, Sebastian Ott wrote:
> Commit 1c913a1c35aa ("KVM: arm64: Iterate arm_pmus list to probe for
> default PMU") introduced a smp_processor_id() call in preemtible context:
> 
> [70506.110187] BUG: using smp_processor_id() in preemptible [00000000] code: qemu-system-aar/3078242
> [70506.119077] caller is debug_smp_processor_id+0x20/0x30
> [70506.124229] CPU: 129 PID: 3078242 Comm: qemu-system-aar Tainted: G        W          6.4.0-rc5 #25
> [70506.133176] Hardware name: GIGABYTE R181-T92-00/MT91-FS4-00, BIOS F34 08/13/2020
> [70506.140559] Call trace:
> [70506.142993]  dump_backtrace+0xa4/0x130
> [70506.146737]  show_stack+0x20/0x38
> [70506.150040]  dump_stack_lvl+0x48/0x60
> [70506.153704]  dump_stack+0x18/0x28
> [70506.157007]  check_preemption_disabled+0xe4/0x108
> [70506.161701]  debug_smp_processor_id+0x20/0x30
> [70506.166046]  kvm_arm_pmu_v3_set_attr+0x460/0x628
> [70506.170662]  kvm_arm_vcpu_arch_set_attr+0x88/0xd8
> [70506.175363]  kvm_arch_vcpu_ioctl+0x258/0x4a8
> [70506.179632]  kvm_vcpu_ioctl+0x32c/0x6b8
> [70506.183465]  __arm64_sys_ioctl+0xb4/0x100
> [70506.187467]  invoke_syscall+0x78/0x108
> [70506.191205]  el0_svc_common.constprop.0+0x4c/0x100
> [70506.195984]  do_el0_svc+0x34/0x50
> [70506.199287]  el0_svc+0x34/0x108
> [70506.202416]  el0t_64_sync_handler+0xf4/0x120
> [70506.206674]  el0t_64_sync+0x194/0x198
> 
> Just disable preemption for this section.

The call from a preemptible context is intentional, so this really
should just be raw_smp_processor_id(). Do you mind if we fix it with the
following?

From 2f4680ee6a5aea5c3cf826c84b86172b0b2c1a67 Mon Sep 17 00:00:00 2001
From: Oliver Upton <oliver.upton@linux.dev>
Date: Tue, 6 Jun 2023 06:44:54 -0700
Subject: [PATCH] KVM: arm64: Use raw_smp_processor_id() in
 kvm_pmu_probe_armpmu()

Sebastian reports that commit 1c913a1c35aa ("KVM: arm64: Iterate
arm_pmus list to probe for default PMU") introduced the following splat
with CONFIG_DEBUG_PREEMPT enabled:

[70506.110187] BUG: using smp_processor_id() in preemptible [00000000] code: qemu-system-aar/3078242
[70506.119077] caller is debug_smp_processor_id+0x20/0x30
[70506.124229] CPU: 129 PID: 3078242 Comm: qemu-system-aar Tainted: G        W          6.4.0-rc5 #25
[70506.133176] Hardware name: GIGABYTE R181-T92-00/MT91-FS4-00, BIOS F34 08/13/2020
[70506.140559] Call trace:
[70506.142993]  dump_backtrace+0xa4/0x130
[70506.146737]  show_stack+0x20/0x38
[70506.150040]  dump_stack_lvl+0x48/0x60
[70506.153704]  dump_stack+0x18/0x28
[70506.157007]  check_preemption_disabled+0xe4/0x108
[70506.161701]  debug_smp_processor_id+0x20/0x30
[70506.166046]  kvm_arm_pmu_v3_set_attr+0x460/0x628
[70506.170662]  kvm_arm_vcpu_arch_set_attr+0x88/0xd8
[70506.175363]  kvm_arch_vcpu_ioctl+0x258/0x4a8
[70506.179632]  kvm_vcpu_ioctl+0x32c/0x6b8
[70506.183465]  __arm64_sys_ioctl+0xb4/0x100
[70506.187467]  invoke_syscall+0x78/0x108
[70506.191205]  el0_svc_common.constprop.0+0x4c/0x100
[70506.195984]  do_el0_svc+0x34/0x50
[70506.199287]  el0_svc+0x34/0x108
[70506.202416]  el0t_64_sync_handler+0xf4/0x120
[70506.206674]  el0t_64_sync+0x194/0x198

Nonetheless, there's no functional requirement for disabling preemption,
as the cpu # is only used to walk the arm_pmus list. Fix it by using
raw_smp_processor_id() instead.

Fixes: 1c913a1c35aa ("KVM: arm64: Iterate arm_pmus list to probe for default PMU")
Reported-by: Sebastian Ott <sebott@redhat.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/pmu-emul.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 491ca7eb2a4c..933a6331168b 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -700,7 +700,7 @@ static struct arm_pmu *kvm_pmu_probe_armpmu(void)
 
 	mutex_lock(&arm_pmus_lock);
 
-	cpu = smp_processor_id();
+	cpu = raw_smp_processor_id();
 	list_for_each_entry(entry, &arm_pmus, entry) {
 		tmp = entry->arm_pmu;
 

base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7
-- 
2.41.0.rc0.172.g3f132b7071-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Fix smp_processor_id() call in preemptible context

[Sebastian Ott]

On Tue, 6 Jun 2023, Oliver Upton wrote:
> The call from a preemptible context is intentional, so this really
> should just be raw_smp_processor_id(). Do you mind if we fix it with the
> following?

Works for me.

Thanks,
Sebastian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Fix smp_processor_id() call in preemptible context

[Sean Christopherson]

On Tue, Jun 06, 2023, Oliver Upton wrote:
> The call from a preemptible context is intentional, so this really
> should just be raw_smp_processor_id(). Do you mind if we fix it with the
> following?

...

> Nonetheless, there's no functional requirement for disabling preemption,
> as the cpu # is only used to walk the arm_pmus list. Fix it by using
> raw_smp_processor_id() instead.

As a partial outsider, that needs an explanation, and the code could really use a
comment.  I assume KVM's ABI is that it's userspace's responsibility to ensure that
the CPU(s) used for KVM_RUN is compatible with the CPU used for KVM_ARM_VCPU_PMU_V3_CTRL,
but neither the original changelog nor the above state that, nor does anything
explain what happens if userspace doesn't uphold its side of things.  That stuff
might be covered in documentation somewhere, but for someone just looking at git
blame, this is all very magical.

> Fixes: 1c913a1c35aa ("KVM: arm64: Iterate arm_pmus list to probe for default PMU")
> Reported-by: Sebastian Ott <sebott@redhat.com>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/pmu-emul.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 491ca7eb2a4c..933a6331168b 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -700,7 +700,7 @@ static struct arm_pmu *kvm_pmu_probe_armpmu(void)
>  
>  	mutex_lock(&arm_pmus_lock);
>  
> -	cpu = smp_processor_id();
> +	cpu = raw_smp_processor_id();
>  	list_for_each_entry(entry, &arm_pmus, entry) {
>  		tmp = entry->arm_pmu;
>  
> 
> base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7
> -- 
> 2.41.0.rc0.172.g3f132b7071-goog
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Fix smp_processor_id() call in preemptible context

[Oliver Upton]

On Tue, Jun 06, 2023 at 07:29:16AM -0700, Sean Christopherson wrote:
> On Tue, Jun 06, 2023, Oliver Upton wrote:
> > The call from a preemptible context is intentional, so this really
> > should just be raw_smp_processor_id(). Do you mind if we fix it with the
> > following?
> 
> ...
> 
> > Nonetheless, there's no functional requirement for disabling preemption,
> > as the cpu # is only used to walk the arm_pmus list. Fix it by using
> > raw_smp_processor_id() instead.
> 
> As a partial outsider, that needs an explanation, and the code could really use a
> comment.  I assume KVM's ABI is that it's userspace's responsibility to ensure that
> the CPU(s) used for KVM_RUN is compatible with the CPU used for KVM_ARM_VCPU_PMU_V3_CTRL,
> but neither the original changelog nor the above state that, nor does anything
> explain what happens if userspace doesn't uphold its side of things.

See commit 40e54cad4540 ("KVM: arm64: Document default vPMU behavior on
heterogeneous systems"), which documents the subtlety of vCPU scheduling
with the 'old' ABI at the callsite of this function. I don't want to
bleed any details of this crap into user documentation, since the entire
scheme is irretrievably broken.

See Documentation/virt/kvm/api/devices/vcpu.rst 1.4 for the 'new' ABI
where userspace explicitly selects a vPMU instance.

> That stuff might be covered in documentation somewhere, but for someone
> just looking at git blame, this is all very magical.

Personally, I find any other fix that involves disabling preemption to be
quite a lot more 'magical', as there isn't any percpu data we're working
with in the loop.

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Fix smp_processor_id() call in preemptible context

[Sean Christopherson]

On Tue, Jun 06, 2023, Oliver Upton wrote:
> On Tue, Jun 06, 2023 at 07:29:16AM -0700, Sean Christopherson wrote:
> > On Tue, Jun 06, 2023, Oliver Upton wrote:
> > > The call from a preemptible context is intentional, so this really
> > > should just be raw_smp_processor_id(). Do you mind if we fix it with the
> > > following?
> > 
> > ...
> > 
> > > Nonetheless, there's no functional requirement for disabling preemption,
> > > as the cpu # is only used to walk the arm_pmus list. Fix it by using
> > > raw_smp_processor_id() instead.
> > 
> > As a partial outsider, that needs an explanation, and the code could really use a
> > comment.  I assume KVM's ABI is that it's userspace's responsibility to ensure that
> > the CPU(s) used for KVM_RUN is compatible with the CPU used for KVM_ARM_VCPU_PMU_V3_CTRL,
> > but neither the original changelog nor the above state that, nor does anything
> > explain what happens if userspace doesn't uphold its side of things.
> 
> See commit 40e54cad4540 ("KVM: arm64: Document default vPMU behavior on
> heterogeneous systems"), which documents the subtlety of vCPU scheduling
> with the 'old' ABI at the callsite of this function. I don't want to
> bleed any details of this crap into user documentation, since the entire
> scheme is irretrievably broken.

I wasn't suggesting adding anything to user documentation, I was suggesting/asking
that the changelog explain the assertion "there's no functional requirement for
disabling preemption".

> See Documentation/virt/kvm/api/devices/vcpu.rst 1.4 for the 'new' ABI
> where userspace explicitly selects a vPMU instance.
> 
> > That stuff might be covered in documentation somewhere, but for someone
> > just looking at git blame, this is all very magical.
> 
> Personally, I find any other fix that involves disabling preemption to be
> quite a lot more 'magical', as there isn't any percpu data we're working
> with in the loop.

Heh, and?  I wasn't suggesting you take Sebastian's fix, nor was I arguing that
disabling preemption is any more or less magical.  I was simply calling out that
for folks that aren't already familiar with the code, it's not obvious why using
an unstable CPU is functionally correct.

Poking around more, it looks like the answer is that kvm_arch_vcpu_load() will
mark the vCPU as being loaded on an unsupported pCPU, which will prevent running
on the target pCPU, and thus presumably prevent creating new perf events on the
incompatible pCPU.

Though following the breadcrumbs from the commit you referenced above, the set of
"supported" CPUs at this point is all possible CPUs in the system.  So unless I'm
missing something, testing that the current, unstable CPU is a "supported" CPU is
unnecessary because running on an impossible CPU is, um, impossible.

I.e. can't you just do?

---
 arch/arm64/kvm/pmu-emul.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 491ca7eb2a4c..b899d580ff73 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -692,29 +692,6 @@ void kvm_host_pmu_init(struct arm_pmu *pmu)
 	mutex_unlock(&arm_pmus_lock);
 }
 
-static struct arm_pmu *kvm_pmu_probe_armpmu(void)
-{
-	struct arm_pmu *tmp, *pmu = NULL;
-	struct arm_pmu_entry *entry;
-	int cpu;
-
-	mutex_lock(&arm_pmus_lock);
-
-	cpu = smp_processor_id();
-	list_for_each_entry(entry, &arm_pmus, entry) {
-		tmp = entry->arm_pmu;
-
-		if (cpumask_test_cpu(cpu, &tmp->supported_cpus)) {
-			pmu = tmp;
-			break;
-		}
-	}
-
-	mutex_unlock(&arm_pmus_lock);
-
-	return pmu;
-}
-
 u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
 {
 	unsigned long *bmap = vcpu->kvm->arch.pmu_filter;
@@ -900,8 +877,14 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 		 * upholds the preexisting behavior on heterogeneous systems
 		 * where vCPUs can be scheduled on any core but the guest
 		 * counters could stop working.
+		 *
+		 * In short, the set of "supported" CPUS for the default PMU is
+		 * all possible CPUs in the system, simply grab the first PMU.
 		 */
-		kvm->arch.arm_pmu = kvm_pmu_probe_armpmu();
+		mutex_lock(&arm_pmus_lock);
+		kvm->arch.arm_pmu = list_first_entry_or_null(&arm_pmus);
+		mutex_unlock(&arm_pmus_lock);
+
 		if (!kvm->arch.arm_pmu)
 			return -ENODEV;
 	}

base-commit: 40e54cad454076172cc3e2bca60aa924650a3e4b
-- 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Fix smp_processor_id() call in preemptible context

[Oliver Upton]

On Tue, Jun 06, 2023 at 03:46:09PM +0000, Sean Christopherson wrote:
> On Tue, Jun 06, 2023, Oliver Upton wrote:
> > On Tue, Jun 06, 2023 at 07:29:16AM -0700, Sean Christopherson wrote:
> > > On Tue, Jun 06, 2023, Oliver Upton wrote:
> > > > The call from a preemptible context is intentional, so this really
> > > > should just be raw_smp_processor_id(). Do you mind if we fix it with the
> > > > following?
> > > 
> > > ...
> > > 
> > > > Nonetheless, there's no functional requirement for disabling preemption,
> > > > as the cpu # is only used to walk the arm_pmus list. Fix it by using
> > > > raw_smp_processor_id() instead.
> > > 
> > > As a partial outsider, that needs an explanation, and the code could really use a
> > > comment.  I assume KVM's ABI is that it's userspace's responsibility to ensure that
> > > the CPU(s) used for KVM_RUN is compatible with the CPU used for KVM_ARM_VCPU_PMU_V3_CTRL,
> > > but neither the original changelog nor the above state that, nor does anything
> > > explain what happens if userspace doesn't uphold its side of things.
> > 
> > See commit 40e54cad4540 ("KVM: arm64: Document default vPMU behavior on
> > heterogeneous systems"), which documents the subtlety of vCPU scheduling
> > with the 'old' ABI at the callsite of this function. I don't want to
> > bleed any details of this crap into user documentation, since the entire
> > scheme is irretrievably broken.
> 
> I wasn't suggesting adding anything to user documentation, I was suggesting/asking
> that the changelog explain the assertion "there's no functional requirement for
> disabling preemption".

Let's continue this on Marc's reply (don't want to spread context over
multiple threads).

> Poking around more, it looks like the answer is that kvm_arch_vcpu_load() will
> mark the vCPU as being loaded on an unsupported pCPU, which will prevent running
> on the target pCPU, and thus presumably prevent creating new perf events on the
> incompatible pCPU.

That's only the case with the 'new' ABI, where userspace has explicitly
selected a PMU instance. The answer for the 'old' interface is here:

	/*
	 * No PMU set, get the default one.
	 *
	 * The observant among you will notice that the supported_cpus
	 * mask does not get updated for the default PMU even though it
	 * is quite possible the selected instance supports only a
	 * subset of cores in the system. This is intentional, and
	 * upholds the preexisting behavior on heterogeneous systems
	 * where vCPUs can be scheduled on any core but the guest
	 * counters could stop working.
	 */
	kvm->arch.arm_pmu = kvm_pmu_probe_armpmu();


> Though following the breadcrumbs from the commit you referenced above, the set of
> "supported" CPUs at this point is all possible CPUs in the system.  So unless I'm
> missing something, testing that the current, unstable CPU is a "supported" CPU is
> unnecessary because running on an impossible CPU is, um, impossible.

Careful -- arm_pmu->supported_cpus is different from kvm->arch.supported_cpus.
The former describes a PMU instance in the system and the latter
enforces our scheduling requirements when userspace _explicitly_ sets a
PMU type, i.e. KVM_ARM_VCPU_PMU_V3_SET_PMU.

So it is probable that some of the PMUs in the arm_pmus list *do not*
support the calling CPU.

> I.e. can't you just do?

No, this would break the 'old' ABI, but if we decide to deliberately
break it I prefer your suggestion over using CPU0.

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Fix smp_processor_id() call in preemptible context

[Sean Christopherson]

On Tue, Jun 06, 2023, Oliver Upton wrote:
> On Tue, Jun 06, 2023 at 03:46:09PM +0000, Sean Christopherson wrote:
> > I.e. can't you just do?
> 
> No, this would break the 'old' ABI, but if we decide to deliberately
> break it I prefer your suggestion over using CPU0.

Ah, if userspace pins the task when doing KVM_ARM_VCPU_PMU_V3_CTRL, then factoring
in the pCPU matters.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Fix smp_processor_id() call in preemptible context

[Marc Zyngier]

On Tue, 06 Jun 2023 15:10:44 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Hi Sebastian,
> 
> On Tue, Jun 06, 2023 at 12:37:30PM +0200, Sebastian Ott wrote:
> > Commit 1c913a1c35aa ("KVM: arm64: Iterate arm_pmus list to probe for
> > default PMU") introduced a smp_processor_id() call in preemtible context:
> > 
> > [70506.110187] BUG: using smp_processor_id() in preemptible [00000000] code: qemu-system-aar/3078242
> > [70506.119077] caller is debug_smp_processor_id+0x20/0x30
> > [70506.124229] CPU: 129 PID: 3078242 Comm: qemu-system-aar Tainted: G        W          6.4.0-rc5 #25
> > [70506.133176] Hardware name: GIGABYTE R181-T92-00/MT91-FS4-00, BIOS F34 08/13/2020
> > [70506.140559] Call trace:
> > [70506.142993]  dump_backtrace+0xa4/0x130
> > [70506.146737]  show_stack+0x20/0x38
> > [70506.150040]  dump_stack_lvl+0x48/0x60
> > [70506.153704]  dump_stack+0x18/0x28
> > [70506.157007]  check_preemption_disabled+0xe4/0x108
> > [70506.161701]  debug_smp_processor_id+0x20/0x30
> > [70506.166046]  kvm_arm_pmu_v3_set_attr+0x460/0x628
> > [70506.170662]  kvm_arm_vcpu_arch_set_attr+0x88/0xd8
> > [70506.175363]  kvm_arch_vcpu_ioctl+0x258/0x4a8
> > [70506.179632]  kvm_vcpu_ioctl+0x32c/0x6b8
> > [70506.183465]  __arm64_sys_ioctl+0xb4/0x100
> > [70506.187467]  invoke_syscall+0x78/0x108
> > [70506.191205]  el0_svc_common.constprop.0+0x4c/0x100
> > [70506.195984]  do_el0_svc+0x34/0x50
> > [70506.199287]  el0_svc+0x34/0x108
> > [70506.202416]  el0t_64_sync_handler+0xf4/0x120
> > [70506.206674]  el0t_64_sync+0x194/0x198
> > 
> > Just disable preemption for this section.
> 
> The call from a preemptible context is intentional, so this really
> should just be raw_smp_processor_id(). Do you mind if we fix it with the
> following?
> 
> From 2f4680ee6a5aea5c3cf826c84b86172b0b2c1a67 Mon Sep 17 00:00:00 2001
> From: Oliver Upton <oliver.upton@linux.dev>
> Date: Tue, 6 Jun 2023 06:44:54 -0700
> Subject: [PATCH] KVM: arm64: Use raw_smp_processor_id() in
>  kvm_pmu_probe_armpmu()
> 
> Sebastian reports that commit 1c913a1c35aa ("KVM: arm64: Iterate
> arm_pmus list to probe for default PMU") introduced the following splat
> with CONFIG_DEBUG_PREEMPT enabled:
> 
> [70506.110187] BUG: using smp_processor_id() in preemptible [00000000] code: qemu-system-aar/3078242
> [70506.119077] caller is debug_smp_processor_id+0x20/0x30
> [70506.124229] CPU: 129 PID: 3078242 Comm: qemu-system-aar Tainted: G        W          6.4.0-rc5 #25
> [70506.133176] Hardware name: GIGABYTE R181-T92-00/MT91-FS4-00, BIOS F34 08/13/2020
> [70506.140559] Call trace:
> [70506.142993]  dump_backtrace+0xa4/0x130
> [70506.146737]  show_stack+0x20/0x38
> [70506.150040]  dump_stack_lvl+0x48/0x60
> [70506.153704]  dump_stack+0x18/0x28
> [70506.157007]  check_preemption_disabled+0xe4/0x108
> [70506.161701]  debug_smp_processor_id+0x20/0x30
> [70506.166046]  kvm_arm_pmu_v3_set_attr+0x460/0x628
> [70506.170662]  kvm_arm_vcpu_arch_set_attr+0x88/0xd8
> [70506.175363]  kvm_arch_vcpu_ioctl+0x258/0x4a8
> [70506.179632]  kvm_vcpu_ioctl+0x32c/0x6b8
> [70506.183465]  __arm64_sys_ioctl+0xb4/0x100
> [70506.187467]  invoke_syscall+0x78/0x108
> [70506.191205]  el0_svc_common.constprop.0+0x4c/0x100
> [70506.195984]  do_el0_svc+0x34/0x50
> [70506.199287]  el0_svc+0x34/0x108
> [70506.202416]  el0t_64_sync_handler+0xf4/0x120
> [70506.206674]  el0t_64_sync+0x194/0x198
> 
> Nonetheless, there's no functional requirement for disabling preemption,
> as the cpu # is only used to walk the arm_pmus list. Fix it by using
> raw_smp_processor_id() instead.
> 
> Fixes: 1c913a1c35aa ("KVM: arm64: Iterate arm_pmus list to probe for default PMU")
> Reported-by: Sebastian Ott <sebott@redhat.com>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/pmu-emul.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 491ca7eb2a4c..933a6331168b 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -700,7 +700,7 @@ static struct arm_pmu *kvm_pmu_probe_armpmu(void)
>  
>  	mutex_lock(&arm_pmus_lock);
>  
> -	cpu = smp_processor_id();
> +	cpu = raw_smp_processor_id();
>  	list_for_each_entry(entry, &arm_pmus, entry) {
>  		tmp = entry->arm_pmu;
>  
> 

If preemption doesn't matter (and I really don't think it does), why
are we looking for a the current CPU? I'd rather we pick the PMU that
is associated with CPU0 (we're pretty sure it exists), and be done
with it.

Something like:

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 491ca7eb2a4c..fce9d07fe26b 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -696,15 +696,14 @@ static struct arm_pmu *kvm_pmu_probe_armpmu(void)
 {
 	struct arm_pmu *tmp, *pmu = NULL;
 	struct arm_pmu_entry *entry;
-	int cpu;
 
 	mutex_lock(&arm_pmus_lock);
 
-	cpu = smp_processor_id();
 	list_for_each_entry(entry, &arm_pmus, entry) {
 		tmp = entry->arm_pmu;
 
-		if (cpumask_test_cpu(cpu, &tmp->supported_cpus)) {
+		/* Pick the CPU associated with CPU0 as the default */
+		if (cpumask_test_cpu(0, &tmp->supported_cpus)) {
 			pmu = tmp;
 			break;
 		}

At least, it saves us wondering about the rationale for picking one or
the other.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Fix smp_processor_id() call in preemptible context

[Oliver Upton]

On Tue, Jun 06, 2023 at 05:17:34PM +0100, Marc Zyngier wrote:
> On Tue, 06 Jun 2023 15:10:44 +0100, Oliver Upton <oliver.upton@linux.dev> wrote:
> > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > index 491ca7eb2a4c..933a6331168b 100644
> > --- a/arch/arm64/kvm/pmu-emul.c
> > +++ b/arch/arm64/kvm/pmu-emul.c
> > @@ -700,7 +700,7 @@ static struct arm_pmu *kvm_pmu_probe_armpmu(void)
> >  
> >  	mutex_lock(&arm_pmus_lock);
> >  
> > -	cpu = smp_processor_id();
> > +	cpu = raw_smp_processor_id();
> >  	list_for_each_entry(entry, &arm_pmus, entry) {
> >  		tmp = entry->arm_pmu;
> >  
> > 
> 
> If preemption doesn't matter (and I really don't think it does), why
> are we looking for a the current CPU? I'd rather we pick the PMU that
> is associated with CPU0 (we're pretty sure it exists), and be done
> with it.

Getting the current CPU is still useful, we just don't care about that
cpu# being stale. Unconditionally using CPU0 could break existing usage
patterns.

A not-too-contrived example would be to taskset QEMU onto a cluster of
cores in a big.LITTLE system (I do this). The current behavior would
assign the right PMU to the guest. I've made my opinions about the 'old'
ABI quite clear, but I don't have too great of an appetite for breakage,
though fragile.

Can we proceed with the fix I had suggested along with a more complete
description of the baggage that we're carrying?

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Fix smp_processor_id() call in preemptible context

[Marc Zyngier]

On Tue, 06 Jun 2023 17:48:14 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Tue, Jun 06, 2023 at 05:17:34PM +0100, Marc Zyngier wrote:
> > On Tue, 06 Jun 2023 15:10:44 +0100, Oliver Upton <oliver.upton@linux.dev> wrote:
> > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > > index 491ca7eb2a4c..933a6331168b 100644
> > > --- a/arch/arm64/kvm/pmu-emul.c
> > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > @@ -700,7 +700,7 @@ static struct arm_pmu *kvm_pmu_probe_armpmu(void)
> > >  
> > >  	mutex_lock(&arm_pmus_lock);
> > >  
> > > -	cpu = smp_processor_id();
> > > +	cpu = raw_smp_processor_id();
> > >  	list_for_each_entry(entry, &arm_pmus, entry) {
> > >  		tmp = entry->arm_pmu;
> > >  
> > > 
> > 
> > If preemption doesn't matter (and I really don't think it does), why
> > are we looking for a the current CPU? I'd rather we pick the PMU that
> > is associated with CPU0 (we're pretty sure it exists), and be done
> > with it.
> 
> Getting the current CPU is still useful, we just don't care about that
> cpu# being stale. Unconditionally using CPU0 could break existing usage
> patterns.
>
> A not-too-contrived example would be to taskset QEMU onto a cluster of
> cores in a big.LITTLE system (I do this). The current behavior would
> assign the right PMU to the guest. I've made my opinions about the 'old'
> ABI quite clear, but I don't have too great of an appetite for breakage,
> though fragile.

Fair enough.

> 
> Can we proceed with the fix I had suggested along with a more complete
> description of the baggage that we're carrying?

Sure. Please post a separate patch and I'll queue that together with
Reiji's EL0 PMU stuff for the next bag of fixes.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel