| Message ID | 20170511123246.31308-4-maxime.coquelin@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 11, 2017 at 02:32:43PM +0200, Maxime Coquelin wrote: > Vhost-kernel backend need to receive IOTLB entries for rings > information early, but vhost-user need the same information > earlier, before VHOST_USER_SET_VRING_ADDR is sent. Weird. What does VHOST_USER_SET_VRING_ADDR have to do with it? According to Starting and stopping rings in vhost user spec, vhost user does not access anything until ring is started and enabled. > This patch also trigger IOTLB miss for all rings informations > for robustness, even if in practice these adresses are on the > same page. > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > hw/virtio/vhost.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 748e331..817f6d0 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -799,7 +799,17 @@ static int vhost_virtqueue_set_addr(struct vhost_dev *dev, > .log_guest_addr = vq->used_phys, > .flags = enable_log ? (1 << VHOST_VRING_F_LOG) : 0, > }; > - int r = dev->vhost_ops->vhost_set_vring_addr(dev, &addr); > + int r; > + > + /* Update rings information for IOTLB to work correctly, > + * vhost-kernel & vhost-user backends require for this.*/ Any requirements must be in the spec. Pls add them there. Pls fix comment style as you move code. > + if (vhost_dev_has_iommu(dev)) { > + vhost_device_iotlb_miss(dev, addr.desc_user_addr, true); > + vhost_device_iotlb_miss(dev, addr.used_user_addr, true); > + vhost_device_iotlb_miss(dev, addr.avail_user_addr, true); > + } > + > + r = dev->vhost_ops->vhost_set_vring_addr(dev, &addr); > if (r < 0) { > VHOST_OPS_DEBUG("vhost_set_vring_addr failed"); > return -errno; > @@ -1551,13 +1561,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) > > if (vhost_dev_has_iommu(hdev)) { > hdev->vhost_ops->vhost_set_iotlb_callback(hdev, true); > - > - /* Update used ring information for IOTLB to work correctly, > - * vhost-kernel code requires for this.*/ > - for (i = 0; i < hdev->nvqs; ++i) { > - struct vhost_virtqueue *vq = hdev->vqs + i; > - vhost_device_iotlb_miss(hdev, vq->used_phys, true); > - } > } > return 0; > fail_log: > -- > 2.9.3
On 05/11/2017 07:33 PM, Michael S. Tsirkin wrote: > On Thu, May 11, 2017 at 02:32:43PM +0200, Maxime Coquelin wrote: >> Vhost-kernel backend need to receive IOTLB entries for rings >> information early, but vhost-user need the same information >> earlier, before VHOST_USER_SET_VRING_ADDR is sent. > > Weird. What does VHOST_USER_SET_VRING_ADDR have to do with it? > > According to > Starting and stopping rings > in vhost user spec, vhost user does not access > anything until ring is started and enabled. > > >> This patch also trigger IOTLB miss for all rings informations >> for robustness, even if in practice these adresses are on the >> same page. Actually, the DPDK vhost-user backend is compliant with the spec, but when handling VHOST_USER_SET_VRING_ADDR request, it translates the guest addresses into backend VAs, and check they are valid. I will make the commit message clearer about this in next revision. The check could be done later, for example when the ring are started, but it wouldn't change the need to trigger a miss at some point. Or it could be done in the processing threads, the first time the addresses are used, but it would add a check for every burst of packets processed. >> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >> --- >> hw/virtio/vhost.c | 19 +++++++++++-------- >> 1 file changed, 11 insertions(+), 8 deletions(-) >> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >> index 748e331..817f6d0 100644 >> --- a/hw/virtio/vhost.c >> +++ b/hw/virtio/vhost.c >> @@ -799,7 +799,17 @@ static int vhost_virtqueue_set_addr(struct vhost_dev *dev, >> .log_guest_addr = vq->used_phys, >> .flags = enable_log ? (1 << VHOST_VRING_F_LOG) : 0, >> }; >> - int r = dev->vhost_ops->vhost_set_vring_addr(dev, &addr); >> + int r; >> + >> + /* Update rings information for IOTLB to work correctly, >> + * vhost-kernel & vhost-user backends require for this.*/ > > Any requirements must be in the spec. Pls add them there. > Pls fix comment style as you move code. Sure. Thanks, Maxime
On Fri, May 12, 2017 at 01:21:18PM +0200, Maxime Coquelin wrote: > > > On 05/11/2017 07:33 PM, Michael S. Tsirkin wrote: > > On Thu, May 11, 2017 at 02:32:43PM +0200, Maxime Coquelin wrote: > > > Vhost-kernel backend need to receive IOTLB entries for rings > > > information early, but vhost-user need the same information > > > earlier, before VHOST_USER_SET_VRING_ADDR is sent. > > > > Weird. What does VHOST_USER_SET_VRING_ADDR have to do with it? > > > > According to > > Starting and stopping rings > > in vhost user spec, vhost user does not access > > anything until ring is started and enabled. > > > > > > > This patch also trigger IOTLB miss for all rings informations > > > for robustness, even if in practice these adresses are on the > > > same page. > > Actually, the DPDK vhost-user backend is compliant with the spec, > but when handling VHOST_USER_SET_VRING_ADDR request, it translates the > guest addresses into backend VAs, and check they are valid. I will make the > commit message clearer about this in next revision. > > The check could be done later, for example when the ring are started, > but it wouldn't change the need to trigger a miss at some point. I think it should be done later, yes. As long as ring is not started addresses should not be interpreted. > Or it could be done in the processing threads, the first time the > addresses are used, but it would add a check for every burst of packets > processed. We don't want that, I agree. > > > > > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > > > --- > > > hw/virtio/vhost.c | 19 +++++++++++-------- > > > 1 file changed, 11 insertions(+), 8 deletions(-) > > > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > > index 748e331..817f6d0 100644 > > > --- a/hw/virtio/vhost.c > > > +++ b/hw/virtio/vhost.c > > > @@ -799,7 +799,17 @@ static int vhost_virtqueue_set_addr(struct vhost_dev *dev, > > > .log_guest_addr = vq->used_phys, > > > .flags = enable_log ? (1 << VHOST_VRING_F_LOG) : 0, > > > }; > > > - int r = dev->vhost_ops->vhost_set_vring_addr(dev, &addr); > > > + int r; > > > + > > > + /* Update rings information for IOTLB to work correctly, > > > + * vhost-kernel & vhost-user backends require for this.*/ > > > > Any requirements must be in the spec. Pls add them there. > > Pls fix comment style as you move code. > Sure. > > Thanks, > Maxime
On 05/17/2017 06:41 PM, Michael S. Tsirkin wrote: > On Fri, May 12, 2017 at 01:21:18PM +0200, Maxime Coquelin wrote: >> >> On 05/11/2017 07:33 PM, Michael S. Tsirkin wrote: >>> On Thu, May 11, 2017 at 02:32:43PM +0200, Maxime Coquelin wrote: >>>> Vhost-kernel backend need to receive IOTLB entries for rings >>>> information early, but vhost-user need the same information >>>> earlier, before VHOST_USER_SET_VRING_ADDR is sent. >>> Weird. What does VHOST_USER_SET_VRING_ADDR have to do with it? >>> >>> According to >>> Starting and stopping rings >>> in vhost user spec, vhost user does not access >>> anything until ring is started and enabled. >>> >>> >>>> This patch also trigger IOTLB miss for all rings informations >>>> for robustness, even if in practice these adresses are on the >>>> same page. >> Actually, the DPDK vhost-user backend is compliant with the spec, >> but when handling VHOST_USER_SET_VRING_ADDR request, it translates the >> guest addresses into backend VAs, and check they are valid. I will make the >> commit message clearer about this in next revision. >> >> The check could be done later, for example when the ring are started, >> but it wouldn't change the need to trigger a miss at some point. > I think it should be done later, yes. As long as ring is not > started addresses should not be interpreted. > Ok, then I'll move these addresses translations in the VHOST_USER_SET_VRING_KICK handler. Thanks, Maxime
Hi Michael, On 05/18/2017 09:35 AM, Maxime Coquelin wrote: > > > On 05/17/2017 06:41 PM, Michael S. Tsirkin wrote: >> On Fri, May 12, 2017 at 01:21:18PM +0200, Maxime Coquelin wrote: >>> >>> On 05/11/2017 07:33 PM, Michael S. Tsirkin wrote: >>>> On Thu, May 11, 2017 at 02:32:43PM +0200, Maxime Coquelin wrote: >>>>> Vhost-kernel backend need to receive IOTLB entries for rings >>>>> information early, but vhost-user need the same information >>>>> earlier, before VHOST_USER_SET_VRING_ADDR is sent. >>>> Weird. What does VHOST_USER_SET_VRING_ADDR have to do with it? >>>> >>>> According to >>>> Starting and stopping rings >>>> in vhost user spec, vhost user does not access >>>> anything until ring is started and enabled. >>>> >>>> >>>>> This patch also trigger IOTLB miss for all rings informations >>>>> for robustness, even if in practice these adresses are on the >>>>> same page. >>> Actually, the DPDK vhost-user backend is compliant with the spec, >>> but when handling VHOST_USER_SET_VRING_ADDR request, it translates the >>> guest addresses into backend VAs, and check they are valid. I will >>> make the >>> commit message clearer about this in next revision. >>> >>> The check could be done later, for example when the ring are started, >>> but it wouldn't change the need to trigger a miss at some point. >> I think it should be done later, yes. As long as ring is not >> started addresses should not be interpreted. >> > > Ok, then I'll move these addresses translations in the > VHOST_USER_SET_VRING_KICK handler. s/VHOST_USER_SET_VRING_KICK/VHOST_USER_SET_VRING_ENABLE/ I just looked at implementing this change, but I'm not convinced this is the right thing to do. On backend side, it means saving temporarily the vhost_vring_addr struct into the vq struct, and moving all what is done currently in SET_VRING_ADDR handler to SET_VRING_ENABLE one. My understanding of the "Starting and stopping rings" chapter of the spec is that the ring must not be processed as long as not started and enabled, not that the addresses passed should not be checked/translated as it is done today both in DPDK and libvhost-user. If the addresses are invalid, isn't it better to know as soon as possible? Cheers, Maxime
On Thu, May 18, 2017 at 04:45:23PM +0200, Maxime Coquelin wrote: > Hi Michael, > > On 05/18/2017 09:35 AM, Maxime Coquelin wrote: > > > > > > On 05/17/2017 06:41 PM, Michael S. Tsirkin wrote: > > > On Fri, May 12, 2017 at 01:21:18PM +0200, Maxime Coquelin wrote: > > > > > > > > On 05/11/2017 07:33 PM, Michael S. Tsirkin wrote: > > > > > On Thu, May 11, 2017 at 02:32:43PM +0200, Maxime Coquelin wrote: > > > > > > Vhost-kernel backend need to receive IOTLB entries for rings > > > > > > information early, but vhost-user need the same information > > > > > > earlier, before VHOST_USER_SET_VRING_ADDR is sent. > > > > > Weird. What does VHOST_USER_SET_VRING_ADDR have to do with it? > > > > > > > > > > According to > > > > > Starting and stopping rings > > > > > in vhost user spec, vhost user does not access > > > > > anything until ring is started and enabled. > > > > > > > > > > > > > > > > This patch also trigger IOTLB miss for all rings informations > > > > > > for robustness, even if in practice these adresses are on the > > > > > > same page. > > > > Actually, the DPDK vhost-user backend is compliant with the spec, > > > > but when handling VHOST_USER_SET_VRING_ADDR request, it translates the > > > > guest addresses into backend VAs, and check they are valid. I > > > > will make the > > > > commit message clearer about this in next revision. > > > > > > > > The check could be done later, for example when the ring are started, > > > > but it wouldn't change the need to trigger a miss at some point. > > > I think it should be done later, yes. As long as ring is not > > > started addresses should not be interpreted. > > > > > > > Ok, then I'll move these addresses translations in the > > VHOST_USER_SET_VRING_KICK handler. > s/VHOST_USER_SET_VRING_KICK/VHOST_USER_SET_VRING_ENABLE/ Note that when protocol features are off ring is started in enabled state, but iommu requires protocol features. > I just looked at implementing this change, but I'm not convinced this is > the right thing to do. > > On backend side, it means saving temporarily the vhost_vring_addr struct > into the vq struct, and moving all what is done currently in > SET_VRING_ADDR handler to SET_VRING_ENABLE one. Yes, and this is consistent with what the kernel does. > My understanding of the "Starting and stopping rings" chapter of the > spec is that the ring must not be processed as long as not started and > enabled, not that the addresses passed should not be checked/translated > as it is done today both in DPDK and libvhost-user. > > If the addresses are invalid, isn't it better to know as soon as > possible? > > Cheers, > Maxime There could be valid reasons to set an invalid address temporarily. For example to make sure connection is reset.
On 05/18/2017 05:24 PM, Michael S. Tsirkin wrote: > On Thu, May 18, 2017 at 04:45:23PM +0200, Maxime Coquelin wrote: >> Hi Michael, >> >> On 05/18/2017 09:35 AM, Maxime Coquelin wrote: >>> >>> >>> On 05/17/2017 06:41 PM, Michael S. Tsirkin wrote: >>>> On Fri, May 12, 2017 at 01:21:18PM +0200, Maxime Coquelin wrote: >>>>> >>>>> On 05/11/2017 07:33 PM, Michael S. Tsirkin wrote: >>>>>> On Thu, May 11, 2017 at 02:32:43PM +0200, Maxime Coquelin wrote: >>>>>>> Vhost-kernel backend need to receive IOTLB entries for rings >>>>>>> information early, but vhost-user need the same information >>>>>>> earlier, before VHOST_USER_SET_VRING_ADDR is sent. >>>>>> Weird. What does VHOST_USER_SET_VRING_ADDR have to do with it? >>>>>> >>>>>> According to >>>>>> Starting and stopping rings >>>>>> in vhost user spec, vhost user does not access >>>>>> anything until ring is started and enabled. >>>>>> >>>>>> >>>>>>> This patch also trigger IOTLB miss for all rings informations >>>>>>> for robustness, even if in practice these adresses are on the >>>>>>> same page. >>>>> Actually, the DPDK vhost-user backend is compliant with the spec, >>>>> but when handling VHOST_USER_SET_VRING_ADDR request, it translates the >>>>> guest addresses into backend VAs, and check they are valid. I >>>>> will make the >>>>> commit message clearer about this in next revision. >>>>> >>>>> The check could be done later, for example when the ring are started, >>>>> but it wouldn't change the need to trigger a miss at some point. >>>> I think it should be done later, yes. As long as ring is not >>>> started addresses should not be interpreted. >>>> >>> >>> Ok, then I'll move these addresses translations in the >>> VHOST_USER_SET_VRING_KICK handler. >> s/VHOST_USER_SET_VRING_KICK/VHOST_USER_SET_VRING_ENABLE/ > > Note that when protocol features are off ring is started in > enabled state, but iommu requires protocol features. OK, I will take care of this. Note that currently in DPDK, the ring is created in enabled state, so it is enabled as soon as started even with protocol features. I have done the patch to fix this, will be posted with the patch that do the ring addresses translations only when starting/enabling the ring. Also, note that disabling VHOST_USER_F_PROTOCOL_FEATURES with latest DPDK and QEMU seems broken. I'll add this to my todo list to understand where the problem is, but this is lower priority. >> I just looked at implementing this change, but I'm not convinced this is >> the right thing to do. >> >> On backend side, it means saving temporarily the vhost_vring_addr struct >> into the vq struct, and moving all what is done currently in >> SET_VRING_ADDR handler to SET_VRING_ENABLE one. > > Yes, and this is consistent with what the kernel does. > >> My understanding of the "Starting and stopping rings" chapter of the >> spec is that the ring must not be processed as long as not started and >> enabled, not that the addresses passed should not be checked/translated >> as it is done today both in DPDK and libvhost-user. >> >> If the addresses are invalid, isn't it better to know as soon as >> possible? >> >> Cheers, >> Maxime > > There could be valid reasons to set an invalid address temporarily. > For example to make sure connection is reset. Ok. Thanks, Maxime
On Fri, May 19, 2017 at 11:48:52AM +0200, Maxime Coquelin wrote: > > > On 05/18/2017 05:24 PM, Michael S. Tsirkin wrote: > > On Thu, May 18, 2017 at 04:45:23PM +0200, Maxime Coquelin wrote: > > > Hi Michael, > > > > > > On 05/18/2017 09:35 AM, Maxime Coquelin wrote: > > > > > > > > > > > > On 05/17/2017 06:41 PM, Michael S. Tsirkin wrote: > > > > > On Fri, May 12, 2017 at 01:21:18PM +0200, Maxime Coquelin wrote: > > > > > > > > > > > > On 05/11/2017 07:33 PM, Michael S. Tsirkin wrote: > > > > > > > On Thu, May 11, 2017 at 02:32:43PM +0200, Maxime Coquelin wrote: > > > > > > > > Vhost-kernel backend need to receive IOTLB entries for rings > > > > > > > > information early, but vhost-user need the same information > > > > > > > > earlier, before VHOST_USER_SET_VRING_ADDR is sent. > > > > > > > Weird. What does VHOST_USER_SET_VRING_ADDR have to do with it? > > > > > > > > > > > > > > According to > > > > > > > Starting and stopping rings > > > > > > > in vhost user spec, vhost user does not access > > > > > > > anything until ring is started and enabled. > > > > > > > > > > > > > > > > > > > > > > This patch also trigger IOTLB miss for all rings informations > > > > > > > > for robustness, even if in practice these adresses are on the > > > > > > > > same page. > > > > > > Actually, the DPDK vhost-user backend is compliant with the spec, > > > > > > but when handling VHOST_USER_SET_VRING_ADDR request, it translates the > > > > > > guest addresses into backend VAs, and check they are valid. I > > > > > > will make the > > > > > > commit message clearer about this in next revision. > > > > > > > > > > > > The check could be done later, for example when the ring are started, > > > > > > but it wouldn't change the need to trigger a miss at some point. > > > > > I think it should be done later, yes. As long as ring is not > > > > > started addresses should not be interpreted. > > > > > > > > > > > > > Ok, then I'll move these addresses translations in the > > > > VHOST_USER_SET_VRING_KICK handler. > > > s/VHOST_USER_SET_VRING_KICK/VHOST_USER_SET_VRING_ENABLE/ > > > > Note that when protocol features are off ring is started in > > enabled state, but iommu requires protocol features. > > OK, I will take care of this. > > Note that currently in DPDK, the ring is created in enabled state, > so it is enabled as soon as started even with protocol features. > I have done the patch to fix this, will be posted with the patch that > do the ring addresses translations only when starting/enabling the ring. > > Also, note that disabling VHOST_USER_F_PROTOCOL_FEATURES with latest > DPDK and QEMU seems broken. I'll add this to my todo list to understand > where the problem is, but this is lower priority. And hopefully add a unit test without this so we don't break it in the future. > > > I just looked at implementing this change, but I'm not convinced this is > > > the right thing to do. > > > > > > On backend side, it means saving temporarily the vhost_vring_addr struct > > > into the vq struct, and moving all what is done currently in > > > SET_VRING_ADDR handler to SET_VRING_ENABLE one. > > > > Yes, and this is consistent with what the kernel does. > > > > > My understanding of the "Starting and stopping rings" chapter of the > > > spec is that the ring must not be processed as long as not started and > > > enabled, not that the addresses passed should not be checked/translated > > > as it is done today both in DPDK and libvhost-user. > > > > > > If the addresses are invalid, isn't it better to know as soon as > > > possible? > > > > > > Cheers, > > > Maxime > > > > There could be valid reasons to set an invalid address temporarily. > > For example to make sure connection is reset. > > Ok. > > Thanks, > Maxime
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 748e331..817f6d0 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -799,7 +799,17 @@ static int vhost_virtqueue_set_addr(struct vhost_dev *dev, .log_guest_addr = vq->used_phys, .flags = enable_log ? (1 << VHOST_VRING_F_LOG) : 0, }; - int r = dev->vhost_ops->vhost_set_vring_addr(dev, &addr); + int r; + + /* Update rings information for IOTLB to work correctly, + * vhost-kernel & vhost-user backends require for this.*/ + if (vhost_dev_has_iommu(dev)) { + vhost_device_iotlb_miss(dev, addr.desc_user_addr, true); + vhost_device_iotlb_miss(dev, addr.used_user_addr, true); + vhost_device_iotlb_miss(dev, addr.avail_user_addr, true); + } + + r = dev->vhost_ops->vhost_set_vring_addr(dev, &addr); if (r < 0) { VHOST_OPS_DEBUG("vhost_set_vring_addr failed"); return -errno; @@ -1551,13 +1561,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) if (vhost_dev_has_iommu(hdev)) { hdev->vhost_ops->vhost_set_iotlb_callback(hdev, true); - - /* Update used ring information for IOTLB to work correctly, - * vhost-kernel code requires for this.*/ - for (i = 0; i < hdev->nvqs; ++i) { - struct vhost_virtqueue *vq = hdev->vqs + i; - vhost_device_iotlb_miss(hdev, vq->used_phys, true); - } } return 0; fail_log:
Vhost-kernel backend need to receive IOTLB entries for rings information early, but vhost-user need the same information earlier, before VHOST_USER_SET_VRING_ADDR is sent. This patch also trigger IOTLB miss for all rings informations for robustness, even if in practice these adresses are on the same page. Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- hw/virtio/vhost.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)