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

[Burakov, Anatoly]

On 04-Jul-19 10:18 AM, David Marchand wrote:
> 
> 
> On Wed, Jul 3, 2019 at 12:45 PM Burakov, Anatoly 
> <anatoly.burakov at intel.com <mailto:anatoly.burakov at intel.com>> wrote:
> 
>     On 14-Jun-19 10:39 AM, David Marchand wrote:
>      > From: Ben Walker <benjamin.walker at intel.com
>     <mailto:benjamin.walker at 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 at intel.com
>     <mailto:benjamin.walker at intel.com>>
>      > Signed-off-by: David Marchand <david.marchand at redhat.com
>     <mailto:david.marchand at 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?
> 

Yep, you're correct :)

Reviewed-by: Anatoly Burakov <anatoly.burakov at intel.com>

> 
> -- 
> David Marchand


-- 
Thanks,
Anatoly