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

["Michael S. Tsirkin" <mst@redhat.com>]

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