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

[Jason Wang]

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.

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?

Thanks

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