Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Santosh Shukla <sshukla@mvista.com>]

On Thu, Jan 21, 2016 at 8:16 PM, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
> 2016-01-21 17:34, Santosh Shukla:
>> On Thu, Jan 21, 2016 at 4:58 PM, Thomas Monjalon
>> <thomas.monjalon@6wind.com> wrote:
>> > 2016-01-21 16:43, Santosh Shukla:
>> >> David Marchand <david.marchand@6wind.com> wrote:
>> >> > This is a mode (specific to vfio), not a new kernel driver.
>> >> >
>> >> Yes, Specific to VFIO and this is why noiommu appended after vfio i.e..
>> >> __VFIO and __VFIO_NOIOMMU.
>> >
>> > Woaaa! Your logic is really disappointing :)
>> > Specific to VFIO => append _NOIOMMU
>> > If it's for VFIO, it should be called VFIO (that's my logic).
>> >
>> I am confused by reading your comment. vfio works for default iommu
>> and now with noiommu. drv->kdrv need to know driver mode for vfio
>> case. So that user can simply read drv->kdrv value in their driver and
>> accordingly use vfio rd/wr api for example {pread/pwrite}. This is how
>> rte_eal_pci_vfio_read/write_bar() api implemented.
>
> Sorry I don't understand. Why EAL read/write functions should be different
> depending of the VFIO mode?
>

no, EAL rd/wr functions are not different for vfio or vfio modes {same
for iommu or noiommu}. Pl. see pci_eal_read/write_bar() api. Those
apis currently used for VFIO, Irrespective of vfio mode. If required,
we can add UIO bar_rd/wr api too. pci_eal_rd/wr_bar() are abstract
apis. Underneath implementation can be vfio or uio type.

>> And Yes it is called VFIO but with with specifics appended in it.
>>
>> >> > How come we need to distinguish between with/without iommu modes ?
>> >>
>> >> By default vfio framework assumes iommu i.,e., iommu present. Unless user
>> >> explicitly set "enable_unsafe_noiommu_mode" param. so in my opinion, we
>> >> care to parse vfio driver for _noiommu_ mode only.
>> >
>> > Why do we care to parse noiommu only?
>>
>> Because pmd drivers example virtio can work with vfio only in
>> _noiommu_ mode. In particular, virtio spec 0.95 / legacy virtio.
>
> Please could you explain the limitation (except IOMMU availability)?
>

Ok.

I believe - we both agree that noiommu mode is a need for pmd drivers
like virtio, right? if so then other reason is implementation driven
i.e..

Pl. look at virtio_pci.c in this patch.. VIRTIO_RD/WR/_1/2/4()
implementation. They are in-use and applicable to  virtio spec 0.95,
so far support uio/ioport-way rd/wr. Now to support vfio-way rd/wr -
need to check drv->kdrv value, that value should be of vfio_noiommu
types __not__  generic _vfio types.

>> So at
>> the initialization (example .. virtio-net) of such pmd driver, pmd
>> driver should know that vfio-with-noiommu mode enabled or not? for
>> that pmd driver simply checks drv->kdrv value.
>
> If a check is needed, I would prefer using your function
> pci_vfio_is_noiommu() and remove driver modes from struct rte_kernel_driver.
>

I don't think calling pci_vfio_no_iommu() inside
virtio_reg_rd/wr_1/2/3() would be a good idea.

>> Currently virtio-net
>> pmd driver does resource parsing then resource init for interfaces
>> like UIO/ioport, I intend to do same but only parsing, as resource
>> init for vfio case already taken care by pci_xx_vfio_map() api in
>> virtio-net pmd driver (refer Yaun recently virtio 1.0 recently
>> submitted rte_eal_pci_map patch)
>>
>> Also Yuan in one of earlier thread inclined to remove all the resource
>> parsing api from virtio-net pmd driver. Pl. refer this thread [1]
>>
>> [1] http://dpdk.org/dev/patchwork/patch/9862/
>
> Yes he said
> "we should try to avoid getting UIO/VFIO stuff inside virtio pmd driver".
> I agree we must try to have those abstractions in EAL.

Right,

IMO vfio now has that abstraction in place and pmd driver only to
check driver type, which virtio-net pmd doing for vfio case. `

> The only exception seems to be the switch ioport/PCI bar to read/write
> in virtio.
>

Right, and only applicable for x86 arch, wont work for non-x86 archs.

>> > Even if virtio cannot work in an IOMMU case, there is no reason to add
>> > a VFIO_NOIOMMU type here.
>> >
>> >> > Should not vfio behave the same way from an api point of view ?
>> >> >
>> >> Yes It should. vfio gives similar file_ops i.e.. read/write/mmap/seek etc..
>> >> I am little confused on your question, do you see any issue in vfio bar
>> >> rd/wr api implementation?
>> >
>> > I think you should just consider the VFIO API and let the noiommu option
>> > as a kernel configuration detail.
>>
>> vfio apis _are_ considered at low level rd/wr implementation, Has
>> nothing to do with iommu/noiommu mode. See pci_vfio_read/write_bar()
>> implementation, they are using pread64/pwrite64() vfio rd/wr api.
>
> So you agree that the VFIO API abstract the iommu availability details?
>

didn't understood your question,
>