| Message ID | 20210127204449.745365-1-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vhost-user: Check for iotlb callback in iotlb_miss | expand |
On 2021/1/28 上午4:44, Eugenio Pérez wrote: > Not registering this can lead to vhost_backend_handle_iotlb_msg and > vhost_device_iotlb_miss if backend issue a miss after qemu vhost device > stop. > > This causes a try to access dev->vdev->dma_as with vdev == NULL. Hi Eugenio: What condition can we get this? Did you mean we receive IOTLB_MISS before vhost_dev_start()? If yes, it looks to me a bug somewhere else. Thanks > > Reproduced rebooting a guest with testpmd in txonly forward mode. > #0 0x0000559ffff94394 in vhost_device_iotlb_miss ( > dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1) > at ../hw/virtio/vhost.c:1013 > #1 0x0000559ffff9ac31 in vhost_backend_handle_iotlb_msg ( > imsg=0x7ffddcfd32c0, dev=0x55a0012f6680) > at ../hw/virtio/vhost-backend.c:411 > #2 vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680, > imsg=imsg@entry=0x7ffddcfd32c0) > at ../hw/virtio/vhost-backend.c:404 > #3 0x0000559fffeded7b in slave_read (opaque=0x55a0012f6680) > at ../hw/virtio/vhost-user.c:1464 > #4 0x000055a0000c541b in aio_dispatch_handler ( > ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00) > at ../util/aio-posix.c:329 > > Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support") > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > hw/virtio/vhost-user.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 2fdd5daf74..a49b2229fb 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -238,6 +238,7 @@ struct vhost_user { > /* Shared between vhost devs of the same virtio device */ > VhostUserState *user; > int slave_fd; > + bool iotlb_enabled; > NotifierWithReturn postcopy_notifier; > struct PostCopyFD postcopy_fd; > uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS]; > @@ -1461,7 +1462,11 @@ static void slave_read(void *opaque) > > switch (hdr.request) { > case VHOST_USER_SLAVE_IOTLB_MSG: > - ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); > + if (likely(u->iotlb_enabled)) { > + ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); > + } else { > + ret = -EFAULT; > + } > break; > case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG : > ret = vhost_user_slave_handle_config_change(dev); > @@ -2044,7 +2049,8 @@ static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev, > > static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled) > { > - /* No-op as the receive channel is not dedicated to IOTLB messages. */ > + struct vhost_user *u = dev->opaque; > + u->iotlb_enabled = enabled; > } > > static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
Hi Jason. On Thu, Jan 28, 2021 at 3:32 AM Jason Wang <jasowang@redhat.com> wrote: > > > On 2021/1/28 上午4:44, Eugenio Pérez wrote: > > Not registering this can lead to vhost_backend_handle_iotlb_msg and > > vhost_device_iotlb_miss if backend issue a miss after qemu vhost device > > stop. > > > > This causes a try to access dev->vdev->dma_as with vdev == NULL. > > > Hi Eugenio: > > What condition can we get this? Did you mean we receive IOTLB_MISS > before vhost_dev_start()? > In the VM reboot case, yes, between vhost_dev_stop() and vhost_dev_start(). But I can reproduce the bug in VM shutdown too, with no vhost_dev_start expected. I didn't try to send IOTLB_MISS before starting vhost_dev, but this patch should solve that problem too. I think we can get this with whatever malicious/buggy vhost-user backend sending unexpected iotlb misses, but I didn't test so deeply. > If yes, it looks to me a bug somewhere else. Where should I look for it? Thanks! > > Thanks > > > > > > Reproduced rebooting a guest with testpmd in txonly forward mode. > > #0 0x0000559ffff94394 in vhost_device_iotlb_miss ( > > dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1) > > at ../hw/virtio/vhost.c:1013 > > #1 0x0000559ffff9ac31 in vhost_backend_handle_iotlb_msg ( > > imsg=0x7ffddcfd32c0, dev=0x55a0012f6680) > > at ../hw/virtio/vhost-backend.c:411 > > #2 vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680, > > imsg=imsg@entry=0x7ffddcfd32c0) > > at ../hw/virtio/vhost-backend.c:404 > > #3 0x0000559fffeded7b in slave_read (opaque=0x55a0012f6680) > > at ../hw/virtio/vhost-user.c:1464 > > #4 0x000055a0000c541b in aio_dispatch_handler ( > > ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00) > > at ../util/aio-posix.c:329 > > > > Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support") > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > hw/virtio/vhost-user.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > > index 2fdd5daf74..a49b2229fb 100644 > > --- a/hw/virtio/vhost-user.c > > +++ b/hw/virtio/vhost-user.c > > @@ -238,6 +238,7 @@ struct vhost_user { > > /* Shared between vhost devs of the same virtio device */ > > VhostUserState *user; > > int slave_fd; > > + bool iotlb_enabled; > > NotifierWithReturn postcopy_notifier; > > struct PostCopyFD postcopy_fd; > > uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS]; > > @@ -1461,7 +1462,11 @@ static void slave_read(void *opaque) > > > > switch (hdr.request) { > > case VHOST_USER_SLAVE_IOTLB_MSG: > > - ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); > > + if (likely(u->iotlb_enabled)) { > > + ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); > > + } else { > > + ret = -EFAULT; > > + } > > break; > > case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG : > > ret = vhost_user_slave_handle_config_change(dev); > > @@ -2044,7 +2049,8 @@ static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev, > > > > static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled) > > { > > - /* No-op as the receive channel is not dedicated to IOTLB messages. */ > > + struct vhost_user *u = dev->opaque; > > + u->iotlb_enabled = enabled; > > } > > > > static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config, >
On 2021/1/28 下午5:37, Eugenio Perez Martin wrote: > Hi Jason. > > On Thu, Jan 28, 2021 at 3:32 AM Jason Wang <jasowang@redhat.com> wrote: >> >> On 2021/1/28 上午4:44, Eugenio Pérez wrote: >>> Not registering this can lead to vhost_backend_handle_iotlb_msg and >>> vhost_device_iotlb_miss if backend issue a miss after qemu vhost device >>> stop. >>> >>> This causes a try to access dev->vdev->dma_as with vdev == NULL. >> >> Hi Eugenio: >> >> What condition can we get this? Did you mean we receive IOTLB_MISS >> before vhost_dev_start()? >> > In the VM reboot case, yes, between vhost_dev_stop() and > vhost_dev_start(). But I can reproduce the bug in VM shutdown too, > with no vhost_dev_start expected. > > I didn't try to send IOTLB_MISS before starting vhost_dev, but this > patch should solve that problem too. > > I think we can get this with whatever malicious/buggy vhost-user > backend sending unexpected iotlb misses, but I didn't test so deeply. I see. > >> If yes, it looks to me a bug somewhere else. > Where should I look for it? So I winder whether or not we can simply ignore IOTLB message if vhost device is not started. Thanks > > Thanks! > >> Thanks >> >> >>> Reproduced rebooting a guest with testpmd in txonly forward mode. >>> #0 0x0000559ffff94394 in vhost_device_iotlb_miss ( >>> dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1) >>> at ../hw/virtio/vhost.c:1013 >>> #1 0x0000559ffff9ac31 in vhost_backend_handle_iotlb_msg ( >>> imsg=0x7ffddcfd32c0, dev=0x55a0012f6680) >>> at ../hw/virtio/vhost-backend.c:411 >>> #2 vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680, >>> imsg=imsg@entry=0x7ffddcfd32c0) >>> at ../hw/virtio/vhost-backend.c:404 >>> #3 0x0000559fffeded7b in slave_read (opaque=0x55a0012f6680) >>> at ../hw/virtio/vhost-user.c:1464 >>> #4 0x000055a0000c541b in aio_dispatch_handler ( >>> ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00) >>> at ../util/aio-posix.c:329 >>> >>> Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support") >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> >>> --- >>> hw/virtio/vhost-user.c | 10 ++++++++-- >>> 1 file changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c >>> index 2fdd5daf74..a49b2229fb 100644 >>> --- a/hw/virtio/vhost-user.c >>> +++ b/hw/virtio/vhost-user.c >>> @@ -238,6 +238,7 @@ struct vhost_user { >>> /* Shared between vhost devs of the same virtio device */ >>> VhostUserState *user; >>> int slave_fd; >>> + bool iotlb_enabled; >>> NotifierWithReturn postcopy_notifier; >>> struct PostCopyFD postcopy_fd; >>> uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS]; >>> @@ -1461,7 +1462,11 @@ static void slave_read(void *opaque) >>> >>> switch (hdr.request) { >>> case VHOST_USER_SLAVE_IOTLB_MSG: >>> - ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); >>> + if (likely(u->iotlb_enabled)) { >>> + ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); >>> + } else { >>> + ret = -EFAULT; >>> + } >>> break; >>> case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG : >>> ret = vhost_user_slave_handle_config_change(dev); >>> @@ -2044,7 +2049,8 @@ static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev, >>> >>> static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled) >>> { >>> - /* No-op as the receive channel is not dedicated to IOTLB messages. */ >>> + struct vhost_user *u = dev->opaque; >>> + u->iotlb_enabled = enabled; >>> } >>> >>> static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
On Fri, Jan 29, 2021 at 4:29 AM Jason Wang <jasowang@redhat.com> wrote: > > > On 2021/1/28 下午5:37, Eugenio Perez Martin wrote: > > Hi Jason. > > > > On Thu, Jan 28, 2021 at 3:32 AM Jason Wang <jasowang@redhat.com> wrote: > >> > >> On 2021/1/28 上午4:44, Eugenio Pérez wrote: > >>> Not registering this can lead to vhost_backend_handle_iotlb_msg and > >>> vhost_device_iotlb_miss if backend issue a miss after qemu vhost device > >>> stop. > >>> > >>> This causes a try to access dev->vdev->dma_as with vdev == NULL. > >> > >> Hi Eugenio: > >> > >> What condition can we get this? Did you mean we receive IOTLB_MISS > >> before vhost_dev_start()? > >> > > In the VM reboot case, yes, between vhost_dev_stop() and > > vhost_dev_start(). But I can reproduce the bug in VM shutdown too, > > with no vhost_dev_start expected. > > > > I didn't try to send IOTLB_MISS before starting vhost_dev, but this > > patch should solve that problem too. > > > > I think we can get this with whatever malicious/buggy vhost-user > > backend sending unexpected iotlb misses, but I didn't test so deeply. > > > I see. > > > > > >> If yes, it looks to me a bug somewhere else. > > Where should I look for it? > > > So I winder whether or not we can simply ignore IOTLB message if vhost > device is not started. > Do you mean like an early return in vhost_device_iotlb_miss? That was my first first option, but this seems cleaner to me. I'm ok with both options. Or do you mean not to return -EFAULT but 0 if !u->iotlb_enabled ? Thanks! > Thanks > > > > > > Thanks! > > > >> Thanks > >> > >> > >>> Reproduced rebooting a guest with testpmd in txonly forward mode. > >>> #0 0x0000559ffff94394 in vhost_device_iotlb_miss ( > >>> dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1) > >>> at ../hw/virtio/vhost.c:1013 > >>> #1 0x0000559ffff9ac31 in vhost_backend_handle_iotlb_msg ( > >>> imsg=0x7ffddcfd32c0, dev=0x55a0012f6680) > >>> at ../hw/virtio/vhost-backend.c:411 > >>> #2 vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680, > >>> imsg=imsg@entry=0x7ffddcfd32c0) > >>> at ../hw/virtio/vhost-backend.c:404 > >>> #3 0x0000559fffeded7b in slave_read (opaque=0x55a0012f6680) > >>> at ../hw/virtio/vhost-user.c:1464 > >>> #4 0x000055a0000c541b in aio_dispatch_handler ( > >>> ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00) > >>> at ../util/aio-posix.c:329 > >>> > >>> Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support") > >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > >>> --- > >>> hw/virtio/vhost-user.c | 10 ++++++++-- > >>> 1 file changed, 8 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > >>> index 2fdd5daf74..a49b2229fb 100644 > >>> --- a/hw/virtio/vhost-user.c > >>> +++ b/hw/virtio/vhost-user.c > >>> @@ -238,6 +238,7 @@ struct vhost_user { > >>> /* Shared between vhost devs of the same virtio device */ > >>> VhostUserState *user; > >>> int slave_fd; > >>> + bool iotlb_enabled; > >>> NotifierWithReturn postcopy_notifier; > >>> struct PostCopyFD postcopy_fd; > >>> uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS]; > >>> @@ -1461,7 +1462,11 @@ static void slave_read(void *opaque) > >>> > >>> switch (hdr.request) { > >>> case VHOST_USER_SLAVE_IOTLB_MSG: > >>> - ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); > >>> + if (likely(u->iotlb_enabled)) { > >>> + ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); > >>> + } else { > >>> + ret = -EFAULT; > >>> + } > >>> break; > >>> case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG : > >>> ret = vhost_user_slave_handle_config_change(dev); > >>> @@ -2044,7 +2049,8 @@ static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev, > >>> > >>> static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled) > >>> { > >>> - /* No-op as the receive channel is not dedicated to IOTLB messages. */ > >>> + struct vhost_user *u = dev->opaque; > >>> + u->iotlb_enabled = enabled; > >>> } > >>> > >>> static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config, >
On 2021/1/29 下午3:22, Eugenio Perez Martin wrote: > On Fri, Jan 29, 2021 at 4:29 AM Jason Wang <jasowang@redhat.com> wrote: >> >> On 2021/1/28 下午5:37, Eugenio Perez Martin wrote: >>> Hi Jason. >>> >>> On Thu, Jan 28, 2021 at 3:32 AM Jason Wang <jasowang@redhat.com> wrote: >>>> On 2021/1/28 上午4:44, Eugenio Pérez wrote: >>>>> Not registering this can lead to vhost_backend_handle_iotlb_msg and >>>>> vhost_device_iotlb_miss if backend issue a miss after qemu vhost device >>>>> stop. >>>>> >>>>> This causes a try to access dev->vdev->dma_as with vdev == NULL. >>>> Hi Eugenio: >>>> >>>> What condition can we get this? Did you mean we receive IOTLB_MISS >>>> before vhost_dev_start()? >>>> >>> In the VM reboot case, yes, between vhost_dev_stop() and >>> vhost_dev_start(). But I can reproduce the bug in VM shutdown too, >>> with no vhost_dev_start expected. >>> >>> I didn't try to send IOTLB_MISS before starting vhost_dev, but this >>> patch should solve that problem too. >>> >>> I think we can get this with whatever malicious/buggy vhost-user >>> backend sending unexpected iotlb misses, but I didn't test so deeply. >> >> I see. >> >> >>>> If yes, it looks to me a bug somewhere else. >>> Where should I look for it? >> >> So I winder whether or not we can simply ignore IOTLB message if vhost >> device is not started. >> > Do you mean like an early return in vhost_device_iotlb_miss? Yes or probably a little bit earlier in vhost_backend_handle_iotlb_msg() which somehow a warn there. Anyhow it's meaningless to process IOTLB message in this case and we don't need to introduce a dedicated variable for this. Thanks > That was > my first first option, but this seems cleaner to me. I'm ok with both > options. > > Or do you mean not to return -EFAULT but 0 if !u->iotlb_enabled ? > > Thanks! > >> Thanks >> >> >>> Thanks! >>> >>>> Thanks >>>> >>>> >>>>> Reproduced rebooting a guest with testpmd in txonly forward mode. >>>>> #0 0x0000559ffff94394 in vhost_device_iotlb_miss ( >>>>> dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1) >>>>> at ../hw/virtio/vhost.c:1013 >>>>> #1 0x0000559ffff9ac31 in vhost_backend_handle_iotlb_msg ( >>>>> imsg=0x7ffddcfd32c0, dev=0x55a0012f6680) >>>>> at ../hw/virtio/vhost-backend.c:411 >>>>> #2 vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680, >>>>> imsg=imsg@entry=0x7ffddcfd32c0) >>>>> at ../hw/virtio/vhost-backend.c:404 >>>>> #3 0x0000559fffeded7b in slave_read (opaque=0x55a0012f6680) >>>>> at ../hw/virtio/vhost-user.c:1464 >>>>> #4 0x000055a0000c541b in aio_dispatch_handler ( >>>>> ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00) >>>>> at ../util/aio-posix.c:329 >>>>> >>>>> Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support") >>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> >>>>> --- >>>>> hw/virtio/vhost-user.c | 10 ++++++++-- >>>>> 1 file changed, 8 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c >>>>> index 2fdd5daf74..a49b2229fb 100644 >>>>> --- a/hw/virtio/vhost-user.c >>>>> +++ b/hw/virtio/vhost-user.c >>>>> @@ -238,6 +238,7 @@ struct vhost_user { >>>>> /* Shared between vhost devs of the same virtio device */ >>>>> VhostUserState *user; >>>>> int slave_fd; >>>>> + bool iotlb_enabled; >>>>> NotifierWithReturn postcopy_notifier; >>>>> struct PostCopyFD postcopy_fd; >>>>> uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS]; >>>>> @@ -1461,7 +1462,11 @@ static void slave_read(void *opaque) >>>>> >>>>> switch (hdr.request) { >>>>> case VHOST_USER_SLAVE_IOTLB_MSG: >>>>> - ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); >>>>> + if (likely(u->iotlb_enabled)) { >>>>> + ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); >>>>> + } else { >>>>> + ret = -EFAULT; >>>>> + } >>>>> break; >>>>> case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG : >>>>> ret = vhost_user_slave_handle_config_change(dev); >>>>> @@ -2044,7 +2049,8 @@ static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev, >>>>> >>>>> static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled) >>>>> { >>>>> - /* No-op as the receive channel is not dedicated to IOTLB messages. */ >>>>> + struct vhost_user *u = dev->opaque; >>>>> + u->iotlb_enabled = enabled; >>>>> } >>>>> >>>>> static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
On Fri, Jan 29, 2021 at 8:29 AM Jason Wang <jasowang@redhat.com> wrote: > > > On 2021/1/29 下午3:22, Eugenio Perez Martin wrote: > > On Fri, Jan 29, 2021 at 4:29 AM Jason Wang <jasowang@redhat.com> wrote: > >> > >> On 2021/1/28 下午5:37, Eugenio Perez Martin wrote: > >>> Hi Jason. > >>> > >>> On Thu, Jan 28, 2021 at 3:32 AM Jason Wang <jasowang@redhat.com> wrote: > >>>> On 2021/1/28 上午4:44, Eugenio Pérez wrote: > >>>>> Not registering this can lead to vhost_backend_handle_iotlb_msg and > >>>>> vhost_device_iotlb_miss if backend issue a miss after qemu vhost device > >>>>> stop. > >>>>> > >>>>> This causes a try to access dev->vdev->dma_as with vdev == NULL. > >>>> Hi Eugenio: > >>>> > >>>> What condition can we get this? Did you mean we receive IOTLB_MISS > >>>> before vhost_dev_start()? > >>>> > >>> In the VM reboot case, yes, between vhost_dev_stop() and > >>> vhost_dev_start(). But I can reproduce the bug in VM shutdown too, > >>> with no vhost_dev_start expected. > >>> > >>> I didn't try to send IOTLB_MISS before starting vhost_dev, but this > >>> patch should solve that problem too. > >>> > >>> I think we can get this with whatever malicious/buggy vhost-user > >>> backend sending unexpected iotlb misses, but I didn't test so deeply. > >> > >> I see. > >> > >> > >>>> If yes, it looks to me a bug somewhere else. > >>> Where should I look for it? > >> > >> So I winder whether or not we can simply ignore IOTLB message if vhost > >> device is not started. > >> > > Do you mean like an early return in vhost_device_iotlb_miss? > > > Yes or probably a little bit earlier in vhost_backend_handle_iotlb_msg() > which somehow a warn there. > > Anyhow it's meaningless to process IOTLB message in this case and we > don't need to introduce a dedicated variable for this. > Start a new series, since the patch title does not reflect the changes anymore: https://patchew.org/QEMU/20210129090728.831208-1-eperezma@redhat.com/ Thanks! > Thanks > > > > That was > > my first first option, but this seems cleaner to me. I'm ok with both > > options. > > > > Or do you mean not to return -EFAULT but 0 if !u->iotlb_enabled ? > > > > Thanks! > > > >> Thanks > >> > >> > >>> Thanks! > >>> > >>>> Thanks > >>>> > >>>> > >>>>> Reproduced rebooting a guest with testpmd in txonly forward mode. > >>>>> #0 0x0000559ffff94394 in vhost_device_iotlb_miss ( > >>>>> dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1) > >>>>> at ../hw/virtio/vhost.c:1013 > >>>>> #1 0x0000559ffff9ac31 in vhost_backend_handle_iotlb_msg ( > >>>>> imsg=0x7ffddcfd32c0, dev=0x55a0012f6680) > >>>>> at ../hw/virtio/vhost-backend.c:411 > >>>>> #2 vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680, > >>>>> imsg=imsg@entry=0x7ffddcfd32c0) > >>>>> at ../hw/virtio/vhost-backend.c:404 > >>>>> #3 0x0000559fffeded7b in slave_read (opaque=0x55a0012f6680) > >>>>> at ../hw/virtio/vhost-user.c:1464 > >>>>> #4 0x000055a0000c541b in aio_dispatch_handler ( > >>>>> ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00) > >>>>> at ../util/aio-posix.c:329 > >>>>> > >>>>> Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support") > >>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > >>>>> --- > >>>>> hw/virtio/vhost-user.c | 10 ++++++++-- > >>>>> 1 file changed, 8 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > >>>>> index 2fdd5daf74..a49b2229fb 100644 > >>>>> --- a/hw/virtio/vhost-user.c > >>>>> +++ b/hw/virtio/vhost-user.c > >>>>> @@ -238,6 +238,7 @@ struct vhost_user { > >>>>> /* Shared between vhost devs of the same virtio device */ > >>>>> VhostUserState *user; > >>>>> int slave_fd; > >>>>> + bool iotlb_enabled; > >>>>> NotifierWithReturn postcopy_notifier; > >>>>> struct PostCopyFD postcopy_fd; > >>>>> uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS]; > >>>>> @@ -1461,7 +1462,11 @@ static void slave_read(void *opaque) > >>>>> > >>>>> switch (hdr.request) { > >>>>> case VHOST_USER_SLAVE_IOTLB_MSG: > >>>>> - ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); > >>>>> + if (likely(u->iotlb_enabled)) { > >>>>> + ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); > >>>>> + } else { > >>>>> + ret = -EFAULT; > >>>>> + } > >>>>> break; > >>>>> case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG : > >>>>> ret = vhost_user_slave_handle_config_change(dev); > >>>>> @@ -2044,7 +2049,8 @@ static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev, > >>>>> > >>>>> static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled) > >>>>> { > >>>>> - /* No-op as the receive channel is not dedicated to IOTLB messages. */ > >>>>> + struct vhost_user *u = dev->opaque; > >>>>> + u->iotlb_enabled = enabled; > >>>>> } > >>>>> > >>>>> static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config, >
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 2fdd5daf74..a49b2229fb 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -238,6 +238,7 @@ struct vhost_user { /* Shared between vhost devs of the same virtio device */ VhostUserState *user; int slave_fd; + bool iotlb_enabled; NotifierWithReturn postcopy_notifier; struct PostCopyFD postcopy_fd; uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS]; @@ -1461,7 +1462,11 @@ static void slave_read(void *opaque) switch (hdr.request) { case VHOST_USER_SLAVE_IOTLB_MSG: - ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); + if (likely(u->iotlb_enabled)) { + ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); + } else { + ret = -EFAULT; + } break; case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG : ret = vhost_user_slave_handle_config_change(dev); @@ -2044,7 +2049,8 @@ static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev, static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled) { - /* No-op as the receive channel is not dedicated to IOTLB messages. */ + struct vhost_user *u = dev->opaque; + u->iotlb_enabled = enabled; } static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
Not registering this can lead to vhost_backend_handle_iotlb_msg and vhost_device_iotlb_miss if backend issue a miss after qemu vhost device stop. This causes a try to access dev->vdev->dma_as with vdev == NULL. Reproduced rebooting a guest with testpmd in txonly forward mode. #0 0x0000559ffff94394 in vhost_device_iotlb_miss ( dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1) at ../hw/virtio/vhost.c:1013 #1 0x0000559ffff9ac31 in vhost_backend_handle_iotlb_msg ( imsg=0x7ffddcfd32c0, dev=0x55a0012f6680) at ../hw/virtio/vhost-backend.c:411 #2 vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680, imsg=imsg@entry=0x7ffddcfd32c0) at ../hw/virtio/vhost-backend.c:404 #3 0x0000559fffeded7b in slave_read (opaque=0x55a0012f6680) at ../hw/virtio/vhost-user.c:1464 #4 0x000055a0000c541b in aio_dispatch_handler ( ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00) at ../util/aio-posix.c:329 Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support") Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- hw/virtio/vhost-user.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)