Re: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev args

[Thomas Monjalon <thomas@monjalon.net>]

16/07/2018 13:05, Pavan Nikhilesh:
> On Mon, Jul 16, 2018 at 12:25:29AM +0200, Thomas Monjalon wrote:
> > External Email
> >
> > 10/07/2018 12:19, Pavan Nikhilesh:
> > > Hi Gaëtan,Ferruh,
> > >
> > > On Wed, Jun 27, 2018 at 11:57:36AM +0200, Gaëtan Rivet wrote:
> > > > On Wed, Jun 27, 2018 at 02:25:30PM +0530, Pavan Nikhilesh wrote:
> > > > > Hi Gaëtan,
> > > > >
> > > > > On Wed, Jun 27, 2018 at 10:39:59AM +0200, Gaëtan Rivet wrote:
> > > > > > Hi Ferruh, Pavan,
> > > > > >
> > > > > > sorry for the delay,
> > > > > >
> > > > > > On Tue, Jun 26, 2018 at 04:40:21PM +0100, Ferruh Yigit wrote:
> > > > > > > On 6/26/2018 1:48 PM, Shahaf Shuler wrote:
> > > > > > > > Hi Pavan,
> > > > > > > >
> > > > > > > > Friday, June 15, 2018 7:44 AM, Pavan Nikhilesh:
> > > > > > > >> Subject: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev
> > > > > > > >> args
> > > > > > > >>
> > > > > > > >> Currently, the only way of supplying device argument to a pci device is to
> > > > > > > >> whitelist it i.e. -w 000X:00:0X.0,self_test=1. This is not a very feasible method
> > > > > > > >> as whitelisting a device has its own side effects i.e only the whitelisted pci
> > > > > > > >> devices are probed.
> > > > > > > >>
> > > > > > > >> Add a new eal command line option --pci-args to pass device args without the
> > > > > > > >> need to whitelist the devices.
> > > > > > > >>            --pci-args 000X:00:0X.0,self_test=1
> > > > > > > >>
> > > > > > > >> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > > > > > > >
> > > > > > > > Tested-by: Shahaf Shuler <shahafs@mellanox.com>
> > > > > > > >
> > > > > > > > It seems to work.
> > > > > > > > Please see small comments below
> > > > > > >
> > > > > > > Isn't this conflict with Gaetan's devarg work which has wider scope?
> > > > > > >
> > > > > >
> > > > > > Indeed it does.
> > > > > >
> > > > > > Pavan, I have submitted a new version of a series adding generic kvargs
> > > > > > to several layers (bus, class, driver).
> > > > > >
> > > > > > It does cover this exact use-case.
> > > > > >
> > > > > > However, while writing it, I wasn't able to find PCI bus specific
> > > > > > parameters, that could showcase the functionality.
> > > > >
> > > > > The idea of the patch is to avoid whitelising a device when we want to
> > > > > supply kvargs to it, I tried mapping it to devargs rework patchset but couldn't
> > > > > do it at a glance. For example, the following patch[1] reads kvargs through
> > > > > whitelisting which should be avoided.
> > > > >
> > > > > [1]http://patches.dpdk.org/patch/41223/
> > > > >
> > > >
> > > > I see.
> > > >
> > > > Actually, your use-case won't be covered by the devargs rework.
> > > >
> > > > I am still dumbfounded by how this blacklist/whitelist mode stuff is
> > > > kept against all odds. But that's not the time to deal with it.
> > > >
> > > > The issue is that the two features "declaring a device" and
> > > > "configuring a bus" are currently awkwardly merged. You are piling stuff
> > > > on the "declaring a device" part to enhance the "configuring a bus"
> > > > feature.
> > >
> > > The feature is very much needed to avoid polluting the cmdline args when we are
> > > trying to configure a device at probe (for now).
> > >
> > > >
> > > > Instead of going this way, I would advise to separate the two features.
> > > >
> > > > If buses could be configured with a generic EAL option
> > > > "--blacklist=pci,vdev" for example, then you could provide devargs as
> > > > much as you want, the buses themselves would stay properly configured.
> > > >
> > > > This means removing devargs policy, device types and rewriting bus logic
> > > > about it.
> > >
> > > I think this can be done as a future work and is not in the scope of this
> > > patch.
> > >
> > > @Ferruh,
> > > As Gaëtan mentioned this patch is not related to devargs rework can we make
> > > some forward progress.
> >
> > No, we should not add a new parameter just to fix one use case for one bus.
> > The work of Gaetan is opening the door to a generic syntax which can be
> > used for device matching (like for whitelisting), or for settings
> > (what you need) of any bus, any device class or any driver.
> > We can discuss about which option to add for generic device settings,
> > and whether or not it should be mixed with whitelisting,
> > but please let's work on a generic solution.
> 
> Ok, I guess this can be taken up once Gaëtan devarsg patches are completely
> merged. If we split the work into a smaller list we could load balance the
> work and work towards 18.11?.

Yes, we must split and share the work.
Some patches from Gaetan are in 18.08 in order to provide some parsing helpers.
What we must do next:
- implement identification by bus properties
- implement identification by device class properties
- choose how we want to manage whitelist/blacklist
- choose syntax to set some properties (to be used by API or command line or config file)