virtio: break and reset virtio devices on device_shutdown()

[PATCH] virtio: break and reset virtio devices on device_shutdown()

Posted by Michael S. Tsirkin 9 months, 4 weeks ago

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.

The issue is that virtio-console continues to write to the MMIO even after
underlying virtio-pci device is reset.

Additionally, Eric noticed that IOMMUs are reset before devices, if
devices are not reset on shutdown they continue to poke at guest memory
and get errors from the IOMMU. Some devices get wedged then.

The problem can be solved by breaking all virtio devices on virtio
bus shutdown, then resetting them.

Reported-by: Eric Auger <eauger@redhat.com>
Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index c1cc1157b380..e5b29520d3b2 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -377,6 +377,36 @@ 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);
+
+	/*
+	 * Stop accesses to or from the device.
+	 * We only need to do it if there's a driver - no accesses otherwise.
+	 */
+	if (!drv)
+		return;
+
+	/*
+	 * 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);
+	/*
+	 * As IOMMUs are reset on shutdown, this will block device access to memory.
+	 * Some devices get wedged if this happens, so reset to make sure it does not.
+	 */
+	dev->config->reset(dev);
+}
+
 static const struct bus_type virtio_bus = {
 	.name  = "virtio",
 	.match = virtio_dev_match,
@@ -384,6 +414,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)
-- 
MST

Re: [PATCH] virtio: break and reset virtio devices on device_shutdown()

Posted by Eric Auger 9 months, 3 weeks ago

Hi Michael,

On 2/21/25 12:42 AM, Michael S. Tsirkin 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.
> 
> The issue is that virtio-console continues to write to the MMIO even after
> underlying virtio-pci device is reset.
> 
> Additionally, Eric noticed that IOMMUs are reset before devices, if
> devices are not reset on shutdown they continue to poke at guest memory
> and get errors from the IOMMU. Some devices get wedged then.
> 
> The problem can be solved by breaking all virtio devices on virtio
> bus shutdown, then resetting them.
> 
> Reported-by: Eric Auger <eauger@redhat.com>
> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>

Thanks!

Eric
> ---
>  drivers/virtio/virtio.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index c1cc1157b380..e5b29520d3b2 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -377,6 +377,36 @@ 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);
> +
> +	/*
> +	 * Stop accesses to or from the device.
> +	 * We only need to do it if there's a driver - no accesses otherwise.
> +	 */
> +	if (!drv)
> +		return;
> +
> +	/*
> +	 * 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);
> +	/*
> +	 * As IOMMUs are reset on shutdown, this will block device access to memory.
> +	 * Some devices get wedged if this happens, so reset to make sure it does not.
> +	 */
> +	dev->config->reset(dev);
> +}
> +
>  static const struct bus_type virtio_bus = {
>  	.name  = "virtio",
>  	.match = virtio_dev_match,
> @@ -384,6 +414,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: break and reset virtio devices on device_shutdown()

Posted by Michael S. Tsirkin 9 months, 3 weeks ago

On Mon, Feb 24, 2025 at 08:49:09AM +0100, Eric Auger wrote:
> Hi Michael,
> 
> On 2/21/25 12:42 AM, Michael S. Tsirkin 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.
> > 
> > The issue is that virtio-console continues to write to the MMIO even after
> > underlying virtio-pci device is reset.
> > 
> > Additionally, Eric noticed that IOMMUs are reset before devices, if
> > devices are not reset on shutdown they continue to poke at guest memory
> > and get errors from the IOMMU. Some devices get wedged then.
> > 
> > The problem can be solved by breaking all virtio devices on virtio
> > bus shutdown, then resetting them.
> > 
> > Reported-by: Eric Auger <eauger@redhat.com>
> > Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>
> 
> Thanks!
> 
> Eric
> > ---
> >  drivers/virtio/virtio.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index c1cc1157b380..e5b29520d3b2 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -377,6 +377,36 @@ 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);
> > +
> > +	/*
> > +	 * Stop accesses to or from the device.
> > +	 * We only need to do it if there's a driver - no accesses otherwise.
> > +	 */
> > +	if (!drv)
> > +		return;
> > +
> > +	/*
> > +	 * 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);
> > +	/*
> > +	 * As IOMMUs are reset on shutdown, this will block device access to memory.
> > +	 * Some devices get wedged if this happens, so reset to make sure it does not.
> > +	 */

Eric,
Could you pls drop the below line (reset), and retest?
I want to make sure the above comment is right.
Thanks!

> > +	dev->config->reset(dev);


> > +}
> > +
> >  static const struct bus_type virtio_bus = {
> >  	.name  = "virtio",
> >  	.match = virtio_dev_match,
> > @@ -384,6 +414,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: break and reset virtio devices on device_shutdown()

Posted by Eric Auger 9 months, 3 weeks ago

Hi Michael,

On 2/24/25 8:31 PM, Michael S. Tsirkin wrote:
> On Mon, Feb 24, 2025 at 08:49:09AM +0100, Eric Auger wrote:
>> Hi Michael,
>>
>> On 2/21/25 12:42 AM, Michael S. Tsirkin 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.
>>>
>>> The issue is that virtio-console continues to write to the MMIO even after
>>> underlying virtio-pci device is reset.
>>>
>>> Additionally, Eric noticed that IOMMUs are reset before devices, if
>>> devices are not reset on shutdown they continue to poke at guest memory
>>> and get errors from the IOMMU. Some devices get wedged then.
>>>
>>> The problem can be solved by breaking all virtio devices on virtio
>>> bus shutdown, then resetting them.
>>>
>>> Reported-by: Eric Auger <eauger@redhat.com>
>>> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> Tested-by: Eric Auger <eric.auger@redhat.com>
>>
>> Thanks!
>>
>> Eric
>>> ---
>>>  drivers/virtio/virtio.c | 31 +++++++++++++++++++++++++++++++
>>>  1 file changed, 31 insertions(+)
>>>
>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>> index c1cc1157b380..e5b29520d3b2 100644
>>> --- a/drivers/virtio/virtio.c
>>> +++ b/drivers/virtio/virtio.c
>>> @@ -377,6 +377,36 @@ 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);
>>> +
>>> +	/*
>>> +	 * Stop accesses to or from the device.
>>> +	 * We only need to do it if there's a driver - no accesses otherwise.
>>> +	 */
>>> +	if (!drv)
>>> +		return;
>>> +
>>> +	/*
>>> +	 * 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);
>>> +	/*
>>> +	 * As IOMMUs are reset on shutdown, this will block device access to memory.
>>> +	 * Some devices get wedged if this happens, so reset to make sure it does not.
>>> +	 */
> 
> Eric,
> Could you pls drop the below line (reset), and retest?
> I want to make sure the above comment is right.
> Thanks!
If I removed the line below, I hit the issue again:

qemu-system-aarch64: virtio: zero sized buffers are not allowed

So to me this is needed and the comment is right.

Eric
> 
>>> +	dev->config->reset(dev);
> 
> 
>>> +}
>>> +
>>>  static const struct bus_type virtio_bus = {
>>>  	.name  = "virtio",
>>>  	.match = virtio_dev_match,
>>> @@ -384,6 +414,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: break and reset virtio devices on device_shutdown()

Posted by Michael S. Tsirkin 9 months, 3 weeks ago

On Mon, Feb 24, 2025 at 08:49:09AM +0100, Eric Auger wrote:
> Hi Michael,
> 
> On 2/21/25 12:42 AM, Michael S. Tsirkin 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.
> > 
> > The issue is that virtio-console continues to write to the MMIO even after
> > underlying virtio-pci device is reset.
> > 
> > Additionally, Eric noticed that IOMMUs are reset before devices, if
> > devices are not reset on shutdown they continue to poke at guest memory
> > and get errors from the IOMMU. Some devices get wedged then.
> > 
> > The problem can be solved by breaking all virtio devices on virtio
> > bus shutdown, then resetting them.
> > 
> > Reported-by: Eric Auger <eauger@redhat.com>
> > Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>
> 
> Thanks!
> 
> Eric



Will be in the next pull. Thanks!

Having said that


> > ---
> >  drivers/virtio/virtio.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index c1cc1157b380..e5b29520d3b2 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -377,6 +377,36 @@ 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);
> > +
> > +	/*
> > +	 * Stop accesses to or from the device.
> > +	 * We only need to do it if there's a driver - no accesses otherwise.
> > +	 */
> > +	if (!drv)
> > +		return;
> > +
> > +	/*
> > +	 * 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);
> > +	/*
> > +	 * As IOMMUs are reset on shutdown, this will block device access to memory.
> > +	 * Some devices get wedged if this happens, so reset to make sure it does not.
> > +	 */
> > +	dev->config->reset(dev);


I feel wedging devices to the point reset does not recover them
if the driver is buggy, isn't good. This is something we should
maybe fix if it's observed with qemu. Let's discuss on that list.

> > +}
> > +
> >  static const struct bus_type virtio_bus = {
> >  	.name  = "virtio",
> >  	.match = virtio_dev_match,
> > @@ -384,6 +414,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: break and reset virtio devices on device_shutdown()

Posted by Jason Wang 9 months, 4 weeks ago

On Fri, Feb 21, 2025 at 7:42 AM Michael S. Tsirkin <mst@redhat.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.
>
> The issue is that virtio-console continues to write to the MMIO even after
> underlying virtio-pci device is reset.

Some of my questions were not answered so I need to post them again.

Do we need to fix vitio-console?

Note that we've already break the device in virtio_pci_remove():

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);

...

>
> Additionally, Eric noticed that IOMMUs are reset before devices, if
> devices are not reset on shutdown they continue to poke at guest memory
> and get errors from the IOMMU. Some devices get wedged then.
>
> The problem can be solved by breaking all virtio devices on virtio
> bus shutdown, then resetting them.
>
> Reported-by: Eric Auger <eauger@redhat.com>
> Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/virtio/virtio.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index c1cc1157b380..e5b29520d3b2 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -377,6 +377,36 @@ 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);
> +
> +       /*
> +        * Stop accesses to or from the device.
> +        * We only need to do it if there's a driver - no accesses otherwise.
> +        */
> +       if (!drv)
> +               return;
> +
> +       /*
> +        * 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 looks like a partial re-introduction of the hardening work, but
the ccw part is still in-completed e.g the synchronization requires a
read_lock() and depends on CONFIG_VIRTIO_HARDEN_NOTIFICATION (which is
marked as broken now).

Should it better to

1) fix the virtio-console
2) simply rest in shutdown
3) wait for the hardening work to be done in the future?

Thanks

> +       /*
> +        * As IOMMUs are reset on shutdown, this will block device access to memory.
> +        * Some devices get wedged if this happens, so reset to make sure it does not.
> +        */
> +       dev->config->reset(dev);
> +}
> +
>  static const struct bus_type virtio_bus = {
>         .name  = "virtio",
>         .match = virtio_dev_match,
> @@ -384,6 +414,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)
> --
> MST
>

Re: [PATCH] virtio: break and reset virtio devices on device_shutdown()

Posted by Michael S. Tsirkin 9 months, 4 weeks ago

On Fri, Feb 21, 2025 at 09:11:51AM +0800, Jason Wang wrote:
> On Fri, Feb 21, 2025 at 7:42 AM Michael S. Tsirkin <mst@redhat.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.
> >
> > The issue is that virtio-console continues to write to the MMIO even after
> > underlying virtio-pci device is reset.
> 
> Some of my questions were not answered so I need to post them again.
> 
> Do we need to fix vitio-console?

this fixes all devices so no.

> Note that we've already break the device in virtio_pci_remove():
> 
> 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);
> 
> ...


shutdown path does not invoke remove, and I do not want to,
since that will slow down reboots for no good reason.

> >
> > Additionally, Eric noticed that IOMMUs are reset before devices, if
> > devices are not reset on shutdown they continue to poke at guest memory
> > and get errors from the IOMMU. Some devices get wedged then.
> >
> > The problem can be solved by breaking all virtio devices on virtio
> > bus shutdown, then resetting them.
> >
> > Reported-by: Eric Auger <eauger@redhat.com>
> > Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  drivers/virtio/virtio.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index c1cc1157b380..e5b29520d3b2 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -377,6 +377,36 @@ 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);
> > +
> > +       /*
> > +        * Stop accesses to or from the device.
> > +        * We only need to do it if there's a driver - no accesses otherwise.
> > +        */
> > +       if (!drv)
> > +               return;
> > +
> > +       /*
> > +        * 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 looks like a partial re-introduction of the hardening work, but
> the ccw part is still in-completed e.g the synchronization requires a
> read_lock() and depends on CONFIG_VIRTIO_HARDEN_NOTIFICATION (which is
> marked as broken now).

Hmm I don't understand why we can not just take a subchannel lock.
worth thinking about it. as this doesn't regress i think
I can live with it for now.

> Should it better to
> 
> 1) fix the virtio-console

the problem is not unique to console. it was just observed there.
any device can trigger this.

> 2) simply rest in shutdown

then we are back with devices wedged as they are kicked
while reset.

> 3) wait for the hardening work to be done in the future?
> 
> Thanks

as users are seeing this on x86, I want to fix it.
finding a way to fix virtio_synchronize_cbs seems best.

does not need to block this patch.

> > +       /*
> > +        * As IOMMUs are reset on shutdown, this will block device access to memory.
> > +        * Some devices get wedged if this happens, so reset to make sure it does not.
> > +        */
> > +       dev->config->reset(dev);
> > +}
> > +
> >  static const struct bus_type virtio_bus = {
> >         .name  = "virtio",
> >         .match = virtio_dev_match,
> > @@ -384,6 +414,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)
> > --
> > MST
> >

Re: [PATCH] virtio: break and reset virtio devices on device_shutdown()

Posted by Jason Wang 9 months, 3 weeks ago

On Fri, Feb 21, 2025 at 9:37 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Feb 21, 2025 at 09:11:51AM +0800, Jason Wang wrote:
> > On Fri, Feb 21, 2025 at 7:42 AM Michael S. Tsirkin <mst@redhat.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.
> > >
> > > The issue is that virtio-console continues to write to the MMIO even after
> > > underlying virtio-pci device is reset.
> >
> > Some of my questions were not answered so I need to post them again.
> >
> > Do we need to fix vitio-console?
>
> this fixes all devices so no.
>
> > Note that we've already break the device in virtio_pci_remove():
> >
> > 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);
> >
> > ...
>
>
> shutdown path does not invoke remove, and I do not want to,
> since that will slow down reboots for no good reason.
>
> > >
> > > Additionally, Eric noticed that IOMMUs are reset before devices, if
> > > devices are not reset on shutdown they continue to poke at guest memory
> > > and get errors from the IOMMU. Some devices get wedged then.
> > >
> > > The problem can be solved by breaking all virtio devices on virtio
> > > bus shutdown, then resetting them.
> > >
> > > Reported-by: Eric Auger <eauger@redhat.com>
> > > Reported-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  drivers/virtio/virtio.c | 31 +++++++++++++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >
> > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > index c1cc1157b380..e5b29520d3b2 100644
> > > --- a/drivers/virtio/virtio.c
> > > +++ b/drivers/virtio/virtio.c
> > > @@ -377,6 +377,36 @@ 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);
> > > +
> > > +       /*
> > > +        * Stop accesses to or from the device.
> > > +        * We only need to do it if there's a driver - no accesses otherwise.
> > > +        */
> > > +       if (!drv)
> > > +               return;
> > > +
> > > +       /*
> > > +        * 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 looks like a partial re-introduction of the hardening work, but
> > the ccw part is still in-completed e.g the synchronization requires a
> > read_lock() and depends on CONFIG_VIRTIO_HARDEN_NOTIFICATION (which is
> > marked as broken now).
>
> Hmm I don't understand why we can not just take a subchannel lock.

Might be, just want to point that ccw (and there maybe other transport
that) lacks synchronization.

> worth thinking about it. as this doesn't regress i think
> I can live with it for now.
>
> > Should it better to
> >
> > 1) fix the virtio-console
>
> the problem is not unique to console. it was just observed there.
> any device can trigger this.
>
> > 2) simply rest in shutdown
>
> then we are back with devices wedged as they are kicked
> while reset.
>
> > 3) wait for the hardening work to be done in the future?
> >
> > Thanks
>
> as users are seeing this on x86, I want to fix it.
> finding a way to fix virtio_synchronize_cbs seems best.
>
> does not need to block this patch.

Ok.

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

>
> > > +       /*
> > > +        * As IOMMUs are reset on shutdown, this will block device access to memory.
> > > +        * Some devices get wedged if this happens, so reset to make sure it does not.
> > > +        */
> > > +       dev->config->reset(dev);
> > > +}
> > > +
> > >  static const struct bus_type virtio_bus = {
> > >         .name  = "virtio",
> > >         .match = virtio_dev_match,
> > > @@ -384,6 +414,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)
> > > --
> > > MST
> > >
>