Re: [dpdk-dev] [PATCH v2 2/4] eal: fix IOVA mode selection as VA for pci drivers

[Jerin Jacob Kollanukkaran <jerinj@marvell.com>]

> 
> On 16-Jul-19 2:46 PM, jerinj@marvell.com wrote:
> > From: David Marchand <david.marchand@redhat.com>
> >
> > The incriminated commit broke the use of RTE_PCI_DRV_IOVA_AS_VA
> which
> > was intended to mean "driver only supports VA" but had been understood
> > as "driver supports both PA and VA" by most net drivers and used to
> > let dpdk processes to run as non root (which do not have access to
> > physical addresses on recent kernels).
> >
> > The check on physical addresses actually closed the gap for those
> > drivers. We don't need to mark them with RTE_PCI_DRV_IOVA_AS_VA and
> > this flag can retain its intended meaning.
> > Document explicitly its meaning.
> >
> > We can check that a driver requirement wrt to IOVA mode is fulfilled
> > before trying to probe a device.
> >
> > Finally, document the heuristic used to select the IOVA mode and hope
> > that we won't break it again.
> >
> > Fixes: 703458e19c16 ("bus/pci: consider only usable devices for IOVA
> > mode")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > Reviewed-by: Jerin Jacob <jerinj@marvell.com>
> > Tested-by: Jerin Jacob <jerinj@marvell.com>
> > ---
> 
> <snip>
> 
> > @@ -629,12 +643,16 @@ rte_pci_get_iommu_class(void)
> >   				devices_want_va = true;
> >   		}
> >   	}
> > -	if (devices_want_pa) {
> > -		iova_mode = RTE_IOVA_PA;
> > -		if (devices_want_va)
> > -			RTE_LOG(WARNING, EAL, "Some devices want 'VA'
> but forcing 'PA' because other devices want it\n");
> > -	} else if (devices_want_va) {
> > +	if (devices_want_va && !devices_want_pa) {
> >   		iova_mode = RTE_IOVA_VA;
> > +	} else if (devices_want_pa && !devices_want_va) {
> > +		iova_mode = RTE_IOVA_PA;
> > +	} else {
> > +		iova_mode = RTE_IOVA_DC;
> > +		if (devices_want_va) {
> > +			RTE_LOG(WARNING, EAL, "Some devices want 'VA'
> but forcing 'DC' because other devices want 'PA'.\n");
> > +			RTE_LOG(WARNING, EAL, "Depending on the final
> decision by the EAL,
> > +part of your devices won't initialise.\n");
> 
> Tiny nitpick - i generally don't like personal appeals in log messages, so
> perhaps drop the "your"? I.e. "Depending on final decision by EAL, not all
> devices may be able to initialize."? Same applies to the other instance of this
> error message.

I will fix it in v3.	


> 
> Otherwise, LGTM
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> --
> Thanks,
> Anatoly