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

[Eric Auger]

On 2/6/25 11:04 AM, Kirill A. Shutemov wrote:
> On Thu, Feb 06, 2025 at 09:59:58AM +0100, 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?
>> gentle ping.
> 
> I am on vacation this week. Will try to get around to it next week.
OK. Enjoy your vacation!

Eric
>