This patch introduces the support for VIRTIO_F_IO_BARRIER. When this feature is negotiated, driver will use the barriers suitable for hardware devices. Signed-off-by: Tiwei Bie <tiwei.bie at intel.com> --- drivers/virtio/virtio_ring.c | 5 +++++ include/uapi/linux/virtio_config.h | 8 +++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 21d464a29cf8..edb565643bf4 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -996,6 +996,9 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index, !context; vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX); + if (virtio_has_feature(vdev, VIRTIO_F_IO_BARRIER)) + vq->weak_barriers = false; + /* No callback? Tell other side not to bother us. */ if (!callback) { vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT; @@ -1164,6 +1167,8 @@ void vring_transport_features(struct virtio_device *vdev) break; case VIRTIO_F_IOMMU_PLATFORM: break; + case VIRTIO_F_IO_BARRIER: + break; default: /* We don't understand this bit. */ __virtio_clear_bit(vdev, i); diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h index 308e2096291f..6ca8d24bf468 100644 --- a/include/uapi/linux/virtio_config.h +++ b/include/uapi/linux/virtio_config.h @@ -49,7 +49,7 @@ * transport being used (eg. virtio_ring), the rest are per-device feature * bits. */ #define VIRTIO_TRANSPORT_F_START 28 -#define VIRTIO_TRANSPORT_F_END 34 +#define VIRTIO_TRANSPORT_F_END 38 #ifndef VIRTIO_CONFIG_NO_LEGACY /* Do we get callbacks when the ring is completely used, even if we've @@ -71,4 +71,10 @@ * this is for compatibility with legacy systems. */ #define VIRTIO_F_IOMMU_PLATFORM 33 + +/* + * If clear - driver may use barriers suitable for CPU cores. + * If set - driver must use barriers suitable for hardware devices. + */ +#define VIRTIO_F_IO_BARRIER 37 #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */ -- 2.11.0
On 2018?05?03? 10:59, Tiwei Bie wrote:> This patch introduces the support for VIRTIO_F_IO_BARRIER. > When this feature is negotiated, driver will use the barriers > suitable for hardware devices. > > Signed-off-by: Tiwei Bie <tiwei.bie at intel.com> > --- > drivers/virtio/virtio_ring.c | 5 +++++ > include/uapi/linux/virtio_config.h | 8 +++++++- > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 21d464a29cf8..edb565643bf4 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -996,6 +996,9 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index, > !context; > vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX); > > + if (virtio_has_feature(vdev, VIRTIO_F_IO_BARRIER)) > + vq->weak_barriers = false; > + > /* No callback? Tell other side not to bother us. */ > if (!callback) { > vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT; > @@ -1164,6 +1167,8 @@ void vring_transport_features(struct virtio_device *vdev) > break; > case VIRTIO_F_IOMMU_PLATFORM: > break; > + case VIRTIO_F_IO_BARRIER: > + break; > default: > /* We don't understand this bit. */ > __virtio_clear_bit(vdev, i); > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h > index 308e2096291f..6ca8d24bf468 100644 > --- a/include/uapi/linux/virtio_config.h > +++ b/include/uapi/linux/virtio_config.h > @@ -49,7 +49,7 @@ > * transport being used (eg. virtio_ring), the rest are per-device feature > * bits. */ > #define VIRTIO_TRANSPORT_F_START 28 > -#define VIRTIO_TRANSPORT_F_END 34 > +#define VIRTIO_TRANSPORT_F_END 38 > > #ifndef VIRTIO_CONFIG_NO_LEGACY > /* Do we get callbacks when the ring is completely used, even if we've > @@ -71,4 +71,10 @@ > * this is for compatibility with legacy systems. > */ > #define VIRTIO_F_IOMMU_PLATFORM 33 > + > +/* > + * If clear - driver may use barriers suitable for CPU cores. > + * If set - driver must use barriers suitable for hardware devices. > + */ > +#define VIRTIO_F_IO_BARRIER 37 > #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */Hi: I believe this depends on Michael's patch of "[PATCH] virtio_ring: switch to dma_XX barriers for rpmsg" ? Thanks
On Thu, May 03, 2018 at 03:30:03PM +0800, Jason Wang wrote:> On 2018?05?03? 10:59, Tiwei Bie wrote: > > This patch introduces the support for VIRTIO_F_IO_BARRIER. > > When this feature is negotiated, driver will use the barriers > > suitable for hardware devices. > > > > Signed-off-by: Tiwei Bie <tiwei.bie at intel.com> > > --- > > drivers/virtio/virtio_ring.c | 5 +++++ > > include/uapi/linux/virtio_config.h | 8 +++++++- > > 2 files changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 21d464a29cf8..edb565643bf4 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -996,6 +996,9 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index, > > !context; > > vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX); > > + if (virtio_has_feature(vdev, VIRTIO_F_IO_BARRIER)) > > + vq->weak_barriers = false; > > + > > /* No callback? Tell other side not to bother us. */ > > if (!callback) { > > vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT; > > @@ -1164,6 +1167,8 @@ void vring_transport_features(struct virtio_device *vdev) > > break; > > case VIRTIO_F_IOMMU_PLATFORM: > > break; > > + case VIRTIO_F_IO_BARRIER: > > + break; > > default: > > /* We don't understand this bit. */ > > __virtio_clear_bit(vdev, i); > > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h > > index 308e2096291f..6ca8d24bf468 100644 > > --- a/include/uapi/linux/virtio_config.h > > +++ b/include/uapi/linux/virtio_config.h > > @@ -49,7 +49,7 @@ > > * transport being used (eg. virtio_ring), the rest are per-device feature > > * bits. */ > > #define VIRTIO_TRANSPORT_F_START 28 > > -#define VIRTIO_TRANSPORT_F_END 34 > > +#define VIRTIO_TRANSPORT_F_END 38 > > #ifndef VIRTIO_CONFIG_NO_LEGACY > > /* Do we get callbacks when the ring is completely used, even if we've > > @@ -71,4 +71,10 @@ > > * this is for compatibility with legacy systems. > > */ > > #define VIRTIO_F_IOMMU_PLATFORM 33 > > + > > +/* > > + * If clear - driver may use barriers suitable for CPU cores. > > + * If set - driver must use barriers suitable for hardware devices. > > + */ > > +#define VIRTIO_F_IO_BARRIER 37 > > #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */ > > Hi: > > I believe this depends on Michael's patch of > > "[PATCH] virtio_ring: switch to dma_XX barriers for rpmsg" > > ? > > ThanksWe already have below commit and some other related commits in the tree: 7b21e34fd1c2 ("virtio: harsher barriers for rpmsg.") They should have already guaranteed that virtio_Xmb() will be OK for hardware devices when vq->weak_barriers is false. If my understanding is correct, the barriers used in this case are overkill. So Michael's patch is to make the barriers weaker (or better). Best regards, Tiwei Bie
On Thu, May 03, 2018 at 10:59:55AM +0800, Tiwei Bie wrote:> This patch introduces the support for VIRTIO_F_IO_BARRIER. > When this feature is negotiated, driver will use the barriers > suitable for hardware devices. > > Signed-off-by: Tiwei Bie <tiwei.bie at intel.com>I should have thought of this earlier, but why is a new feature bit necessary? If a hardware virtio device is in use, then the device should already negotiate VIRTIO_F_IOMMU_PLATFORM (i.e. use DMA APIs and IOMMU callbacks). Does disabling weak_barriers when VIRTIO_F_IOMMU_PLATFORM is set solve the problem? -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 455 bytes Desc: not available URL: <lists.linuxfoundation.org/pipermail/virtualization/attachments/20180503/efa35a3b/attachment.sig>
On Thu, May 03, 2018 at 10:06:52AM +0100, Stefan Hajnoczi wrote:> On Thu, May 03, 2018 at 10:59:55AM +0800, Tiwei Bie wrote: > > This patch introduces the support for VIRTIO_F_IO_BARRIER. > > When this feature is negotiated, driver will use the barriers > > suitable for hardware devices. > > > > Signed-off-by: Tiwei Bie <tiwei.bie at intel.com> > > I should have thought of this earlier, but why is a new feature bit > necessary? If a hardware virtio device is in use, then the device > should already negotiate VIRTIO_F_IOMMU_PLATFORM (i.e. use DMA APIs and > IOMMU callbacks). > > Does disabling weak_barriers when VIRTIO_F_IOMMU_PLATFORM is set solve > the problem?The VIRTIO_F_IOMMU_PLATFORM feature can be set when the device is implemented in software. And I think we don't want the performance drop in this case. Best regards, Tiwei Bie
On Thu, May 03, 2018 at 10:59:55AM +0800, Tiwei Bie wrote:> This patch introduces the support for VIRTIO_F_IO_BARRIER. > When this feature is negotiated, driver will use the barriers > suitable for hardware devices. > > Signed-off-by: Tiwei Bie <tiwei.bie at intel.com>Thanks!> --- > drivers/virtio/virtio_ring.c | 5 +++++ > include/uapi/linux/virtio_config.h | 8 +++++++- > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 21d464a29cf8..edb565643bf4 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -996,6 +996,9 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index, > !context; > vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX); > > + if (virtio_has_feature(vdev, VIRTIO_F_IO_BARRIER)) > + vq->weak_barriers = false; > + > /* No callback? Tell other side not to bother us. */ > if (!callback) { > vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;One issue worth looking at is that at least on Intel strong barriers are actually typically overkill. We should probably switch weak_barriers =false case over to dma barriers.> @@ -1164,6 +1167,8 @@ void vring_transport_features(struct virtio_device *vdev) > break; > case VIRTIO_F_IOMMU_PLATFORM: > break; > + case VIRTIO_F_IO_BARRIER: > + break; > default: > /* We don't understand this bit. */ > __virtio_clear_bit(vdev, i); > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h > index 308e2096291f..6ca8d24bf468 100644 > --- a/include/uapi/linux/virtio_config.h > +++ b/include/uapi/linux/virtio_config.hAny virtio UAPI changes must be CC'd to one of the virtio TC mailing lists (subscriber-only, sorry about that).> @@ -49,7 +49,7 @@ > * transport being used (eg. virtio_ring), the rest are per-device feature > * bits. */ > #define VIRTIO_TRANSPORT_F_START 28 > -#define VIRTIO_TRANSPORT_F_END 34 > +#define VIRTIO_TRANSPORT_F_END 38 > > #ifndef VIRTIO_CONFIG_NO_LEGACY > /* Do we get callbacks when the ring is completely used, even if we've > @@ -71,4 +71,10 @@ > * this is for compatibility with legacy systems. > */ > #define VIRTIO_F_IOMMU_PLATFORM 33 > + > +/* > + * If clear - driver may use barriers suitable for CPU cores. > + * If set - driver must use barriers suitable for hardware devices. > + */ > +#define VIRTIO_F_IO_BARRIER 37 > #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */Why 37? I'd use 34 I think.> -- > 2.11.0
On Thu, May 03, 2018 at 08:57:20PM +0300, Michael S. Tsirkin wrote:> On Thu, May 03, 2018 at 10:59:55AM +0800, Tiwei Bie wrote: > > This patch introduces the support for VIRTIO_F_IO_BARRIER. > > When this feature is negotiated, driver will use the barriers > > suitable for hardware devices. > > > > Signed-off-by: Tiwei Bie <tiwei.bie at intel.com> > > Thanks! > > > --- > > drivers/virtio/virtio_ring.c | 5 +++++ > > include/uapi/linux/virtio_config.h | 8 +++++++- > > 2 files changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 21d464a29cf8..edb565643bf4 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -996,6 +996,9 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index, > > !context; > > vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX); > > > > + if (virtio_has_feature(vdev, VIRTIO_F_IO_BARRIER)) > > + vq->weak_barriers = false; > > + > > /* No callback? Tell other side not to bother us. */ > > if (!callback) { > > vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT; > > One issue worth looking at is that at least on Intel strong barriers are > actually typically overkill. We should probably switch weak_barriers => false case over to dma barriers.Jason suggested me to add a reference or some notes in this patch about your patch: "[PATCH] virtio_ring: switch to dma_XX barriers for rpmsg"> > > @@ -1164,6 +1167,8 @@ void vring_transport_features(struct virtio_device *vdev) > > break; > > case VIRTIO_F_IOMMU_PLATFORM: > > break; > > + case VIRTIO_F_IO_BARRIER: > > + break; > > default: > > /* We don't understand this bit. */ > > __virtio_clear_bit(vdev, i); > > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h > > index 308e2096291f..6ca8d24bf468 100644 > > --- a/include/uapi/linux/virtio_config.h > > +++ b/include/uapi/linux/virtio_config.h > > Any virtio UAPI changes must be CC'd to one of the virtio TC mailing lists > (subscriber-only, sorry about that).Got it! I'll send a new version and Cc virtio-dev.> > > @@ -49,7 +49,7 @@ > > * transport being used (eg. virtio_ring), the rest are per-device feature > > * bits. */ > > #define VIRTIO_TRANSPORT_F_START 28 > > -#define VIRTIO_TRANSPORT_F_END 34 > > +#define VIRTIO_TRANSPORT_F_END 38 > > > > #ifndef VIRTIO_CONFIG_NO_LEGACY > > /* Do we get callbacks when the ring is completely used, even if we've > > @@ -71,4 +71,10 @@ > > * this is for compatibility with legacy systems. > > */ > > #define VIRTIO_F_IOMMU_PLATFORM 33 > > + > > +/* > > + * If clear - driver may use barriers suitable for CPU cores. > > + * If set - driver must use barriers suitable for hardware devices. > > + */ > > +#define VIRTIO_F_IO_BARRIER 37 > > #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */ > > Why 37? I'd use 34 I think.In the latest virtio spec draft, 34 and 35 have been taken by VIRTIO_F_RING_PACKED and VIRTIO_F_IN_ORDER. And 36 had been taken by VIRTIO_F_NOTIFICATION_DATA previously when I sent below proposal: lists.oasis-open.org/archives/virtio-dev/201804/msg00310.html But I just noticed that NOTIFICATION_DATA has been reverted from the repo, which means 36 is the next available bit. So I'll use it. Thanks for the reminder! Best regards, Tiwei Bie> > > -- > > 2.11.0