Re: [dpdk-dev] [PATCH v2 3/5] bus/pci: use IOVAs check when setting IOVA mode

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 04-Oct-18 2:35 PM, Alejandro Lucero wrote:
> 
> 
> On Wed, Oct 3, 2018 at 1:56 PM Burakov, Anatoly 
> <anatoly.burakov@intel.com <mailto:anatoly.burakov@intel.com>> wrote:
> 
>     On 31-Aug-18 1:50 PM, Alejandro Lucero wrote:
>      > Although VT-d emulation currently only supports 39 bits, it could
>      > be iovas being within that supported range. This patch allows
>      > IOVA mode in such a case.
>      >
>      > Indeed, memory initialization code can be modified for using lower
>      > virtual addresses than those used by the kernel for 64 bits processes
>      > by default, and therefore memsegs iovas can use 39 bits or less for
>      > most system. And this is likely 100% true for VMs.
>      >
>      > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com
>     <mailto:alejandro.lucero@netronome.com>>
>      > ---
>      >   drivers/bus/pci/linux/pci.c | 15 ++++++++++-----
>      >   1 file changed, 10 insertions(+), 5 deletions(-)
>      >
>      > diff --git a/drivers/bus/pci/linux/pci.c
>     b/drivers/bus/pci/linux/pci.c
>      > index 04648ac..215dc10 100644
>      > --- a/drivers/bus/pci/linux/pci.c
>      > +++ b/drivers/bus/pci/linux/pci.c
>      > @@ -588,10 +588,11 @@
>      >       fclose(fp);
>      >
>      >       mgaw = ((vtd_cap_reg & VTD_CAP_MGAW_MASK) >>
>     VTD_CAP_MGAW_SHIFT) + 1;
>      > -     if (mgaw < X86_VA_WIDTH)
>      > -             return false;
>      >
>      > -     return true;
>      > +     if (!rte_eal_check_dma_mask(mgaw))
>      > +             return true;
>      > +     else
>      > +             return false;
> 
>     return rte_eal_check_dma_mask(mgaw) == 0; ?
> 
> 
> I guess that works and is more elegant.
> Thanks.
> 
> 
>      >   }
>      >   #elif defined(RTE_ARCH_PPC_64)
>      >   static bool
>      > @@ -615,13 +616,17 @@
>      >   {
>      >       struct rte_pci_device *dev = NULL;
>      >       struct rte_pci_driver *drv = NULL;
>      > +     int iommu_dma_mask_check_done = 0;
>      >
>      >       FOREACH_DRIVER_ON_PCIBUS(drv) {
>      >               FOREACH_DEVICE_ON_PCIBUS(dev) {
>      >                       if (!rte_pci_match(drv, dev))
>      >                               continue;
>      > -                     if (!pci_one_device_iommu_support_va(dev))
>      > -                             return false;
>      > +                     if (!iommu_dma_mask_check_done) {
>      > +                             if
>     (!pci_one_device_iommu_support_va(dev))
>      > +                                     return false;
>      > +                             iommu_dma_mask_check_done  = 1;
>      > +                     }
>      >               }
> 
>     The commit message doesn't explain why are we only checking a single
>     device. Indeed, i am not 100% clear as to why, so some explanation in
>     the commit message and preferably a comment in code would be more than
>     welcome :)
> 
> 
> Because the pci_one_device_iommu_support_va function does always the 
> same whatever the device is used in the call.

So, this code was always wrong and needlessly checked each device when 
it could've checked it a single time? OK, that makes it a bit clearer. 
Still, needs to be documented in comments/commit message :) The commit 
message IMO looks quite irrelevant to what happens in the commit. It 
almost feels like this commit should be split in two - first change the 
mgaw check, and then fix the PCI bus code to not check needlessly.

-- 
Thanks,
Anatoly