On Fri, Jan 29, 2021 at 8:29 AM Jason Wang <jasow...@redhat.com> wrote: > > > On 2021/1/29 下午3:22, Eugenio Perez Martin wrote: > > On Fri, Jan 29, 2021 at 4:29 AM Jason Wang <jasow...@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 <jasow...@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-epere...@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 <epere...@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, >
