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

[Santosh Shukla <sshukla@mvista.com>]

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