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

[Jason Wang <jasowang@redhat.com>]

On Wed, Jan 22, 2025 at 3:55 PM Eric Auger <eric.auger@redhat.com> wrote:
>
> Hi Jason,
>
>
> On 1/22/25 8:17 AM, Jason Wang wrote:
> > On Wed, Jan 22, 2025 at 12:25 AM Eric Auger <eric.auger@redhat.com> wrote:
> >>
> >> Hi Jason,
> >>
> >> On 1/21/25 4:27 AM, Jason Wang wrote:
> >>> On Tue, Jan 21, 2025 at 1:33 AM Eric Auger <eric.auger@redhat.com> 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);
> >>>> +    }
> >>> So the current code assumes:
> >>>
> >>> 1) IOMMU is enabled before vhost starts
> >>> 2) IOMMU is disabled after vhost stops
> >>>
> >>> This patch seems to fix 2) but not 1). Do we need to deal with the
> >>> IOMMU enabled after vhost starts?
> >> sorry I initially misunderstood the above comment. Indeed in the reboot
> >> case assumption 2) happens to be wrong. However what I currently do is:
> >> stop listening to iotlb miss requests from the kernel because my
> >> understanding is those requests are just spurious ones, generate
> >> warnings and we do not care since we are rebooting the system.
> >>
> >> However I do not claim this could handle the case where the IOMMU MR
> >> would be turned off and then turned on. I think in that case we should
> >> also flush the kernel IOTLB and this is not taken care of in this patch.
> >> Is it a relevant use case?
> > Not sure.
> >
> >> wrt removing assumption 1) and allow IOMMU enabled after vhost start. Is
> >> that a valid use case as the virtio driver is using the dma api?
> > It should not be but we can't assume the behaviour of the guest. It
> > could be buggy or even malicious.
>
> agreed
> >
> > Btw, we had the following codes while handling te:
> >
> > /* Handle Translation Enable/Disable */
> > static void vtd_handle_gcmd_te(IntelIOMMUState *s, bool en)
> > {
> >     if (s->dmar_enabled == en) {
> >         return;
> >     }
> >
> >     trace_vtd_dmar_enable(en);
> >
> > ...
> >
> >     vtd_reset_caches(s);
> >     vtd_address_space_refresh_all(s);
> > }
> >
> > vtd_address_space_refresh_all() will basically disable the iommu
> > memory region. It looks not sufficient to trigger the region_del
> > callback, maybe we should delete the region or introduce listener
> > callback?
>
> This is exactly the code path which is entered in my use case.
>
> vtd_address_space_refresh_all(s) induces the vhost_iommu_region_del. But given the current implement of this latter the IOTLB callback is not unset and the kernel IOTLB is not refreshed. Also as I pointed out the  hdev->mem->regions are not updated? shouldn't they. Can you explain what they correspond to?

Adding Peter for more ideas.

I think it's better to find a way to trigger the listener here, probably:

1) add/delete the memory regions instead of enable/disable

or

2) introduce new listener ops that can be triggered when a region is
enabled or disabled

Thanks

>
> Thanks
>
> Eric
>
> >
> > Thanks
> >
> >> Eric
> >>
> >>
> >>> Thanks
> >>>
> >>>>  }
> >>>>
> >>>>  void vhost_toggle_device_iotlb(VirtIODevice *vdev)
> >>>> --
> >>>> 2.47.1
> >>>>
>