On 2020-02-07 16:18, Peter Xu wrote:
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.
*/
Sure. I'll add it and post an actual patch in a moment.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
