On Fri, Jan 15, 2016 at 11:57 AM, Yuanhan Liu <yuanhan.liu at linux.intel.com> wrote: > On Thu, Jan 14, 2016 at 06:58:31PM +0530, Santosh Shukla wrote: >> 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 <sshukla at mvista.com> > ... >> +/* 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. >> + */ >> + 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? > >> +}; >> + >> 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