Re: [PATCH] hw/virtio/vhost: Disable IOTLB callbacks when IOMMU gets disabled

[Eric Auger <eric.auger@redhat.com>]

Hi Stefano,


On 1/21/25 9:45 AM, Stefano Garzarella wrote:
> On Tue, Jan 21, 2025 at 09:31:53AM +0100, Laurent Vivier wrote:
>> On 20/01/2025 18:33, Eric Auger wrote:
>>> When a guest exposed with a vhost device and protected by an
>>> intel IOMMU gets rebooted, we sometimes observe a spurious warning:
>>>
>>> Fail to lookup the translated address ffffe000
>>>
>>> We observe that the IOMMU gets disabled through a write to the global
>>> command register (CMAR_GCMD.TE) before the vhost device gets stopped.
>>> When this warning happens it can be observed an inflight IOTLB
>>> miss occurs after the IOMMU disable and before the vhost stop. In
>>> that case a flat translation occurs and the check in
>>> vhost_memory_region_lookup() fails.
>>>
>>> Let's disable the IOTLB callbacks when all IOMMU MRs have been
>>> unregistered.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>>  hw/virtio/vhost.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>> index 6aa72fd434..128c2ab094 100644
>>> --- a/hw/virtio/vhost.c
>>> +++ b/hw/virtio/vhost.c
>>> @@ -931,6 +931,10 @@ static void
>>> vhost_iommu_region_del(MemoryListener *listener,
>>>              break;
>>>          }
>>>      }
>>> +    if (QLIST_EMPTY(&dev->iommu_list) &&
>>> +        dev->vhost_ops->vhost_set_iotlb_callback) {
>>> +        dev->vhost_ops->vhost_set_iotlb_callback(dev, false);
>>> +    }
>>>  }
>>>  void vhost_toggle_device_iotlb(VirtIODevice *vdev)
>>
>> I think you need the counterpart in vhost_iommu_region_del() (for 
>
> I guess you meant vhost_iommu_region_add(). I was going to comment
> exactly on that, I agree here.
>
>> instance if we have an add after a del that results in an empty list).
>> But you cannot unconditionally enable it (for instance if vhost is
>> not started)
>
> Good point.
>
>>
>> Perhaps you should move the vhost_set_iotlb_callback() call from
>> vhost_start()/vhost_stop() to
>> vhost_iommu_region_add()/vhost_iommu_region_del()?
>
> I also like this idea.

OK makes sense. I will go in this direction.

Eric
>
> Stefano
>