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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Feb 07, 2020 at 10:25:23AM +0000, Marc Zyngier wrote:
> On 2020-02-07 10:19, Zenghui Yu wrote:
> > Hi Marc,
> > 
> > On 2020/2/7 17:19, Marc Zyngier wrote:
> > > 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.

Sorry for not noticing this before.

> > 
> > I see, thanks!
> > 
> > > 
> > > Can you please give it a shot?
> > 
> > Yes, it works for me:
> > 
> > Tested-by: Zenghui Yu <yuzenghui@xxxxxxxxxx>
> 
> Actually, maybe a better/simpler fix would be this:
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 67ae2d5c37b2..3cf7719d3177 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4414,7 +4414,13 @@ static void kvm_sched_out(struct preempt_notifier
> *pn,
>   */
>  struct kvm_vcpu *kvm_get_running_vcpu(void)
>  {
> -        return __this_cpu_read(kvm_running_vcpu);
> +	struct kvm_vcpu *vcpu;
> +
> +	preempt_disable();
> +	vcpu = __this_cpu_read(kvm_running_vcpu);
> +	preempt_enable();
> +
> +	return vcpu;
>  }
> 
>  /**
> 
> which matches the comment that comes with the function.
> 
> Paolo, which one do you prefer? It seems to me that the intent of moving
> this to core code was to provide a high level API that works at all times.

Not sure about Paolo, but this looks better at least to me.  Shall we
also move the comment from vgic-mmio.c to here?  And we can remove the
1st paragraph:

/*
 * We can disable preemption locally around accessing the per-CPU variable,
 * and use the resolved vcpu pointer after enabling preemption again, because
 * even if the current thread is migrated to another CPU, reading the per-CPU
 * value later will give us the same value as we update the per-CPU variable
 * in the preempt notifier handlers.
 */

Thanks!

-- 
Peter Xu




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux