Re: [dpdk-dev] [PATCH v2 3/3] bus/pci: only consider usable devices to select IOVA mode

[David Marchand <david.marchand@redhat.com>]

On Wed, Jul 3, 2019 at 12:45 PM Burakov, Anatoly <anatoly.burakov@intel.com>
wrote:

> On 14-Jun-19 10:39 AM, David Marchand wrote:
> > From: Ben Walker <benjamin.walker@intel.com>
> >
> > When selecting the preferred IOVA mode of the pci bus, the current
> > heuristic ("are devices bound?", "are devices bound to UIO?", "are pmd
> > drivers supporting IOVA as VA?" etc..) should honor the device
> > white/blacklist so that an unwanted device does not impact the decision.
> >
> > There is no reason to consider a device which has no driver available.
> >
> > This applies to all OS, so implements this in common code then call a
> > OS specific callback.
> >
> > On Linux side:
> > - the VFIO special considerations should be evaluated only if VFIO
> >    support is built,
> > - there is no strong requirement on using VA rather than PA if a driver
> >    supports VA, so defaulting to DC in such a case.
> >
> > Signed-off-by: Ben Walker <benjamin.walker@intel.com>
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
>
> <snip>
>
> > +                  const struct rte_pci_device *pdev)
> >   {
> > -     struct rte_pci_device *dev = NULL;
> > -     struct rte_pci_driver *drv = NULL;
> > +     enum rte_iova_mode iova_mode = RTE_IOVA_DC;
> > +     static int iommu_no_va = -1;
> >
> > -     FOREACH_DRIVER_ON_PCIBUS(drv) {
> > -             FOREACH_DEVICE_ON_PCIBUS(dev) {
> > -                     if (!rte_pci_match(drv, dev))
> > -                             continue;
> > -                     /*
> > -                      * just one PCI device needs to be checked out
> because
> > -                      * the IOMMU hardware is the same for all of them.
> > -                      */
> > -                     return pci_one_device_iommu_support_va(dev);
> > +     switch (pdev->kdrv) {
> > +     case RTE_KDRV_VFIO: {
> > +#ifdef VFIO_PRESENT
> > +             static int is_vfio_noiommu_enabled = -1;
> > +
> > +             if (is_vfio_noiommu_enabled == -1) {
> > +                     if (rte_vfio_noiommu_is_enabled() == 1)
> > +                             is_vfio_noiommu_enabled = 1;
> > +                     else
> > +                             is_vfio_noiommu_enabled = 0;
> > +             }
> > +             if ((pdrv->drv_flags & RTE_PCI_DRV_IOVA_AS_VA) == 0) {
> > +                     iova_mode = RTE_IOVA_PA;
> > +             } else if (is_vfio_noiommu_enabled != 0) {
> > +                     RTE_LOG(DEBUG, EAL, "Forcing to 'PA', vfio-noiommu
> mode configured\n");
> > +                     iova_mode = RTE_IOVA_PA;
> >               }
> > +#endif
> > +             break;
>
> I'm not too well-versed in bus code, so please excuse my ignorance of
> this codebase.
>
> It seems that we would be ignoring drv_flags in case VFIO wasn't
> compiled - if the driver has no RTE_PCI_DRV_IOVA_AS_VA flag, i'm pretty
> sure we can set IOVA mode to PA without caring about VFIO at all. I
> think it would be better to have something like this:
>
> if ((pdrv->drv_flags & RTE_PCI_DRV_IOVA_AS_VA) == 0) {
>         iova_mode = RTE_IOVA_PA;
>         break; // early exit
> }
>

If the device is bound to VFIO, but the dpdk binary has no vfio support, we
don't need to consider this device in the decision.
Did I miss something in what you suggest?


-- 
David Marchand