Re: [dpdk-dev] [PATCH v2 2/3] net/virtio: virtual PCI requires smp barriers

[Gavin Hu <Gavin.Hu@arm.com>]

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
> 
> > +