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

["Ning, Hongyu" <hongyu.ning@linux.intel.com>]

On 2025/2/18 8:12, Michael S. Tsirkin wrote:
> On Tue, Feb 18, 2025 at 07:57:23AM +0800, Ning, Hongyu wrote:
>>
>>
>> On 2025/2/17 17:25, 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@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?
>>> Would that still been done in the virtio_dev_shutdown()?
>>>
>>> Is that what you tested Hongyu?
>>
>> Hi Eric,
>>
>> my patch applied based on Michael's comments:
>>
>> ---
>>   drivers/virtio/virtio.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index ba37665188b5..458dc28be060 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -395,6 +395,25 @@ 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);
>> +        /*
>> +         * 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);
>> +       //virtio_reset_device(dev);
>> +}
>> +
>>   static const struct bus_type virtio_bus = {
>>          .name  = "virtio",
>>          .match = virtio_dev_match,
>> @@ -403,6 +422,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
> 
> 
> 
> yes, that's the patch. Does it work for you? Addresses the
> problem? If yes I'll repost.
> 

yes, issue fixed on my side. Thanks.

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