[v2,2/3] net/virtio: virtual PCI requires smp barriers
diff mbox series

Message ID 1576811391-19131-3-git-send-email-gavin.hu@arm.com
State Superseded, archived
Delegated to: David Marchand
Headers show
Series
  • relax io barrier for aarch64 and use smp barriers for virtual pci memory
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Gavin Hu Dec. 20, 2019, 3:09 a.m. UTC
Other than real PCI reads and writes to the device memory requiring
the io barriers, virtual pci memories are normal memory in the smp
configuration, which requires the smp barriers.

Since the smp barriers and io barriers are identical on x86 and PPC,
this change has only effect on aarch64.

As far as peripheral coherence order for ‘virtual’ devices, the arch
intent is that the Hypervisor view of things takes precedence – since
translations are made in holistic manner as the full stage1+stage2
regime, there is no such thing as a transaction taking on “EL1”
mapping as far as ordering. If the Hypervisor maps stage2 as Normal
but the OS at EL1 maps it as Device-nGnRE, then it’s Normal memory and
follows the ordering rules for Normal memory.

Signed-off-by: Gavin Hu <gavin.hu@arm.com>
---
 drivers/net/virtio/virtio_pci.c | 108 +++++++++++++++++++++++++++++-----------
 1 file changed, 78 insertions(+), 30 deletions(-)

Comments

Tiwei Bie Dec. 20, 2019, 8:17 a.m. UTC | #1
On Fri, Dec 20, 2019 at 11:09:50AM +0800, Gavin Hu wrote:
> Other than real PCI reads and writes to the device memory requiring
> the io barriers, virtual pci memories are normal memory in the smp
> configuration, which requires the smp barriers.
> 
> Since the smp barriers and io barriers are identical on x86 and PPC,
> this change has only effect on aarch64.
> 
> As far as peripheral coherence order for ‘virtual’ devices, the arch
> intent is that the Hypervisor view of things takes precedence – since
> translations are made in holistic manner as the full stage1+stage2
> regime, there is no such thing as a transaction taking on “EL1”
> mapping as far as ordering. If the Hypervisor maps stage2 as Normal
> but the OS at EL1 maps it as Device-nGnRE, then it’s Normal memory and
> follows the ordering rules for Normal memory.
> 
> Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> ---
>  drivers/net/virtio/virtio_pci.c | 108 +++++++++++++++++++++++++++++-----------
>  1 file changed, 78 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
> index 4468e89..64aa0a0 100644
> --- a/drivers/net/virtio/virtio_pci.c
> +++ b/drivers/net/virtio/virtio_pci.c
> @@ -24,6 +24,54 @@
>  #define PCI_CAP_ID_VNDR		0x09
>  #define PCI_CAP_ID_MSIX		0x11
>  
> +static __rte_always_inline uint8_t
> +virtio_pci_read8(const volatile void *addr)
> +{
> +	uint8_t val;
> +	val = rte_read8_relaxed(addr);
> +	rte_smp_rmb();
> +	return val;
> +}
> +
> +static __rte_always_inline uint16_t
> +virtio_pci_read16(const volatile void *addr)
> +{
> +	uint16_t val;
> +	val = rte_read16_relaxed(addr);
> +	rte_smp_rmb();
> +	return val;
> +}
> +
> +static __rte_always_inline uint32_t
> +virtio_pci_read32(const volatile void *addr)
> +{
> +	uint32_t val;
> +	val = rte_read32_relaxed(addr);
> +	rte_smp_rmb();
> +	return val;
> +}
> +
> +static __rte_always_inline void
> +virtio_pci_write8(uint8_t value, volatile void *addr)
> +{
> +	rte_smp_wmb();
> +	rte_write8_relaxed(value, addr);
> +}
> +
> +static __rte_always_inline void
> +virtio_pci_write16(uint16_t value, volatile void *addr)
> +{
> +	rte_smp_wmb();
> +	rte_write16_relaxed(value, addr);
> +}
> +
> +static __rte_always_inline void
> +virtio_pci_write32(uint32_t value, volatile void *addr)
> +{
> +	rte_smp_wmb();
> +	rte_write32_relaxed(value, addr);
> +}

We can't assume that virtio device is software running
in an SMP configuration unless VIRTIO_F_ORDER_PLATFORM
isn't negotiated.

https://github.com/oasis-tcs/virtio-spec/blob/94520b3af19c/content.tex#L5788

> +
Gavin Hu Dec. 20, 2019, 10:19 a.m. UTC | #2
Hi Tiwei,

Thanks for review.

> -----Original Message-----
> From: Tiwei Bie <tiwei.bie@intel.com>
> Sent: Friday, December 20, 2019 4:18 PM
> To: Gavin Hu <Gavin.Hu@arm.com>
> Cc: dev@dpdk.org; nd <nd@arm.com>; david.marchand@redhat.com;
> thomas@monjalon.net; rasland@mellanox.com;
> maxime.coquelin@redhat.com; hemant.agrawal@nxp.com;
> jerinj@marvell.com; pbhagavatula@marvell.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; Joyce Kong
> <Joyce.Kong@arm.com>; Steve Capper <Steve.Capper@arm.com>
> Subject: Re: [PATCH v2 2/3] net/virtio: virtual PCI requires smp barriers
> 
> On Fri, Dec 20, 2019 at 11:09:50AM +0800, Gavin Hu wrote:
> > Other than real PCI reads and writes to the device memory requiring
> > the io barriers, virtual pci memories are normal memory in the smp
> > configuration, which requires the smp barriers.
> >
> > Since the smp barriers and io barriers are identical on x86 and PPC,
> > this change has only effect on aarch64.
> >
> > As far as peripheral coherence order for ‘virtual’ devices, the arch
> > intent is that the Hypervisor view of things takes precedence – since
> > translations are made in holistic manner as the full stage1+stage2
> > regime, there is no such thing as a transaction taking on “EL1”
> > mapping as far as ordering. If the Hypervisor maps stage2 as Normal
> > but the OS at EL1 maps it as Device-nGnRE, then it’s Normal memory and
> > follows the ordering rules for Normal memory.
> >
> > Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> > ---
> >  drivers/net/virtio/virtio_pci.c | 108 +++++++++++++++++++++++++++++---
> --------
> >  1 file changed, 78 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
> > index 4468e89..64aa0a0 100644
> > --- a/drivers/net/virtio/virtio_pci.c
> > +++ b/drivers/net/virtio/virtio_pci.c
> > @@ -24,6 +24,54 @@
> >  #define PCI_CAP_ID_VNDR		0x09
> >  #define PCI_CAP_ID_MSIX		0x11
> >
> > +static __rte_always_inline uint8_t
> > +virtio_pci_read8(const volatile void *addr)
> > +{
> > +	uint8_t val;
> > +	val = rte_read8_relaxed(addr);
> > +	rte_smp_rmb();
> > +	return val;
> > +}
> > +
> > +static __rte_always_inline uint16_t
> > +virtio_pci_read16(const volatile void *addr)
> > +{
> > +	uint16_t val;
> > +	val = rte_read16_relaxed(addr);
> > +	rte_smp_rmb();
> > +	return val;
> > +}
> > +
> > +static __rte_always_inline uint32_t
> > +virtio_pci_read32(const volatile void *addr)
> > +{
> > +	uint32_t val;
> > +	val = rte_read32_relaxed(addr);
> > +	rte_smp_rmb();
> > +	return val;
> > +}
> > +
> > +static __rte_always_inline void
> > +virtio_pci_write8(uint8_t value, volatile void *addr)
> > +{
> > +	rte_smp_wmb();
> > +	rte_write8_relaxed(value, addr);
> > +}
> > +
> > +static __rte_always_inline void
> > +virtio_pci_write16(uint16_t value, volatile void *addr)
> > +{
> > +	rte_smp_wmb();
> > +	rte_write16_relaxed(value, addr);
> > +}
> > +
> > +static __rte_always_inline void
> > +virtio_pci_write32(uint32_t value, volatile void *addr)
> > +{
> > +	rte_smp_wmb();
> > +	rte_write32_relaxed(value, addr);
> > +}
> 
> We can't assume that virtio device is software running
> in an SMP configuration unless VIRTIO_F_ORDER_PLATFORM
> isn't negotiated.
That's true, rte_smp is sufficient for *non*- VIRTIO_F_ORDER_PLATFORM,
As in the previous patch, rte_io is relaxed to a compiler barrier, rte_smp is stronger than that, put another way, rte_smp is also sufficient for * VIRTIO_F_ORDER_PLATFORM* case.
As X86 and PPC make no difference about rte_smp and rte_io, so everything goes fine? 

Anyway, I will update the commit log to reflect the above points. 
> 
> https://github.com/oasis-tcs/virtio-
> spec/blob/94520b3af19c/content.tex#L5788
> 
> > +

Patch
diff mbox series

diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index 4468e89..64aa0a0 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -24,6 +24,54 @@ 
 #define PCI_CAP_ID_VNDR		0x09
 #define PCI_CAP_ID_MSIX		0x11
 
+static __rte_always_inline uint8_t
+virtio_pci_read8(const volatile void *addr)
+{
+	uint8_t val;
+	val = rte_read8_relaxed(addr);
+	rte_smp_rmb();
+	return val;
+}
+
+static __rte_always_inline uint16_t
+virtio_pci_read16(const volatile void *addr)
+{
+	uint16_t val;
+	val = rte_read16_relaxed(addr);
+	rte_smp_rmb();
+	return val;
+}
+
+static __rte_always_inline uint32_t
+virtio_pci_read32(const volatile void *addr)
+{
+	uint32_t val;
+	val = rte_read32_relaxed(addr);
+	rte_smp_rmb();
+	return val;
+}
+
+static __rte_always_inline void
+virtio_pci_write8(uint8_t value, volatile void *addr)
+{
+	rte_smp_wmb();
+	rte_write8_relaxed(value, addr);
+}
+
+static __rte_always_inline void
+virtio_pci_write16(uint16_t value, volatile void *addr)
+{
+	rte_smp_wmb();
+	rte_write16_relaxed(value, addr);
+}
+
+static __rte_always_inline void
+virtio_pci_write32(uint32_t value, volatile void *addr)
+{
+	rte_smp_wmb();
+	rte_write32_relaxed(value, addr);
+}
+
 /*
  * The remaining space is defined by each driver as the per-driver
  * configuration space.
@@ -260,8 +308,8 @@  const struct virtio_pci_ops legacy_ops = {
 static inline void
 io_write64_twopart(uint64_t val, uint32_t *lo, uint32_t *hi)
 {
-	rte_write32(val & ((1ULL << 32) - 1), lo);
-	rte_write32(val >> 32,		     hi);
+	virtio_pci_write32(val & ((1ULL << 32) - 1), lo);
+	virtio_pci_write32(val >> 32,		     hi);
 }
 
 static void
@@ -273,13 +321,13 @@  modern_read_dev_config(struct virtio_hw *hw, size_t offset,
 	uint8_t old_gen, new_gen;
 
 	do {
-		old_gen = rte_read8(&hw->common_cfg->config_generation);
+		old_gen = virtio_pci_read8(&hw->common_cfg->config_generation);
 
 		p = dst;
 		for (i = 0;  i < length; i++)
-			*p++ = rte_read8((uint8_t *)hw->dev_cfg + offset + i);
+			*p++ = virtio_pci_read8((uint8_t *)hw->dev_cfg + offset + i);
 
-		new_gen = rte_read8(&hw->common_cfg->config_generation);
+		new_gen = virtio_pci_read8(&hw->common_cfg->config_generation);
 	} while (old_gen != new_gen);
 }
 
@@ -291,7 +339,7 @@  modern_write_dev_config(struct virtio_hw *hw, size_t offset,
 	const uint8_t *p = src;
 
 	for (i = 0;  i < length; i++)
-		rte_write8((*p++), (((uint8_t *)hw->dev_cfg) + offset + i));
+		virtio_pci_write8((*p++), (((uint8_t *)hw->dev_cfg) + offset + i));
 }
 
 static uint64_t
@@ -299,11 +347,11 @@  modern_get_features(struct virtio_hw *hw)
 {
 	uint32_t features_lo, features_hi;
 
-	rte_write32(0, &hw->common_cfg->device_feature_select);
-	features_lo = rte_read32(&hw->common_cfg->device_feature);
+	virtio_pci_write32(0, &hw->common_cfg->device_feature_select);
+	features_lo = virtio_pci_read32(&hw->common_cfg->device_feature);
 
-	rte_write32(1, &hw->common_cfg->device_feature_select);
-	features_hi = rte_read32(&hw->common_cfg->device_feature);
+	virtio_pci_write32(1, &hw->common_cfg->device_feature_select);
+	features_hi = virtio_pci_read32(&hw->common_cfg->device_feature);
 
 	return ((uint64_t)features_hi << 32) | features_lo;
 }
@@ -311,53 +359,53 @@  modern_get_features(struct virtio_hw *hw)
 static void
 modern_set_features(struct virtio_hw *hw, uint64_t features)
 {
-	rte_write32(0, &hw->common_cfg->guest_feature_select);
-	rte_write32(features & ((1ULL << 32) - 1),
+	virtio_pci_write32(0, &hw->common_cfg->guest_feature_select);
+	virtio_pci_write32(features & ((1ULL << 32) - 1),
 		    &hw->common_cfg->guest_feature);
 
-	rte_write32(1, &hw->common_cfg->guest_feature_select);
-	rte_write32(features >> 32,
+	virtio_pci_write32(1, &hw->common_cfg->guest_feature_select);
+	virtio_pci_write32(features >> 32,
 		    &hw->common_cfg->guest_feature);
 }
 
 static uint8_t
 modern_get_status(struct virtio_hw *hw)
 {
-	return rte_read8(&hw->common_cfg->device_status);
+	return virtio_pci_read8(&hw->common_cfg->device_status);
 }
 
 static void
 modern_set_status(struct virtio_hw *hw, uint8_t status)
 {
-	rte_write8(status, &hw->common_cfg->device_status);
+	virtio_pci_write8(status, &hw->common_cfg->device_status);
 }
 
 static uint8_t
 modern_get_isr(struct virtio_hw *hw)
 {
-	return rte_read8(hw->isr);
+	return virtio_pci_read8(hw->isr);
 }
 
 static uint16_t
 modern_set_config_irq(struct virtio_hw *hw, uint16_t vec)
 {
-	rte_write16(vec, &hw->common_cfg->msix_config);
-	return rte_read16(&hw->common_cfg->msix_config);
+	virtio_pci_write16(vec, &hw->common_cfg->msix_config);
+	return virtio_pci_read16(&hw->common_cfg->msix_config);
 }
 
 static uint16_t
 modern_set_queue_irq(struct virtio_hw *hw, struct virtqueue *vq, uint16_t vec)
 {
-	rte_write16(vq->vq_queue_index, &hw->common_cfg->queue_select);
-	rte_write16(vec, &hw->common_cfg->queue_msix_vector);
-	return rte_read16(&hw->common_cfg->queue_msix_vector);
+	virtio_pci_write16(vq->vq_queue_index, &hw->common_cfg->queue_select);
+	virtio_pci_write16(vec, &hw->common_cfg->queue_msix_vector);
+	return virtio_pci_read16(&hw->common_cfg->queue_msix_vector);
 }
 
 static uint16_t
 modern_get_queue_num(struct virtio_hw *hw, uint16_t queue_id)
 {
-	rte_write16(queue_id, &hw->common_cfg->queue_select);
-	return rte_read16(&hw->common_cfg->queue_size);
+	virtio_pci_write16(queue_id, &hw->common_cfg->queue_select);
+	return virtio_pci_read16(&hw->common_cfg->queue_size);
 }
 
 static int
@@ -375,7 +423,7 @@  modern_setup_queue(struct virtio_hw *hw, struct virtqueue *vq)
 							 ring[vq->vq_nentries]),
 				   VIRTIO_PCI_VRING_ALIGN);
 
-	rte_write16(vq->vq_queue_index, &hw->common_cfg->queue_select);
+	virtio_pci_write16(vq->vq_queue_index, &hw->common_cfg->queue_select);
 
 	io_write64_twopart(desc_addr, &hw->common_cfg->queue_desc_lo,
 				      &hw->common_cfg->queue_desc_hi);
@@ -384,11 +432,11 @@  modern_setup_queue(struct virtio_hw *hw, struct virtqueue *vq)
 	io_write64_twopart(used_addr, &hw->common_cfg->queue_used_lo,
 				      &hw->common_cfg->queue_used_hi);
 
-	notify_off = rte_read16(&hw->common_cfg->queue_notify_off);
+	notify_off = virtio_pci_read16(&hw->common_cfg->queue_notify_off);
 	vq->notify_addr = (void *)((uint8_t *)hw->notify_base +
 				notify_off * hw->notify_off_multiplier);
 
-	rte_write16(1, &hw->common_cfg->queue_enable);
+	virtio_pci_write16(1, &hw->common_cfg->queue_enable);
 
 	PMD_INIT_LOG(DEBUG, "queue %u addresses:", vq->vq_queue_index);
 	PMD_INIT_LOG(DEBUG, "\t desc_addr: %" PRIx64, desc_addr);
@@ -403,7 +451,7 @@  modern_setup_queue(struct virtio_hw *hw, struct virtqueue *vq)
 static void
 modern_del_queue(struct virtio_hw *hw, struct virtqueue *vq)
 {
-	rte_write16(vq->vq_queue_index, &hw->common_cfg->queue_select);
+	virtio_pci_write16(vq->vq_queue_index, &hw->common_cfg->queue_select);
 
 	io_write64_twopart(0, &hw->common_cfg->queue_desc_lo,
 				  &hw->common_cfg->queue_desc_hi);
@@ -412,13 +460,13 @@  modern_del_queue(struct virtio_hw *hw, struct virtqueue *vq)
 	io_write64_twopart(0, &hw->common_cfg->queue_used_lo,
 				  &hw->common_cfg->queue_used_hi);
 
-	rte_write16(0, &hw->common_cfg->queue_enable);
+	virtio_pci_write16(0, &hw->common_cfg->queue_enable);
 }
 
 static void
 modern_notify_queue(struct virtio_hw *hw __rte_unused, struct virtqueue *vq)
 {
-	rte_write16(vq->vq_queue_index, vq->notify_addr);
+	virtio_pci_write16(vq->vq_queue_index, vq->notify_addr);
 }
 
 const struct virtio_pci_ops modern_ops = {