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

From: Jason Wang
Date: Sun Feb 16 2025 - 22:29:33 EST

Next message: Waiman Long: "Re: [PATCH v2] kasan: Don't call find_vm_area() in RT kernel"
Previous message: Dongliang Mu: "Re: [PATCH 2/2] docs/zh_CN: add few request for Chinese translation"
Next in thread: Ning, Hongyu: "Re: [PATCH] virtio: Remove virtio devices on device_shutdown()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Feb 14, 2025 at 8:16 PM Michael S. Tsirkin <mst@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > >>>>>> Reported-by: Hongyu Ning <hongyu.ning@xxxxxxxxxxxxxxx>
> > >>>>>
> > >>>>> 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@xxxxxxxxxx/)
> > >>>>>
> > >>>>>
> > >>>>> Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx>
> > >>>>> Tested-by: Eric Auger <eric.auger@xxxxxxxxxx>
> > >>>>>
> > >>>>> 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)
> > >>>>
> > >>>
> > >>
> > >
>

Next message: Waiman Long: "Re: [PATCH v2] kasan: Don't call find_vm_area() in RT kernel"
Previous message: Dongliang Mu: "Re: [PATCH 2/2] docs/zh_CN: add few request for Chinese translation"
Next in thread: Ning, Hongyu: "Re: [PATCH] virtio: Remove virtio devices on device_shutdown()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]