Message-ID: <20250217181953-mutt-send-email-mst@kernel.org>
Date: Mon, 17 Feb 2025 18:20:17 -0500
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Eric Auger <eauger@...hat.com>
Cc: "Ning, Hongyu" <hongyu.ning@...ux.intel.com>,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Jason Wang <jasowang@...hat.com>,
	Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
	Eugenio Pérez <eperezma@...hat.com>,
	Zhenzhong Duan <zhenzhong.duan@...el.com>,
	virtualization@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

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@...ux.intel.com>
> >>>>>>>>> Reported-by: Hongyu Ning <hongyu.ning@...ux.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@...hat.com/)
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Reviewed-by: Eric Auger <eric.auger@...hat.com>
> >>>>>>>> Tested-by: Eric Auger <eric.auger@...hat.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)
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>
> > 

Powered by blists - more mailing lists