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

[Marc Zyngier <maz@kernel.org>]

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.