Patchwork [05/11] KVM: arm/arm64: Reset the VCPU without preemption and vcpu state loaded

login
register
mail settings
Submitter Marc Zyngier
Date March 4, 2019, 5:06 p.m.
Message ID <ec205905-6b7b-2a6d-f70f-9bca574dd593@arm.com>
Download mbox | patch
Permalink /patch/740835/
State New
Headers show

Comments

Marc Zyngier - March 4, 2019, 5:06 p.m.
On 04/03/2019 16:30, Julien Grall wrote:
> Hi,
> 
> I noticed some issues with this patch when rebooting a guest after using perf.
> 
> [  577.513447] BUG: sleeping function called from invalid context at 
> kernel/locking/mutex.c:908
> [  577.521926] in_atomic(): 1, irqs_disabled(): 0, pid: 2323, name: qemu-system aar
> [  577.529354] 1 lock held by qemu-system-aar/2323:
> [  577.533998]  #0: 00000000f4f96804 (&vcpu->mutex){+.+.}, at: 
> kvm_vcpu_ioctl+0x74/0xac0
> [  577.541865] Preemption disabled at:
> [  577.541871] [<ffff0000100cc82c>] kvm_reset_vcpu+0x1c/0x1d0
> [  577.550882] CPU: 6 PID: 2323 Comm: qemu-system-aar Tainted: G        W  5.0.0 
> #1277
> [  577.559137] Hardware name: AMD Seattle (Rev.B0) Development Board (Overdrive) 
> (DT)
> [  577.566698] Call trace:
> [  577.569138]  dump_backtrace+0x0/0x140
> [  577.572793]  show_stack+0x14/0x20
> [  577.576103]  dump_stack+0xa0/0xd4
> [  577.579412]  ___might_sleep+0x1e4/0x2b0
> [  577.583241]  __might_sleep+0x60/0xb8
> [  577.586810]  __mutex_lock+0x58/0x860
> [  577.590378]  mutex_lock_nested+0x1c/0x28
> [  577.594294]  perf_event_ctx_lock_nested+0xf4/0x238
> [  577.599078]  perf_event_read_value+0x24/0x60
> [  577.603341]  kvm_pmu_get_counter_value+0x80/0xe8
> [  577.607950]  kvm_pmu_stop_counter+0x2c/0x98
> [  577.612126]  kvm_pmu_vcpu_reset+0x58/0xd0
> [  577.616128]  kvm_reset_vcpu+0xec/0x1d0
> [  577.619869]  kvm_arch_vcpu_ioctl+0x6b0/0x860
> [  577.624131]  kvm_vcpu_ioctl+0xe0/0xac0
> [  577.627876]  do_vfs_ioctl+0xbc/0x910
> [  577.631443]  ksys_ioctl+0x78/0xa8
> [  577.634751]  __arm64_sys_ioctl+0x1c/0x28
> [  577.638667]  el0_svc_common+0x90/0x118
> [  577.642408]  el0_svc_handler+0x2c/0x80
> [  577.646150]  el0_svc+0x8/0xc
> 
> This is happening because the vCPU reset code is now running with preemption 
> disable. However, the perf code cannot be called with preemption disabled as it 
> is using mutex.
> 
> Do you have any suggestion on the way to fix this potential issue?

Given that the PMU is entirely emulated, it never has any state loaded
on the CPU. It thus doesn't need to be part of the non-preemptible section.

Can you please give this (untested) patchlet one a go? It's not exactly
pretty, but I believe it will do the trick.



Thanks,

	M.
Julien Grall - March 4, 2019, 5:31 p.m.
Hi,

On 04/03/2019 17:06, Marc Zyngier wrote:
> On 04/03/2019 16:30, Julien Grall wrote:
>> Hi,
>>
>> I noticed some issues with this patch when rebooting a guest after using perf.
>>
>> [  577.513447] BUG: sleeping function called from invalid context at
>> kernel/locking/mutex.c:908
>> [  577.521926] in_atomic(): 1, irqs_disabled(): 0, pid: 2323, name: qemu-system aar
>> [  577.529354] 1 lock held by qemu-system-aar/2323:
>> [  577.533998]  #0: 00000000f4f96804 (&vcpu->mutex){+.+.}, at:
>> kvm_vcpu_ioctl+0x74/0xac0
>> [  577.541865] Preemption disabled at:
>> [  577.541871] [<ffff0000100cc82c>] kvm_reset_vcpu+0x1c/0x1d0
>> [  577.550882] CPU: 6 PID: 2323 Comm: qemu-system-aar Tainted: G        W  5.0.0
>> #1277
>> [  577.559137] Hardware name: AMD Seattle (Rev.B0) Development Board (Overdrive)
>> (DT)
>> [  577.566698] Call trace:
>> [  577.569138]  dump_backtrace+0x0/0x140
>> [  577.572793]  show_stack+0x14/0x20
>> [  577.576103]  dump_stack+0xa0/0xd4
>> [  577.579412]  ___might_sleep+0x1e4/0x2b0
>> [  577.583241]  __might_sleep+0x60/0xb8
>> [  577.586810]  __mutex_lock+0x58/0x860
>> [  577.590378]  mutex_lock_nested+0x1c/0x28
>> [  577.594294]  perf_event_ctx_lock_nested+0xf4/0x238
>> [  577.599078]  perf_event_read_value+0x24/0x60
>> [  577.603341]  kvm_pmu_get_counter_value+0x80/0xe8
>> [  577.607950]  kvm_pmu_stop_counter+0x2c/0x98
>> [  577.612126]  kvm_pmu_vcpu_reset+0x58/0xd0
>> [  577.616128]  kvm_reset_vcpu+0xec/0x1d0
>> [  577.619869]  kvm_arch_vcpu_ioctl+0x6b0/0x860
>> [  577.624131]  kvm_vcpu_ioctl+0xe0/0xac0
>> [  577.627876]  do_vfs_ioctl+0xbc/0x910
>> [  577.631443]  ksys_ioctl+0x78/0xa8
>> [  577.634751]  __arm64_sys_ioctl+0x1c/0x28
>> [  577.638667]  el0_svc_common+0x90/0x118
>> [  577.642408]  el0_svc_handler+0x2c/0x80
>> [  577.646150]  el0_svc+0x8/0xc
>>
>> This is happening because the vCPU reset code is now running with preemption
>> disable. However, the perf code cannot be called with preemption disabled as it
>> is using mutex.
>>
>> Do you have any suggestion on the way to fix this potential issue?
> 
> Given that the PMU is entirely emulated, it never has any state loaded
> on the CPU. It thus doesn't need to be part of the non-preemptible section.
> 
> Can you please give this (untested) patchlet one a go? It's not exactly
> pretty, but I believe it will do the trick.

It does the trick. Are you going to submit the patch?

Cheers,
Marc Zyngier - March 4, 2019, 5:37 p.m.
On 04/03/2019 17:31, Julien Grall wrote:
> Hi,
> 
> On 04/03/2019 17:06, Marc Zyngier wrote:
>> On 04/03/2019 16:30, Julien Grall wrote:
>>> Hi,
>>>
>>> I noticed some issues with this patch when rebooting a guest after using perf.
>>>
>>> [  577.513447] BUG: sleeping function called from invalid context at
>>> kernel/locking/mutex.c:908
>>> [  577.521926] in_atomic(): 1, irqs_disabled(): 0, pid: 2323, name: qemu-system aar
>>> [  577.529354] 1 lock held by qemu-system-aar/2323:
>>> [  577.533998]  #0: 00000000f4f96804 (&vcpu->mutex){+.+.}, at:
>>> kvm_vcpu_ioctl+0x74/0xac0
>>> [  577.541865] Preemption disabled at:
>>> [  577.541871] [<ffff0000100cc82c>] kvm_reset_vcpu+0x1c/0x1d0
>>> [  577.550882] CPU: 6 PID: 2323 Comm: qemu-system-aar Tainted: G        W  5.0.0
>>> #1277
>>> [  577.559137] Hardware name: AMD Seattle (Rev.B0) Development Board (Overdrive)
>>> (DT)
>>> [  577.566698] Call trace:
>>> [  577.569138]  dump_backtrace+0x0/0x140
>>> [  577.572793]  show_stack+0x14/0x20
>>> [  577.576103]  dump_stack+0xa0/0xd4
>>> [  577.579412]  ___might_sleep+0x1e4/0x2b0
>>> [  577.583241]  __might_sleep+0x60/0xb8
>>> [  577.586810]  __mutex_lock+0x58/0x860
>>> [  577.590378]  mutex_lock_nested+0x1c/0x28
>>> [  577.594294]  perf_event_ctx_lock_nested+0xf4/0x238
>>> [  577.599078]  perf_event_read_value+0x24/0x60
>>> [  577.603341]  kvm_pmu_get_counter_value+0x80/0xe8
>>> [  577.607950]  kvm_pmu_stop_counter+0x2c/0x98
>>> [  577.612126]  kvm_pmu_vcpu_reset+0x58/0xd0
>>> [  577.616128]  kvm_reset_vcpu+0xec/0x1d0
>>> [  577.619869]  kvm_arch_vcpu_ioctl+0x6b0/0x860
>>> [  577.624131]  kvm_vcpu_ioctl+0xe0/0xac0
>>> [  577.627876]  do_vfs_ioctl+0xbc/0x910
>>> [  577.631443]  ksys_ioctl+0x78/0xa8
>>> [  577.634751]  __arm64_sys_ioctl+0x1c/0x28
>>> [  577.638667]  el0_svc_common+0x90/0x118
>>> [  577.642408]  el0_svc_handler+0x2c/0x80
>>> [  577.646150]  el0_svc+0x8/0xc
>>>
>>> This is happening because the vCPU reset code is now running with preemption
>>> disable. However, the perf code cannot be called with preemption disabled as it
>>> is using mutex.
>>>
>>> Do you have any suggestion on the way to fix this potential issue?
>>
>> Given that the PMU is entirely emulated, it never has any state loaded
>> on the CPU. It thus doesn't need to be part of the non-preemptible section.
>>
>> Can you please give this (untested) patchlet one a go? It's not exactly
>> pretty, but I believe it will do the trick.
> 
> It does the trick. Are you going to submit the patch?
Patch? Which patch? ;-)

I'll get around to it at some point after the merge window.

Thanks,

	M.

Patch

diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 54788eb9e695..16e773f3019f 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -128,6 +128,9 @@  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 	int ret = -EINVAL;
 	bool loaded;

+	/* Reset PMU outside of the non-preemptible section */
+	kvm_pmu_vcpu_reset(vcpu);
+
 	preempt_disable();
 	loaded = (vcpu->cpu != -1);
 	if (loaded)
@@ -177,9 +180,6 @@  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 		vcpu->arch.reset_state.reset = false;
 	}

-	/* Reset PMU */
-	kvm_pmu_vcpu_reset(vcpu);
-
 	/* Default workaround setup is enabled (if supported) */
 	if (kvm_arm_have_ssbd() == KVM_SSBD_KERNEL)
 		vcpu->arch.workaround_flags |= VCPU_WORKAROUND_2_FLAG;