* [PATCH] virtio: support VIRTIO_F_ORDER_PLATFORM
@ 2019-01-22 17:03 Tiwei Bie
2019-01-23 3:08 ` [virtio-dev] " Jason Wang
2019-01-23 4:04 ` Michael S. Tsirkin
0 siblings, 2 replies; 7+ messages in thread
From: Tiwei Bie @ 2019-01-22 17:03 UTC (permalink / raw)
To: mst, jasowang, virtualization, linux-kernel, virtio-dev
This patch introduces the support for VIRTIO_F_ORDER_PLATFORM.
When this feature is negotiated, driver will use the barriers
suitable for hardware devices.
Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
drivers/virtio/virtio_ring.c | 8 ++++++++
include/uapi/linux/virtio_config.h | 6 ++++++
2 files changed, 14 insertions(+)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index cd7e755484e3..27d3f057493e 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1609,6 +1609,9 @@ static struct virtqueue *vring_create_virtqueue_packed(
!context;
vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
+ if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
+ vq->weak_barriers = false;
+
vq->packed.ring_dma_addr = ring_dma_addr;
vq->packed.driver_event_dma_addr = driver_event_dma_addr;
vq->packed.device_event_dma_addr = device_event_dma_addr;
@@ -2079,6 +2082,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_ORDER_PLATFORM))
+ vq->weak_barriers = false;
+
vq->split.queue_dma_addr = 0;
vq->split.queue_size_in_bytes = 0;
@@ -2213,6 +2219,8 @@ void vring_transport_features(struct virtio_device *vdev)
break;
case VIRTIO_F_RING_PACKED:
break;
+ case VIRTIO_F_ORDER_PLATFORM:
+ 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 1196e1c1d4f6..ff8e7dc9d4dd 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -78,6 +78,12 @@
/* This feature indicates support for the packed virtqueue layout. */
#define VIRTIO_F_RING_PACKED 34
+/*
+ * This feature indicates that memory accesses by the driver and the
+ * device are ordered in a way described by the platform.
+ */
+#define VIRTIO_F_ORDER_PLATFORM 36
+
/*
* Does the device support Single Root I/O Virtualization?
*/
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [virtio-dev] [PATCH] virtio: support VIRTIO_F_ORDER_PLATFORM
2019-01-22 17:03 [PATCH] virtio: support VIRTIO_F_ORDER_PLATFORM Tiwei Bie
@ 2019-01-23 3:08 ` Jason Wang
2019-01-23 3:49 ` Michael S. Tsirkin
2019-01-23 4:04 ` Michael S. Tsirkin
1 sibling, 1 reply; 7+ messages in thread
From: Jason Wang @ 2019-01-23 3:08 UTC (permalink / raw)
To: Tiwei Bie, mst, virtualization, linux-kernel, virtio-dev
On 2019/1/23 上午1:03, Tiwei Bie wrote:
> This patch introduces the support for VIRTIO_F_ORDER_PLATFORM.
> When this feature is negotiated, driver will use the barriers
> suitable for hardware devices.
>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> drivers/virtio/virtio_ring.c | 8 ++++++++
> include/uapi/linux/virtio_config.h | 6 ++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index cd7e755484e3..27d3f057493e 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1609,6 +1609,9 @@ static struct virtqueue *vring_create_virtqueue_packed(
> !context;
> vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
>
> + if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
> + vq->weak_barriers = false;
> +
> vq->packed.ring_dma_addr = ring_dma_addr;
> vq->packed.driver_event_dma_addr = driver_event_dma_addr;
> vq->packed.device_event_dma_addr = device_event_dma_addr;
> @@ -2079,6 +2082,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_ORDER_PLATFORM))
> + vq->weak_barriers = false;
> +
> vq->split.queue_dma_addr = 0;
> vq->split.queue_size_in_bytes = 0;
>
> @@ -2213,6 +2219,8 @@ void vring_transport_features(struct virtio_device *vdev)
> break;
> case VIRTIO_F_RING_PACKED:
> break;
> + case VIRTIO_F_ORDER_PLATFORM:
> + 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 1196e1c1d4f6..ff8e7dc9d4dd 100644
> --- a/include/uapi/linux/virtio_config.h
> +++ b/include/uapi/linux/virtio_config.h
> @@ -78,6 +78,12 @@
> /* This feature indicates support for the packed virtqueue layout. */
> #define VIRTIO_F_RING_PACKED 34
>
> +/*
> + * This feature indicates that memory accesses by the driver and the
> + * device are ordered in a way described by the platform.
> + */
> +#define VIRTIO_F_ORDER_PLATFORM 36
> +
> /*
> * Does the device support Single Root I/O Virtualization?
> */
I wonder whether or not this is sufficient. Is dma barrier implies a
mmio barrier? Looks not.
See ia64/include/asm/barrier.h:
* Note: "mb()" and its variants cannot be used as a fence to order
* accesses to memory mapped I/O registers. For that, mf.a needs to
* be used. However, we don't want to always use mf.a because (a)
* it's (presumably) much slower than mf and (b) mf.a is supported for
* sequential memory pages only.
*/
#define mb() ia64_mf()
#define rmb() mb()
#define wmb() mb()
#define dma_rmb() mb()
=>efine dma_wmb() mb()
Thanks
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [virtio-dev] [PATCH] virtio: support VIRTIO_F_ORDER_PLATFORM
2019-01-23 3:08 ` [virtio-dev] " Jason Wang
@ 2019-01-23 3:49 ` Michael S. Tsirkin
2019-01-23 6:56 ` Jason Wang
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2019-01-23 3:49 UTC (permalink / raw)
To: Jason Wang; +Cc: Tiwei Bie, virtualization, linux-kernel, virtio-dev
On Wed, Jan 23, 2019 at 11:08:04AM +0800, Jason Wang wrote:
>
> On 2019/1/23 上午1:03, Tiwei Bie wrote:
> > This patch introduces the support for VIRTIO_F_ORDER_PLATFORM.
> > When this feature is negotiated, driver will use the barriers
> > suitable for hardware devices.
> >
> > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > ---
> > drivers/virtio/virtio_ring.c | 8 ++++++++
> > include/uapi/linux/virtio_config.h | 6 ++++++
> > 2 files changed, 14 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index cd7e755484e3..27d3f057493e 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -1609,6 +1609,9 @@ static struct virtqueue *vring_create_virtqueue_packed(
> > !context;
> > vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
> > + if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
> > + vq->weak_barriers = false;
> > +
> > vq->packed.ring_dma_addr = ring_dma_addr;
> > vq->packed.driver_event_dma_addr = driver_event_dma_addr;
> > vq->packed.device_event_dma_addr = device_event_dma_addr;
> > @@ -2079,6 +2082,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_ORDER_PLATFORM))
> > + vq->weak_barriers = false;
> > +
> > vq->split.queue_dma_addr = 0;
> > vq->split.queue_size_in_bytes = 0;
> > @@ -2213,6 +2219,8 @@ void vring_transport_features(struct virtio_device *vdev)
> > break;
> > case VIRTIO_F_RING_PACKED:
> > break;
> > + case VIRTIO_F_ORDER_PLATFORM:
> > + 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 1196e1c1d4f6..ff8e7dc9d4dd 100644
> > --- a/include/uapi/linux/virtio_config.h
> > +++ b/include/uapi/linux/virtio_config.h
> > @@ -78,6 +78,12 @@
> > /* This feature indicates support for the packed virtqueue layout. */
> > #define VIRTIO_F_RING_PACKED 34
> > +/*
> > + * This feature indicates that memory accesses by the driver and the
> > + * device are ordered in a way described by the platform.
> > + */
> > +#define VIRTIO_F_ORDER_PLATFORM 36
> > +
> > /*
> > * Does the device support Single Root I/O Virtualization?
> > */
>
>
> I wonder whether or not this is sufficient. Is dma barrier implies a mmio
> barrier? Looks not.
IIUC we don't need an mmio barrier because we are using a
serializing API: Documentation/memory-barriers.txt says:
Note that, when using writel(), a prior
wmb() is not needed to guarantee that the cache coherent memory writes
have completed before writing to the MMIO region.
> See ia64/include/asm/barrier.h:
>
> * Note: "mb()" and its variants cannot be used as a fence to order
> * accesses to memory mapped I/O registers. For that, mf.a needs to
> * be used. However, we don't want to always use mf.a because (a)
> * it's (presumably) much slower than mf and (b) mf.a is supported for
> * sequential memory pages only.
> */
> #define mb() ia64_mf()
> #define rmb() mb()
> #define wmb() mb()
>
> #define dma_rmb() mb()
> =>efine dma_wmb() mb()
>
> Thanks
Frankly no idea about ia64. Sorry. Are any less esoteric platforms
affected?
--
MST
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virtio: support VIRTIO_F_ORDER_PLATFORM
2019-01-22 17:03 [PATCH] virtio: support VIRTIO_F_ORDER_PLATFORM Tiwei Bie
2019-01-23 3:08 ` [virtio-dev] " Jason Wang
@ 2019-01-23 4:04 ` Michael S. Tsirkin
2019-01-23 8:02 ` Tiwei Bie
1 sibling, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2019-01-23 4:04 UTC (permalink / raw)
To: Tiwei Bie; +Cc: jasowang, virtualization, linux-kernel, virtio-dev
On Wed, Jan 23, 2019 at 01:03:46AM +0800, Tiwei Bie wrote:
> This patch introduces the support for VIRTIO_F_ORDER_PLATFORM.
> When this feature is negotiated, driver will use the barriers
> suitable for hardware devices.
>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
Could you pls add a bit more explanation in the commit log?
E.g. which configurations are broken without this patch?
How severe is the problem?
I'm trying to decide whether this belongs in 5.0 or 5.1.
> ---
> drivers/virtio/virtio_ring.c | 8 ++++++++
> include/uapi/linux/virtio_config.h | 6 ++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index cd7e755484e3..27d3f057493e 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1609,6 +1609,9 @@ static struct virtqueue *vring_create_virtqueue_packed(
> !context;
> vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
>
> + if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
> + vq->weak_barriers = false;
> +
> vq->packed.ring_dma_addr = ring_dma_addr;
> vq->packed.driver_event_dma_addr = driver_event_dma_addr;
> vq->packed.device_event_dma_addr = device_event_dma_addr;
> @@ -2079,6 +2082,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_ORDER_PLATFORM))
> + vq->weak_barriers = false;
> +
> vq->split.queue_dma_addr = 0;
> vq->split.queue_size_in_bytes = 0;
>
> @@ -2213,6 +2219,8 @@ void vring_transport_features(struct virtio_device *vdev)
> break;
> case VIRTIO_F_RING_PACKED:
> break;
> + case VIRTIO_F_ORDER_PLATFORM:
> + 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 1196e1c1d4f6..ff8e7dc9d4dd 100644
> --- a/include/uapi/linux/virtio_config.h
> +++ b/include/uapi/linux/virtio_config.h
> @@ -78,6 +78,12 @@
> /* This feature indicates support for the packed virtqueue layout. */
> #define VIRTIO_F_RING_PACKED 34
>
> +/*
> + * This feature indicates that memory accesses by the driver and the
> + * device are ordered in a way described by the platform.
> + */
> +#define VIRTIO_F_ORDER_PLATFORM 36
> +
> /*
> * Does the device support Single Root I/O Virtualization?
> */
> --
> 2.17.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [virtio-dev] [PATCH] virtio: support VIRTIO_F_ORDER_PLATFORM
2019-01-23 3:49 ` Michael S. Tsirkin
@ 2019-01-23 6:56 ` Jason Wang
2019-01-23 13:49 ` Michael S. Tsirkin
0 siblings, 1 reply; 7+ messages in thread
From: Jason Wang @ 2019-01-23 6:56 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Tiwei Bie, virtualization, linux-kernel, virtio-dev
On 2019/1/23 上午11:49, Michael S. Tsirkin wrote:
> On Wed, Jan 23, 2019 at 11:08:04AM +0800, Jason Wang wrote:
>> On 2019/1/23 上午1:03, Tiwei Bie wrote:
>>> This patch introduces the support for VIRTIO_F_ORDER_PLATFORM.
>>> When this feature is negotiated, driver will use the barriers
>>> suitable for hardware devices.
>>>
>>> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
>>> ---
>>> drivers/virtio/virtio_ring.c | 8 ++++++++
>>> include/uapi/linux/virtio_config.h | 6 ++++++
>>> 2 files changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>> index cd7e755484e3..27d3f057493e 100644
>>> --- a/drivers/virtio/virtio_ring.c
>>> +++ b/drivers/virtio/virtio_ring.c
>>> @@ -1609,6 +1609,9 @@ static struct virtqueue *vring_create_virtqueue_packed(
>>> !context;
>>> vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
>>> + if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
>>> + vq->weak_barriers = false;
>>> +
>>> vq->packed.ring_dma_addr = ring_dma_addr;
>>> vq->packed.driver_event_dma_addr = driver_event_dma_addr;
>>> vq->packed.device_event_dma_addr = device_event_dma_addr;
>>> @@ -2079,6 +2082,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_ORDER_PLATFORM))
>>> + vq->weak_barriers = false;
>>> +
>>> vq->split.queue_dma_addr = 0;
>>> vq->split.queue_size_in_bytes = 0;
>>> @@ -2213,6 +2219,8 @@ void vring_transport_features(struct virtio_device *vdev)
>>> break;
>>> case VIRTIO_F_RING_PACKED:
>>> break;
>>> + case VIRTIO_F_ORDER_PLATFORM:
>>> + 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 1196e1c1d4f6..ff8e7dc9d4dd 100644
>>> --- a/include/uapi/linux/virtio_config.h
>>> +++ b/include/uapi/linux/virtio_config.h
>>> @@ -78,6 +78,12 @@
>>> /* This feature indicates support for the packed virtqueue layout. */
>>> #define VIRTIO_F_RING_PACKED 34
>>> +/*
>>> + * This feature indicates that memory accesses by the driver and the
>>> + * device are ordered in a way described by the platform.
>>> + */
>>> +#define VIRTIO_F_ORDER_PLATFORM 36
>>> +
>>> /*
>>> * Does the device support Single Root I/O Virtualization?
>>> */
>>
>> I wonder whether or not this is sufficient. Is dma barrier implies a mmio
>> barrier? Looks not.
> IIUC we don't need an mmio barrier because we are using a
> serializing API: Documentation/memory-barriers.txt says:
>
> Note that, when using writel(), a prior
> wmb() is not needed to guarantee that the cache coherent memory writes
> have completed before writing to the MMIO region.
Ah, I get this.
>
>
>> See ia64/include/asm/barrier.h:
>>
>> * Note: "mb()" and its variants cannot be used as a fence to order
>> * accesses to memory mapped I/O registers. For that, mf.a needs to
>> * be used. However, we don't want to always use mf.a because (a)
>> * it's (presumably) much slower than mf and (b) mf.a is supported for
>> * sequential memory pages only.
>> */
>> #define mb() ia64_mf()
>> #define rmb() mb()
>> #define wmb() mb()
>>
>> #define dma_rmb() mb()
>> =>efine dma_wmb() mb()
>>
>> Thanks
> Frankly no idea about ia64.
Neither did me.
> Sorry. Are any less esoteric platforms
> affected?
>
E.g ppc64?
define dma_wmb() __asm__ __volatile__ (stringify_in_c(SMPWMB) : :
:"memo\
ry")
/*
* Enforce synchronisation of stores vs. spin_unlock
* (this does it explicitly, though our implementation of spin_unlock
* does it implicitely too)
*/
static inline void mmiowb(void)
{
unsigned long tmp;
__asm__ __volatile__("sync; li %0,0; stb %0,%1(13)"
: "=&r" (tmp) : "i" (offsetof(struct paca_struct, io_sync))
: "memory");
}
dma_wmb() is lwsync which is more lightweight than sync I guess?
Thanks
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virtio: support VIRTIO_F_ORDER_PLATFORM
2019-01-23 4:04 ` Michael S. Tsirkin
@ 2019-01-23 8:02 ` Tiwei Bie
0 siblings, 0 replies; 7+ messages in thread
From: Tiwei Bie @ 2019-01-23 8:02 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: jasowang, virtualization, linux-kernel, virtio-dev
On Tue, Jan 22, 2019 at 11:04:29PM -0500, Michael S. Tsirkin wrote:
> On Wed, Jan 23, 2019 at 01:03:46AM +0800, Tiwei Bie wrote:
> > This patch introduces the support for VIRTIO_F_ORDER_PLATFORM.
> > When this feature is negotiated, driver will use the barriers
> > suitable for hardware devices.
> >
> > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
>
> Could you pls add a bit more explanation in the commit log?
> E.g. which configurations are broken without this patch?
> How severe is the problem?
Sure. Will do that.
Thanks
>
> I'm trying to decide whether this belongs in 5.0 or 5.1.
>
> > ---
> > drivers/virtio/virtio_ring.c | 8 ++++++++
> > include/uapi/linux/virtio_config.h | 6 ++++++
> > 2 files changed, 14 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index cd7e755484e3..27d3f057493e 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -1609,6 +1609,9 @@ static struct virtqueue *vring_create_virtqueue_packed(
> > !context;
> > vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
> >
> > + if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
> > + vq->weak_barriers = false;
> > +
> > vq->packed.ring_dma_addr = ring_dma_addr;
> > vq->packed.driver_event_dma_addr = driver_event_dma_addr;
> > vq->packed.device_event_dma_addr = device_event_dma_addr;
> > @@ -2079,6 +2082,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_ORDER_PLATFORM))
> > + vq->weak_barriers = false;
> > +
> > vq->split.queue_dma_addr = 0;
> > vq->split.queue_size_in_bytes = 0;
> >
> > @@ -2213,6 +2219,8 @@ void vring_transport_features(struct virtio_device *vdev)
> > break;
> > case VIRTIO_F_RING_PACKED:
> > break;
> > + case VIRTIO_F_ORDER_PLATFORM:
> > + 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 1196e1c1d4f6..ff8e7dc9d4dd 100644
> > --- a/include/uapi/linux/virtio_config.h
> > +++ b/include/uapi/linux/virtio_config.h
> > @@ -78,6 +78,12 @@
> > /* This feature indicates support for the packed virtqueue layout. */
> > #define VIRTIO_F_RING_PACKED 34
> >
> > +/*
> > + * This feature indicates that memory accesses by the driver and the
> > + * device are ordered in a way described by the platform.
> > + */
> > +#define VIRTIO_F_ORDER_PLATFORM 36
> > +
> > /*
> > * Does the device support Single Root I/O Virtualization?
> > */
> > --
> > 2.17.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [virtio-dev] [PATCH] virtio: support VIRTIO_F_ORDER_PLATFORM
2019-01-23 6:56 ` Jason Wang
@ 2019-01-23 13:49 ` Michael S. Tsirkin
0 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2019-01-23 13:49 UTC (permalink / raw)
To: Jason Wang; +Cc: Tiwei Bie, virtualization, linux-kernel, virtio-dev
On Wed, Jan 23, 2019 at 02:56:00PM +0800, Jason Wang wrote:
>
> On 2019/1/23 上午11:49, Michael S. Tsirkin wrote:
> > On Wed, Jan 23, 2019 at 11:08:04AM +0800, Jason Wang wrote:
> > > On 2019/1/23 上午1:03, Tiwei Bie wrote:
> > > > This patch introduces the support for VIRTIO_F_ORDER_PLATFORM.
> > > > When this feature is negotiated, driver will use the barriers
> > > > suitable for hardware devices.
> > > >
> > > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > > > ---
> > > > drivers/virtio/virtio_ring.c | 8 ++++++++
> > > > include/uapi/linux/virtio_config.h | 6 ++++++
> > > > 2 files changed, 14 insertions(+)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index cd7e755484e3..27d3f057493e 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -1609,6 +1609,9 @@ static struct virtqueue *vring_create_virtqueue_packed(
> > > > !context;
> > > > vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
> > > > + if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
> > > > + vq->weak_barriers = false;
> > > > +
> > > > vq->packed.ring_dma_addr = ring_dma_addr;
> > > > vq->packed.driver_event_dma_addr = driver_event_dma_addr;
> > > > vq->packed.device_event_dma_addr = device_event_dma_addr;
> > > > @@ -2079,6 +2082,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_ORDER_PLATFORM))
> > > > + vq->weak_barriers = false;
> > > > +
> > > > vq->split.queue_dma_addr = 0;
> > > > vq->split.queue_size_in_bytes = 0;
> > > > @@ -2213,6 +2219,8 @@ void vring_transport_features(struct virtio_device *vdev)
> > > > break;
> > > > case VIRTIO_F_RING_PACKED:
> > > > break;
> > > > + case VIRTIO_F_ORDER_PLATFORM:
> > > > + 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 1196e1c1d4f6..ff8e7dc9d4dd 100644
> > > > --- a/include/uapi/linux/virtio_config.h
> > > > +++ b/include/uapi/linux/virtio_config.h
> > > > @@ -78,6 +78,12 @@
> > > > /* This feature indicates support for the packed virtqueue layout. */
> > > > #define VIRTIO_F_RING_PACKED 34
> > > > +/*
> > > > + * This feature indicates that memory accesses by the driver and the
> > > > + * device are ordered in a way described by the platform.
> > > > + */
> > > > +#define VIRTIO_F_ORDER_PLATFORM 36
> > > > +
> > > > /*
> > > > * Does the device support Single Root I/O Virtualization?
> > > > */
> > >
> > > I wonder whether or not this is sufficient. Is dma barrier implies a mmio
> > > barrier? Looks not.
> > IIUC we don't need an mmio barrier because we are using a
> > serializing API: Documentation/memory-barriers.txt says:
> >
> > Note that, when using writel(), a prior
> > wmb() is not needed to guarantee that the cache coherent memory writes
> > have completed before writing to the MMIO region.
>
>
> Ah, I get this.
>
>
> >
> >
> > > See ia64/include/asm/barrier.h:
> > >
> > > * Note: "mb()" and its variants cannot be used as a fence to order
> > > * accesses to memory mapped I/O registers. For that, mf.a needs to
> > > * be used. However, we don't want to always use mf.a because (a)
> > > * it's (presumably) much slower than mf and (b) mf.a is supported for
> > > * sequential memory pages only.
> > > */
> > > #define mb() ia64_mf()
> > > #define rmb() mb()
> > > #define wmb() mb()
> > >
> > > #define dma_rmb() mb()
> > > =>efine dma_wmb() mb()
> > >
> > > Thanks
> > Frankly no idea about ia64.
>
>
> Neither did me.
>
>
> > Sorry. Are any less esoteric platforms
> > affected?
> >
>
> E.g ppc64?
So
void iowrite32(u32 val, void __iomem *addr)
{
writel(val, addr);
}
and that eventually gets to this one:
#define DEF_MMIO_OUT_D(name, size, insn) \
static inline void name(volatile u##size __iomem *addr, u##size val) \
{ \
__asm__ __volatile__("sync;"#insn"%U0%X0 %1,%0" \
: "=m" (*addr) : "r" (val) : "memory"); \
IO_SET_SYNC_FLAG(); \
}
and
#ifdef CONFIG_PPC64
#define IO_SET_SYNC_FLAG() do { local_paca->io_sync = 1; } while(0)
#else
#define IO_SET_SYNC_FLAG()
#endif
> define dma_wmb() __asm__ __volatile__ (stringify_in_c(SMPWMB) : :
> :"memo\
> ry")
>
> /*
> * Enforce synchronisation of stores vs. spin_unlock
> * (this does it explicitly, though our implementation of spin_unlock
I don't know which spin_unlock does it refer to here.
> * does it implicitely too)
> */
> static inline void mmiowb(void)
> {
> unsigned long tmp;
>
> __asm__ __volatile__("sync; li %0,0; stb %0,%1(13)"
> : "=&r" (tmp) : "i" (offsetof(struct paca_struct, io_sync))
> : "memory");
> }
So sync+set io_sync here and sync+io_sync above.
> dma_wmb() is lwsync which is more lightweight than sync I guess?
>
> Thanks
>
Sounds about right.
--
MST
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-01-23 13:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-22 17:03 [PATCH] virtio: support VIRTIO_F_ORDER_PLATFORM Tiwei Bie
2019-01-23 3:08 ` [virtio-dev] " Jason Wang
2019-01-23 3:49 ` Michael S. Tsirkin
2019-01-23 6:56 ` Jason Wang
2019-01-23 13:49 ` Michael S. Tsirkin
2019-01-23 4:04 ` Michael S. Tsirkin
2019-01-23 8:02 ` Tiwei Bie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).