[dpdk-dev,v4,08/14] virtio: pci: extend virtio pci rw api for vfio interface

  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@mvista.com>
---
v3->v4:
- Removed #indef RTE_EAL_VFIO and made it arch agnostic such now virtio_pci
  rd/wr api to handle both vfio and ig_uio/ioport interfaces, depending upon
  is_vfio flags set or unset.
- Tested for x86 for igb_uio and vfio interface, also  tested for arm64 for vfio
  interface.

drivers/net/virtio/virtio_pci.h |   84 ++++++++++++++++++++++++++++++++-------
 1 file changed, 70 insertions(+), 14 deletions(-)
  

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@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.

> +					 */
> +	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.

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

On Fri, Jan 15, 2016 at 11:57 AM, Yuanhan Liu
<yuanhan.liu@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@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
  

On Fri, Jan 15, 2016 at 6:13 PM, Santosh Shukla <sshukla@mvista.com> wrote:
> On Fri, Jan 15, 2016 at 11:57 AM, Yuanhan Liu
> <yuanhan.liu@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@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?

Ignore, I figured out in other thread,
qemu version >2.4, such as 2.4.1 or 2.5.0.

>>> +};
>>> +
>>>  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 "?:".
>>

Ok.
>>         --yliu
  

On Fri, Jan 15, 2016 at 07:12:04PM +0530, Santosh Shukla wrote:
> On Fri, Jan 15, 2016 at 6:13 PM, Santosh Shukla <sshukla@mvista.com> wrote:
> > On Fri, Jan 15, 2016 at 11:57 AM, Yuanhan Liu
> > <yuanhan.liu@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@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?
> 
> Ignore, I figured out in other thread,
> qemu version >2.4, such as 2.4.1 or 2.5.0.

It will not matter. You can continue using the old legacy virtio, which
is the default case: my patchset keeps the backward compatibility.

What's worty noting is that virtio 1.0 uses memory mmaped bar space for
configuration, instead of ioport reading/writing. Therefore, I'd suggest
you to keep testing with legacy virtio, to make sure the VFIO stuff works.
And off course, virtio 1.0 testing is also welcome, to make sure it works
on ARM as well.

	--yliu
  

On Mon, Jan 18, 2016 at 11:41 AM, Yuanhan Liu
<yuanhan.liu@linux.intel.com> wrote:
> On Fri, Jan 15, 2016 at 07:12:04PM +0530, Santosh Shukla wrote:
>> On Fri, Jan 15, 2016 at 6:13 PM, Santosh Shukla <sshukla@mvista.com> wrote:
>> > On Fri, Jan 15, 2016 at 11:57 AM, Yuanhan Liu
>> > <yuanhan.liu@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@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?
>>
>> Ignore, I figured out in other thread,
>> qemu version >2.4, such as 2.4.1 or 2.5.0.
>
> It will not matter. You can continue using the old legacy virtio, which
> is the default case: my patchset keeps the backward compatibility.
>
> What's worty noting is that virtio 1.0 uses memory mmaped bar space for
> configuration, instead of ioport reading/writing. Therefore, I'd suggest
> you to keep testing with legacy virtio, to make sure the VFIO stuff works.
> And off course, virtio 1.0 testing is also welcome, to make sure it works
> on ARM as well.
>

I am testing for virtio 1.0 and 0.95 for arm including your patch,
soon we;ll post the patch series that is rebased on / dependent on
below patchset:
- virtio 1.0
- vfio-noiommu
- KDRV check by huawei

IMO, we should start merging the dependent patches as because I'll
have to rebase, then do regression across the platform at least for
x86/arm64 and it's quite a work now.

Beside that I have few question specific to vfio in virtio pmd driver;
- vfio don't need resource_init functionality as it uses struct
rte_pci_dev but it need parsing so to make sure
    1. user has setted no_iommu mode
    2. virtio pci device attached to vfio-no-iommu driver or not.

So for 1) I am thinking to add RTE_KDRV_VFIO_NOIOMMU mode and a helper
function like pci_vfio_is_iommu(), such that  pc_xxx_scan() function
updates dev->kdrv with RTE_KDRV_VFIO_NOIOMMU at driver probe time.

case 2) would check for _noiommu mode and then would verify that
driver is attached or not?

above two case applicable to both virtio spec 1.0 and 0.95. I have
done changes for those two case for v5 patch series,l any comment
welcome before I push patch for review.

Thanks.

>         --yliu
  

On 01/14/2016 09:28 PM, 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@mvista.com>
> ---
> v3->v4:
> - Removed #indef RTE_EAL_VFIO and made it arch agnostic such now virtio_pci
>   rd/wr api to handle both vfio and ig_uio/ioport interfaces, depending upon
>   is_vfio flags set or unset.
> - Tested for x86 for igb_uio and vfio interface, also  tested for arm64 for vfio
>   interface.
>
> drivers/net/virtio/virtio_pci.h |   84 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 70 insertions(+), 14 deletions(-)

Interesting. I'm working on IOMMU cooperation with virtio[1]. For pmd,
it looks like the only thing missed is RTE_PCI_DRV_NEED_MAPPING for
virito-net pmd.

So I'm curious whether this is still necessary if IOMMU can work with
virito in the future.

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg337079.html

Thanks

>
> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
> index 8b5b031..8526c07 100644
> --- a/drivers/net/virtio/virtio_pci.h
> +++ b/drivers/net/virtio/virtio_pci.h
> @@ -46,6 +46,8 @@
>  #endif
>  
>  #include <rte_ethdev.h>
> +#include <stdbool.h>
> +#include "virtio_vfio_rw.h"
>  
>  struct virtqueue;
>  
> @@ -165,6 +167,14 @@ struct virtqueue;
>   */
>  #define VIRTIO_MAX_VIRTQUEUES 8
>  
> +/* For vfio only */
> +struct virtio_vfio_dev {
> +	bool		is_vfio;	/* True: vfio i/f,
> +					 * False: not a vfio i/f
> +					 */
> +	struct rte_pci_device *pci_dev; /* vfio dev */
> +};
> +
>  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;								\
> +})
> +
> +#define VIRTIO_WRITE_REG_1(hw, reg, value)				\
> +({									\
> +	struct virtio_vfio_dev *vdev;					\
> +	(vdev) = (&(hw)->dev);						\
> +	(((vdev)->is_vfio) ?						\
> +	(ioport_outb_p(((vdev)->pci_dev), reg, (uint8_t)(value))) :	\
> +	(outb_p((unsigned char)(value), (VIRTIO_PCI_REG_ADDR((hw), (reg)))))); \
> +})
> +
> +#define VIRTIO_READ_REG_2(hw, reg)					\
> +({									\
> +	uint16_t ret;							\
> +	struct virtio_vfio_dev *vdev;					\
> +	(vdev) = (&(hw)->dev);						\
> +	(((vdev)->is_vfio) ?						\
> +	(ioport_inw(((vdev)->pci_dev), reg, &ret)) :			\
> +	((ret) = (inw((VIRTIO_PCI_REG_ADDR((hw), (reg)))))));		\
> +	ret;								\
> +})
> +
> +#define VIRTIO_WRITE_REG_2(hw, reg, value)				\
> +({									\
> +	struct virtio_vfio_dev *vdev;					\
> +	(vdev) = (&(hw)->dev);						\
> +	(((vdev)->is_vfio) ?						\
> +	(ioport_outw_p(((vdev)->pci_dev), reg, (uint16_t)(value))) :	\
> +	(outw_p((unsigned short)(value), (VIRTIO_PCI_REG_ADDR((hw), (reg)))))); \
> +})
> +
> +#define VIRTIO_READ_REG_4(hw, reg)					\
> +({									\
> +	uint32_t ret;							\
> +	struct virtio_vfio_dev *vdev;					\
> +	(vdev) = (&(hw)->dev);						\
> +	(((vdev)->is_vfio) ?						\
> +	(ioport_inl(((vdev)->pci_dev), reg, &ret)) :			\
> +	((ret) = (inl((VIRTIO_PCI_REG_ADDR((hw), (reg)))))));		\
> +	ret;								\
> +})
> +
> +#define VIRTIO_WRITE_REG_4(hw, reg, value)				\
> +({									\
> +	struct virtio_vfio_dev *vdev;					\
> +	(vdev) = (&(hw)->dev);						\
> +	(((vdev)->is_vfio) ?						\
> +	(ioport_outl_p(((vdev)->pci_dev), reg, (uint32_t)(value))) :	\
> +	(outl_p((unsigned int)(value), (VIRTIO_PCI_REG_ADDR((hw), (reg)))))); \
> +})
>  
>  static inline int
>  vtpci_with_feature(struct virtio_hw *hw, uint32_t bit)
  

On Mon, Jan 18, 2016 at 12:15:40PM +0530, Santosh Shukla wrote:
> I am testing for virtio 1.0 and 0.95 for arm including your patch,
> soon we;ll post the patch series that is rebased on / dependent on
> below patchset:
> - virtio 1.0
> - vfio-noiommu
> - KDRV check by huawei
> 
> IMO, we should start merging the dependent patches as because I'll

Yep, agreed. That's why I was keep pushing Huawei for ACK and
validation team for testing, although I have tested that. :)

> have to rebase, then do regression across the platform at least for
> x86/arm64 and it's quite a work now.
> 
> Beside that I have few question specific to vfio in virtio pmd driver;
> - vfio don't need resource_init functionality as it uses struct
> rte_pci_dev but it need parsing so to make sure
>     1. user has setted no_iommu mode
>     2. virtio pci device attached to vfio-no-iommu driver or not.
> 
> So for 1) I am thinking to add RTE_KDRV_VFIO_NOIOMMU mode and a helper
> function like pci_vfio_is_iommu(), such that  pc_xxx_scan() function
> updates dev->kdrv with RTE_KDRV_VFIO_NOIOMMU at driver probe time.

That sounds better to me. And that's also what I want to comment on
your another patch [09/14], that we should try to avoid getting UIO/VFIO
stuff inside virtio pmd driver, unless it's a must. (yes, I know
UIO is already an example here, but I don't like it, and badly, I don't
have the time to check if I can remove it.)

> 
> case 2) would check for _noiommu mode and then would verify that
> driver is attached or not?

Sorry, very limited VFIO and noiommu knowledge, and I can't answer, so
far.

	--yliu
> 
> above two case applicable to both virtio spec 1.0 and 0.95. I have
> done changes for those two case for v5 patch series,l any comment
> welcome before I push patch for review.
> 
> Thanks.
  

On Mon, Jan 18, 2016 at 12:29 PM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 01/14/2016 09:28 PM, 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@mvista.com>
>> ---
>> v3->v4:
>> - Removed #indef RTE_EAL_VFIO and made it arch agnostic such now virtio_pci
>>   rd/wr api to handle both vfio and ig_uio/ioport interfaces, depending upon
>>   is_vfio flags set or unset.
>> - Tested for x86 for igb_uio and vfio interface, also  tested for arm64 for vfio
>>   interface.
>>
>> drivers/net/virtio/virtio_pci.h |   84 ++++++++++++++++++++++++++++++++-------
>>  1 file changed, 70 insertions(+), 14 deletions(-)
>
> Interesting. I'm working on IOMMU cooperation with virtio[1]. For pmd,
> it looks like the only thing missed is RTE_PCI_DRV_NEED_MAPPING for
> virito-net pmd.
>

not missing anymore, I am using rte_eal_pci_map() api so to map virtio
pci dev to userspace.

> So I'm curious whether this is still necessary if IOMMU can work with
> virito in the future.
>
So far I'm using pmd driver in vfio-noiommu way. AFAIk, pmd driver use
dma for tx side. I glanced through your patch, to me it would be
interesting to try out, but right now I am not sure. We'll come back I
guess. Thanks

> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg337079.html
>
> Thanks
>
>>
>> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
>> index 8b5b031..8526c07 100644
>> --- a/drivers/net/virtio/virtio_pci.h
>> +++ b/drivers/net/virtio/virtio_pci.h
>> @@ -46,6 +46,8 @@
>>  #endif
>>
>>  #include <rte_ethdev.h>
>> +#include <stdbool.h>
>> +#include "virtio_vfio_rw.h"
>>
>>  struct virtqueue;
>>
>> @@ -165,6 +167,14 @@ struct virtqueue;
>>   */
>>  #define VIRTIO_MAX_VIRTQUEUES 8
>>
>> +/* For vfio only */
>> +struct virtio_vfio_dev {
>> +     bool            is_vfio;        /* True: vfio i/f,
>> +                                      * False: not a vfio i/f
>> +                                      */
>> +     struct rte_pci_device *pci_dev; /* vfio dev */
>> +};
>> +
>>  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;                                                            \
>> +})
>> +
>> +#define VIRTIO_WRITE_REG_1(hw, reg, value)                           \
>> +({                                                                   \
>> +     struct virtio_vfio_dev *vdev;                                   \
>> +     (vdev) = (&(hw)->dev);                                          \
>> +     (((vdev)->is_vfio) ?                                            \
>> +     (ioport_outb_p(((vdev)->pci_dev), reg, (uint8_t)(value))) :     \
>> +     (outb_p((unsigned char)(value), (VIRTIO_PCI_REG_ADDR((hw), (reg)))))); \
>> +})
>> +
>> +#define VIRTIO_READ_REG_2(hw, reg)                                   \
>> +({                                                                   \
>> +     uint16_t ret;                                                   \
>> +     struct virtio_vfio_dev *vdev;                                   \
>> +     (vdev) = (&(hw)->dev);                                          \
>> +     (((vdev)->is_vfio) ?                                            \
>> +     (ioport_inw(((vdev)->pci_dev), reg, &ret)) :                    \
>> +     ((ret) = (inw((VIRTIO_PCI_REG_ADDR((hw), (reg)))))));           \
>> +     ret;                                                            \
>> +})
>> +
>> +#define VIRTIO_WRITE_REG_2(hw, reg, value)                           \
>> +({                                                                   \
>> +     struct virtio_vfio_dev *vdev;                                   \
>> +     (vdev) = (&(hw)->dev);                                          \
>> +     (((vdev)->is_vfio) ?                                            \
>> +     (ioport_outw_p(((vdev)->pci_dev), reg, (uint16_t)(value))) :    \
>> +     (outw_p((unsigned short)(value), (VIRTIO_PCI_REG_ADDR((hw), (reg)))))); \
>> +})
>> +
>> +#define VIRTIO_READ_REG_4(hw, reg)                                   \
>> +({                                                                   \
>> +     uint32_t ret;                                                   \
>> +     struct virtio_vfio_dev *vdev;                                   \
>> +     (vdev) = (&(hw)->dev);                                          \
>> +     (((vdev)->is_vfio) ?                                            \
>> +     (ioport_inl(((vdev)->pci_dev), reg, &ret)) :                    \
>> +     ((ret) = (inl((VIRTIO_PCI_REG_ADDR((hw), (reg)))))));           \
>> +     ret;                                                            \
>> +})
>> +
>> +#define VIRTIO_WRITE_REG_4(hw, reg, value)                           \
>> +({                                                                   \
>> +     struct virtio_vfio_dev *vdev;                                   \
>> +     (vdev) = (&(hw)->dev);                                          \
>> +     (((vdev)->is_vfio) ?                                            \
>> +     (ioport_outl_p(((vdev)->pci_dev), reg, (uint32_t)(value))) :    \
>> +     (outl_p((unsigned int)(value), (VIRTIO_PCI_REG_ADDR((hw), (reg)))))); \
>> +})
>>
>>  static inline int
>>  vtpci_with_feature(struct virtio_hw *hw, uint32_t bit)
>
  

On Mon, Jan 18, 2016 at 12:47 PM, Yuanhan Liu
<yuanhan.liu@linux.intel.com> wrote:
> On Mon, Jan 18, 2016 at 12:15:40PM +0530, Santosh Shukla wrote:
>> I am testing for virtio 1.0 and 0.95 for arm including your patch,
>> soon we;ll post the patch series that is rebased on / dependent on
>> below patchset:
>> - virtio 1.0
>> - vfio-noiommu
>> - KDRV check by huawei
>>
>> IMO, we should start merging the dependent patches as because I'll
>
> Yep, agreed. That's why I was keep pushing Huawei for ACK and
> validation team for testing, although I have tested that. :)
>
>> have to rebase, then do regression across the platform at least for
>> x86/arm64 and it's quite a work now.
>>
>> Beside that I have few question specific to vfio in virtio pmd driver;
>> - vfio don't need resource_init functionality as it uses struct
>> rte_pci_dev but it need parsing so to make sure
>>     1. user has setted no_iommu mode
>>     2. virtio pci device attached to vfio-no-iommu driver or not.
>>
>> So for 1) I am thinking to add RTE_KDRV_VFIO_NOIOMMU mode and a helper
>> function like pci_vfio_is_iommu(), such that  pc_xxx_scan() function
>> updates dev->kdrv with RTE_KDRV_VFIO_NOIOMMU at driver probe time.
>
> That sounds better to me. And that's also what I want to comment on
> your another patch [09/14], that we should try to avoid getting UIO/VFIO
> stuff inside virtio pmd driver, unless it's a must. (yes, I know
> UIO is already an example here, but I don't like it, and badly, I don't
> have the time to check if I can remove it.)
>

Understood, So I'm moving possible required driver/parsing check in
eal_pci.c rather keeping it in virtio, as those parsing/driver
dependency checks are generic, has nothing to do with virtio.

>>
>> case 2) would check for _noiommu mode and then would verify that
>> driver is attached or not?
>
> Sorry, very limited VFIO and noiommu knowledge, and I can't answer, so
> far.
>
>         --yliu
>>
>> above two case applicable to both virtio spec 1.0 and 0.95. I have
>> done changes for those two case for v5 patch series,l any comment
>> welcome before I push patch for review.
>>
>> Thanks.