[dpdk-dev,v2,6/7] virtio: fix pci accesses for ppc64 in legacy mode

Message ID 000301d1b1ae$cebf1470$6c3d3d50$@linux.vnet.ibm.com (mailing list archive)
State Not Applicable, archived
Headers

Commit Message

Chao Zhu May 19, 2016, 9:13 a.m. UTC
  Olivier,

Thanks for the patches! 
Just one comment:
POWER8 machine only supports little endian OS on bare metal. In VM guest, it
can support both little endian and big endian OS. Did you try to run it on
both host (little endian) and guest (big endian and little endian)?

-----Original Message-----
From: Olivier Matz [mailto:olivier.matz@6wind.com] 
Sent: 2016年5月17日 18:00
To: dev@dpdk.org
Cc: david.marchand@6wind.com; chaozhu@linux.vnet.ibm.com; yuanhan.liu@linux.
intel.com; huawei.xie@intel.com
Subject: [PATCH v2 6/7] virtio: fix pci accesses for ppc64 in legacy mode

From: David Marchand <david.marchand@6wind.com>

Although ppc supports both endianesses, qemu supposes that the cpu is big
endian and enforces this for the virtio-net stuff.

Fix PCI accesses in legacy mode. Only ppc64le is supported at the moment.

Signed-off-by: David Marchand <david.marchand@6wind.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_pci.c | 68
+++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)


 static uint64_t
--
2.8.0.rc3
  

Comments

Olivier Matz May 20, 2016, 12:11 p.m. UTC | #1
Hi Chao,

On 05/19/2016 11:13 AM, Chao Zhu wrote:
> Olivier,
> 
> Thanks for the patches! 
> Just one comment:
> POWER8 machine only supports little endian OS on bare metal. In VM guest, it
> can support both little endian and big endian OS. Did you try to run it on
> both host (little endian) and guest (big endian and little endian)?

No I didn't test big endian in the guest.

I don't have any big endian VM image. Is it required for the
test? I mean, is it possible to run a big endian DPDK userland
application on a little endian kernel? (and if yes, is it
known to work?)

Maybe I could replace in the patch:
  #ifdef RTE_ARCH_PPC_64
By something like :
  #if defined(RTE_ARCH_PPC_64) && (RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN)

If you have the ability to test it easily, it would be appreciated :)

Thanks for the review!
Olivier
  
Olivier Matz May 20, 2016, 12:18 p.m. UTC | #2
On 05/20/2016 02:11 PM, Olivier Matz wrote:
> Maybe I could replace in the patch:
>   #ifdef RTE_ARCH_PPC_64
> By something like :
>   #if defined(RTE_ARCH_PPC_64) && (RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN)

Hmm it's probably useless since we already do a
rte_be_to_cpu_*(), so it should do nothing on BE
targets.
  
Chao Zhu May 24, 2016, 6:28 a.m. UTC | #3
Olivier,

The patch set looks good. I think little endian is good enough. We
internally decide to only support little endian previously. You can keep the
endian conversion such kind of things in the code.

-----Original Message-----
From: Olivier Matz [mailto:olivier.matz@6wind.com] 
Sent: 2016年5月20日 20:11
To: Chao Zhu <chaozhu@linux.vnet.ibm.com>; dev@dpdk.org
Cc: david.marchand@6wind.com; yuanhan.liu@linux.intel.com; huawei.xie@intel.
com
Subject: Re: [PATCH v2 6/7] virtio: fix pci accesses for ppc64 in legacy
mode

Hi Chao,

On 05/19/2016 11:13 AM, Chao Zhu wrote:
> Olivier,
> 
> Thanks for the patches! 
> Just one comment:
> POWER8 machine only supports little endian OS on bare metal. In VM 
> guest, it can support both little endian and big endian OS. Did you 
> try to run it on both host (little endian) and guest (big endian and
little endian)?

No I didn't test big endian in the guest.

I don't have any big endian VM image. Is it required for the test? I mean,
is it possible to run a big endian DPDK userland application on a little
endian kernel? (and if yes, is it known to work?)

Maybe I could replace in the patch:
  #ifdef RTE_ARCH_PPC_64
By something like :
  #if defined(RTE_ARCH_PPC_64) && (RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN)

If you have the ability to test it easily, it would be appreciated :)

Thanks for the review!
Olivier
  

Patch

diff --git a/drivers/net/virtio/virtio_pci.c
b/drivers/net/virtio/virtio_pci.c index 9cdca06..ebf4cf7 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -55,20 +55,88 @@ 
  */
 #define VIRTIO_PCI_CONFIG(hw) (((hw)->use_msix) ? 24 : 20)

+/*
+ * Since we are in legacy mode:
+ * http://ozlabs.org/~rusty/virtio-spec/virtio-0.9.5.pdf
+ *
+ * "Note that this is possible because while the virtio header is PCI (i.e.
+ * little) endian, the device-specific region is encoded in the native 
+endian of
+ * the guest (where such distinction is applicable)."
+ *
+ * For powerpc which supports both, qemu supposes that cpu is big 
+endian and
+ * enforces this for the virtio-net stuff.
+ */
+
 static void
 legacy_read_dev_config(struct virtio_hw *hw, size_t offset,
 		       void *dst, int length)
 {
+#ifdef RTE_ARCH_PPC_64
+	int size;
+
+	while (length > 0) {
+		if (length >= 4) {
+			size = 4;
+			rte_eal_pci_ioport_read(&hw->io, dst, size,
+				VIRTIO_PCI_CONFIG(hw) + offset);
+			*(uint32_t *)dst = rte_be_to_cpu_32(*(uint32_t
*)dst);
+		} else if (length >= 2) {
+			size = 2;
+			rte_eal_pci_ioport_read(&hw->io, dst, size,
+				VIRTIO_PCI_CONFIG(hw) + offset);
+			*(uint16_t *)dst = rte_be_to_cpu_16(*(uint16_t
*)dst);
+		} else {
+			size = 1;
+			rte_eal_pci_ioport_read(&hw->io, dst, size,
+				VIRTIO_PCI_CONFIG(hw) + offset);
+		}
+
+		dst = (char *)dst + size;
+		offset += size;
+		length -= size;
+	}
+#else
 	rte_eal_pci_ioport_read(&hw->io, dst, length,
 				VIRTIO_PCI_CONFIG(hw) + offset);
+#endif
 }

 static void
 legacy_write_dev_config(struct virtio_hw *hw, size_t offset,
 			const void *src, int length)
 {
+#ifdef RTE_ARCH_PPC_64
+	union {
+		uint32_t u32;
+		uint16_t u16;
+	} tmp;
+	int size;
+
+	while (length > 0) {
+		if (length >= 4) {
+			size = 4;
+			tmp.u32 = rte_cpu_to_be_32(*(const uint32_t *)src);
+			rte_eal_pci_ioport_write(&hw->io, &tmp.u32, size,
+				VIRTIO_PCI_CONFIG(hw) + offset);
+		} else if (length >= 2) {
+			size = 2;
+			tmp.u16 = rte_cpu_to_be_16(*(const uint16_t *)src);
+			rte_eal_pci_ioport_write(&hw->io, &tmp.u16, size,
+				VIRTIO_PCI_CONFIG(hw) + offset);
+		} else {
+			size = 1;
+			rte_eal_pci_ioport_write(&hw->io, src, size,
+				VIRTIO_PCI_CONFIG(hw) + offset);
+		}
+
+		src = (const char *)src + size;
+		offset += size;
+		length -= size;
+	}
+#else
 	rte_eal_pci_ioport_write(&hw->io, src, length,
 				 VIRTIO_PCI_CONFIG(hw) + offset);
+#endif
 }