Message-ID: <CACGkMEuKb040MZHeNEX2b2XhuJMLpCwBgTms_zL5NEw9KFgzew@mail.gmail.com>
Date: Mon, 24 Feb 2025 10:07:11 +0800
From: Jason Wang <jasowang@...hat.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: linux-kernel@...r.kernel.org, Eric Auger <eauger@...hat.com>, 
	Hongyu Ning <hongyu.ning@...ux.intel.com>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, 
	Eugenio Pérez <eperezma@...hat.com>, 
	virtualization@...ts.linux.dev
Subject: Re: [PATCH] virtio: break and reset virtio devices on device_shutdown()

On Fri, Feb 21, 2025 at 9:37 AM Michael S. Tsirkin <mst@...hat.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@...hat.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@...hat.com>
> > > Reported-by: Hongyu Ning <hongyu.ning@...ux.intel.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@...hat.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@...hat.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
> > >
>

Powered by blists - more mailing lists