Re: [PATCH v4 08/14] virtio: pci: extend virtio pci rw api for vfio interface

On Fri, Jan 15, 2016 at 11:57 AM, Yuanhan Liu
[off-list ref] wrote:

On Thu, Jan 14, 2016 at 06:58:31PM +0530, Santosh Shukla wrote:

quoted

So far virtio handle rw access for uio / ioport interface, This patch to extend
the support for vfio interface. For that introducing private struct
virtio_vfio_dev{
      - is_vfio
      - pci_dev
      };
Signed-off-by: Santosh Shukla <redacted>

...

quoted

+/* For vfio only */
+struct virtio_vfio_dev {
+     bool            is_vfio;        /* True: vfio i/f,
+                                      * False: not a vfio i/f

Well, this is weird; you are adding a flag to tell whether it's a
vfio device __inside__ a vfio struct.

Back to the topic, this flag is not necessary to me: you can
check the pci_dev->kdrv flag.

yes, I'll replace is_vfio with pci_dev->kdrv.

quoted

+                                      */
+     struct rte_pci_device *pci_dev; /* vfio dev */

Note that I have already added this field into virtio_hw struct
at my latest virtio 1.0 pmd patchset.

While I told you before that you should not develop patches based
on my patcheset, I guess you can do that now. Since it should be
in good shape and close to be merged.

Okay, Before rebasing my v5 patch on your 1.0 virtio patch, I like to
understand which qemu version support virtio 1.0 spec?

quoted

+};
+
 struct virtio_hw {
      struct virtqueue *cvq;
      uint32_t    io_base;

@@ -176,6 +186,7 @@ struct virtio_hw {
      uint8_t     use_msix;
      uint8_t     started;
      uint8_t     mac_addr[ETHER_ADDR_LEN];
+     struct virtio_vfio_dev dev;
 };

 /*

@@ -231,20 +242,65 @@ outl_p(unsigned int data, unsigned int port)
 #define VIRTIO_PCI_REG_ADDR(hw, reg) \
      (unsigned short)((hw)->io_base + (reg))

-#define VIRTIO_READ_REG_1(hw, reg) \
-     inb((VIRTIO_PCI_REG_ADDR((hw), (reg))))
-#define VIRTIO_WRITE_REG_1(hw, reg, value) \
-     outb_p((unsigned char)(value), (VIRTIO_PCI_REG_ADDR((hw), (reg))))
-
-#define VIRTIO_READ_REG_2(hw, reg) \
-     inw((VIRTIO_PCI_REG_ADDR((hw), (reg))))
-#define VIRTIO_WRITE_REG_2(hw, reg, value) \
-     outw_p((unsigned short)(value), (VIRTIO_PCI_REG_ADDR((hw), (reg))))
-
-#define VIRTIO_READ_REG_4(hw, reg) \
-     inl((VIRTIO_PCI_REG_ADDR((hw), (reg))))
-#define VIRTIO_WRITE_REG_4(hw, reg, value) \
-     outl_p((unsigned int)(value), (VIRTIO_PCI_REG_ADDR((hw), (reg))))
+#define VIRTIO_READ_REG_1(hw, reg)                                   \
+({                                                                   \
+     uint8_t ret;                                                    \
+     struct virtio_vfio_dev *vdev;                                   \
+     (vdev) = (&(hw)->dev);                                          \
+     (((vdev)->is_vfio) ?                                            \
+     (ioport_inb(((vdev)->pci_dev), reg, &ret)) :                    \
+     ((ret) = (inb((VIRTIO_PCI_REG_ADDR((hw), (reg)))))));           \
+     ret;                                                            \
+})

It becomes unreadable. I'd suggest to define them as iniline
functions, and use "if .. else .." instead of "?:".

        --yliu