Re: BUG: using __this_cpu_read() in preemptible [00000000] code

[Marc Zyngier <maz@kernel.org>]

Hi Zenghui,

On 2020-02-07 09:00, Zenghui Yu wrote:
> Hi,
> 
> Running a latest preemptible kernel and some guests on it,
> I got the following message,
> 
> ---8<---
> 
> [  630.031870] BUG: using __this_cpu_read() in preemptible [00000000]
> code: qemu-system-aar/37270
> [  630.031872] caller is kvm_get_running_vcpu+0x1c/0x38
> [  630.031874] CPU: 32 PID: 37270 Comm: qemu-system-aar Kdump: loaded
> Not tainted 5.5.0+
> [  630.031876] Hardware name: Huawei TaiShan 2280 /BC11SPCD, BIOS 1.58
> 10/29/2018
> [  630.031876] Call trace:
> [  630.031878]  dump_backtrace+0x0/0x200
> [  630.031880]  show_stack+0x24/0x30
> [  630.031882]  dump_stack+0xb0/0xf4
> [  630.031884]  __this_cpu_preempt_check+0xc8/0xd0
> [  630.031886]  kvm_get_running_vcpu+0x1c/0x38
> [  630.031888]  vgic_mmio_change_active.isra.4+0x2c/0xe0
> [  630.031890]  __vgic_mmio_write_cactive+0x80/0xc8
> [  630.031892]  vgic_mmio_uaccess_write_cactive+0x3c/0x50
> [  630.031894]  vgic_uaccess+0xcc/0x138
> [  630.031896]  vgic_v3_redist_uaccess+0x7c/0xa8
> [  630.031898]  vgic_v3_attr_regs_access+0x1a8/0x230
> [  630.031901]  vgic_v3_set_attr+0x1b4/0x290
> [  630.031903]  kvm_device_ioctl_attr+0xbc/0x110
> [  630.031905]  kvm_device_ioctl+0xc4/0x108
> [  630.031907]  ksys_ioctl+0xb4/0xd0
> [  630.031909]  __arm64_sys_ioctl+0x28/0x38
> [  630.031911]  el0_svc_common.constprop.1+0x7c/0x1a0
> [  630.031913]  do_el0_svc+0x34/0xa0
> [  630.031915]  el0_sync_handler+0x124/0x274
> [  630.031916]  el0_sync+0x140/0x180
> 
> ---8<---
> 
> I'm now at commit 90568ecf561540fa330511e21fcd823b0c3829c6.
> 
> And it looks like vgic_get_mmio_requester_vcpu() was broken by
> 7495e22bb165 ("KVM: Move running VCPU from ARM to common code").
> 
> Could anyone please have a look?

Here you go:

diff --git a/virt/kvm/arm/vgic/vgic-mmio.c 
b/virt/kvm/arm/vgic/vgic-mmio.c
index d656ebd5f9d4..e1735f19c924 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -190,6 +190,15 @@ unsigned long vgic_mmio_read_pending(struct 
kvm_vcpu *vcpu,
   * value later will give us the same value as we update the per-CPU 
variable
   * in the preempt notifier handlers.
   */
+static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
+{
+	struct kvm_vcpu *vcpu;
+
+	preempt_disable();
+	vcpu = kvm_get_running_vcpu();
+	preempt_enable();
+	return vcpu;
+}

  /* Must be called with irq->irq_lock held */
  static void vgic_hw_irq_spending(struct kvm_vcpu *vcpu, struct vgic_irq 
*irq,
@@ -212,7 +221,7 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
  			      gpa_t addr, unsigned int len,
  			      unsigned long val)
  {
-	bool is_uaccess = !kvm_get_running_vcpu();
+	bool is_uaccess = !vgic_get_mmio_requester_vcpu();
  	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
  	int i;
  	unsigned long flags;
@@ -265,7 +274,7 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
  			      gpa_t addr, unsigned int len,
  			      unsigned long val)
  {
-	bool is_uaccess = !kvm_get_running_vcpu();
+	bool is_uaccess = !vgic_get_mmio_requester_vcpu();
  	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
  	int i;
  	unsigned long flags;
@@ -326,7 +335,7 @@ static void vgic_mmio_change_active(struct kvm_vcpu 
*vcpu, struct vgic_irq *irq,
  				    bool active)
  {
  	unsigned long flags;
-	struct kvm_vcpu *requester_vcpu = kvm_get_running_vcpu();
+	struct kvm_vcpu *requester_vcpu = vgic_get_mmio_requester_vcpu();

  	raw_spin_lock_irqsave(&irq->irq_lock, flags);


That's basically a revert of the offending code. The comment right above
vgic_get_mmio_requester_vcpu() explains *why* this is valid, and why
preempt_disable() is needed.

Can you please give it a shot?

Thanks,

          M.
-- 
Jazz is not dead. It just smells funny...