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

[Eric Auger <eauger@redhat.com>]

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