| Message ID | 20250120173339.865681-1-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/virtio/vhost: Disable IOTLB callbacks when IOMMU gets disabled | expand |
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? Thanks > } > > void vhost_toggle_device_iotlb(VirtIODevice *vdev) > -- > 2.47.1 >
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? This patch handles the case where the IOMMU is disabled *before* vhost stops (not 2). This is what I concretely observe on guest reboot. But maybe I misunderstood your comments/questions? Thanks Eric > > Thanks > >> } >> >> void vhost_toggle_device_iotlb(VirtIODevice *vdev) >> -- >> 2.47.1 >>
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 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) Perhaps you should move the vhost_set_iotlb_callback() call from vhost_start()/vhost_stop() to vhost_iommu_region_add()/vhost_iommu_region_del()? Thanks, Laurent
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. Stefano
On 1/21/25 9:31 AM, 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 > instance if we have an add after a del that results in an empty list). I guess you meant vhost_iommu_region_add() > But you cannot unconditionally enable it (for instance if vhost is not > started) agreed. I will further look at the control path. > > Perhaps you should move the vhost_set_iotlb_callback() call from > vhost_start()/vhost_stop() to > vhost_iommu_region_add()/vhost_iommu_region_del()? Interesting. I will study that. Thanks! Eric > > Thanks, > Laurent >
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 >
Hi Eric, >-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: [PATCH] hw/virtio/vhost: Disable IOTLB callbacks when IOMMU gets >disabled > >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 Do you see this print once during one time reboot? > >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. Try to understand the sequence, is it like below? vhost vcpu call into vtd_iommu_translate(); set s->dmar_enabled = false; switch off iommu address space; disable vhost IOTLB callbacks; check if !s->dmar_enabled; return flat translation and trigger warning Thanks Zhenzhong > >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) >-- >2.47.1
Hi Zhenzhong, On 1/21/25 10:18 AM, Duan, Zhenzhong wrote: > Hi Eric, > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Subject: [PATCH] hw/virtio/vhost: Disable IOTLB callbacks when IOMMU gets >> disabled >> >> 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 > Do you see this print once during one time reboot? Actually this happens rarely on reboot. The reproducibility is of the order of magnitude of 1/10 for me. I use a vm with vhost net device + virtual intel iommu featuring a crontab job. @reboot /usr/sbin/reboot > >> 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. > Try to understand the sequence, is it like below? > > vhost vcpu > > call into vtd_iommu_translate(); No that's a kernel vhost translate request that normally tries to find out the translated address on kernel side in the IOTLB but since the data is not there it ends up asking for the translation to user space ... > > set s->dmar_enabled = false; > switch off iommu address space; > disable vhost IOTLB callbacks; vtd_handle_gcmd_write/vtd_handle_gcmd_te/vtd_handle_gcmd_te which eventually calls vhost_iommu_region_del > > check if !s->dmar_enabled; > return flat translation and trigger warning vhost inflight translation reaches user space through vhost_device_iotlb_miss() > > Thanks > Zhenzhong > >> 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) >> -- >> 2.47.1
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: Re: [PATCH] hw/virtio/vhost: Disable IOTLB callbacks when IOMMU gets >disabled > >Hi Zhenzhong, > > >On 1/21/25 10:18 AM, Duan, Zhenzhong wrote: >> Hi Eric, >> >>> -----Original Message----- >>> From: Eric Auger <eric.auger@redhat.com> >>> Subject: [PATCH] hw/virtio/vhost: Disable IOTLB callbacks when IOMMU gets >>> disabled >>> >>> 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 >> Do you see this print once during one time reboot? >Actually this happens rarely on reboot. The reproducibility is of the >order of magnitude of 1/10 for me. I use a vm with vhost net device + >virtual intel iommu featuring a crontab job. >@reboot /usr/sbin/reboot >> >>> 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. >> Try to understand the sequence, is it like below? >> >> vhost vcpu >> >> call into vtd_iommu_translate(); >No that's a kernel vhost translate request that normally tries to find >out the translated address on kernel side in the IOTLB but since the >data is not there it ends up asking for the translation to user space ... >> >> set s->dmar_enabled = false; >> switch off iommu address space; >> disable vhost IOTLB callbacks; >vtd_handle_gcmd_write/vtd_handle_gcmd_te/vtd_handle_gcmd_te which >eventually calls vhost_iommu_region_del >> >> check if !s->dmar_enabled; >> return flat translation and trigger warning >vhost inflight translation reaches user space through >vhost_device_iotlb_miss() Understood, thanks Eric! BRs. Zhenzhong
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)
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(+)