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
