virtualization.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] virtio: use virtio_device_ready() in virtio_device_restore()
@ 2022-03-22 11:43 Stefano Garzarella
  2022-03-22 14:07 ` Michael S. Tsirkin
  0 siblings, 1 reply; 5+ messages in thread
From: Stefano Garzarella @ 2022-03-22 11:43 UTC (permalink / raw)
  To: virtualization; +Cc: linux-kernel, Michael S. Tsirkin

After waking up a suspended VM, the kernel prints the following trace
for virtio drivers which do not directly call virtio_device_ready() in
the .restore:

    PM: suspend exit
    irq 22: nobody cared (try booting with the "irqpoll" option)
    Call Trace:
     <IRQ>
     dump_stack_lvl+0x38/0x49
     dump_stack+0x10/0x12
     __report_bad_irq+0x3a/0xaf
     note_interrupt.cold+0xb/0x60
     handle_irq_event+0x71/0x80
     handle_fasteoi_irq+0x95/0x1e0
     __common_interrupt+0x6b/0x110
     common_interrupt+0x63/0xe0
     asm_common_interrupt+0x1e/0x40
     ? __do_softirq+0x75/0x2f3
     irq_exit_rcu+0x93/0xe0
     sysvec_apic_timer_interrupt+0xac/0xd0
     </IRQ>
     <TASK>
     asm_sysvec_apic_timer_interrupt+0x12/0x20
     arch_cpu_idle+0x12/0x20
     default_idle_call+0x39/0xf0
     do_idle+0x1b5/0x210
     cpu_startup_entry+0x20/0x30
     start_secondary+0xf3/0x100
     secondary_startup_64_no_verify+0xc3/0xcb
     </TASK>
    handlers:
    [<000000008f9bac49>] vp_interrupt
    [<000000008f9bac49>] vp_interrupt
    Disabling IRQ #22

This happens because we don't invoke .enable_cbs callback in
virtio_device_restore(). That callback is used by some transports
(e.g. virtio-pci) to enable interrupts.

Let's fix it, by calling virtio_device_ready() as we do in
virtio_dev_probe(). This function calls .enable_cts callback and sets
DRIVER_OK status bit.

This fix also avoids setting DRIVER_OK twice for those drivers that
call virtio_device_ready() in the .restore.

Fixes: d50497eb4e55 ("virtio_config: introduce a new .enable_cbs method")
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---

I'm not sure about the fixes tag. That one is more generic, but the
following one I think introduced the issue.

Fixes: 9e35276a5344 ("virtio_pci: harden MSI-X interrupts")

Thanks,
Stefano
---
 drivers/virtio/virtio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 22f15f444f75..75c8d560bbd3 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -526,8 +526,9 @@ int virtio_device_restore(struct virtio_device *dev)
 			goto err;
 	}
 
-	/* Finally, tell the device we're all set */
-	virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
+	/* If restore didn't do it, mark device DRIVER_OK ourselves. */
+	if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
+		virtio_device_ready(dev);
 
 	virtio_config_enable(dev);
 
-- 
2.35.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] virtio: use virtio_device_ready() in virtio_device_restore()
  2022-03-22 11:43 [PATCH] virtio: use virtio_device_ready() in virtio_device_restore() Stefano Garzarella
@ 2022-03-22 14:07 ` Michael S. Tsirkin
  2022-03-23  3:10   ` Jason Wang
  0 siblings, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2022-03-22 14:07 UTC (permalink / raw)
  To: Stefano Garzarella; +Cc: linux-kernel, virtualization

On Tue, Mar 22, 2022 at 12:43:13PM +0100, Stefano Garzarella wrote:
> After waking up a suspended VM, the kernel prints the following trace
> for virtio drivers which do not directly call virtio_device_ready() in
> the .restore:
> 
>     PM: suspend exit
>     irq 22: nobody cared (try booting with the "irqpoll" option)
>     Call Trace:
>      <IRQ>
>      dump_stack_lvl+0x38/0x49
>      dump_stack+0x10/0x12
>      __report_bad_irq+0x3a/0xaf
>      note_interrupt.cold+0xb/0x60
>      handle_irq_event+0x71/0x80
>      handle_fasteoi_irq+0x95/0x1e0
>      __common_interrupt+0x6b/0x110
>      common_interrupt+0x63/0xe0
>      asm_common_interrupt+0x1e/0x40
>      ? __do_softirq+0x75/0x2f3
>      irq_exit_rcu+0x93/0xe0
>      sysvec_apic_timer_interrupt+0xac/0xd0
>      </IRQ>
>      <TASK>
>      asm_sysvec_apic_timer_interrupt+0x12/0x20
>      arch_cpu_idle+0x12/0x20
>      default_idle_call+0x39/0xf0
>      do_idle+0x1b5/0x210
>      cpu_startup_entry+0x20/0x30
>      start_secondary+0xf3/0x100
>      secondary_startup_64_no_verify+0xc3/0xcb
>      </TASK>
>     handlers:
>     [<000000008f9bac49>] vp_interrupt
>     [<000000008f9bac49>] vp_interrupt
>     Disabling IRQ #22
> 
> This happens because we don't invoke .enable_cbs callback in
> virtio_device_restore(). That callback is used by some transports
> (e.g. virtio-pci) to enable interrupts.
> 
> Let's fix it, by calling virtio_device_ready() as we do in
> virtio_dev_probe(). This function calls .enable_cts callback and sets
> DRIVER_OK status bit.
> 
> This fix also avoids setting DRIVER_OK twice for those drivers that
> call virtio_device_ready() in the .restore.
> 
> Fixes: d50497eb4e55 ("virtio_config: introduce a new .enable_cbs method")
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> 
> I'm not sure about the fixes tag. That one is more generic, but the
> following one I think introduced the issue.
> 
> Fixes: 9e35276a5344 ("virtio_pci: harden MSI-X interrupts")

Jason what should we do about this one BTW? Just revert? We have other
issues ...


> Thanks,
> Stefano
> ---
>  drivers/virtio/virtio.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 22f15f444f75..75c8d560bbd3 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -526,8 +526,9 @@ int virtio_device_restore(struct virtio_device *dev)
>  			goto err;
>  	}
>  
> -	/* Finally, tell the device we're all set */
> -	virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> +	/* If restore didn't do it, mark device DRIVER_OK ourselves. */
> +	if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
> +		virtio_device_ready(dev);
>  
>  	virtio_config_enable(dev);
>  
> -- 
> 2.35.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] virtio: use virtio_device_ready() in virtio_device_restore()
  2022-03-22 14:07 ` Michael S. Tsirkin
@ 2022-03-23  3:10   ` Jason Wang
  2022-03-23  8:04     ` Stefano Garzarella
  0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2022-03-23  3:10 UTC (permalink / raw)
  To: Michael S. Tsirkin, Parav Pandit; +Cc: virtualization, linux-kernel

On Tue, Mar 22, 2022 at 10:07 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Mar 22, 2022 at 12:43:13PM +0100, Stefano Garzarella wrote:
> > After waking up a suspended VM, the kernel prints the following trace
> > for virtio drivers which do not directly call virtio_device_ready() in
> > the .restore:
> >
> >     PM: suspend exit
> >     irq 22: nobody cared (try booting with the "irqpoll" option)
> >     Call Trace:
> >      <IRQ>
> >      dump_stack_lvl+0x38/0x49
> >      dump_stack+0x10/0x12
> >      __report_bad_irq+0x3a/0xaf
> >      note_interrupt.cold+0xb/0x60
> >      handle_irq_event+0x71/0x80
> >      handle_fasteoi_irq+0x95/0x1e0
> >      __common_interrupt+0x6b/0x110
> >      common_interrupt+0x63/0xe0
> >      asm_common_interrupt+0x1e/0x40
> >      ? __do_softirq+0x75/0x2f3
> >      irq_exit_rcu+0x93/0xe0
> >      sysvec_apic_timer_interrupt+0xac/0xd0
> >      </IRQ>
> >      <TASK>
> >      asm_sysvec_apic_timer_interrupt+0x12/0x20
> >      arch_cpu_idle+0x12/0x20
> >      default_idle_call+0x39/0xf0
> >      do_idle+0x1b5/0x210
> >      cpu_startup_entry+0x20/0x30
> >      start_secondary+0xf3/0x100
> >      secondary_startup_64_no_verify+0xc3/0xcb
> >      </TASK>
> >     handlers:
> >     [<000000008f9bac49>] vp_interrupt
> >     [<000000008f9bac49>] vp_interrupt
> >     Disabling IRQ #22
> >
> > This happens because we don't invoke .enable_cbs callback in
> > virtio_device_restore(). That callback is used by some transports
> > (e.g. virtio-pci) to enable interrupts.
> >
> > Let's fix it, by calling virtio_device_ready() as we do in
> > virtio_dev_probe(). This function calls .enable_cts callback and sets
> > DRIVER_OK status bit.
> >
> > This fix also avoids setting DRIVER_OK twice for those drivers that
> > call virtio_device_ready() in the .restore.
> >
> > Fixes: d50497eb4e55 ("virtio_config: introduce a new .enable_cbs method")
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> >
> > I'm not sure about the fixes tag. That one is more generic, but the
> > following one I think introduced the issue.
> >
> > Fixes: 9e35276a5344 ("virtio_pci: harden MSI-X interrupts")
>
> Jason what should we do about this one BTW? Just revert? We have other
> issues ...

Let me post a patch to revert it and give it a rework.

Thanks

>
>
> > Thanks,
> > Stefano
> > ---
> >  drivers/virtio/virtio.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index 22f15f444f75..75c8d560bbd3 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -526,8 +526,9 @@ int virtio_device_restore(struct virtio_device *dev)
> >                       goto err;
> >       }
> >
> > -     /* Finally, tell the device we're all set */
> > -     virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> > +     /* If restore didn't do it, mark device DRIVER_OK ourselves. */
> > +     if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
> > +             virtio_device_ready(dev);
> >
> >       virtio_config_enable(dev);
> >
> > --
> > 2.35.1
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] virtio: use virtio_device_ready() in virtio_device_restore()
  2022-03-23  3:10   ` Jason Wang
@ 2022-03-23  8:04     ` Stefano Garzarella
  2022-03-23  8:16       ` Jason Wang
  0 siblings, 1 reply; 5+ messages in thread
From: Stefano Garzarella @ 2022-03-23  8:04 UTC (permalink / raw)
  To: Jason Wang; +Cc: virtualization, linux-kernel, Michael S. Tsirkin

On Wed, Mar 23, 2022 at 11:10:27AM +0800, Jason Wang wrote:
>On Tue, Mar 22, 2022 at 10:07 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Tue, Mar 22, 2022 at 12:43:13PM +0100, Stefano Garzarella wrote:
>> > After waking up a suspended VM, the kernel prints the following trace
>> > for virtio drivers which do not directly call virtio_device_ready() in
>> > the .restore:
>> >
>> >     PM: suspend exit
>> >     irq 22: nobody cared (try booting with the "irqpoll" option)
>> >     Call Trace:
>> >      <IRQ>
>> >      dump_stack_lvl+0x38/0x49
>> >      dump_stack+0x10/0x12
>> >      __report_bad_irq+0x3a/0xaf
>> >      note_interrupt.cold+0xb/0x60
>> >      handle_irq_event+0x71/0x80
>> >      handle_fasteoi_irq+0x95/0x1e0
>> >      __common_interrupt+0x6b/0x110
>> >      common_interrupt+0x63/0xe0
>> >      asm_common_interrupt+0x1e/0x40
>> >      ? __do_softirq+0x75/0x2f3
>> >      irq_exit_rcu+0x93/0xe0
>> >      sysvec_apic_timer_interrupt+0xac/0xd0
>> >      </IRQ>
>> >      <TASK>
>> >      asm_sysvec_apic_timer_interrupt+0x12/0x20
>> >      arch_cpu_idle+0x12/0x20
>> >      default_idle_call+0x39/0xf0
>> >      do_idle+0x1b5/0x210
>> >      cpu_startup_entry+0x20/0x30
>> >      start_secondary+0xf3/0x100
>> >      secondary_startup_64_no_verify+0xc3/0xcb
>> >      </TASK>
>> >     handlers:
>> >     [<000000008f9bac49>] vp_interrupt
>> >     [<000000008f9bac49>] vp_interrupt
>> >     Disabling IRQ #22
>> >
>> > This happens because we don't invoke .enable_cbs callback in
>> > virtio_device_restore(). That callback is used by some transports
>> > (e.g. virtio-pci) to enable interrupts.
>> >
>> > Let's fix it, by calling virtio_device_ready() as we do in
>> > virtio_dev_probe(). This function calls .enable_cts callback and sets
>> > DRIVER_OK status bit.
>> >
>> > This fix also avoids setting DRIVER_OK twice for those drivers that
>> > call virtio_device_ready() in the .restore.
>> >
>> > Fixes: d50497eb4e55 ("virtio_config: introduce a new .enable_cbs method")
>> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> > ---
>> >
>> > I'm not sure about the fixes tag. That one is more generic, but the
>> > following one I think introduced the issue.
>> >
>> > Fixes: 9e35276a5344 ("virtio_pci: harden MSI-X interrupts")
>>
>> Jason what should we do about this one BTW? Just revert? We have other
>> issues ...
>
>Let me post a patch to revert it and give it a rework.

Thanks for reverting it.

Should we queue this patch anyway to prevent future issues and avoid 
setting DRIVER_OK twice?

Please, let me know if I have to resend it by removing the call trace 
that after the revert should no longer occur.

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] virtio: use virtio_device_ready() in virtio_device_restore()
  2022-03-23  8:04     ` Stefano Garzarella
@ 2022-03-23  8:16       ` Jason Wang
  0 siblings, 0 replies; 5+ messages in thread
From: Jason Wang @ 2022-03-23  8:16 UTC (permalink / raw)
  To: Stefano Garzarella; +Cc: virtualization, linux-kernel, Michael S. Tsirkin

On Wed, Mar 23, 2022 at 4:04 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Wed, Mar 23, 2022 at 11:10:27AM +0800, Jason Wang wrote:
> >On Tue, Mar 22, 2022 at 10:07 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >>
> >> On Tue, Mar 22, 2022 at 12:43:13PM +0100, Stefano Garzarella wrote:
> >> > After waking up a suspended VM, the kernel prints the following trace
> >> > for virtio drivers which do not directly call virtio_device_ready() in
> >> > the .restore:
> >> >
> >> >     PM: suspend exit
> >> >     irq 22: nobody cared (try booting with the "irqpoll" option)
> >> >     Call Trace:
> >> >      <IRQ>
> >> >      dump_stack_lvl+0x38/0x49
> >> >      dump_stack+0x10/0x12
> >> >      __report_bad_irq+0x3a/0xaf
> >> >      note_interrupt.cold+0xb/0x60
> >> >      handle_irq_event+0x71/0x80
> >> >      handle_fasteoi_irq+0x95/0x1e0
> >> >      __common_interrupt+0x6b/0x110
> >> >      common_interrupt+0x63/0xe0
> >> >      asm_common_interrupt+0x1e/0x40
> >> >      ? __do_softirq+0x75/0x2f3
> >> >      irq_exit_rcu+0x93/0xe0
> >> >      sysvec_apic_timer_interrupt+0xac/0xd0
> >> >      </IRQ>
> >> >      <TASK>
> >> >      asm_sysvec_apic_timer_interrupt+0x12/0x20
> >> >      arch_cpu_idle+0x12/0x20
> >> >      default_idle_call+0x39/0xf0
> >> >      do_idle+0x1b5/0x210
> >> >      cpu_startup_entry+0x20/0x30
> >> >      start_secondary+0xf3/0x100
> >> >      secondary_startup_64_no_verify+0xc3/0xcb
> >> >      </TASK>
> >> >     handlers:
> >> >     [<000000008f9bac49>] vp_interrupt
> >> >     [<000000008f9bac49>] vp_interrupt
> >> >     Disabling IRQ #22
> >> >
> >> > This happens because we don't invoke .enable_cbs callback in
> >> > virtio_device_restore(). That callback is used by some transports
> >> > (e.g. virtio-pci) to enable interrupts.
> >> >
> >> > Let's fix it, by calling virtio_device_ready() as we do in
> >> > virtio_dev_probe(). This function calls .enable_cts callback and sets
> >> > DRIVER_OK status bit.
> >> >
> >> > This fix also avoids setting DRIVER_OK twice for those drivers that
> >> > call virtio_device_ready() in the .restore.
> >> >
> >> > Fixes: d50497eb4e55 ("virtio_config: introduce a new .enable_cbs method")
> >> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >> > ---
> >> >
> >> > I'm not sure about the fixes tag. That one is more generic, but the
> >> > following one I think introduced the issue.
> >> >
> >> > Fixes: 9e35276a5344 ("virtio_pci: harden MSI-X interrupts")
> >>
> >> Jason what should we do about this one BTW? Just revert? We have other
> >> issues ...
> >
> >Let me post a patch to revert it and give it a rework.
>
> Thanks for reverting it.
>
> Should we queue this patch anyway to prevent future issues and avoid
> setting DRIVER_OK twice?
>
> Please, let me know if I have to resend it by removing the call trace
> that after the revert should no longer occur.

I think we can have another version.

Thanks

>
> Thanks,
> Stefano
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-03-23  8:16 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-22 11:43 [PATCH] virtio: use virtio_device_ready() in virtio_device_restore() Stefano Garzarella
2022-03-22 14:07 ` Michael S. Tsirkin
2022-03-23  3:10   ` Jason Wang
2022-03-23  8:04     ` Stefano Garzarella
2022-03-23  8:16       ` Jason Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).