[PATCH] virtio: Remove virtio devices on device_shutdown()

[PATCH] virtio: Remove virtio devices on device_shutdown()

[Kirill A. Shutemov]

Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
accesses during the hang.

	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
	...

It was traced down to virtio-console. Kexec works fine if virtio-console
is not in use.

Looks like virtio-console continues to write to the MMIO even after
underlying virtio-pci device is removed.

The problem can be mitigated by removing all virtio devices on virtio
bus shutdown.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
---
 drivers/virtio/virtio.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index a9b93e99c23a..6c2f908eb22c 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
 	of_node_put(dev->dev.of_node);
 }
 
+static void virtio_dev_shutdown(struct device *_d)
+{
+	struct virtio_device *dev = dev_to_virtio(_d);
+	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
+
+	if (drv && drv->remove)
+		drv->remove(dev);
+}
+
 static const struct bus_type virtio_bus = {
 	.name  = "virtio",
 	.match = virtio_dev_match,
@@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
 	.uevent = virtio_uevent,
 	.probe = virtio_dev_probe,
 	.remove = virtio_dev_remove,
+	.shutdown = virtio_dev_shutdown,
 };
 
 int __register_virtio_driver(struct virtio_driver *driver, struct module *owner)
-- 
2.43.0

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Jiri Pirko]

Thu, Aug 08, 2024 at 09:51:41AM CEST, kirill.shutemov@linux.intel.com wrote:
>Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
>accesses during the hang.
>
>	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
>	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
>	...
>
>It was traced down to virtio-console. Kexec works fine if virtio-console
>is not in use.
>
>Looks like virtio-console continues to write to the MMIO even after
>underlying virtio-pci device is removed.
>
>The problem can be mitigated by removing all virtio devices on virtio
>bus shutdown.
>
>Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>

Could you provide a "Fixes:" tag pointing to the commit that introduced
the bug?



>---
> drivers/virtio/virtio.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
>diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>index a9b93e99c23a..6c2f908eb22c 100644
>--- a/drivers/virtio/virtio.c
>+++ b/drivers/virtio/virtio.c
>@@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
> 	of_node_put(dev->dev.of_node);
> }
> 
>+static void virtio_dev_shutdown(struct device *_d)
>+{
>+	struct virtio_device *dev = dev_to_virtio(_d);
>+	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>+
>+	if (drv && drv->remove)
>+		drv->remove(dev);
>+}
>+
> static const struct bus_type virtio_bus = {
> 	.name  = "virtio",
> 	.match = virtio_dev_match,
>@@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
> 	.uevent = virtio_uevent,
> 	.probe = virtio_dev_probe,
> 	.remove = virtio_dev_remove,
>+	.shutdown = virtio_dev_shutdown,
> };
> 
> int __register_virtio_driver(struct virtio_driver *driver, struct module *owner)
>-- 
>2.43.0
>
>

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Michael S. Tsirkin]

On Thu, Aug 08, 2024 at 10:51:41AM +0300, Kirill A. Shutemov wrote:
> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> accesses during the hang.
> 
> 	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
> 	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
> 	...
> 
> It was traced down to virtio-console. Kexec works fine if virtio-console
> is not in use.

virtio is not doing a lot of 16 bit reads.
Are these the reads:

                virtio_cread(vdev, struct virtio_console_config, cols, &cols);
                virtio_cread(vdev, struct virtio_console_config, rows, &rows);

?

write is a bit puzzling too. This one?

bool vp_notify(struct virtqueue *vq)
{       
        /* we write the queue's selector into the notification register to
         * signal the other end */
        iowrite16(vq->index, (void __iomem *)vq->priv);
        return true;
}
        
> 
> Looks like virtio-console continues to write to the MMIO even after
> underlying virtio-pci device is removed.

You mention both MMIO and pci, I am confused.
Removed by what? In what sense?




> 
> The problem can be mitigated by removing all virtio devices on virtio
> bus shutdown.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>

A bit worried about doing so much activity on shutdown,
and for all devices, too. I'd like to understand what
is going on a bit better - could be a symptom of
a bigger problem (e.g. missing handling for suprise
removal?).




> ---
>  drivers/virtio/virtio.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a9b93e99c23a..6c2f908eb22c 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
>  	of_node_put(dev->dev.of_node);
>  }
>  
> +static void virtio_dev_shutdown(struct device *_d)
> +{
> +	struct virtio_device *dev = dev_to_virtio(_d);
> +	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> +
> +	if (drv && drv->remove)
> +		drv->remove(dev);
> +}
> +
>  static const struct bus_type virtio_bus = {
>  	.name  = "virtio",
>  	.match = virtio_dev_match,
> @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
>  	.uevent = virtio_uevent,
>  	.probe = virtio_dev_probe,
>  	.remove = virtio_dev_remove,
> +	.shutdown = virtio_dev_shutdown,
>  };
>  
>  int __register_virtio_driver(struct virtio_driver *driver, struct module *owner)
> -- 
> 2.43.0

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Kirill A. Shutemov]

On Thu, Aug 08, 2024 at 01:37:42PM +0200, Jiri Pirko wrote:
> Thu, Aug 08, 2024 at 09:51:41AM CEST, kirill.shutemov@linux.intel.com wrote:
> >Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> >accesses during the hang.
> >
> >	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
> >	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
> >	...
> >
> >It was traced down to virtio-console. Kexec works fine if virtio-console
> >is not in use.
> >
> >Looks like virtio-console continues to write to the MMIO even after
> >underlying virtio-pci device is removed.
> >
> >The problem can be mitigated by removing all virtio devices on virtio
> >bus shutdown.
> >
> >Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> >Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> 
> Could you provide a "Fixes:" tag pointing to the commit that introduced
> the bug?

I am not sure if it is a regression. Maybe it was there forever.

Hongyu, could you please check with bisect if it is possible to point to a
specific commit?

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Kirill A. Shutemov]

On Thu, Aug 08, 2024 at 08:10:34AM -0400, Michael S. Tsirkin wrote:
> On Thu, Aug 08, 2024 at 10:51:41AM +0300, Kirill A. Shutemov wrote:
> > Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> > accesses during the hang.
> > 
> > 	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
> > 	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
> > 	...
> > 
> > It was traced down to virtio-console. Kexec works fine if virtio-console
> > is not in use.
> 
> virtio is not doing a lot of 16 bit reads.
> Are these the reads:
> 
>                 virtio_cread(vdev, struct virtio_console_config, cols, &cols);
>                 virtio_cread(vdev, struct virtio_console_config, rows, &rows);
> 
> ?
> 
> write is a bit puzzling too. This one?
> 
> bool vp_notify(struct virtqueue *vq)
> {       
>         /* we write the queue's selector into the notification register to
>          * signal the other end */
>         iowrite16(vq->index, (void __iomem *)vq->priv);
>         return true;
> }

Given that we are talking about console issue, any suggestion on how to
check?

> > 
> > Looks like virtio-console continues to write to the MMIO even after
> > underlying virtio-pci device is removed.
> 
> You mention both MMIO and pci, I am confused.

By MMIO, I mean accesses to PCI BARs. But it is only my *guess* on the
situation, I have limited knowledge of the area. I am not drivers guy.

> Removed by what? In what sense?

So device_shutdown() iterates over all device and we hit the problem when
we get to virtio-pci devices and call pci_device_shutdown on them.

I *think* PCI BAR (or something else?) becomes unavailable after that but
it is still accessed.

> > 
> > The problem can be mitigated by removing all virtio devices on virtio
> > bus shutdown.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> 
> A bit worried about doing so much activity on shutdown,
> and for all devices, too. I'd like to understand what
> is going on a bit better - could be a symptom of
> a bigger problem (e.g. missing handling for suprise
> removal?).

I probably should have marked the patch as RFC. The patch was intended to
start conversation. I am not sure it is correct. This patch just happened
to work in our setup.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Michael S. Tsirkin]

On Thu, Aug 08, 2024 at 04:15:25PM +0300, Kirill A. Shutemov wrote:
> On Thu, Aug 08, 2024 at 08:10:34AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Aug 08, 2024 at 10:51:41AM +0300, Kirill A. Shutemov wrote:
> > > Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> > > accesses during the hang.
> > > 
> > > 	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
> > > 	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
> > > 	...
> > > 
> > > It was traced down to virtio-console. Kexec works fine if virtio-console
> > > is not in use.
> > 
> > virtio is not doing a lot of 16 bit reads.
> > Are these the reads:
> > 
> >                 virtio_cread(vdev, struct virtio_console_config, cols, &cols);
> >                 virtio_cread(vdev, struct virtio_console_config, rows, &rows);
> > 
> > ?
> > 
> > write is a bit puzzling too. This one?
> > 
> > bool vp_notify(struct virtqueue *vq)
> > {       
> >         /* we write the queue's selector into the notification register to
> >          * signal the other end */
> >         iowrite16(vq->index, (void __iomem *)vq->priv);
> >         return true;
> > }
> 
> Given that we are talking about console issue, any suggestion on how to
> check?


If you do lspci -v on the device, we'll know where the BARs are,
and can compare to 0x102877002, 0x102877A44.

> > > 
> > > Looks like virtio-console continues to write to the MMIO even after
> > > underlying virtio-pci device is removed.
> > 
> > You mention both MMIO and pci, I am confused.
> 
> By MMIO, I mean accesses to PCI BARs. But it is only my *guess* on the
> situation, I have limited knowledge of the area. I am not drivers guy.
> 
> > Removed by what? In what sense?
> 
> So device_shutdown() iterates over all device and we hit the problem when
> we get to virtio-pci devices and call pci_device_shutdown on them.

Hmm that clears bus master.  So maybe what we see is actually
device trying to do DMA and failing? We'll need to know where do
these addresses are on your system.

> I *think* PCI BAR (or something else?) becomes unavailable after that but
> it is still accessed.
> 
> > > 
> > > The problem can be mitigated by removing all virtio devices on virtio
> > > bus shutdown.
> > > 
> > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> > 
> > A bit worried about doing so much activity on shutdown,
> > and for all devices, too. I'd like to understand what
> > is going on a bit better - could be a symptom of
> > a bigger problem (e.g. missing handling for suprise
> > removal?).
> 
> I probably should have marked the patch as RFC. The patch was intended to
> start conversation. I am not sure it is correct. This patch just happened
> to work in our setup.
> 
> -- 
>   Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Kirill A. Shutemov]

On Thu, Aug 08, 2024 at 11:03:30AM -0400, Michael S. Tsirkin wrote:
> On Thu, Aug 08, 2024 at 04:15:25PM +0300, Kirill A. Shutemov wrote:
> > On Thu, Aug 08, 2024 at 08:10:34AM -0400, Michael S. Tsirkin wrote:
> > > On Thu, Aug 08, 2024 at 10:51:41AM +0300, Kirill A. Shutemov wrote:
> > > > Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> > > > accesses during the hang.
> > > > 
> > > > 	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
> > > > 	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
> > > > 	...
> > > > 
> > > > It was traced down to virtio-console. Kexec works fine if virtio-console
> > > > is not in use.
> > > 
> > > virtio is not doing a lot of 16 bit reads.
> > > Are these the reads:
> > > 
> > >                 virtio_cread(vdev, struct virtio_console_config, cols, &cols);
> > >                 virtio_cread(vdev, struct virtio_console_config, rows, &rows);
> > > 
> > > ?
> > > 
> > > write is a bit puzzling too. This one?
> > > 
> > > bool vp_notify(struct virtqueue *vq)
> > > {       
> > >         /* we write the queue's selector into the notification register to
> > >          * signal the other end */
> > >         iowrite16(vq->index, (void __iomem *)vq->priv);
> > >         return true;
> > > }
> > 
> > Given that we are talking about console issue, any suggestion on how to
> > check?
> 
> 
> If you do lspci -v on the device, we'll know where the BARs are,
> and can compare to 0x102877002, 0x102877A44.

00:01.0 Ethernet controller: Red Hat, Inc. Virtio 1.0 network device (rev 01)
	Subsystem: Red Hat, Inc. Device 1100
	Flags: bus master, fast devsel, latency 0, IRQ 21
	Memory at 80005000 (32-bit, non-prefetchable) [size=4K]
	Memory at 380000000000 (64-bit, prefetchable) [size=16K]
	Capabilities: [98] MSI-X: Enable+ Count=4 Masked-
	Capabilities: [84] Vendor Specific Information: VirtIO: <unknown>
	Capabilities: [70] Vendor Specific Information: VirtIO: Notify
	Capabilities: [60] Vendor Specific Information: VirtIO: DeviceCfg
	Capabilities: [50] Vendor Specific Information: VirtIO: ISR
	Capabilities: [40] Vendor Specific Information: VirtIO: CommonCfg
	Kernel driver in use: virtio-pci

00:02.0 Communication controller: Red Hat, Inc. Virtio 1.0 socket (rev 01)
	Subsystem: Red Hat, Inc. Device 1100
	Flags: bus master, fast devsel, latency 0, IRQ 22
	Memory at 80004000 (32-bit, non-prefetchable) [size=4K]
	Memory at 380000004000 (64-bit, prefetchable) [size=16K]
	Capabilities: [98] MSI-X: Enable- Count=3 Masked-
	Capabilities: [84] Vendor Specific Information: VirtIO: <unknown>
	Capabilities: [70] Vendor Specific Information: VirtIO: Notify
	Capabilities: [60] Vendor Specific Information: VirtIO: DeviceCfg
	Capabilities: [50] Vendor Specific Information: VirtIO: ISR
	Capabilities: [40] Vendor Specific Information: VirtIO: CommonCfg
	Kernel driver in use: virtio-pci

00:03.0 Communication controller: Red Hat, Inc. Virtio 1.0 console (rev 01)
	Subsystem: Red Hat, Inc. Device 1100
	Flags: bus master, fast devsel, latency 0, IRQ 23
	Memory at 80003000 (32-bit, non-prefetchable) [size=4K]
	Memory at 380000008000 (64-bit, prefetchable) [size=16K]
	Capabilities: [98] MSI-X: Enable+ Count=2 Masked-
	Capabilities: [84] Vendor Specific Information: VirtIO: <unknown>
	Capabilities: [70] Vendor Specific Information: VirtIO: Notify
	Capabilities: [60] Vendor Specific Information: VirtIO: DeviceCfg
	Capabilities: [50] Vendor Specific Information: VirtIO: ISR
	Capabilities: [40] Vendor Specific Information: VirtIO: CommonCfg
	Kernel driver in use: virtio-pci

00:04.0 SCSI storage controller: Red Hat, Inc. Virtio 1.0 block device (rev 01)
	Subsystem: Red Hat, Inc. Device 1100
	Flags: bus master, fast devsel, latency 0, IRQ 20
	Memory at 80002000 (32-bit, non-prefetchable) [size=4K]
	Memory at 38000000c000 (64-bit, prefetchable) [size=16K]
	Capabilities: [98] MSI-X: Enable+ Count=17 Masked-
	Capabilities: [84] Vendor Specific Information: VirtIO: <unknown>
	Capabilities: [70] Vendor Specific Information: VirtIO: Notify
	Capabilities: [60] Vendor Specific Information: VirtIO: DeviceCfg
	Capabilities: [50] Vendor Specific Information: VirtIO: ISR
	Capabilities: [40] Vendor Specific Information: VirtIO: CommonCfg
	Kernel driver in use: virtio-pci

00:05.0 SCSI storage controller: Red Hat, Inc. Virtio 1.0 block device (rev 01)
	Subsystem: Red Hat, Inc. Device 1100
	Flags: bus master, fast devsel, latency 0, IRQ 21
	Memory at 80001000 (32-bit, non-prefetchable) [size=4K]
	Memory at 380000010000 (64-bit, prefetchable) [size=16K]
	Capabilities: [98] MSI-X: Enable+ Count=17 Masked-
	Capabilities: [84] Vendor Specific Information: VirtIO: <unknown>
	Capabilities: [70] Vendor Specific Information: VirtIO: Notify
	Capabilities: [60] Vendor Specific Information: VirtIO: DeviceCfg
	Capabilities: [50] Vendor Specific Information: VirtIO: ISR
	Capabilities: [40] Vendor Specific Information: VirtIO: CommonCfg
	Kernel driver in use: virtio-pci
....
Invalid read at addr 0x100C37904, size 2, region '(null)', reason: rejected
Invalid read at addr 0x1036F9002, size 2, region '(null)', reason: rejected
Invalid read at addr 0x1036F9002, size 2, region '(null)', reason: rejected
Invalid write at addr 0x1036F9A44, size 2, region '(null)', reason: rejected
Invalid read at addr 0x1036F7002, size 2, region '(null)', reason: rejected
Invalid read at addr 0x1036F7002, size 2, region '(null)', reason: rejected
Invalid write at addr 0x1036F7A44, size 2, region '(null)', reason: rejected
....

Yeah, looks like it is not BARs.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Jiri Pirko]

Thu, Aug 08, 2024 at 02:11:09PM CEST, kirill.shutemov@linux.intel.com wrote:
>On Thu, Aug 08, 2024 at 01:37:42PM +0200, Jiri Pirko wrote:
>> Thu, Aug 08, 2024 at 09:51:41AM CEST, kirill.shutemov@linux.intel.com wrote:
>> >Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
>> >accesses during the hang.
>> >
>> >	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
>> >	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
>> >	...
>> >
>> >It was traced down to virtio-console. Kexec works fine if virtio-console
>> >is not in use.
>> >
>> >Looks like virtio-console continues to write to the MMIO even after
>> >underlying virtio-pci device is removed.
>> >
>> >The problem can be mitigated by removing all virtio devices on virtio
>> >bus shutdown.
>> >
>> >Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> >Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
>> 
>> Could you provide a "Fixes:" tag pointing to the commit that introduced
>> the bug?
>
>I am not sure if it is a regression. Maybe it was there forever.

Does not have to be regression. You can blame the commit that added the
code.


>
>Hongyu, could you please check with bisect if it is possible to point to a
>specific commit?
>
>-- 
>  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Kirill A. Shutemov]

On Thu, Aug 08, 2024 at 06:28:02PM +0300, Kirill A. Shutemov wrote:
> On Thu, Aug 08, 2024 at 11:03:30AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Aug 08, 2024 at 04:15:25PM +0300, Kirill A. Shutemov wrote:
> > > On Thu, Aug 08, 2024 at 08:10:34AM -0400, Michael S. Tsirkin wrote:
> > > > On Thu, Aug 08, 2024 at 10:51:41AM +0300, Kirill A. Shutemov wrote:
> > > > > Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> > > > > accesses during the hang.
> > > > > 
> > > > > 	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
> > > > > 	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
> > > > > 	...
> > > > > 
> > > > > It was traced down to virtio-console. Kexec works fine if virtio-console
> > > > > is not in use.
> > > > 
> > > > virtio is not doing a lot of 16 bit reads.
> > > > Are these the reads:
> > > > 
> > > >                 virtio_cread(vdev, struct virtio_console_config, cols, &cols);
> > > >                 virtio_cread(vdev, struct virtio_console_config, rows, &rows);
> > > > 
> > > > ?
> > > > 
> > > > write is a bit puzzling too. This one?
> > > > 
> > > > bool vp_notify(struct virtqueue *vq)
> > > > {       
> > > >         /* we write the queue's selector into the notification register to
> > > >          * signal the other end */
> > > >         iowrite16(vq->index, (void __iomem *)vq->priv);
> > > >         return true;
> > > > }
> > > 
> > > Given that we are talking about console issue, any suggestion on how to
> > > check?
> > 
> > 
> > If you do lspci -v on the device, we'll know where the BARs are,
> > and can compare to 0x102877002, 0x102877A44.
> 
> 00:01.0 Ethernet controller: Red Hat, Inc. Virtio 1.0 network device (rev 01)
> 	Subsystem: Red Hat, Inc. Device 1100
> 	Flags: bus master, fast devsel, latency 0, IRQ 21
> 	Memory at 80005000 (32-bit, non-prefetchable) [size=4K]
> 	Memory at 380000000000 (64-bit, prefetchable) [size=16K]
> 	Capabilities: [98] MSI-X: Enable+ Count=4 Masked-
> 	Capabilities: [84] Vendor Specific Information: VirtIO: <unknown>
> 	Capabilities: [70] Vendor Specific Information: VirtIO: Notify
> 	Capabilities: [60] Vendor Specific Information: VirtIO: DeviceCfg
> 	Capabilities: [50] Vendor Specific Information: VirtIO: ISR
> 	Capabilities: [40] Vendor Specific Information: VirtIO: CommonCfg
> 	Kernel driver in use: virtio-pci
> 
> 00:02.0 Communication controller: Red Hat, Inc. Virtio 1.0 socket (rev 01)
> 	Subsystem: Red Hat, Inc. Device 1100
> 	Flags: bus master, fast devsel, latency 0, IRQ 22
> 	Memory at 80004000 (32-bit, non-prefetchable) [size=4K]
> 	Memory at 380000004000 (64-bit, prefetchable) [size=16K]
> 	Capabilities: [98] MSI-X: Enable- Count=3 Masked-
> 	Capabilities: [84] Vendor Specific Information: VirtIO: <unknown>
> 	Capabilities: [70] Vendor Specific Information: VirtIO: Notify
> 	Capabilities: [60] Vendor Specific Information: VirtIO: DeviceCfg
> 	Capabilities: [50] Vendor Specific Information: VirtIO: ISR
> 	Capabilities: [40] Vendor Specific Information: VirtIO: CommonCfg
> 	Kernel driver in use: virtio-pci
> 
> 00:03.0 Communication controller: Red Hat, Inc. Virtio 1.0 console (rev 01)
> 	Subsystem: Red Hat, Inc. Device 1100
> 	Flags: bus master, fast devsel, latency 0, IRQ 23
> 	Memory at 80003000 (32-bit, non-prefetchable) [size=4K]
> 	Memory at 380000008000 (64-bit, prefetchable) [size=16K]
> 	Capabilities: [98] MSI-X: Enable+ Count=2 Masked-
> 	Capabilities: [84] Vendor Specific Information: VirtIO: <unknown>
> 	Capabilities: [70] Vendor Specific Information: VirtIO: Notify
> 	Capabilities: [60] Vendor Specific Information: VirtIO: DeviceCfg
> 	Capabilities: [50] Vendor Specific Information: VirtIO: ISR
> 	Capabilities: [40] Vendor Specific Information: VirtIO: CommonCfg
> 	Kernel driver in use: virtio-pci
> 
> 00:04.0 SCSI storage controller: Red Hat, Inc. Virtio 1.0 block device (rev 01)
> 	Subsystem: Red Hat, Inc. Device 1100
> 	Flags: bus master, fast devsel, latency 0, IRQ 20
> 	Memory at 80002000 (32-bit, non-prefetchable) [size=4K]
> 	Memory at 38000000c000 (64-bit, prefetchable) [size=16K]
> 	Capabilities: [98] MSI-X: Enable+ Count=17 Masked-
> 	Capabilities: [84] Vendor Specific Information: VirtIO: <unknown>
> 	Capabilities: [70] Vendor Specific Information: VirtIO: Notify
> 	Capabilities: [60] Vendor Specific Information: VirtIO: DeviceCfg
> 	Capabilities: [50] Vendor Specific Information: VirtIO: ISR
> 	Capabilities: [40] Vendor Specific Information: VirtIO: CommonCfg
> 	Kernel driver in use: virtio-pci
> 
> 00:05.0 SCSI storage controller: Red Hat, Inc. Virtio 1.0 block device (rev 01)
> 	Subsystem: Red Hat, Inc. Device 1100
> 	Flags: bus master, fast devsel, latency 0, IRQ 21
> 	Memory at 80001000 (32-bit, non-prefetchable) [size=4K]
> 	Memory at 380000010000 (64-bit, prefetchable) [size=16K]
> 	Capabilities: [98] MSI-X: Enable+ Count=17 Masked-
> 	Capabilities: [84] Vendor Specific Information: VirtIO: <unknown>
> 	Capabilities: [70] Vendor Specific Information: VirtIO: Notify
> 	Capabilities: [60] Vendor Specific Information: VirtIO: DeviceCfg
> 	Capabilities: [50] Vendor Specific Information: VirtIO: ISR
> 	Capabilities: [40] Vendor Specific Information: VirtIO: CommonCfg
> 	Kernel driver in use: virtio-pci
> ....
> Invalid read at addr 0x100C37904, size 2, region '(null)', reason: rejected
> Invalid read at addr 0x1036F9002, size 2, region '(null)', reason: rejected
> Invalid read at addr 0x1036F9002, size 2, region '(null)', reason: rejected
> Invalid write at addr 0x1036F9A44, size 2, region '(null)', reason: rejected
> Invalid read at addr 0x1036F7002, size 2, region '(null)', reason: rejected
> Invalid read at addr 0x1036F7002, size 2, region '(null)', reason: rejected
> Invalid write at addr 0x1036F7A44, size 2, region '(null)', reason: rejected
> ....
> 
> Yeah, looks like it is not BARs.

Michael, can we get back to this?

So it looks like to be a DMA.

It is a TDX guest. Some TDX context: it has concept of private and shared
memory. Private memory in only accessible to the guest. Shared memory is
accessible by both host and guest. It is used for guest/host communication
including DMA.

By default all memory is private and guest kernel converts some memory to
shared. On kexec, we convert all memory back to private, so the next
kernel can start from a known state. This conversion makes DMA impossible.

I think stopping devices before doing this conversion is a reasonable
solution.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Eric Auger]

Hi Kirill, Michael

On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> accesses during the hang.
> 
> 	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
> 	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
> 	...
> 
> It was traced down to virtio-console. Kexec works fine if virtio-console
> is not in use.
> 
> Looks like virtio-console continues to write to the MMIO even after
> underlying virtio-pci device is removed.
> 
> The problem can be mitigated by removing all virtio devices on virtio
> bus shutdown.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>

Gentle ping on that patch that seems to have fallen though the cracks.

I think this fix is really needed. I have another test case with a
rebooting guest exposed with virtio-net (backed by vhost-net) and
viommu. Since there is currently no shutdown for the virtio-net, on
reboot, the IOMMU is disabled through the native_machine_shutdown()/
x86_platform.iommu_shutdown() while the virtio-net is still alive.

Normally device_shutdown() should call virtio-net shutdown before the
IOMMU tear down and we wouldn't see any spurious transactions after
iommu shutdown.

With that fix, the above test case is fixed and I do not see spurious
vhost IOTLB miss spurious requests.

For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
IOTLB callbacks when IOMMU gets disabled,
https://lore.kernel.org/all/20250120173339.865681-1-eric.auger@redhat.com/)


Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric


> ---
>  drivers/virtio/virtio.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a9b93e99c23a..6c2f908eb22c 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
>  	of_node_put(dev->dev.of_node);
>  }
>  
> +static void virtio_dev_shutdown(struct device *_d)
> +{
> +	struct virtio_device *dev = dev_to_virtio(_d);
> +	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> +
> +	if (drv && drv->remove)
> +		drv->remove(dev);
> +}
> +
>  static const struct bus_type virtio_bus = {
>  	.name  = "virtio",
>  	.match = virtio_dev_match,
> @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
>  	.uevent = virtio_uevent,
>  	.probe = virtio_dev_probe,
>  	.remove = virtio_dev_remove,
> +	.shutdown = virtio_dev_shutdown,
>  };
>  
>  int __register_virtio_driver(struct virtio_driver *driver, struct module *owner)

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Michael S. Tsirkin]

On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
> Hi Kirill, Michael
> 
> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
> > Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> > accesses during the hang.
> > 
> > 	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
> > 	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
> > 	...
> > 
> > It was traced down to virtio-console. Kexec works fine if virtio-console
> > is not in use.
> > 
> > Looks like virtio-console continues to write to the MMIO even after
> > underlying virtio-pci device is removed.
> > 
> > The problem can be mitigated by removing all virtio devices on virtio
> > bus shutdown.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> 
> Gentle ping on that patch that seems to have fallen though the cracks.
> 
> I think this fix is really needed. I have another test case with a
> rebooting guest exposed with virtio-net (backed by vhost-net) and
> viommu. Since there is currently no shutdown for the virtio-net, on
> reboot, the IOMMU is disabled through the native_machine_shutdown()/
> x86_platform.iommu_shutdown() while the virtio-net is still alive.
> 
> Normally device_shutdown() should call virtio-net shutdown before the
> IOMMU tear down and we wouldn't see any spurious transactions after
> iommu shutdown.
> 
> With that fix, the above test case is fixed and I do not see spurious
> vhost IOTLB miss spurious requests.
> 
> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
> IOTLB callbacks when IOMMU gets disabled,
> https://lore.kernel.org/all/20250120173339.865681-1-eric.auger@redhat.com/)
> 
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>
> 
> Thanks
> 
> Eric
> 


OK I'll re-review. Thanks!

> > ---
> >  drivers/virtio/virtio.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index a9b93e99c23a..6c2f908eb22c 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
> >  	of_node_put(dev->dev.of_node);
> >  }
> >  
> > +static void virtio_dev_shutdown(struct device *_d)
> > +{
> > +	struct virtio_device *dev = dev_to_virtio(_d);
> > +	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> > +
> > +	if (drv && drv->remove)
> > +		drv->remove(dev);
> > +}
> > +
> >  static const struct bus_type virtio_bus = {
> >  	.name  = "virtio",
> >  	.match = virtio_dev_match,
> > @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
> >  	.uevent = virtio_uevent,
> >  	.probe = virtio_dev_probe,
> >  	.remove = virtio_dev_remove,
> > +	.shutdown = virtio_dev_shutdown,
> >  };
> >  
> >  int __register_virtio_driver(struct virtio_driver *driver, struct module *owner)

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Michael S. Tsirkin]

On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
> Hi Kirill, Michael
> 
> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
> > Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> > accesses during the hang.
> > 
> > 	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
> > 	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
> > 	...
> > 
> > It was traced down to virtio-console. Kexec works fine if virtio-console
> > is not in use.
> > 
> > Looks like virtio-console continues to write to the MMIO even after
> > underlying virtio-pci device is removed.
> > 
> > The problem can be mitigated by removing all virtio devices on virtio
> > bus shutdown.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> 
> Gentle ping on that patch that seems to have fallen though the cracks.
> 
> I think this fix is really needed. I have another test case with a
> rebooting guest exposed with virtio-net (backed by vhost-net) and
> viommu. Since there is currently no shutdown for the virtio-net, on
> reboot, the IOMMU is disabled through the native_machine_shutdown()/
> x86_platform.iommu_shutdown() while the virtio-net is still alive.
> 
> Normally device_shutdown() should call virtio-net shutdown before the
> IOMMU tear down and we wouldn't see any spurious transactions after
> iommu shutdown.
> 
> With that fix, the above test case is fixed and I do not see spurious
> vhost IOTLB miss spurious requests.
> 
> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
> IOTLB callbacks when IOMMU gets disabled,
> https://lore.kernel.org/all/20250120173339.865681-1-eric.auger@redhat.com/)
> 
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>
> 
> Thanks
> 
> Eric
> 
> > ---
> >  drivers/virtio/virtio.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index a9b93e99c23a..6c2f908eb22c 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
> >  	of_node_put(dev->dev.of_node);
> >  }
> >  
> > +static void virtio_dev_shutdown(struct device *_d)
> > +{
> > +	struct virtio_device *dev = dev_to_virtio(_d);
> > +	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> > +
> > +	if (drv && drv->remove)
> > +		drv->remove(dev);


I am concerned that full remove is a heavyweight operation.
Do not want to slow down reboots even more.
How about just doing a reset, instead?

> > +}
> > +
> >  static const struct bus_type virtio_bus = {
> >  	.name  = "virtio",
> >  	.match = virtio_dev_match,
> > @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
> >  	.uevent = virtio_uevent,
> >  	.probe = virtio_dev_probe,
> >  	.remove = virtio_dev_remove,
> > +	.shutdown = virtio_dev_shutdown,
> >  };
> >  
> >  int __register_virtio_driver(struct virtio_driver *driver, struct module *owner)

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Eric Auger]

Hi,

On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
> On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
>> Hi Kirill, Michael
>>
>> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
>>> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
>>> accesses during the hang.
>>>
>>> 	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
>>> 	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
>>> 	...
>>>
>>> It was traced down to virtio-console. Kexec works fine if virtio-console
>>> is not in use.
>>>
>>> Looks like virtio-console continues to write to the MMIO even after
>>> underlying virtio-pci device is removed.
>>>
>>> The problem can be mitigated by removing all virtio devices on virtio
>>> bus shutdown.
>>>
>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
>>
>> Gentle ping on that patch that seems to have fallen though the cracks.
>>
>> I think this fix is really needed. I have another test case with a
>> rebooting guest exposed with virtio-net (backed by vhost-net) and
>> viommu. Since there is currently no shutdown for the virtio-net, on
>> reboot, the IOMMU is disabled through the native_machine_shutdown()/
>> x86_platform.iommu_shutdown() while the virtio-net is still alive.
>>
>> Normally device_shutdown() should call virtio-net shutdown before the
>> IOMMU tear down and we wouldn't see any spurious transactions after
>> iommu shutdown.
>>
>> With that fix, the above test case is fixed and I do not see spurious
>> vhost IOTLB miss spurious requests.
>>
>> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
>> IOTLB callbacks when IOMMU gets disabled,
>> https://lore.kernel.org/all/20250120173339.865681-1-eric.auger@redhat.com/)
>>
>>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> Tested-by: Eric Auger <eric.auger@redhat.com>
>>
>> Thanks
>>
>> Eric
>>
>>> ---
>>>  drivers/virtio/virtio.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>> index a9b93e99c23a..6c2f908eb22c 100644
>>> --- a/drivers/virtio/virtio.c
>>> +++ b/drivers/virtio/virtio.c
>>> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
>>>  	of_node_put(dev->dev.of_node);
>>>  }
>>>  
>>> +static void virtio_dev_shutdown(struct device *_d)
>>> +{
>>> +	struct virtio_device *dev = dev_to_virtio(_d);
>>> +	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>>> +
>>> +	if (drv && drv->remove)
>>> +		drv->remove(dev);
> 
> 
> I am concerned that full remove is a heavyweight operation.
> Do not want to slow down reboots even more.
> How about just doing a reset, instead?

I tested with

static void virtio_dev_shutdown(struct device *_d)
{
        struct virtio_device *dev = dev_to_virtio(_d);

        virtio_reset_device(dev);
}


and it fixes my issue.

Kirill, would that fix you issue too?

Thanks

Eric
> 
>>> +}
>>> +
>>>  static const struct bus_type virtio_bus = {
>>>  	.name  = "virtio",
>>>  	.match = virtio_dev_match,
>>> @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
>>>  	.uevent = virtio_uevent,
>>>  	.probe = virtio_dev_probe,
>>>  	.remove = virtio_dev_remove,
>>> +	.shutdown = virtio_dev_shutdown,
>>>  };
>>>  
>>>  int __register_virtio_driver(struct virtio_driver *driver, struct module *owner)
> 

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Eric Auger]

Hi,

On 2/4/25 12:46 PM, Eric Auger wrote:
> Hi,
> 
> On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
>> On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
>>> Hi Kirill, Michael
>>>
>>> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
>>>> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
>>>> accesses during the hang.
>>>>
>>>> 	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
>>>> 	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
>>>> 	...
>>>>
>>>> It was traced down to virtio-console. Kexec works fine if virtio-console
>>>> is not in use.
>>>>
>>>> Looks like virtio-console continues to write to the MMIO even after
>>>> underlying virtio-pci device is removed.
>>>>
>>>> The problem can be mitigated by removing all virtio devices on virtio
>>>> bus shutdown.
>>>>
>>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
>>>
>>> Gentle ping on that patch that seems to have fallen though the cracks.
>>>
>>> I think this fix is really needed. I have another test case with a
>>> rebooting guest exposed with virtio-net (backed by vhost-net) and
>>> viommu. Since there is currently no shutdown for the virtio-net, on
>>> reboot, the IOMMU is disabled through the native_machine_shutdown()/
>>> x86_platform.iommu_shutdown() while the virtio-net is still alive.
>>>
>>> Normally device_shutdown() should call virtio-net shutdown before the
>>> IOMMU tear down and we wouldn't see any spurious transactions after
>>> iommu shutdown.
>>>
>>> With that fix, the above test case is fixed and I do not see spurious
>>> vhost IOTLB miss spurious requests.
>>>
>>> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
>>> IOTLB callbacks when IOMMU gets disabled,
>>> https://lore.kernel.org/all/20250120173339.865681-1-eric.auger@redhat.com/)
>>>
>>>
>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>> Tested-by: Eric Auger <eric.auger@redhat.com>
>>>
>>> Thanks
>>>
>>> Eric
>>>
>>>> ---
>>>>  drivers/virtio/virtio.c | 10 ++++++++++
>>>>  1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>>> index a9b93e99c23a..6c2f908eb22c 100644
>>>> --- a/drivers/virtio/virtio.c
>>>> +++ b/drivers/virtio/virtio.c
>>>> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
>>>>  	of_node_put(dev->dev.of_node);
>>>>  }
>>>>  
>>>> +static void virtio_dev_shutdown(struct device *_d)
>>>> +{
>>>> +	struct virtio_device *dev = dev_to_virtio(_d);
>>>> +	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>>>> +
>>>> +	if (drv && drv->remove)
>>>> +		drv->remove(dev);
>>
>>
>> I am concerned that full remove is a heavyweight operation.
>> Do not want to slow down reboots even more.
>> How about just doing a reset, instead?
> 
> I tested with
> 
> static void virtio_dev_shutdown(struct device *_d)
> {
>         struct virtio_device *dev = dev_to_virtio(_d);
> 
>         virtio_reset_device(dev);
> }
> 
> 
> and it fixes my issue.
> 
> Kirill, would that fix you issue too?
gentle ping.

this also fixes another issue with qemu vSMMU + virtio-scsi-pci. With
the above addition I get rid of spurious warning in qemu on guest reboot.

qemu-system-aarch64: virtio: zero sized buffers are not allowed
qemu-system-aarch64: vhost vring error in virtqueue 0: Invalid argument (22)

Would you mind if I respin?

Thanks

Eric




> 
> Thanks
> 
> Eric
>>
>>>> +}
>>>> +
>>>>  static const struct bus_type virtio_bus = {
>>>>  	.name  = "virtio",
>>>>  	.match = virtio_dev_match,
>>>> @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
>>>>  	.uevent = virtio_uevent,
>>>>  	.probe = virtio_dev_probe,
>>>>  	.remove = virtio_dev_remove,
>>>> +	.shutdown = virtio_dev_shutdown,
>>>>  };
>>>>  
>>>>  int __register_virtio_driver(struct virtio_driver *driver, struct module *owner)
>>
> 

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Kirill A. Shutemov]

On Thu, Feb 06, 2025 at 09:59:58AM +0100, Eric Auger wrote:
> Hi,
> 
> On 2/4/25 12:46 PM, Eric Auger wrote:
> > Hi,
> > 
> > On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
> >> On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
> >>> Hi Kirill, Michael
> >>>
> >>> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
> >>>> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> >>>> accesses during the hang.
> >>>>
> >>>> 	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
> >>>> 	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
> >>>> 	...
> >>>>
> >>>> It was traced down to virtio-console. Kexec works fine if virtio-console
> >>>> is not in use.
> >>>>
> >>>> Looks like virtio-console continues to write to the MMIO even after
> >>>> underlying virtio-pci device is removed.
> >>>>
> >>>> The problem can be mitigated by removing all virtio devices on virtio
> >>>> bus shutdown.
> >>>>
> >>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> >>>> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> >>>
> >>> Gentle ping on that patch that seems to have fallen though the cracks.
> >>>
> >>> I think this fix is really needed. I have another test case with a
> >>> rebooting guest exposed with virtio-net (backed by vhost-net) and
> >>> viommu. Since there is currently no shutdown for the virtio-net, on
> >>> reboot, the IOMMU is disabled through the native_machine_shutdown()/
> >>> x86_platform.iommu_shutdown() while the virtio-net is still alive.
> >>>
> >>> Normally device_shutdown() should call virtio-net shutdown before the
> >>> IOMMU tear down and we wouldn't see any spurious transactions after
> >>> iommu shutdown.
> >>>
> >>> With that fix, the above test case is fixed and I do not see spurious
> >>> vhost IOTLB miss spurious requests.
> >>>
> >>> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
> >>> IOTLB callbacks when IOMMU gets disabled,
> >>> https://lore.kernel.org/all/20250120173339.865681-1-eric.auger@redhat.com/)
> >>>
> >>>
> >>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> >>> Tested-by: Eric Auger <eric.auger@redhat.com>
> >>>
> >>> Thanks
> >>>
> >>> Eric
> >>>
> >>>> ---
> >>>>  drivers/virtio/virtio.c | 10 ++++++++++
> >>>>  1 file changed, 10 insertions(+)
> >>>>
> >>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> >>>> index a9b93e99c23a..6c2f908eb22c 100644
> >>>> --- a/drivers/virtio/virtio.c
> >>>> +++ b/drivers/virtio/virtio.c
> >>>> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
> >>>>  	of_node_put(dev->dev.of_node);
> >>>>  }
> >>>>  
> >>>> +static void virtio_dev_shutdown(struct device *_d)
> >>>> +{
> >>>> +	struct virtio_device *dev = dev_to_virtio(_d);
> >>>> +	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> >>>> +
> >>>> +	if (drv && drv->remove)
> >>>> +		drv->remove(dev);
> >>
> >>
> >> I am concerned that full remove is a heavyweight operation.
> >> Do not want to slow down reboots even more.
> >> How about just doing a reset, instead?
> > 
> > I tested with
> > 
> > static void virtio_dev_shutdown(struct device *_d)
> > {
> >         struct virtio_device *dev = dev_to_virtio(_d);
> > 
> >         virtio_reset_device(dev);
> > }
> > 
> > 
> > and it fixes my issue.
> > 
> > Kirill, would that fix you issue too?
> gentle ping.

I am on vacation this week. Will try to get around to it next week.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Eric Auger]

On 2/6/25 11:04 AM, Kirill A. Shutemov wrote:
> On Thu, Feb 06, 2025 at 09:59:58AM +0100, Eric Auger wrote:
>> Hi,
>>
>> On 2/4/25 12:46 PM, Eric Auger wrote:
>>> Hi,
>>>
>>> On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
>>>> On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
>>>>> Hi Kirill, Michael
>>>>>
>>>>> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
>>>>>> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
>>>>>> accesses during the hang.
>>>>>>
>>>>>> 	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
>>>>>> 	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
>>>>>> 	...
>>>>>>
>>>>>> It was traced down to virtio-console. Kexec works fine if virtio-console
>>>>>> is not in use.
>>>>>>
>>>>>> Looks like virtio-console continues to write to the MMIO even after
>>>>>> underlying virtio-pci device is removed.
>>>>>>
>>>>>> The problem can be mitigated by removing all virtio devices on virtio
>>>>>> bus shutdown.
>>>>>>
>>>>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>>>> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
>>>>>
>>>>> Gentle ping on that patch that seems to have fallen though the cracks.
>>>>>
>>>>> I think this fix is really needed. I have another test case with a
>>>>> rebooting guest exposed with virtio-net (backed by vhost-net) and
>>>>> viommu. Since there is currently no shutdown for the virtio-net, on
>>>>> reboot, the IOMMU is disabled through the native_machine_shutdown()/
>>>>> x86_platform.iommu_shutdown() while the virtio-net is still alive.
>>>>>
>>>>> Normally device_shutdown() should call virtio-net shutdown before the
>>>>> IOMMU tear down and we wouldn't see any spurious transactions after
>>>>> iommu shutdown.
>>>>>
>>>>> With that fix, the above test case is fixed and I do not see spurious
>>>>> vhost IOTLB miss spurious requests.
>>>>>
>>>>> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
>>>>> IOTLB callbacks when IOMMU gets disabled,
>>>>> https://lore.kernel.org/all/20250120173339.865681-1-eric.auger@redhat.com/)
>>>>>
>>>>>
>>>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>>>> Tested-by: Eric Auger <eric.auger@redhat.com>
>>>>>
>>>>> Thanks
>>>>>
>>>>> Eric
>>>>>
>>>>>> ---
>>>>>>  drivers/virtio/virtio.c | 10 ++++++++++
>>>>>>  1 file changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>>>>> index a9b93e99c23a..6c2f908eb22c 100644
>>>>>> --- a/drivers/virtio/virtio.c
>>>>>> +++ b/drivers/virtio/virtio.c
>>>>>> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
>>>>>>  	of_node_put(dev->dev.of_node);
>>>>>>  }
>>>>>>  
>>>>>> +static void virtio_dev_shutdown(struct device *_d)
>>>>>> +{
>>>>>> +	struct virtio_device *dev = dev_to_virtio(_d);
>>>>>> +	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>>>>>> +
>>>>>> +	if (drv && drv->remove)
>>>>>> +		drv->remove(dev);
>>>>
>>>>
>>>> I am concerned that full remove is a heavyweight operation.
>>>> Do not want to slow down reboots even more.
>>>> How about just doing a reset, instead?
>>>
>>> I tested with
>>>
>>> static void virtio_dev_shutdown(struct device *_d)
>>> {
>>>         struct virtio_device *dev = dev_to_virtio(_d);
>>>
>>>         virtio_reset_device(dev);
>>> }
>>>
>>>
>>> and it fixes my issue.
>>>
>>> Kirill, would that fix you issue too?
>> gentle ping.
> 
> I am on vacation this week. Will try to get around to it next week.
OK. Enjoy your vacation!

Eric
> 

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Ning, Hongyu]

On 2025/2/6 16:59, Eric Auger wrote:
> Hi,
> 
> On 2/4/25 12:46 PM, Eric Auger wrote:
>> Hi,
>>
>> On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
>>> On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
>>>> Hi Kirill, Michael
>>>>
>>>> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
>>>>> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
>>>>> accesses during the hang.
>>>>>
>>>>> 	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
>>>>> 	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
>>>>> 	...
>>>>>
>>>>> It was traced down to virtio-console. Kexec works fine if virtio-console
>>>>> is not in use.
>>>>>
>>>>> Looks like virtio-console continues to write to the MMIO even after
>>>>> underlying virtio-pci device is removed.
>>>>>
>>>>> The problem can be mitigated by removing all virtio devices on virtio
>>>>> bus shutdown.
>>>>>
>>>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>>> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
>>>>
>>>> Gentle ping on that patch that seems to have fallen though the cracks.
>>>>
>>>> I think this fix is really needed. I have another test case with a
>>>> rebooting guest exposed with virtio-net (backed by vhost-net) and
>>>> viommu. Since there is currently no shutdown for the virtio-net, on
>>>> reboot, the IOMMU is disabled through the native_machine_shutdown()/
>>>> x86_platform.iommu_shutdown() while the virtio-net is still alive.
>>>>
>>>> Normally device_shutdown() should call virtio-net shutdown before the
>>>> IOMMU tear down and we wouldn't see any spurious transactions after
>>>> iommu shutdown.
>>>>
>>>> With that fix, the above test case is fixed and I do not see spurious
>>>> vhost IOTLB miss spurious requests.
>>>>
>>>> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
>>>> IOTLB callbacks when IOMMU gets disabled,
>>>> https://lore.kernel.org/all/20250120173339.865681-1-eric.auger@redhat.com/)
>>>>
>>>>
>>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>>> Tested-by: Eric Auger <eric.auger@redhat.com>
>>>>
>>>> Thanks
>>>>
>>>> Eric
>>>>
>>>>> ---
>>>>>   drivers/virtio/virtio.c | 10 ++++++++++
>>>>>   1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>>>> index a9b93e99c23a..6c2f908eb22c 100644
>>>>> --- a/drivers/virtio/virtio.c
>>>>> +++ b/drivers/virtio/virtio.c
>>>>> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
>>>>>   	of_node_put(dev->dev.of_node);
>>>>>   }
>>>>>   
>>>>> +static void virtio_dev_shutdown(struct device *_d)
>>>>> +{
>>>>> +	struct virtio_device *dev = dev_to_virtio(_d);
>>>>> +	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>>>>> +
>>>>> +	if (drv && drv->remove)
>>>>> +		drv->remove(dev);
>>>
>>>
>>> I am concerned that full remove is a heavyweight operation.
>>> Do not want to slow down reboots even more.
>>> How about just doing a reset, instead?
>>
>> I tested with
>>
>> static void virtio_dev_shutdown(struct device *_d)
>> {
>>          struct virtio_device *dev = dev_to_virtio(_d);
>>
>>          virtio_reset_device(dev);
>> }
>>
>>
>> and it fixes my issue.
>>
>> Kirill, would that fix you issue too?

Hi,

sorry for my late response, I synced with Kirill offline and did a retest.

The issue is still reproduced on my side, kexec will be stuck in case of 
"console=hvc0" append in kernel cmdline and even with such patch applied.

my kernel code base is 6.14.0-rc2.

let me know if any more experiments needed.

---
  drivers/virtio/virtio.c | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index ba37665188b5..f9f885d04763 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -395,6 +395,13 @@ static const struct cpumask 
*virtio_irq_get_affinity(struct device *_d,
         return dev->config->get_vq_affinity(dev, irq_vec);
  }

+static void virtio_dev_shutdown(struct device *_d)
+{
+        struct virtio_device *dev = dev_to_virtio(_d);
+
+        virtio_reset_device(dev);
+}
+
  static const struct bus_type virtio_bus = {
         .name  = "virtio",
         .match = virtio_dev_match,
@@ -403,6 +410,7 @@ static const struct bus_type virtio_bus = {
         .probe = virtio_dev_probe,
         .remove = virtio_dev_remove,
         .irq_get_affinity = virtio_irq_get_affinity,
+       .shutdown = virtio_dev_shutdown,
  };

  int __register_virtio_driver(struct virtio_driver *driver, struct 
module *owner)
--
2.43.0


> gentle ping.
> 
> this also fixes another issue with qemu vSMMU + virtio-scsi-pci. With
> the above addition I get rid of spurious warning in qemu on guest reboot.
> 
> qemu-system-aarch64: virtio: zero sized buffers are not allowed
> qemu-system-aarch64: vhost vring error in virtqueue 0: Invalid argument (22)
> 
> Would you mind if I respin?
> 
> Thanks
> 
> Eric
> 
> 
> 
> 
>>
>> Thanks
>>
>> Eric
>>>
>>>>> +}
>>>>> +
>>>>>   static const struct bus_type virtio_bus = {
>>>>>   	.name  = "virtio",
>>>>>   	.match = virtio_dev_match,
>>>>> @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
>>>>>   	.uevent = virtio_uevent,
>>>>>   	.probe = virtio_dev_probe,
>>>>>   	.remove = virtio_dev_remove,
>>>>> +	.shutdown = virtio_dev_shutdown,
>>>>>   };
>>>>>   
>>>>>   int __register_virtio_driver(struct virtio_driver *driver, struct module *owner)
>>>
>>
> 

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Eric Auger]

Hi,

On 2/14/25 8:21 AM, Ning, Hongyu wrote:
> 
> 
> On 2025/2/6 16:59, Eric Auger wrote:
>> Hi,
>>
>> On 2/4/25 12:46 PM, Eric Auger wrote:
>>> Hi,
>>>
>>> On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
>>>> On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
>>>>> Hi Kirill, Michael
>>>>>
>>>>> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
>>>>>> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
>>>>>> accesses during the hang.
>>>>>>
>>>>>>     Invalid read at addr 0x102877002, size 2, region '(null)',
>>>>>> reason: rejected
>>>>>>     Invalid write at addr 0x102877A44, size 2, region '(null)',
>>>>>> reason: rejected
>>>>>>     ...
>>>>>>
>>>>>> It was traced down to virtio-console. Kexec works fine if virtio-
>>>>>> console
>>>>>> is not in use.
>>>>>>
>>>>>> Looks like virtio-console continues to write to the MMIO even after
>>>>>> underlying virtio-pci device is removed.
>>>>>>
>>>>>> The problem can be mitigated by removing all virtio devices on virtio
>>>>>> bus shutdown.
>>>>>>
>>>>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>>>> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
>>>>>
>>>>> Gentle ping on that patch that seems to have fallen though the cracks.
>>>>>
>>>>> I think this fix is really needed. I have another test case with a
>>>>> rebooting guest exposed with virtio-net (backed by vhost-net) and
>>>>> viommu. Since there is currently no shutdown for the virtio-net, on
>>>>> reboot, the IOMMU is disabled through the native_machine_shutdown()/
>>>>> x86_platform.iommu_shutdown() while the virtio-net is still alive.
>>>>>
>>>>> Normally device_shutdown() should call virtio-net shutdown before the
>>>>> IOMMU tear down and we wouldn't see any spurious transactions after
>>>>> iommu shutdown.
>>>>>
>>>>> With that fix, the above test case is fixed and I do not see spurious
>>>>> vhost IOTLB miss spurious requests.
>>>>>
>>>>> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
>>>>> IOTLB callbacks when IOMMU gets disabled,
>>>>> https://lore.kernel.org/all/20250120173339.865681-1-
>>>>> eric.auger@redhat.com/)
>>>>>
>>>>>
>>>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>>>> Tested-by: Eric Auger <eric.auger@redhat.com>
>>>>>
>>>>> Thanks
>>>>>
>>>>> Eric
>>>>>
>>>>>> ---
>>>>>>   drivers/virtio/virtio.c | 10 ++++++++++
>>>>>>   1 file changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>>>>> index a9b93e99c23a..6c2f908eb22c 100644
>>>>>> --- a/drivers/virtio/virtio.c
>>>>>> +++ b/drivers/virtio/virtio.c
>>>>>> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
>>>>>>       of_node_put(dev->dev.of_node);
>>>>>>   }
>>>>>>   +static void virtio_dev_shutdown(struct device *_d)
>>>>>> +{
>>>>>> +    struct virtio_device *dev = dev_to_virtio(_d);
>>>>>> +    struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>>>>>> +
>>>>>> +    if (drv && drv->remove)
>>>>>> +        drv->remove(dev);
>>>>
>>>>
>>>> I am concerned that full remove is a heavyweight operation.
>>>> Do not want to slow down reboots even more.
>>>> How about just doing a reset, instead?
>>>
>>> I tested with
>>>
>>> static void virtio_dev_shutdown(struct device *_d)
>>> {
>>>          struct virtio_device *dev = dev_to_virtio(_d);
>>>
>>>          virtio_reset_device(dev);
>>> }
>>>
>>>
>>> and it fixes my issue.
>>>
>>> Kirill, would that fix you issue too?
> 
> Hi,
> 
> sorry for my late response, I synced with Kirill offline and did a retest.
> 
> The issue is still reproduced on my side, kexec will be stuck in case of
> "console=hvc0" append in kernel cmdline and even with such patch applied.

Thanks for testing!

Michael, it looks like the initial patch from Kyrill is the one that
fixes both issues. virtio_reset_device() usage does not work for the
initial bug report while it works for me. Other ideas?

Thanks

Eric
> 
> my kernel code base is 6.14.0-rc2.
> 
> let me know if any more experiments needed.
> 
> ---
>  drivers/virtio/virtio.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index ba37665188b5..f9f885d04763 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -395,6 +395,13 @@ static const struct cpumask
> *virtio_irq_get_affinity(struct device *_d,
>         return dev->config->get_vq_affinity(dev, irq_vec);
>  }
> 
> +static void virtio_dev_shutdown(struct device *_d)
> +{
> +        struct virtio_device *dev = dev_to_virtio(_d);
> +
> +        virtio_reset_device(dev);
> +}
> +
>  static const struct bus_type virtio_bus = {
>         .name  = "virtio",
>         .match = virtio_dev_match,
> @@ -403,6 +410,7 @@ static const struct bus_type virtio_bus = {
>         .probe = virtio_dev_probe,
>         .remove = virtio_dev_remove,
>         .irq_get_affinity = virtio_irq_get_affinity,
> +       .shutdown = virtio_dev_shutdown,
>  };
> 
>  int __register_virtio_driver(struct virtio_driver *driver, struct
> module *owner)
> -- 
> 2.43.0
> 
> 
>> gentle ping.
>>
>> this also fixes another issue with qemu vSMMU + virtio-scsi-pci. With
>> the above addition I get rid of spurious warning in qemu on guest reboot.
>>
>> qemu-system-aarch64: virtio: zero sized buffers are not allowed
>> qemu-system-aarch64: vhost vring error in virtqueue 0: Invalid
>> argument (22)
>>
>> Would you mind if I respin?
>>
>> Thanks
>>
>> Eric
>>
>>
>>
>>
>>>
>>> Thanks
>>>
>>> Eric
>>>>
>>>>>> +}
>>>>>> +
>>>>>>   static const struct bus_type virtio_bus = {
>>>>>>       .name  = "virtio",
>>>>>>       .match = virtio_dev_match,
>>>>>> @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
>>>>>>       .uevent = virtio_uevent,
>>>>>>       .probe = virtio_dev_probe,
>>>>>>       .remove = virtio_dev_remove,
>>>>>> +    .shutdown = virtio_dev_shutdown,
>>>>>>   };
>>>>>>     int __register_virtio_driver(struct virtio_driver *driver,
>>>>>> struct module *owner)
>>>>
>>>
>>
> 

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Michael S. Tsirkin]

On Fri, Feb 14, 2025 at 08:56:56AM +0100, Eric Auger wrote:
> Hi,
> 
> On 2/14/25 8:21 AM, Ning, Hongyu wrote:
> > 
> > 
> > On 2025/2/6 16:59, Eric Auger wrote:
> >> Hi,
> >>
> >> On 2/4/25 12:46 PM, Eric Auger wrote:
> >>> Hi,
> >>>
> >>> On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
> >>>> On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
> >>>>> Hi Kirill, Michael
> >>>>>
> >>>>> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
> >>>>>> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> >>>>>> accesses during the hang.
> >>>>>>
> >>>>>>     Invalid read at addr 0x102877002, size 2, region '(null)',
> >>>>>> reason: rejected
> >>>>>>     Invalid write at addr 0x102877A44, size 2, region '(null)',
> >>>>>> reason: rejected
> >>>>>>     ...
> >>>>>>
> >>>>>> It was traced down to virtio-console. Kexec works fine if virtio-
> >>>>>> console
> >>>>>> is not in use.
> >>>>>>
> >>>>>> Looks like virtio-console continues to write to the MMIO even after
> >>>>>> underlying virtio-pci device is removed.
> >>>>>>
> >>>>>> The problem can be mitigated by removing all virtio devices on virtio
> >>>>>> bus shutdown.
> >>>>>>
> >>>>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> >>>>>> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> >>>>>
> >>>>> Gentle ping on that patch that seems to have fallen though the cracks.
> >>>>>
> >>>>> I think this fix is really needed. I have another test case with a
> >>>>> rebooting guest exposed with virtio-net (backed by vhost-net) and
> >>>>> viommu. Since there is currently no shutdown for the virtio-net, on
> >>>>> reboot, the IOMMU is disabled through the native_machine_shutdown()/
> >>>>> x86_platform.iommu_shutdown() while the virtio-net is still alive.
> >>>>>
> >>>>> Normally device_shutdown() should call virtio-net shutdown before the
> >>>>> IOMMU tear down and we wouldn't see any spurious transactions after
> >>>>> iommu shutdown.
> >>>>>
> >>>>> With that fix, the above test case is fixed and I do not see spurious
> >>>>> vhost IOTLB miss spurious requests.
> >>>>>
> >>>>> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
> >>>>> IOTLB callbacks when IOMMU gets disabled,
> >>>>> https://lore.kernel.org/all/20250120173339.865681-1-
> >>>>> eric.auger@redhat.com/)
> >>>>>
> >>>>>
> >>>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> >>>>> Tested-by: Eric Auger <eric.auger@redhat.com>
> >>>>>
> >>>>> Thanks
> >>>>>
> >>>>> Eric
> >>>>>
> >>>>>> ---
> >>>>>>   drivers/virtio/virtio.c | 10 ++++++++++
> >>>>>>   1 file changed, 10 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> >>>>>> index a9b93e99c23a..6c2f908eb22c 100644
> >>>>>> --- a/drivers/virtio/virtio.c
> >>>>>> +++ b/drivers/virtio/virtio.c
> >>>>>> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
> >>>>>>       of_node_put(dev->dev.of_node);
> >>>>>>   }
> >>>>>>   +static void virtio_dev_shutdown(struct device *_d)
> >>>>>> +{
> >>>>>> +    struct virtio_device *dev = dev_to_virtio(_d);
> >>>>>> +    struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> >>>>>> +
> >>>>>> +    if (drv && drv->remove)
> >>>>>> +        drv->remove(dev);
> >>>>
> >>>>
> >>>> I am concerned that full remove is a heavyweight operation.
> >>>> Do not want to slow down reboots even more.
> >>>> How about just doing a reset, instead?
> >>>
> >>> I tested with
> >>>
> >>> static void virtio_dev_shutdown(struct device *_d)
> >>> {
> >>>          struct virtio_device *dev = dev_to_virtio(_d);
> >>>
> >>>          virtio_reset_device(dev);
> >>> }
> >>>
> >>>
> >>> and it fixes my issue.
> >>>
> >>> Kirill, would that fix you issue too?
> > 
> > Hi,
> > 
> > sorry for my late response, I synced with Kirill offline and did a retest.
> > 
> > The issue is still reproduced on my side, kexec will be stuck in case of
> > "console=hvc0" append in kernel cmdline and even with such patch applied.
> 
> Thanks for testing!
> 
> Michael, it looks like the initial patch from Kyrill is the one that
> fixes both issues. virtio_reset_device() usage does not work for the
> initial bug report while it works for me. Other ideas?
> 
> Thanks
> 
> Eric

Ah, wait a second.

Looks like virtio-console continues to write to the MMIO even after
underlying virtio-pci device is removed.

Hmm. I am not sure why that is a problem, but I assume some hypervisors just
hang the system if you try to kick them after reset.
Unfortunate that spec did not disallow it.

If we want to prevent that, we want to do something like this:


	/*
	 * Some devices get wedged if you kick them after they are
	 * reset. Mark all vqs as broken to make sure we don't.
	 */
        virtio_break_device(dev);
        /*
         * The below virtio_synchronize_cbs() guarantees that any
         * interrupt for this line arriving after
         * virtio_synchronize_vqs() has completed is guaranteed to see
         * vq->broken as true.
         */
        virtio_synchronize_cbs(dev);
        dev->config->reset(dev);


I assume this still works for you, yes?



> > 
> > my kernel code base is 6.14.0-rc2.
> > 
> > let me know if any more experiments needed.
> > 
> > ---
> >  drivers/virtio/virtio.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index ba37665188b5..f9f885d04763 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -395,6 +395,13 @@ static const struct cpumask
> > *virtio_irq_get_affinity(struct device *_d,
> >         return dev->config->get_vq_affinity(dev, irq_vec);
> >  }
> > 
> > +static void virtio_dev_shutdown(struct device *_d)
> > +{
> > +        struct virtio_device *dev = dev_to_virtio(_d);
> > +
> > +        virtio_reset_device(dev);
> > +}
> > +
> >  static const struct bus_type virtio_bus = {
> >         .name  = "virtio",
> >         .match = virtio_dev_match,
> > @@ -403,6 +410,7 @@ static const struct bus_type virtio_bus = {
> >         .probe = virtio_dev_probe,
> >         .remove = virtio_dev_remove,
> >         .irq_get_affinity = virtio_irq_get_affinity,
> > +       .shutdown = virtio_dev_shutdown,
> >  };
> > 
> >  int __register_virtio_driver(struct virtio_driver *driver, struct
> > module *owner)
> > -- 
> > 2.43.0
> > 
> > 
> >> gentle ping.
> >>
> >> this also fixes another issue with qemu vSMMU + virtio-scsi-pci. With
> >> the above addition I get rid of spurious warning in qemu on guest reboot.
> >>
> >> qemu-system-aarch64: virtio: zero sized buffers are not allowed
> >> qemu-system-aarch64: vhost vring error in virtqueue 0: Invalid
> >> argument (22)
> >>
> >> Would you mind if I respin?
> >>
> >> Thanks
> >>
> >> Eric
> >>
> >>
> >>
> >>
> >>>
> >>> Thanks
> >>>
> >>> Eric
> >>>>
> >>>>>> +}
> >>>>>> +
> >>>>>>   static const struct bus_type virtio_bus = {
> >>>>>>       .name  = "virtio",
> >>>>>>       .match = virtio_dev_match,
> >>>>>> @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
> >>>>>>       .uevent = virtio_uevent,
> >>>>>>       .probe = virtio_dev_probe,
> >>>>>>       .remove = virtio_dev_remove,
> >>>>>> +    .shutdown = virtio_dev_shutdown,
> >>>>>>   };
> >>>>>>     int __register_virtio_driver(struct virtio_driver *driver,
> >>>>>> struct module *owner)
> >>>>
> >>>
> >>
> > 

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Jason Wang]

On Fri, Feb 14, 2025 at 8:16 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Feb 14, 2025 at 08:56:56AM +0100, Eric Auger wrote:
> > Hi,
> >
> > On 2/14/25 8:21 AM, Ning, Hongyu wrote:
> > >
> > >
> > > On 2025/2/6 16:59, Eric Auger wrote:
> > >> Hi,
> > >>
> > >> On 2/4/25 12:46 PM, Eric Auger wrote:
> > >>> Hi,
> > >>>
> > >>> On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
> > >>>> On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
> > >>>>> Hi Kirill, Michael
> > >>>>>
> > >>>>> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
> > >>>>>> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> > >>>>>> accesses during the hang.
> > >>>>>>
> > >>>>>>     Invalid read at addr 0x102877002, size 2, region '(null)',
> > >>>>>> reason: rejected
> > >>>>>>     Invalid write at addr 0x102877A44, size 2, region '(null)',
> > >>>>>> reason: rejected
> > >>>>>>     ...
> > >>>>>>
> > >>>>>> It was traced down to virtio-console. Kexec works fine if virtio-
> > >>>>>> console
> > >>>>>> is not in use.
> > >>>>>>
> > >>>>>> Looks like virtio-console continues to write to the MMIO even after
> > >>>>>> underlying virtio-pci device is removed.
> > >>>>>>
> > >>>>>> The problem can be mitigated by removing all virtio devices on virtio
> > >>>>>> bus shutdown.
> > >>>>>>
> > >>>>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > >>>>>> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> > >>>>>
> > >>>>> Gentle ping on that patch that seems to have fallen though the cracks.
> > >>>>>
> > >>>>> I think this fix is really needed. I have another test case with a
> > >>>>> rebooting guest exposed with virtio-net (backed by vhost-net) and
> > >>>>> viommu. Since there is currently no shutdown for the virtio-net, on
> > >>>>> reboot, the IOMMU is disabled through the native_machine_shutdown()/
> > >>>>> x86_platform.iommu_shutdown() while the virtio-net is still alive.
> > >>>>>
> > >>>>> Normally device_shutdown() should call virtio-net shutdown before the
> > >>>>> IOMMU tear down and we wouldn't see any spurious transactions after
> > >>>>> iommu shutdown.
> > >>>>>
> > >>>>> With that fix, the above test case is fixed and I do not see spurious
> > >>>>> vhost IOTLB miss spurious requests.
> > >>>>>
> > >>>>> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
> > >>>>> IOTLB callbacks when IOMMU gets disabled,
> > >>>>> https://lore.kernel.org/all/20250120173339.865681-1-
> > >>>>> eric.auger@redhat.com/)
> > >>>>>
> > >>>>>
> > >>>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> > >>>>> Tested-by: Eric Auger <eric.auger@redhat.com>
> > >>>>>
> > >>>>> Thanks
> > >>>>>
> > >>>>> Eric
> > >>>>>
> > >>>>>> ---
> > >>>>>>   drivers/virtio/virtio.c | 10 ++++++++++
> > >>>>>>   1 file changed, 10 insertions(+)
> > >>>>>>
> > >>>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > >>>>>> index a9b93e99c23a..6c2f908eb22c 100644
> > >>>>>> --- a/drivers/virtio/virtio.c
> > >>>>>> +++ b/drivers/virtio/virtio.c
> > >>>>>> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
> > >>>>>>       of_node_put(dev->dev.of_node);
> > >>>>>>   }
> > >>>>>>   +static void virtio_dev_shutdown(struct device *_d)
> > >>>>>> +{
> > >>>>>> +    struct virtio_device *dev = dev_to_virtio(_d);
> > >>>>>> +    struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> > >>>>>> +
> > >>>>>> +    if (drv && drv->remove)
> > >>>>>> +        drv->remove(dev);
> > >>>>
> > >>>>
> > >>>> I am concerned that full remove is a heavyweight operation.
> > >>>> Do not want to slow down reboots even more.
> > >>>> How about just doing a reset, instead?
> > >>>
> > >>> I tested with
> > >>>
> > >>> static void virtio_dev_shutdown(struct device *_d)
> > >>> {
> > >>>          struct virtio_device *dev = dev_to_virtio(_d);
> > >>>
> > >>>          virtio_reset_device(dev);
> > >>> }
> > >>>
> > >>>
> > >>> and it fixes my issue.
> > >>>
> > >>> Kirill, would that fix you issue too?
> > >
> > > Hi,
> > >
> > > sorry for my late response, I synced with Kirill offline and did a retest.
> > >
> > > The issue is still reproduced on my side, kexec will be stuck in case of
> > > "console=hvc0" append in kernel cmdline and even with such patch applied.
> >
> > Thanks for testing!
> >
> > Michael, it looks like the initial patch from Kyrill is the one that
> > fixes both issues. virtio_reset_device() usage does not work for the
> > initial bug report while it works for me. Other ideas?
> >
> > Thanks
> >
> > Eric
>
> Ah, wait a second.
>
> Looks like virtio-console continues to write to the MMIO even after
> underlying virtio-pci device is removed.

Where is such code? I think the virtcons_remove() is called so the
console is unregistered from the subsystem.

Or for surprise removal, we have break the device in:

static void virtio_pci_remove(struct pci_dev *pci_dev)
{
        struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
        struct device *dev = get_device(&vp_dev->vdev.dev);

        /*
         * Device is marked broken on surprise removal so that virtio upper
         * layers can abort any ongoing operation.
         */
        if (!pci_device_is_present(pci_dev))
                virtio_break_device(&vp_dev->vdev);


>
> Hmm. I am not sure why that is a problem, but I assume some hypervisors just
> hang the system if you try to kick them after reset.
> Unfortunate that spec did not disallow it.
>
> If we want to prevent that, we want to do something like this:
>
>
>         /*
>          * Some devices get wedged if you kick them after they are
>          * reset. Mark all vqs as broken to make sure we don't.
>          */
>         virtio_break_device(dev);
>         /*
>          * The below virtio_synchronize_cbs() guarantees that any
>          * interrupt for this line arriving after
>          * virtio_synchronize_vqs() has completed is guaranteed to see
>          * vq->broken as true.
>          */
>         virtio_synchronize_cbs(dev);

This seems to relate to doing this when we reintroduce the
notification hardening.

>         dev->config->reset(dev);
>
>
> I assume this still works for you, yes?
>

Thanks

>
>
> > >
> > > my kernel code base is 6.14.0-rc2.
> > >
> > > let me know if any more experiments needed.
> > >
> > > ---
> > >  drivers/virtio/virtio.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > index ba37665188b5..f9f885d04763 100644
> > > --- a/drivers/virtio/virtio.c
> > > +++ b/drivers/virtio/virtio.c
> > > @@ -395,6 +395,13 @@ static const struct cpumask
> > > *virtio_irq_get_affinity(struct device *_d,
> > >         return dev->config->get_vq_affinity(dev, irq_vec);
> > >  }
> > >
> > > +static void virtio_dev_shutdown(struct device *_d)
> > > +{
> > > +        struct virtio_device *dev = dev_to_virtio(_d);
> > > +
> > > +        virtio_reset_device(dev);
> > > +}
> > > +
> > >  static const struct bus_type virtio_bus = {
> > >         .name  = "virtio",
> > >         .match = virtio_dev_match,
> > > @@ -403,6 +410,7 @@ static const struct bus_type virtio_bus = {
> > >         .probe = virtio_dev_probe,
> > >         .remove = virtio_dev_remove,
> > >         .irq_get_affinity = virtio_irq_get_affinity,
> > > +       .shutdown = virtio_dev_shutdown,
> > >  };
> > >
> > >  int __register_virtio_driver(struct virtio_driver *driver, struct
> > > module *owner)
> > > --
> > > 2.43.0
> > >
> > >
> > >> gentle ping.
> > >>
> > >> this also fixes another issue with qemu vSMMU + virtio-scsi-pci. With
> > >> the above addition I get rid of spurious warning in qemu on guest reboot.
> > >>
> > >> qemu-system-aarch64: virtio: zero sized buffers are not allowed
> > >> qemu-system-aarch64: vhost vring error in virtqueue 0: Invalid
> > >> argument (22)
> > >>
> > >> Would you mind if I respin?
> > >>
> > >> Thanks
> > >>
> > >> Eric
> > >>
> > >>
> > >>
> > >>
> > >>>
> > >>> Thanks
> > >>>
> > >>> Eric
> > >>>>
> > >>>>>> +}
> > >>>>>> +
> > >>>>>>   static const struct bus_type virtio_bus = {
> > >>>>>>       .name  = "virtio",
> > >>>>>>       .match = virtio_dev_match,
> > >>>>>> @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
> > >>>>>>       .uevent = virtio_uevent,
> > >>>>>>       .probe = virtio_dev_probe,
> > >>>>>>       .remove = virtio_dev_remove,
> > >>>>>> +    .shutdown = virtio_dev_shutdown,
> > >>>>>>   };
> > >>>>>>     int __register_virtio_driver(struct virtio_driver *driver,
> > >>>>>> struct module *owner)
> > >>>>
> > >>>
> > >>
> > >
>

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Ning, Hongyu]

On 2025/2/17 11:29, Jason Wang wrote:
> On Fri, Feb 14, 2025 at 8:16 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Fri, Feb 14, 2025 at 08:56:56AM +0100, Eric Auger wrote:
>>> Hi,
>>>
>>> On 2/14/25 8:21 AM, Ning, Hongyu wrote:
>>>>
>>>>
>>>> On 2025/2/6 16:59, Eric Auger wrote:
>>>>> Hi,
>>>>>
>>>>> On 2/4/25 12:46 PM, Eric Auger wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
>>>>>>> On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
>>>>>>>> Hi Kirill, Michael
>>>>>>>>
>>>>>>>> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
>>>>>>>>> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
>>>>>>>>> accesses during the hang.
>>>>>>>>>
>>>>>>>>>      Invalid read at addr 0x102877002, size 2, region '(null)',
>>>>>>>>> reason: rejected
>>>>>>>>>      Invalid write at addr 0x102877A44, size 2, region '(null)',
>>>>>>>>> reason: rejected
>>>>>>>>>      ...
>>>>>>>>>
>>>>>>>>> It was traced down to virtio-console. Kexec works fine if virtio-
>>>>>>>>> console
>>>>>>>>> is not in use.
>>>>>>>>>
>>>>>>>>> Looks like virtio-console continues to write to the MMIO even after
>>>>>>>>> underlying virtio-pci device is removed.
>>>>>>>>>
>>>>>>>>> The problem can be mitigated by removing all virtio devices on virtio
>>>>>>>>> bus shutdown.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>>>>>>> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
>>>>>>>>
>>>>>>>> Gentle ping on that patch that seems to have fallen though the cracks.
>>>>>>>>
>>>>>>>> I think this fix is really needed. I have another test case with a
>>>>>>>> rebooting guest exposed with virtio-net (backed by vhost-net) and
>>>>>>>> viommu. Since there is currently no shutdown for the virtio-net, on
>>>>>>>> reboot, the IOMMU is disabled through the native_machine_shutdown()/
>>>>>>>> x86_platform.iommu_shutdown() while the virtio-net is still alive.
>>>>>>>>
>>>>>>>> Normally device_shutdown() should call virtio-net shutdown before the
>>>>>>>> IOMMU tear down and we wouldn't see any spurious transactions after
>>>>>>>> iommu shutdown.
>>>>>>>>
>>>>>>>> With that fix, the above test case is fixed and I do not see spurious
>>>>>>>> vhost IOTLB miss spurious requests.
>>>>>>>>
>>>>>>>> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
>>>>>>>> IOTLB callbacks when IOMMU gets disabled,
>>>>>>>> https://lore.kernel.org/all/20250120173339.865681-1-
>>>>>>>> eric.auger@redhat.com/)
>>>>>>>>
>>>>>>>>
>>>>>>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>>>>>>> Tested-by: Eric Auger <eric.auger@redhat.com>
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>>> Eric
>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>    drivers/virtio/virtio.c | 10 ++++++++++
>>>>>>>>>    1 file changed, 10 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>>>>>>>> index a9b93e99c23a..6c2f908eb22c 100644
>>>>>>>>> --- a/drivers/virtio/virtio.c
>>>>>>>>> +++ b/drivers/virtio/virtio.c
>>>>>>>>> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
>>>>>>>>>        of_node_put(dev->dev.of_node);
>>>>>>>>>    }
>>>>>>>>>    +static void virtio_dev_shutdown(struct device *_d)
>>>>>>>>> +{
>>>>>>>>> +    struct virtio_device *dev = dev_to_virtio(_d);
>>>>>>>>> +    struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>>>>>>>>> +
>>>>>>>>> +    if (drv && drv->remove)
>>>>>>>>> +        drv->remove(dev);
>>>>>>>
>>>>>>>
>>>>>>> I am concerned that full remove is a heavyweight operation.
>>>>>>> Do not want to slow down reboots even more.
>>>>>>> How about just doing a reset, instead?
>>>>>>
>>>>>> I tested with
>>>>>>
>>>>>> static void virtio_dev_shutdown(struct device *_d)
>>>>>> {
>>>>>>           struct virtio_device *dev = dev_to_virtio(_d);
>>>>>>
>>>>>>           virtio_reset_device(dev);
>>>>>> }
>>>>>>
>>>>>>
>>>>>> and it fixes my issue.
>>>>>>
>>>>>> Kirill, would that fix you issue too?
>>>>
>>>> Hi,
>>>>
>>>> sorry for my late response, I synced with Kirill offline and did a retest.
>>>>
>>>> The issue is still reproduced on my side, kexec will be stuck in case of
>>>> "console=hvc0" append in kernel cmdline and even with such patch applied.
>>>
>>> Thanks for testing!
>>>
>>> Michael, it looks like the initial patch from Kyrill is the one that
>>> fixes both issues. virtio_reset_device() usage does not work for the
>>> initial bug report while it works for me. Other ideas?
>>>
>>> Thanks
>>>
>>> Eric
>>
>> Ah, wait a second.
>>
>> Looks like virtio-console continues to write to the MMIO even after
>> underlying virtio-pci device is removed.
> 
> Where is such code? I think the virtcons_remove() is called so the
> console is unregistered from the subsystem.
> 
> Or for surprise removal, we have break the device in:
> 
> static void virtio_pci_remove(struct pci_dev *pci_dev)
> {
>          struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
>          struct device *dev = get_device(&vp_dev->vdev.dev);
> 
>          /*
>           * Device is marked broken on surprise removal so that virtio upper
>           * layers can abort any ongoing operation.
>           */
>          if (!pci_device_is_present(pci_dev))
>                  virtio_break_device(&vp_dev->vdev);
> 
> 
>>
>> Hmm. I am not sure why that is a problem, but I assume some hypervisors just
>> hang the system if you try to kick them after reset.
>> Unfortunate that spec did not disallow it.
>>
>> If we want to prevent that, we want to do something like this:
>>
>>
>>          /*
>>           * Some devices get wedged if you kick them after they are
>>           * reset. Mark all vqs as broken to make sure we don't.
>>           */
>>          virtio_break_device(dev);
>>          /*
>>           * The below virtio_synchronize_cbs() guarantees that any
>>           * interrupt for this line arriving after
>>           * virtio_synchronize_vqs() has completed is guaranteed to see
>>           * vq->broken as true.
>>           */
>>          virtio_synchronize_cbs(dev);
> 
> This seems to relate to doing this when we reintroduce the
> notification hardening.
> 
>>          dev->config->reset(dev);
>>

quick update on above patch from Michael, it could fix the original 
issue in my test setup of vm kexec kernel switching, which is triggered 
with console=hvc0 cmdline.

>>
>> I assume this still works for you, yes?
>>
> 
> Thanks
> 
>>
>>
>>>>
>>>> my kernel code base is 6.14.0-rc2.
>>>>
>>>> let me know if any more experiments needed.
>>>>
>>>> ---
>>>>   drivers/virtio/virtio.c | 8 ++++++++
>>>>   1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>>> index ba37665188b5..f9f885d04763 100644
>>>> --- a/drivers/virtio/virtio.c
>>>> +++ b/drivers/virtio/virtio.c
>>>> @@ -395,6 +395,13 @@ static const struct cpumask
>>>> *virtio_irq_get_affinity(struct device *_d,
>>>>          return dev->config->get_vq_affinity(dev, irq_vec);
>>>>   }
>>>>
>>>> +static void virtio_dev_shutdown(struct device *_d)
>>>> +{
>>>> +        struct virtio_device *dev = dev_to_virtio(_d);
>>>> +
>>>> +        virtio_reset_device(dev);
>>>> +}
>>>> +
>>>>   static const struct bus_type virtio_bus = {
>>>>          .name  = "virtio",
>>>>          .match = virtio_dev_match,
>>>> @@ -403,6 +410,7 @@ static const struct bus_type virtio_bus = {
>>>>          .probe = virtio_dev_probe,
>>>>          .remove = virtio_dev_remove,
>>>>          .irq_get_affinity = virtio_irq_get_affinity,
>>>> +       .shutdown = virtio_dev_shutdown,
>>>>   };
>>>>
>>>>   int __register_virtio_driver(struct virtio_driver *driver, struct
>>>> module *owner)
>>>> --
>>>> 2.43.0
>>>>
>>>>
>>>>> gentle ping.
>>>>>
>>>>> this also fixes another issue with qemu vSMMU + virtio-scsi-pci. With
>>>>> the above addition I get rid of spurious warning in qemu on guest reboot.
>>>>>
>>>>> qemu-system-aarch64: virtio: zero sized buffers are not allowed
>>>>> qemu-system-aarch64: vhost vring error in virtqueue 0: Invalid
>>>>> argument (22)
>>>>>
>>>>> Would you mind if I respin?
>>>>>
>>>>> Thanks
>>>>>
>>>>> Eric
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Eric
>>>>>>>
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>    static const struct bus_type virtio_bus = {
>>>>>>>>>        .name  = "virtio",
>>>>>>>>>        .match = virtio_dev_match,
>>>>>>>>> @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
>>>>>>>>>        .uevent = virtio_uevent,
>>>>>>>>>        .probe = virtio_dev_probe,
>>>>>>>>>        .remove = virtio_dev_remove,
>>>>>>>>> +    .shutdown = virtio_dev_shutdown,
>>>>>>>>>    };
>>>>>>>>>      int __register_virtio_driver(struct virtio_driver *driver,
>>>>>>>>> struct module *owner)
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
> 
> 

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Eric Auger]

Hi Michael, Hongyu,

On 2/14/25 1:16 PM, Michael S. Tsirkin wrote:
> On Fri, Feb 14, 2025 at 08:56:56AM +0100, Eric Auger wrote:
>> Hi,
>>
>> On 2/14/25 8:21 AM, Ning, Hongyu wrote:
>>>
>>>
>>> On 2025/2/6 16:59, Eric Auger wrote:
>>>> Hi,
>>>>
>>>> On 2/4/25 12:46 PM, Eric Auger wrote:
>>>>> Hi,
>>>>>
>>>>> On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
>>>>>> On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
>>>>>>> Hi Kirill, Michael
>>>>>>>
>>>>>>> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
>>>>>>>> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
>>>>>>>> accesses during the hang.
>>>>>>>>
>>>>>>>>     Invalid read at addr 0x102877002, size 2, region '(null)',
>>>>>>>> reason: rejected
>>>>>>>>     Invalid write at addr 0x102877A44, size 2, region '(null)',
>>>>>>>> reason: rejected
>>>>>>>>     ...
>>>>>>>>
>>>>>>>> It was traced down to virtio-console. Kexec works fine if virtio-
>>>>>>>> console
>>>>>>>> is not in use.
>>>>>>>>
>>>>>>>> Looks like virtio-console continues to write to the MMIO even after
>>>>>>>> underlying virtio-pci device is removed.
>>>>>>>>
>>>>>>>> The problem can be mitigated by removing all virtio devices on virtio
>>>>>>>> bus shutdown.
>>>>>>>>
>>>>>>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>>>>>> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
>>>>>>>
>>>>>>> Gentle ping on that patch that seems to have fallen though the cracks.
>>>>>>>
>>>>>>> I think this fix is really needed. I have another test case with a
>>>>>>> rebooting guest exposed with virtio-net (backed by vhost-net) and
>>>>>>> viommu. Since there is currently no shutdown for the virtio-net, on
>>>>>>> reboot, the IOMMU is disabled through the native_machine_shutdown()/
>>>>>>> x86_platform.iommu_shutdown() while the virtio-net is still alive.
>>>>>>>
>>>>>>> Normally device_shutdown() should call virtio-net shutdown before the
>>>>>>> IOMMU tear down and we wouldn't see any spurious transactions after
>>>>>>> iommu shutdown.
>>>>>>>
>>>>>>> With that fix, the above test case is fixed and I do not see spurious
>>>>>>> vhost IOTLB miss spurious requests.
>>>>>>>
>>>>>>> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
>>>>>>> IOTLB callbacks when IOMMU gets disabled,
>>>>>>> https://lore.kernel.org/all/20250120173339.865681-1-
>>>>>>> eric.auger@redhat.com/)
>>>>>>>
>>>>>>>
>>>>>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>>>>>> Tested-by: Eric Auger <eric.auger@redhat.com>
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> Eric
>>>>>>>
>>>>>>>> ---
>>>>>>>>   drivers/virtio/virtio.c | 10 ++++++++++
>>>>>>>>   1 file changed, 10 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>>>>>>> index a9b93e99c23a..6c2f908eb22c 100644
>>>>>>>> --- a/drivers/virtio/virtio.c
>>>>>>>> +++ b/drivers/virtio/virtio.c
>>>>>>>> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
>>>>>>>>       of_node_put(dev->dev.of_node);
>>>>>>>>   }
>>>>>>>>   +static void virtio_dev_shutdown(struct device *_d)
>>>>>>>> +{
>>>>>>>> +    struct virtio_device *dev = dev_to_virtio(_d);
>>>>>>>> +    struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>>>>>>>> +
>>>>>>>> +    if (drv && drv->remove)
>>>>>>>> +        drv->remove(dev);
>>>>>>
>>>>>>
>>>>>> I am concerned that full remove is a heavyweight operation.
>>>>>> Do not want to slow down reboots even more.
>>>>>> How about just doing a reset, instead?
>>>>>
>>>>> I tested with
>>>>>
>>>>> static void virtio_dev_shutdown(struct device *_d)
>>>>> {
>>>>>          struct virtio_device *dev = dev_to_virtio(_d);
>>>>>
>>>>>          virtio_reset_device(dev);
>>>>> }
>>>>>
>>>>>
>>>>> and it fixes my issue.
>>>>>
>>>>> Kirill, would that fix you issue too?
>>>
>>> Hi,
>>>
>>> sorry for my late response, I synced with Kirill offline and did a retest.
>>>
>>> The issue is still reproduced on my side, kexec will be stuck in case of
>>> "console=hvc0" append in kernel cmdline and even with such patch applied.
>>
>> Thanks for testing!
>>
>> Michael, it looks like the initial patch from Kyrill is the one that
>> fixes both issues. virtio_reset_device() usage does not work for the
>> initial bug report while it works for me. Other ideas?
>>
>> Thanks
>>
>> Eric
> 
> Ah, wait a second.
> 
> Looks like virtio-console continues to write to the MMIO even after
> underlying virtio-pci device is removed.
> 
> Hmm. I am not sure why that is a problem, but I assume some hypervisors just
> hang the system if you try to kick them after reset.
> Unfortunate that spec did not disallow it.
> 
> If we want to prevent that, we want to do something like this:
> 
> 
> 	/*
> 	 * Some devices get wedged if you kick them after they are
> 	 * reset. Mark all vqs as broken to make sure we don't.
> 	 */
>         virtio_break_device(dev);
>         /*
>          * The below virtio_synchronize_cbs() guarantees that any
>          * interrupt for this line arriving after
>          * virtio_synchronize_vqs() has completed is guaranteed to see
>          * vq->broken as true.
>          */
>         virtio_synchronize_cbs(dev);
>         dev->config->reset(dev);
> 
> 
> I assume this still works for you, yes?
Would that still been done in the virtio_dev_shutdown()?

Is that what you tested Hongyu?

Eric
> 
> 
> 
>>>
>>> my kernel code base is 6.14.0-rc2.
>>>
>>> let me know if any more experiments needed.
>>>
>>> ---
>>>  drivers/virtio/virtio.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>> index ba37665188b5..f9f885d04763 100644
>>> --- a/drivers/virtio/virtio.c
>>> +++ b/drivers/virtio/virtio.c
>>> @@ -395,6 +395,13 @@ static const struct cpumask
>>> *virtio_irq_get_affinity(struct device *_d,
>>>         return dev->config->get_vq_affinity(dev, irq_vec);
>>>  }
>>>
>>> +static void virtio_dev_shutdown(struct device *_d)
>>> +{
>>> +        struct virtio_device *dev = dev_to_virtio(_d);
>>> +
>>> +        virtio_reset_device(dev);
>>> +}
>>> +
>>>  static const struct bus_type virtio_bus = {
>>>         .name  = "virtio",
>>>         .match = virtio_dev_match,
>>> @@ -403,6 +410,7 @@ static const struct bus_type virtio_bus = {
>>>         .probe = virtio_dev_probe,
>>>         .remove = virtio_dev_remove,
>>>         .irq_get_affinity = virtio_irq_get_affinity,
>>> +       .shutdown = virtio_dev_shutdown,
>>>  };
>>>
>>>  int __register_virtio_driver(struct virtio_driver *driver, struct
>>> module *owner)
>>> -- 
>>> 2.43.0
>>>
>>>
>>>> gentle ping.
>>>>
>>>> this also fixes another issue with qemu vSMMU + virtio-scsi-pci. With
>>>> the above addition I get rid of spurious warning in qemu on guest reboot.
>>>>
>>>> qemu-system-aarch64: virtio: zero sized buffers are not allowed
>>>> qemu-system-aarch64: vhost vring error in virtqueue 0: Invalid
>>>> argument (22)
>>>>
>>>> Would you mind if I respin?
>>>>
>>>> Thanks
>>>>
>>>> Eric
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>> Thanks
>>>>>
>>>>> Eric
>>>>>>
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>   static const struct bus_type virtio_bus = {
>>>>>>>>       .name  = "virtio",
>>>>>>>>       .match = virtio_dev_match,
>>>>>>>> @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
>>>>>>>>       .uevent = virtio_uevent,
>>>>>>>>       .probe = virtio_dev_probe,
>>>>>>>>       .remove = virtio_dev_remove,
>>>>>>>> +    .shutdown = virtio_dev_shutdown,
>>>>>>>>   };
>>>>>>>>     int __register_virtio_driver(struct virtio_driver *driver,
>>>>>>>> struct module *owner)
>>>>>>
>>>>>
>>>>
>>>
> 

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Eric Auger]

Hi Michael,

On 2/17/25 10:25 AM, Eric Auger wrote:
> Hi Michael, Hongyu,
> 
> On 2/14/25 1:16 PM, Michael S. Tsirkin wrote:
>> On Fri, Feb 14, 2025 at 08:56:56AM +0100, Eric Auger wrote:
>>> Hi,
>>>
>>> On 2/14/25 8:21 AM, Ning, Hongyu wrote:
>>>>
>>>>
>>>> On 2025/2/6 16:59, Eric Auger wrote:
>>>>> Hi,
>>>>>
>>>>> On 2/4/25 12:46 PM, Eric Auger wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
>>>>>>> On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
>>>>>>>> Hi Kirill, Michael
>>>>>>>>
>>>>>>>> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
>>>>>>>>> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
>>>>>>>>> accesses during the hang.
>>>>>>>>>
>>>>>>>>>     Invalid read at addr 0x102877002, size 2, region '(null)',
>>>>>>>>> reason: rejected
>>>>>>>>>     Invalid write at addr 0x102877A44, size 2, region '(null)',
>>>>>>>>> reason: rejected
>>>>>>>>>     ...
>>>>>>>>>
>>>>>>>>> It was traced down to virtio-console. Kexec works fine if virtio-
>>>>>>>>> console
>>>>>>>>> is not in use.
>>>>>>>>>
>>>>>>>>> Looks like virtio-console continues to write to the MMIO even after
>>>>>>>>> underlying virtio-pci device is removed.
>>>>>>>>>
>>>>>>>>> The problem can be mitigated by removing all virtio devices on virtio
>>>>>>>>> bus shutdown.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>>>>>>> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
>>>>>>>>
>>>>>>>> Gentle ping on that patch that seems to have fallen though the cracks.
>>>>>>>>
>>>>>>>> I think this fix is really needed. I have another test case with a
>>>>>>>> rebooting guest exposed with virtio-net (backed by vhost-net) and
>>>>>>>> viommu. Since there is currently no shutdown for the virtio-net, on
>>>>>>>> reboot, the IOMMU is disabled through the native_machine_shutdown()/
>>>>>>>> x86_platform.iommu_shutdown() while the virtio-net is still alive.
>>>>>>>>
>>>>>>>> Normally device_shutdown() should call virtio-net shutdown before the
>>>>>>>> IOMMU tear down and we wouldn't see any spurious transactions after
>>>>>>>> iommu shutdown.
>>>>>>>>
>>>>>>>> With that fix, the above test case is fixed and I do not see spurious
>>>>>>>> vhost IOTLB miss spurious requests.
>>>>>>>>
>>>>>>>> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
>>>>>>>> IOTLB callbacks when IOMMU gets disabled,
>>>>>>>> https://lore.kernel.org/all/20250120173339.865681-1-
>>>>>>>> eric.auger@redhat.com/)
>>>>>>>>
>>>>>>>>
>>>>>>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>>>>>>> Tested-by: Eric Auger <eric.auger@redhat.com>
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>>> Eric
>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>   drivers/virtio/virtio.c | 10 ++++++++++
>>>>>>>>>   1 file changed, 10 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>>>>>>>> index a9b93e99c23a..6c2f908eb22c 100644
>>>>>>>>> --- a/drivers/virtio/virtio.c
>>>>>>>>> +++ b/drivers/virtio/virtio.c
>>>>>>>>> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
>>>>>>>>>       of_node_put(dev->dev.of_node);
>>>>>>>>>   }
>>>>>>>>>   +static void virtio_dev_shutdown(struct device *_d)
>>>>>>>>> +{
>>>>>>>>> +    struct virtio_device *dev = dev_to_virtio(_d);
>>>>>>>>> +    struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>>>>>>>>> +
>>>>>>>>> +    if (drv && drv->remove)
>>>>>>>>> +        drv->remove(dev);
>>>>>>>
>>>>>>>
>>>>>>> I am concerned that full remove is a heavyweight operation.
>>>>>>> Do not want to slow down reboots even more.
>>>>>>> How about just doing a reset, instead?
>>>>>>
>>>>>> I tested with
>>>>>>
>>>>>> static void virtio_dev_shutdown(struct device *_d)
>>>>>> {
>>>>>>          struct virtio_device *dev = dev_to_virtio(_d);
>>>>>>
>>>>>>          virtio_reset_device(dev);
>>>>>> }
>>>>>>
>>>>>>
>>>>>> and it fixes my issue.
>>>>>>
>>>>>> Kirill, would that fix you issue too?
>>>>
>>>> Hi,
>>>>
>>>> sorry for my late response, I synced with Kirill offline and did a retest.
>>>>
>>>> The issue is still reproduced on my side, kexec will be stuck in case of
>>>> "console=hvc0" append in kernel cmdline and even with such patch applied.
>>>
>>> Thanks for testing!
>>>
>>> Michael, it looks like the initial patch from Kyrill is the one that
>>> fixes both issues. virtio_reset_device() usage does not work for the
>>> initial bug report while it works for me. Other ideas?
>>>
>>> Thanks
>>>
>>> Eric
>>
>> Ah, wait a second.
>>
>> Looks like virtio-console continues to write to the MMIO even after
>> underlying virtio-pci device is removed.
>>
>> Hmm. I am not sure why that is a problem, but I assume some hypervisors just
>> hang the system if you try to kick them after reset.
>> Unfortunate that spec did not disallow it.
>>
>> If we want to prevent that, we want to do something like this:
>>
>>
>> 	/*
>> 	 * Some devices get wedged if you kick them after they are
>> 	 * reset. Mark all vqs as broken to make sure we don't.
>> 	 */
>>         virtio_break_device(dev);
>>         /*
>>          * The below virtio_synchronize_cbs() guarantees that any
>>          * interrupt for this line arriving after
>>          * virtio_synchronize_vqs() has completed is guaranteed to see
>>          * vq->broken as true.
>>          */
>>         virtio_synchronize_cbs(dev);
>>         dev->config->reset(dev);
I have tested that code as an implentation of the virtio shutdown
callback and yes it also fixes the viommu/vhost issue.

Michael, will you send a patch then?

Thanks

Eric
>>
>>
>> I assume this still works for you, yes?
> Would that still been done in the virtio_dev_shutdown()?
> 
> Is that what you tested Hongyu?
> 
> Eric
>>
>>
>>
>>>>
>>>> my kernel code base is 6.14.0-rc2.
>>>>
>>>> let me know if any more experiments needed.
>>>>
>>>> ---
>>>>  drivers/virtio/virtio.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>>> index ba37665188b5..f9f885d04763 100644
>>>> --- a/drivers/virtio/virtio.c
>>>> +++ b/drivers/virtio/virtio.c
>>>> @@ -395,6 +395,13 @@ static const struct cpumask
>>>> *virtio_irq_get_affinity(struct device *_d,
>>>>         return dev->config->get_vq_affinity(dev, irq_vec);
>>>>  }
>>>>
>>>> +static void virtio_dev_shutdown(struct device *_d)
>>>> +{
>>>> +        struct virtio_device *dev = dev_to_virtio(_d);
>>>> +
>>>> +        virtio_reset_device(dev);
>>>> +}
>>>> +
>>>>  static const struct bus_type virtio_bus = {
>>>>         .name  = "virtio",
>>>>         .match = virtio_dev_match,
>>>> @@ -403,6 +410,7 @@ static const struct bus_type virtio_bus = {
>>>>         .probe = virtio_dev_probe,
>>>>         .remove = virtio_dev_remove,
>>>>         .irq_get_affinity = virtio_irq_get_affinity,
>>>> +       .shutdown = virtio_dev_shutdown,
>>>>  };
>>>>
>>>>  int __register_virtio_driver(struct virtio_driver *driver, struct
>>>> module *owner)
>>>> -- 
>>>> 2.43.0
>>>>
>>>>
>>>>> gentle ping.
>>>>>
>>>>> this also fixes another issue with qemu vSMMU + virtio-scsi-pci. With
>>>>> the above addition I get rid of spurious warning in qemu on guest reboot.
>>>>>
>>>>> qemu-system-aarch64: virtio: zero sized buffers are not allowed
>>>>> qemu-system-aarch64: vhost vring error in virtqueue 0: Invalid
>>>>> argument (22)
>>>>>
>>>>> Would you mind if I respin?
>>>>>
>>>>> Thanks
>>>>>
>>>>> Eric
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Eric
>>>>>>>
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>   static const struct bus_type virtio_bus = {
>>>>>>>>>       .name  = "virtio",
>>>>>>>>>       .match = virtio_dev_match,
>>>>>>>>> @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
>>>>>>>>>       .uevent = virtio_uevent,
>>>>>>>>>       .probe = virtio_dev_probe,
>>>>>>>>>       .remove = virtio_dev_remove,
>>>>>>>>> +    .shutdown = virtio_dev_shutdown,
>>>>>>>>>   };
>>>>>>>>>     int __register_virtio_driver(struct virtio_driver *driver,
>>>>>>>>> struct module *owner)
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
> 

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Michael S. Tsirkin]

On Mon, Feb 17, 2025 at 05:59:30PM +0100, Eric Auger wrote:
> Hi Michael,
> 
> On 2/17/25 10:25 AM, Eric Auger wrote:
> > Hi Michael, Hongyu,
> > 
> > On 2/14/25 1:16 PM, Michael S. Tsirkin wrote:
> >> On Fri, Feb 14, 2025 at 08:56:56AM +0100, Eric Auger wrote:
> >>> Hi,
> >>>
> >>> On 2/14/25 8:21 AM, Ning, Hongyu wrote:
> >>>>
> >>>>
> >>>> On 2025/2/6 16:59, Eric Auger wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On 2/4/25 12:46 PM, Eric Auger wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
> >>>>>>> On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
> >>>>>>>> Hi Kirill, Michael
> >>>>>>>>
> >>>>>>>> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
> >>>>>>>>> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> >>>>>>>>> accesses during the hang.
> >>>>>>>>>
> >>>>>>>>>     Invalid read at addr 0x102877002, size 2, region '(null)',
> >>>>>>>>> reason: rejected
> >>>>>>>>>     Invalid write at addr 0x102877A44, size 2, region '(null)',
> >>>>>>>>> reason: rejected
> >>>>>>>>>     ...
> >>>>>>>>>
> >>>>>>>>> It was traced down to virtio-console. Kexec works fine if virtio-
> >>>>>>>>> console
> >>>>>>>>> is not in use.
> >>>>>>>>>
> >>>>>>>>> Looks like virtio-console continues to write to the MMIO even after
> >>>>>>>>> underlying virtio-pci device is removed.
> >>>>>>>>>
> >>>>>>>>> The problem can be mitigated by removing all virtio devices on virtio
> >>>>>>>>> bus shutdown.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> >>>>>>>>> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> >>>>>>>>
> >>>>>>>> Gentle ping on that patch that seems to have fallen though the cracks.
> >>>>>>>>
> >>>>>>>> I think this fix is really needed. I have another test case with a
> >>>>>>>> rebooting guest exposed with virtio-net (backed by vhost-net) and
> >>>>>>>> viommu. Since there is currently no shutdown for the virtio-net, on
> >>>>>>>> reboot, the IOMMU is disabled through the native_machine_shutdown()/
> >>>>>>>> x86_platform.iommu_shutdown() while the virtio-net is still alive.
> >>>>>>>>
> >>>>>>>> Normally device_shutdown() should call virtio-net shutdown before the
> >>>>>>>> IOMMU tear down and we wouldn't see any spurious transactions after
> >>>>>>>> iommu shutdown.
> >>>>>>>>
> >>>>>>>> With that fix, the above test case is fixed and I do not see spurious
> >>>>>>>> vhost IOTLB miss spurious requests.
> >>>>>>>>
> >>>>>>>> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
> >>>>>>>> IOTLB callbacks when IOMMU gets disabled,
> >>>>>>>> https://lore.kernel.org/all/20250120173339.865681-1-
> >>>>>>>> eric.auger@redhat.com/)
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> >>>>>>>> Tested-by: Eric Auger <eric.auger@redhat.com>
> >>>>>>>>
> >>>>>>>> Thanks
> >>>>>>>>
> >>>>>>>> Eric
> >>>>>>>>
> >>>>>>>>> ---
> >>>>>>>>>   drivers/virtio/virtio.c | 10 ++++++++++
> >>>>>>>>>   1 file changed, 10 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> >>>>>>>>> index a9b93e99c23a..6c2f908eb22c 100644
> >>>>>>>>> --- a/drivers/virtio/virtio.c
> >>>>>>>>> +++ b/drivers/virtio/virtio.c
> >>>>>>>>> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
> >>>>>>>>>       of_node_put(dev->dev.of_node);
> >>>>>>>>>   }
> >>>>>>>>>   +static void virtio_dev_shutdown(struct device *_d)
> >>>>>>>>> +{
> >>>>>>>>> +    struct virtio_device *dev = dev_to_virtio(_d);
> >>>>>>>>> +    struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> >>>>>>>>> +
> >>>>>>>>> +    if (drv && drv->remove)
> >>>>>>>>> +        drv->remove(dev);
> >>>>>>>
> >>>>>>>
> >>>>>>> I am concerned that full remove is a heavyweight operation.
> >>>>>>> Do not want to slow down reboots even more.
> >>>>>>> How about just doing a reset, instead?
> >>>>>>
> >>>>>> I tested with
> >>>>>>
> >>>>>> static void virtio_dev_shutdown(struct device *_d)
> >>>>>> {
> >>>>>>          struct virtio_device *dev = dev_to_virtio(_d);
> >>>>>>
> >>>>>>          virtio_reset_device(dev);
> >>>>>> }
> >>>>>>
> >>>>>>
> >>>>>> and it fixes my issue.
> >>>>>>
> >>>>>> Kirill, would that fix you issue too?
> >>>>
> >>>> Hi,
> >>>>
> >>>> sorry for my late response, I synced with Kirill offline and did a retest.
> >>>>
> >>>> The issue is still reproduced on my side, kexec will be stuck in case of
> >>>> "console=hvc0" append in kernel cmdline and even with such patch applied.
> >>>
> >>> Thanks for testing!
> >>>
> >>> Michael, it looks like the initial patch from Kyrill is the one that
> >>> fixes both issues. virtio_reset_device() usage does not work for the
> >>> initial bug report while it works for me. Other ideas?
> >>>
> >>> Thanks
> >>>
> >>> Eric
> >>
> >> Ah, wait a second.
> >>
> >> Looks like virtio-console continues to write to the MMIO even after
> >> underlying virtio-pci device is removed.
> >>
> >> Hmm. I am not sure why that is a problem, but I assume some hypervisors just
> >> hang the system if you try to kick them after reset.
> >> Unfortunate that spec did not disallow it.
> >>
> >> If we want to prevent that, we want to do something like this:
> >>
> >>
> >> 	/*
> >> 	 * Some devices get wedged if you kick them after they are
> >> 	 * reset. Mark all vqs as broken to make sure we don't.
> >> 	 */
> >>         virtio_break_device(dev);
> >>         /*
> >>          * The below virtio_synchronize_cbs() guarantees that any
> >>          * interrupt for this line arriving after
> >>          * virtio_synchronize_vqs() has completed is guaranteed to see
> >>          * vq->broken as true.
> >>          */
> >>         virtio_synchronize_cbs(dev);
> >>         dev->config->reset(dev);
> I have tested that code as an implentation of the virtio shutdown
> callback and yes it also fixes the viommu/vhost issue.
> 
> Michael, will you send a patch then?
> 
> Thanks
> 
> Eric


Was hoping for a confirmation from Ning.

> >>
> >>
> >> I assume this still works for you, yes?
> > Would that still been done in the virtio_dev_shutdown()?
> > 
> > Is that what you tested Hongyu?
> > 
> > Eric
> >>
> >>
> >>
> >>>>
> >>>> my kernel code base is 6.14.0-rc2.
> >>>>
> >>>> let me know if any more experiments needed.
> >>>>
> >>>> ---
> >>>>  drivers/virtio/virtio.c | 8 ++++++++
> >>>>  1 file changed, 8 insertions(+)
> >>>>
> >>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> >>>> index ba37665188b5..f9f885d04763 100644
> >>>> --- a/drivers/virtio/virtio.c
> >>>> +++ b/drivers/virtio/virtio.c
> >>>> @@ -395,6 +395,13 @@ static const struct cpumask
> >>>> *virtio_irq_get_affinity(struct device *_d,
> >>>>         return dev->config->get_vq_affinity(dev, irq_vec);
> >>>>  }
> >>>>
> >>>> +static void virtio_dev_shutdown(struct device *_d)
> >>>> +{
> >>>> +        struct virtio_device *dev = dev_to_virtio(_d);
> >>>> +
> >>>> +        virtio_reset_device(dev);
> >>>> +}
> >>>> +
> >>>>  static const struct bus_type virtio_bus = {
> >>>>         .name  = "virtio",
> >>>>         .match = virtio_dev_match,
> >>>> @@ -403,6 +410,7 @@ static const struct bus_type virtio_bus = {
> >>>>         .probe = virtio_dev_probe,
> >>>>         .remove = virtio_dev_remove,
> >>>>         .irq_get_affinity = virtio_irq_get_affinity,
> >>>> +       .shutdown = virtio_dev_shutdown,
> >>>>  };
> >>>>
> >>>>  int __register_virtio_driver(struct virtio_driver *driver, struct
> >>>> module *owner)
> >>>> -- 
> >>>> 2.43.0
> >>>>
> >>>>
> >>>>> gentle ping.
> >>>>>
> >>>>> this also fixes another issue with qemu vSMMU + virtio-scsi-pci. With
> >>>>> the above addition I get rid of spurious warning in qemu on guest reboot.
> >>>>>
> >>>>> qemu-system-aarch64: virtio: zero sized buffers are not allowed
> >>>>> qemu-system-aarch64: vhost vring error in virtqueue 0: Invalid
> >>>>> argument (22)
> >>>>>
> >>>>> Would you mind if I respin?
> >>>>>
> >>>>> Thanks
> >>>>>
> >>>>> Eric
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> Thanks
> >>>>>>
> >>>>>> Eric
> >>>>>>>
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>>   static const struct bus_type virtio_bus = {
> >>>>>>>>>       .name  = "virtio",
> >>>>>>>>>       .match = virtio_dev_match,
> >>>>>>>>> @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
> >>>>>>>>>       .uevent = virtio_uevent,
> >>>>>>>>>       .probe = virtio_dev_probe,
> >>>>>>>>>       .remove = virtio_dev_remove,
> >>>>>>>>> +    .shutdown = virtio_dev_shutdown,
> >>>>>>>>>   };
> >>>>>>>>>     int __register_virtio_driver(struct virtio_driver *driver,
> >>>>>>>>> struct module *owner)
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>
> > 

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Ning, Hongyu]

On 2025/2/17 17:25, Eric Auger wrote:
> Hi Michael, Hongyu,
> 
> On 2/14/25 1:16 PM, Michael S. Tsirkin wrote:
>> On Fri, Feb 14, 2025 at 08:56:56AM +0100, Eric Auger wrote:
>>> Hi,
>>>
>>> On 2/14/25 8:21 AM, Ning, Hongyu wrote:
>>>>
>>>>
>>>> On 2025/2/6 16:59, Eric Auger wrote:
>>>>> Hi,
>>>>>
>>>>> On 2/4/25 12:46 PM, Eric Auger wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
>>>>>>> On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
>>>>>>>> Hi Kirill, Michael
>>>>>>>>
>>>>>>>> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
>>>>>>>>> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
>>>>>>>>> accesses during the hang.
>>>>>>>>>
>>>>>>>>>      Invalid read at addr 0x102877002, size 2, region '(null)',
>>>>>>>>> reason: rejected
>>>>>>>>>      Invalid write at addr 0x102877A44, size 2, region '(null)',
>>>>>>>>> reason: rejected
>>>>>>>>>      ...
>>>>>>>>>
>>>>>>>>> It was traced down to virtio-console. Kexec works fine if virtio-
>>>>>>>>> console
>>>>>>>>> is not in use.
>>>>>>>>>
>>>>>>>>> Looks like virtio-console continues to write to the MMIO even after
>>>>>>>>> underlying virtio-pci device is removed.
>>>>>>>>>
>>>>>>>>> The problem can be mitigated by removing all virtio devices on virtio
>>>>>>>>> bus shutdown.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>>>>>>> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
>>>>>>>>
>>>>>>>> Gentle ping on that patch that seems to have fallen though the cracks.
>>>>>>>>
>>>>>>>> I think this fix is really needed. I have another test case with a
>>>>>>>> rebooting guest exposed with virtio-net (backed by vhost-net) and
>>>>>>>> viommu. Since there is currently no shutdown for the virtio-net, on
>>>>>>>> reboot, the IOMMU is disabled through the native_machine_shutdown()/
>>>>>>>> x86_platform.iommu_shutdown() while the virtio-net is still alive.
>>>>>>>>
>>>>>>>> Normally device_shutdown() should call virtio-net shutdown before the
>>>>>>>> IOMMU tear down and we wouldn't see any spurious transactions after
>>>>>>>> iommu shutdown.
>>>>>>>>
>>>>>>>> With that fix, the above test case is fixed and I do not see spurious
>>>>>>>> vhost IOTLB miss spurious requests.
>>>>>>>>
>>>>>>>> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
>>>>>>>> IOTLB callbacks when IOMMU gets disabled,
>>>>>>>> https://lore.kernel.org/all/20250120173339.865681-1-
>>>>>>>> eric.auger@redhat.com/)
>>>>>>>>
>>>>>>>>
>>>>>>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>>>>>>> Tested-by: Eric Auger <eric.auger@redhat.com>
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>>> Eric
>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>    drivers/virtio/virtio.c | 10 ++++++++++
>>>>>>>>>    1 file changed, 10 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>>>>>>>> index a9b93e99c23a..6c2f908eb22c 100644
>>>>>>>>> --- a/drivers/virtio/virtio.c
>>>>>>>>> +++ b/drivers/virtio/virtio.c
>>>>>>>>> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
>>>>>>>>>        of_node_put(dev->dev.of_node);
>>>>>>>>>    }
>>>>>>>>>    +static void virtio_dev_shutdown(struct device *_d)
>>>>>>>>> +{
>>>>>>>>> +    struct virtio_device *dev = dev_to_virtio(_d);
>>>>>>>>> +    struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>>>>>>>>> +
>>>>>>>>> +    if (drv && drv->remove)
>>>>>>>>> +        drv->remove(dev);
>>>>>>>
>>>>>>>
>>>>>>> I am concerned that full remove is a heavyweight operation.
>>>>>>> Do not want to slow down reboots even more.
>>>>>>> How about just doing a reset, instead?
>>>>>>
>>>>>> I tested with
>>>>>>
>>>>>> static void virtio_dev_shutdown(struct device *_d)
>>>>>> {
>>>>>>           struct virtio_device *dev = dev_to_virtio(_d);
>>>>>>
>>>>>>           virtio_reset_device(dev);
>>>>>> }
>>>>>>
>>>>>>
>>>>>> and it fixes my issue.
>>>>>>
>>>>>> Kirill, would that fix you issue too?
>>>>
>>>> Hi,
>>>>
>>>> sorry for my late response, I synced with Kirill offline and did a retest.
>>>>
>>>> The issue is still reproduced on my side, kexec will be stuck in case of
>>>> "console=hvc0" append in kernel cmdline and even with such patch applied.
>>>
>>> Thanks for testing!
>>>
>>> Michael, it looks like the initial patch from Kyrill is the one that
>>> fixes both issues. virtio_reset_device() usage does not work for the
>>> initial bug report while it works for me. Other ideas?
>>>
>>> Thanks
>>>
>>> Eric
>>
>> Ah, wait a second.
>>
>> Looks like virtio-console continues to write to the MMIO even after
>> underlying virtio-pci device is removed.
>>
>> Hmm. I am not sure why that is a problem, but I assume some hypervisors just
>> hang the system if you try to kick them after reset.
>> Unfortunate that spec did not disallow it.
>>
>> If we want to prevent that, we want to do something like this:
>>
>>
>> 	/*
>> 	 * Some devices get wedged if you kick them after they are
>> 	 * reset. Mark all vqs as broken to make sure we don't.
>> 	 */
>>          virtio_break_device(dev);
>>          /*
>>           * The below virtio_synchronize_cbs() guarantees that any
>>           * interrupt for this line arriving after
>>           * virtio_synchronize_vqs() has completed is guaranteed to see
>>           * vq->broken as true.
>>           */
>>          virtio_synchronize_cbs(dev);
>>          dev->config->reset(dev);
>>
>>
>> I assume this still works for you, yes?
> Would that still been done in the virtio_dev_shutdown()?
> 
> Is that what you tested Hongyu?

Hi Eric,

my patch applied based on Michael's comments:

---
  drivers/virtio/virtio.c | 20 ++++++++++++++++++++
  1 file changed, 20 insertions(+)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index ba37665188b5..458dc28be060 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -395,6 +395,25 @@ static const struct cpumask 
*virtio_irq_get_affinity(struct device *_d,
         return dev->config->get_vq_affinity(dev, irq_vec);
  }

+static void virtio_dev_shutdown(struct device *_d)
+{
+        struct virtio_device *dev = dev_to_virtio(_d);
+        /*
+         * Some devices get wedged if you kick them after they are
+         * reset. Mark all vqs as broken to make sure we don't.
+         */
+        virtio_break_device(dev);
+        /*
+         * The below virtio_synchronize_cbs() guarantees that any
+         * interrupt for this line arriving after
+         * virtio_synchronize_vqs() has completed is guaranteed to see
+         * vq->broken as true.
+         */
+        virtio_synchronize_cbs(dev);
+        dev->config->reset(dev);
+       //virtio_reset_device(dev);
+}
+
  static const struct bus_type virtio_bus = {
         .name  = "virtio",
         .match = virtio_dev_match,
@@ -403,6 +422,7 @@ static const struct bus_type virtio_bus = {
         .probe = virtio_dev_probe,
         .remove = virtio_dev_remove,
         .irq_get_affinity = virtio_irq_get_affinity,
+       .shutdown = virtio_dev_shutdown,
  };

  int __register_virtio_driver(struct virtio_driver *driver, struct 
module *owner)
--
2.43.0


> 
> Eric
>>
>>
>>
>>>>
>>>> my kernel code base is 6.14.0-rc2.
>>>>
>>>> let me know if any more experiments needed.
>>>>
>>>> ---
>>>>   drivers/virtio/virtio.c | 8 ++++++++
>>>>   1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>>> index ba37665188b5..f9f885d04763 100644
>>>> --- a/drivers/virtio/virtio.c
>>>> +++ b/drivers/virtio/virtio.c
>>>> @@ -395,6 +395,13 @@ static const struct cpumask
>>>> *virtio_irq_get_affinity(struct device *_d,
>>>>          return dev->config->get_vq_affinity(dev, irq_vec);
>>>>   }
>>>>
>>>> +static void virtio_dev_shutdown(struct device *_d)
>>>> +{
>>>> +        struct virtio_device *dev = dev_to_virtio(_d);
>>>> +
>>>> +        virtio_reset_device(dev);
>>>> +}
>>>> +
>>>>   static const struct bus_type virtio_bus = {
>>>>          .name  = "virtio",
>>>>          .match = virtio_dev_match,
>>>> @@ -403,6 +410,7 @@ static const struct bus_type virtio_bus = {
>>>>          .probe = virtio_dev_probe,
>>>>          .remove = virtio_dev_remove,
>>>>          .irq_get_affinity = virtio_irq_get_affinity,
>>>> +       .shutdown = virtio_dev_shutdown,
>>>>   };
>>>>
>>>>   int __register_virtio_driver(struct virtio_driver *driver, struct
>>>> module *owner)
>>>> -- 
>>>> 2.43.0
>>>>
>>>>
>>>>> gentle ping.
>>>>>
>>>>> this also fixes another issue with qemu vSMMU + virtio-scsi-pci. With
>>>>> the above addition I get rid of spurious warning in qemu on guest reboot.
>>>>>
>>>>> qemu-system-aarch64: virtio: zero sized buffers are not allowed
>>>>> qemu-system-aarch64: vhost vring error in virtqueue 0: Invalid
>>>>> argument (22)
>>>>>
>>>>> Would you mind if I respin?
>>>>>
>>>>> Thanks
>>>>>
>>>>> Eric
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Eric
>>>>>>>
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>    static const struct bus_type virtio_bus = {
>>>>>>>>>        .name  = "virtio",
>>>>>>>>>        .match = virtio_dev_match,
>>>>>>>>> @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
>>>>>>>>>        .uevent = virtio_uevent,
>>>>>>>>>        .probe = virtio_dev_probe,
>>>>>>>>>        .remove = virtio_dev_remove,
>>>>>>>>> +    .shutdown = virtio_dev_shutdown,
>>>>>>>>>    };
>>>>>>>>>      int __register_virtio_driver(struct virtio_driver *driver,
>>>>>>>>> struct module *owner)
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
> 
> 

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Michael S. Tsirkin]

On Tue, Feb 18, 2025 at 07:57:23AM +0800, Ning, Hongyu wrote:
> 
> 
> On 2025/2/17 17:25, Eric Auger wrote:
> > Hi Michael, Hongyu,
> > 
> > On 2/14/25 1:16 PM, Michael S. Tsirkin wrote:
> > > On Fri, Feb 14, 2025 at 08:56:56AM +0100, Eric Auger wrote:
> > > > Hi,
> > > > 
> > > > On 2/14/25 8:21 AM, Ning, Hongyu wrote:
> > > > > 
> > > > > 
> > > > > On 2025/2/6 16:59, Eric Auger wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On 2/4/25 12:46 PM, Eric Auger wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
> > > > > > > > On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
> > > > > > > > > Hi Kirill, Michael
> > > > > > > > > 
> > > > > > > > > On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
> > > > > > > > > > Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> > > > > > > > > > accesses during the hang.
> > > > > > > > > > 
> > > > > > > > > >      Invalid read at addr 0x102877002, size 2, region '(null)',
> > > > > > > > > > reason: rejected
> > > > > > > > > >      Invalid write at addr 0x102877A44, size 2, region '(null)',
> > > > > > > > > > reason: rejected
> > > > > > > > > >      ...
> > > > > > > > > > 
> > > > > > > > > > It was traced down to virtio-console. Kexec works fine if virtio-
> > > > > > > > > > console
> > > > > > > > > > is not in use.
> > > > > > > > > > 
> > > > > > > > > > Looks like virtio-console continues to write to the MMIO even after
> > > > > > > > > > underlying virtio-pci device is removed.
> > > > > > > > > > 
> > > > > > > > > > The problem can be mitigated by removing all virtio devices on virtio
> > > > > > > > > > bus shutdown.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > > > > > > > > Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> > > > > > > > > 
> > > > > > > > > Gentle ping on that patch that seems to have fallen though the cracks.
> > > > > > > > > 
> > > > > > > > > I think this fix is really needed. I have another test case with a
> > > > > > > > > rebooting guest exposed with virtio-net (backed by vhost-net) and
> > > > > > > > > viommu. Since there is currently no shutdown for the virtio-net, on
> > > > > > > > > reboot, the IOMMU is disabled through the native_machine_shutdown()/
> > > > > > > > > x86_platform.iommu_shutdown() while the virtio-net is still alive.
> > > > > > > > > 
> > > > > > > > > Normally device_shutdown() should call virtio-net shutdown before the
> > > > > > > > > IOMMU tear down and we wouldn't see any spurious transactions after
> > > > > > > > > iommu shutdown.
> > > > > > > > > 
> > > > > > > > > With that fix, the above test case is fixed and I do not see spurious
> > > > > > > > > vhost IOTLB miss spurious requests.
> > > > > > > > > 
> > > > > > > > > For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
> > > > > > > > > IOTLB callbacks when IOMMU gets disabled,
> > > > > > > > > https://lore.kernel.org/all/20250120173339.865681-1-
> > > > > > > > > eric.auger@redhat.com/)
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> > > > > > > > > Tested-by: Eric Auger <eric.auger@redhat.com>
> > > > > > > > > 
> > > > > > > > > Thanks
> > > > > > > > > 
> > > > > > > > > Eric
> > > > > > > > > 
> > > > > > > > > > ---
> > > > > > > > > >    drivers/virtio/virtio.c | 10 ++++++++++
> > > > > > > > > >    1 file changed, 10 insertions(+)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > > > > > > index a9b93e99c23a..6c2f908eb22c 100644
> > > > > > > > > > --- a/drivers/virtio/virtio.c
> > > > > > > > > > +++ b/drivers/virtio/virtio.c
> > > > > > > > > > @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
> > > > > > > > > >        of_node_put(dev->dev.of_node);
> > > > > > > > > >    }
> > > > > > > > > >    +static void virtio_dev_shutdown(struct device *_d)
> > > > > > > > > > +{
> > > > > > > > > > +    struct virtio_device *dev = dev_to_virtio(_d);
> > > > > > > > > > +    struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> > > > > > > > > > +
> > > > > > > > > > +    if (drv && drv->remove)
> > > > > > > > > > +        drv->remove(dev);
> > > > > > > > 
> > > > > > > > 
> > > > > > > > I am concerned that full remove is a heavyweight operation.
> > > > > > > > Do not want to slow down reboots even more.
> > > > > > > > How about just doing a reset, instead?
> > > > > > > 
> > > > > > > I tested with
> > > > > > > 
> > > > > > > static void virtio_dev_shutdown(struct device *_d)
> > > > > > > {
> > > > > > >           struct virtio_device *dev = dev_to_virtio(_d);
> > > > > > > 
> > > > > > >           virtio_reset_device(dev);
> > > > > > > }
> > > > > > > 
> > > > > > > 
> > > > > > > and it fixes my issue.
> > > > > > > 
> > > > > > > Kirill, would that fix you issue too?
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > sorry for my late response, I synced with Kirill offline and did a retest.
> > > > > 
> > > > > The issue is still reproduced on my side, kexec will be stuck in case of
> > > > > "console=hvc0" append in kernel cmdline and even with such patch applied.
> > > > 
> > > > Thanks for testing!
> > > > 
> > > > Michael, it looks like the initial patch from Kyrill is the one that
> > > > fixes both issues. virtio_reset_device() usage does not work for the
> > > > initial bug report while it works for me. Other ideas?
> > > > 
> > > > Thanks
> > > > 
> > > > Eric
> > > 
> > > Ah, wait a second.
> > > 
> > > Looks like virtio-console continues to write to the MMIO even after
> > > underlying virtio-pci device is removed.
> > > 
> > > Hmm. I am not sure why that is a problem, but I assume some hypervisors just
> > > hang the system if you try to kick them after reset.
> > > Unfortunate that spec did not disallow it.
> > > 
> > > If we want to prevent that, we want to do something like this:
> > > 
> > > 
> > > 	/*
> > > 	 * Some devices get wedged if you kick them after they are
> > > 	 * reset. Mark all vqs as broken to make sure we don't.
> > > 	 */
> > >          virtio_break_device(dev);
> > >          /*
> > >           * The below virtio_synchronize_cbs() guarantees that any
> > >           * interrupt for this line arriving after
> > >           * virtio_synchronize_vqs() has completed is guaranteed to see
> > >           * vq->broken as true.
> > >           */
> > >          virtio_synchronize_cbs(dev);
> > >          dev->config->reset(dev);
> > > 
> > > 
> > > I assume this still works for you, yes?
> > Would that still been done in the virtio_dev_shutdown()?
> > 
> > Is that what you tested Hongyu?
> 
> Hi Eric,
> 
> my patch applied based on Michael's comments:
> 
> ---
>  drivers/virtio/virtio.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index ba37665188b5..458dc28be060 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -395,6 +395,25 @@ static const struct cpumask
> *virtio_irq_get_affinity(struct device *_d,
>         return dev->config->get_vq_affinity(dev, irq_vec);
>  }
> 
> +static void virtio_dev_shutdown(struct device *_d)
> +{
> +        struct virtio_device *dev = dev_to_virtio(_d);
> +        /*
> +         * Some devices get wedged if you kick them after they are
> +         * reset. Mark all vqs as broken to make sure we don't.
> +         */
> +        virtio_break_device(dev);
> +        /*
> +         * The below virtio_synchronize_cbs() guarantees that any
> +         * interrupt for this line arriving after
> +         * virtio_synchronize_vqs() has completed is guaranteed to see
> +         * vq->broken as true.
> +         */
> +        virtio_synchronize_cbs(dev);
> +        dev->config->reset(dev);
> +       //virtio_reset_device(dev);
> +}
> +
>  static const struct bus_type virtio_bus = {
>         .name  = "virtio",
>         .match = virtio_dev_match,
> @@ -403,6 +422,7 @@ static const struct bus_type virtio_bus = {
>         .probe = virtio_dev_probe,
>         .remove = virtio_dev_remove,
>         .irq_get_affinity = virtio_irq_get_affinity,
> +       .shutdown = virtio_dev_shutdown,
>  };
> 
>  int __register_virtio_driver(struct virtio_driver *driver, struct module
> *owner)
> --
> 2.43.0



yes, that's the patch. Does it work for you? Addresses the
problem? If yes I'll repost.

> 
> > 
> > Eric
> > > 
> > > 
> > > 
> > > > > 
> > > > > my kernel code base is 6.14.0-rc2.
> > > > > 
> > > > > let me know if any more experiments needed.
> > > > > 
> > > > > ---
> > > > >   drivers/virtio/virtio.c | 8 ++++++++
> > > > >   1 file changed, 8 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > index ba37665188b5..f9f885d04763 100644
> > > > > --- a/drivers/virtio/virtio.c
> > > > > +++ b/drivers/virtio/virtio.c
> > > > > @@ -395,6 +395,13 @@ static const struct cpumask
> > > > > *virtio_irq_get_affinity(struct device *_d,
> > > > >          return dev->config->get_vq_affinity(dev, irq_vec);
> > > > >   }
> > > > > 
> > > > > +static void virtio_dev_shutdown(struct device *_d)
> > > > > +{
> > > > > +        struct virtio_device *dev = dev_to_virtio(_d);
> > > > > +
> > > > > +        virtio_reset_device(dev);
> > > > > +}
> > > > > +
> > > > >   static const struct bus_type virtio_bus = {
> > > > >          .name  = "virtio",
> > > > >          .match = virtio_dev_match,
> > > > > @@ -403,6 +410,7 @@ static const struct bus_type virtio_bus = {
> > > > >          .probe = virtio_dev_probe,
> > > > >          .remove = virtio_dev_remove,
> > > > >          .irq_get_affinity = virtio_irq_get_affinity,
> > > > > +       .shutdown = virtio_dev_shutdown,
> > > > >   };
> > > > > 
> > > > >   int __register_virtio_driver(struct virtio_driver *driver, struct
> > > > > module *owner)
> > > > > -- 
> > > > > 2.43.0
> > > > > 
> > > > > 
> > > > > > gentle ping.
> > > > > > 
> > > > > > this also fixes another issue with qemu vSMMU + virtio-scsi-pci. With
> > > > > > the above addition I get rid of spurious warning in qemu on guest reboot.
> > > > > > 
> > > > > > qemu-system-aarch64: virtio: zero sized buffers are not allowed
> > > > > > qemu-system-aarch64: vhost vring error in virtqueue 0: Invalid
> > > > > > argument (22)
> > > > > > 
> > > > > > Would you mind if I respin?
> > > > > > 
> > > > > > Thanks
> > > > > > 
> > > > > > Eric
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > Thanks
> > > > > > > 
> > > > > > > Eric
> > > > > > > > 
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > >    static const struct bus_type virtio_bus = {
> > > > > > > > > >        .name  = "virtio",
> > > > > > > > > >        .match = virtio_dev_match,
> > > > > > > > > > @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
> > > > > > > > > >        .uevent = virtio_uevent,
> > > > > > > > > >        .probe = virtio_dev_probe,
> > > > > > > > > >        .remove = virtio_dev_remove,
> > > > > > > > > > +    .shutdown = virtio_dev_shutdown,
> > > > > > > > > >    };
> > > > > > > > > >      int __register_virtio_driver(struct virtio_driver *driver,
> > > > > > > > > > struct module *owner)
> > > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > 
> > > 
> > 
> > 

Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

[Ning, Hongyu]

On 2025/2/18 8:12, Michael S. Tsirkin wrote:
> On Tue, Feb 18, 2025 at 07:57:23AM +0800, Ning, Hongyu wrote:
>>
>>
>> On 2025/2/17 17:25, Eric Auger wrote:
>>> Hi Michael, Hongyu,
>>>
>>> On 2/14/25 1:16 PM, Michael S. Tsirkin wrote:
>>>> On Fri, Feb 14, 2025 at 08:56:56AM +0100, Eric Auger wrote:
>>>>> Hi,
>>>>>
>>>>> On 2/14/25 8:21 AM, Ning, Hongyu wrote:
>>>>>>
>>>>>>
>>>>>> On 2025/2/6 16:59, Eric Auger wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 2/4/25 12:46 PM, Eric Auger wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
>>>>>>>>> On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
>>>>>>>>>> Hi Kirill, Michael
>>>>>>>>>>
>>>>>>>>>> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
>>>>>>>>>>> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
>>>>>>>>>>> accesses during the hang.
>>>>>>>>>>>
>>>>>>>>>>>       Invalid read at addr 0x102877002, size 2, region '(null)',
>>>>>>>>>>> reason: rejected
>>>>>>>>>>>       Invalid write at addr 0x102877A44, size 2, region '(null)',
>>>>>>>>>>> reason: rejected
>>>>>>>>>>>       ...
>>>>>>>>>>>
>>>>>>>>>>> It was traced down to virtio-console. Kexec works fine if virtio-
>>>>>>>>>>> console
>>>>>>>>>>> is not in use.
>>>>>>>>>>>
>>>>>>>>>>> Looks like virtio-console continues to write to the MMIO even after
>>>>>>>>>>> underlying virtio-pci device is removed.
>>>>>>>>>>>
>>>>>>>>>>> The problem can be mitigated by removing all virtio devices on virtio
>>>>>>>>>>> bus shutdown.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>>>>>>>>> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
>>>>>>>>>>
>>>>>>>>>> Gentle ping on that patch that seems to have fallen though the cracks.
>>>>>>>>>>
>>>>>>>>>> I think this fix is really needed. I have another test case with a
>>>>>>>>>> rebooting guest exposed with virtio-net (backed by vhost-net) and
>>>>>>>>>> viommu. Since there is currently no shutdown for the virtio-net, on
>>>>>>>>>> reboot, the IOMMU is disabled through the native_machine_shutdown()/
>>>>>>>>>> x86_platform.iommu_shutdown() while the virtio-net is still alive.
>>>>>>>>>>
>>>>>>>>>> Normally device_shutdown() should call virtio-net shutdown before the
>>>>>>>>>> IOMMU tear down and we wouldn't see any spurious transactions after
>>>>>>>>>> iommu shutdown.
>>>>>>>>>>
>>>>>>>>>> With that fix, the above test case is fixed and I do not see spurious
>>>>>>>>>> vhost IOTLB miss spurious requests.
>>>>>>>>>>
>>>>>>>>>> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
>>>>>>>>>> IOTLB callbacks when IOMMU gets disabled,
>>>>>>>>>> https://lore.kernel.org/all/20250120173339.865681-1-
>>>>>>>>>> eric.auger@redhat.com/)
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>>>>>>>>> Tested-by: Eric Auger <eric.auger@redhat.com>
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>>
>>>>>>>>>> Eric
>>>>>>>>>>
>>>>>>>>>>> ---
>>>>>>>>>>>     drivers/virtio/virtio.c | 10 ++++++++++
>>>>>>>>>>>     1 file changed, 10 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>>>>>>>>>> index a9b93e99c23a..6c2f908eb22c 100644
>>>>>>>>>>> --- a/drivers/virtio/virtio.c
>>>>>>>>>>> +++ b/drivers/virtio/virtio.c
>>>>>>>>>>> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
>>>>>>>>>>>         of_node_put(dev->dev.of_node);
>>>>>>>>>>>     }
>>>>>>>>>>>     +static void virtio_dev_shutdown(struct device *_d)
>>>>>>>>>>> +{
>>>>>>>>>>> +    struct virtio_device *dev = dev_to_virtio(_d);
>>>>>>>>>>> +    struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>>>>>>>>>>> +
>>>>>>>>>>> +    if (drv && drv->remove)
>>>>>>>>>>> +        drv->remove(dev);
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I am concerned that full remove is a heavyweight operation.
>>>>>>>>> Do not want to slow down reboots even more.
>>>>>>>>> How about just doing a reset, instead?
>>>>>>>>
>>>>>>>> I tested with
>>>>>>>>
>>>>>>>> static void virtio_dev_shutdown(struct device *_d)
>>>>>>>> {
>>>>>>>>            struct virtio_device *dev = dev_to_virtio(_d);
>>>>>>>>
>>>>>>>>            virtio_reset_device(dev);
>>>>>>>> }
>>>>>>>>
>>>>>>>>
>>>>>>>> and it fixes my issue.
>>>>>>>>
>>>>>>>> Kirill, would that fix you issue too?
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> sorry for my late response, I synced with Kirill offline and did a retest.
>>>>>>
>>>>>> The issue is still reproduced on my side, kexec will be stuck in case of
>>>>>> "console=hvc0" append in kernel cmdline and even with such patch applied.
>>>>>
>>>>> Thanks for testing!
>>>>>
>>>>> Michael, it looks like the initial patch from Kyrill is the one that
>>>>> fixes both issues. virtio_reset_device() usage does not work for the
>>>>> initial bug report while it works for me. Other ideas?
>>>>>
>>>>> Thanks
>>>>>
>>>>> Eric
>>>>
>>>> Ah, wait a second.
>>>>
>>>> Looks like virtio-console continues to write to the MMIO even after
>>>> underlying virtio-pci device is removed.
>>>>
>>>> Hmm. I am not sure why that is a problem, but I assume some hypervisors just
>>>> hang the system if you try to kick them after reset.
>>>> Unfortunate that spec did not disallow it.
>>>>
>>>> If we want to prevent that, we want to do something like this:
>>>>
>>>>
>>>> 	/*
>>>> 	 * Some devices get wedged if you kick them after they are
>>>> 	 * reset. Mark all vqs as broken to make sure we don't.
>>>> 	 */
>>>>           virtio_break_device(dev);
>>>>           /*
>>>>            * The below virtio_synchronize_cbs() guarantees that any
>>>>            * interrupt for this line arriving after
>>>>            * virtio_synchronize_vqs() has completed is guaranteed to see
>>>>            * vq->broken as true.
>>>>            */
>>>>           virtio_synchronize_cbs(dev);
>>>>           dev->config->reset(dev);
>>>>
>>>>
>>>> I assume this still works for you, yes?
>>> Would that still been done in the virtio_dev_shutdown()?
>>>
>>> Is that what you tested Hongyu?
>>
>> Hi Eric,
>>
>> my patch applied based on Michael's comments:
>>
>> ---
>>   drivers/virtio/virtio.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index ba37665188b5..458dc28be060 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -395,6 +395,25 @@ static const struct cpumask
>> *virtio_irq_get_affinity(struct device *_d,
>>          return dev->config->get_vq_affinity(dev, irq_vec);
>>   }
>>
>> +static void virtio_dev_shutdown(struct device *_d)
>> +{
>> +        struct virtio_device *dev = dev_to_virtio(_d);
>> +        /*
>> +         * Some devices get wedged if you kick them after they are
>> +         * reset. Mark all vqs as broken to make sure we don't.
>> +         */
>> +        virtio_break_device(dev);
>> +        /*
>> +         * The below virtio_synchronize_cbs() guarantees that any
>> +         * interrupt for this line arriving after
>> +         * virtio_synchronize_vqs() has completed is guaranteed to see
>> +         * vq->broken as true.
>> +         */
>> +        virtio_synchronize_cbs(dev);
>> +        dev->config->reset(dev);
>> +       //virtio_reset_device(dev);
>> +}
>> +
>>   static const struct bus_type virtio_bus = {
>>          .name  = "virtio",
>>          .match = virtio_dev_match,
>> @@ -403,6 +422,7 @@ static const struct bus_type virtio_bus = {
>>          .probe = virtio_dev_probe,
>>          .remove = virtio_dev_remove,
>>          .irq_get_affinity = virtio_irq_get_affinity,
>> +       .shutdown = virtio_dev_shutdown,
>>   };
>>
>>   int __register_virtio_driver(struct virtio_driver *driver, struct module
>> *owner)
>> --
>> 2.43.0
> 
> 
> 
> yes, that's the patch. Does it work for you? Addresses the
> problem? If yes I'll repost.
> 

yes, issue fixed on my side. Thanks.

>>
>>>
>>> Eric
>>>>
>>>>
>>>>
>>>>>>
>>>>>> my kernel code base is 6.14.0-rc2.
>>>>>>
>>>>>> let me know if any more experiments needed.
>>>>>>
>>>>>> ---
>>>>>>    drivers/virtio/virtio.c | 8 ++++++++
>>>>>>    1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>>>>> index ba37665188b5..f9f885d04763 100644
>>>>>> --- a/drivers/virtio/virtio.c
>>>>>> +++ b/drivers/virtio/virtio.c
>>>>>> @@ -395,6 +395,13 @@ static const struct cpumask
>>>>>> *virtio_irq_get_affinity(struct device *_d,
>>>>>>           return dev->config->get_vq_affinity(dev, irq_vec);
>>>>>>    }
>>>>>>
>>>>>> +static void virtio_dev_shutdown(struct device *_d)
>>>>>> +{
>>>>>> +        struct virtio_device *dev = dev_to_virtio(_d);
>>>>>> +
>>>>>> +        virtio_reset_device(dev);
>>>>>> +}
>>>>>> +
>>>>>>    static const struct bus_type virtio_bus = {
>>>>>>           .name  = "virtio",
>>>>>>           .match = virtio_dev_match,
>>>>>> @@ -403,6 +410,7 @@ static const struct bus_type virtio_bus = {
>>>>>>           .probe = virtio_dev_probe,
>>>>>>           .remove = virtio_dev_remove,
>>>>>>           .irq_get_affinity = virtio_irq_get_affinity,
>>>>>> +       .shutdown = virtio_dev_shutdown,
>>>>>>    };
>>>>>>
>>>>>>    int __register_virtio_driver(struct virtio_driver *driver, struct
>>>>>> module *owner)
>>>>>> -- 
>>>>>> 2.43.0
>>>>>>
>>>>>>
>>>>>>> gentle ping.
>>>>>>>
>>>>>>> this also fixes another issue with qemu vSMMU + virtio-scsi-pci. With
>>>>>>> the above addition I get rid of spurious warning in qemu on guest reboot.
>>>>>>>
>>>>>>> qemu-system-aarch64: virtio: zero sized buffers are not allowed
>>>>>>> qemu-system-aarch64: vhost vring error in virtqueue 0: Invalid
>>>>>>> argument (22)
>>>>>>>
>>>>>>> Would you mind if I respin?
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> Eric
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>>> Eric
>>>>>>>>>
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>>     static const struct bus_type virtio_bus = {
>>>>>>>>>>>         .name  = "virtio",
>>>>>>>>>>>         .match = virtio_dev_match,
>>>>>>>>>>> @@ -363,6 +372,7 @@ static const struct bus_type virtio_bus = {
>>>>>>>>>>>         .uevent = virtio_uevent,
>>>>>>>>>>>         .probe = virtio_dev_probe,
>>>>>>>>>>>         .remove = virtio_dev_remove,
>>>>>>>>>>> +    .shutdown = virtio_dev_shutdown,
>>>>>>>>>>>     };
>>>>>>>>>>>       int __register_virtio_driver(struct virtio_driver *driver,
>>>>>>>>>>> struct module *owner)
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>>
> 
>