Hi Gaetan
From: Gaëtan Rivet, Wednesday, January 17, 2018 12:31 AM
> Hi Matan,
>
> On Tue, Jan 16, 2018 at 05:20:27PM +0000, Matan Azrad wrote:
> > Hi Gaetan
> >
>
> <snip>
>
> > > > > In 18.05, or 18.08 there should be an EAL function that would be
> > > > > able to identify a device given a specific ID string (very close
> > > > > to an
> > > rte_devargs).
> > > > > Currently, this API does not exist.
> > > > >
> > > > > You can hack your way around this for the moment, IF you really,
> > > > > really
> > > > > want: parse your devargs, get the bus, use the bus->parse()
> > > > > function to get a binary device representation, and compare
> > > > > bytes per bytes the binary representation given by your devargs
> > > > > and by the device-
> > > >name.
> > > > >
> > > > > But this is a hack, and a pretty ugly one at that: you have no
> > > > > way of knowing the size taken by this binary representation, so
> > > > > you can restrict yourself to the vdev and PCI bus for the moment
> > > > > and take the larger of an rte_vdev_driver pointer and an
> rte_pci_addr....
> > > > >
> > > > > {
> > > > > union {
> > > > > rte_vdev_driver *drv;
> > > > > struct rte_pci_addr pci_addr;
> > > > > } bindev1, bindev2;
> > > > > memset(&bindev1, 0, sizeof(bindev1));
> > > > > memset(&bindev2, 0, sizeof(bindev2));
> > > > > rte_eal_devargs_parse(device->name, da1);
> > > > > rte_eal_devargs_parse(your_devstr, da2);
> > > > > RTE_ASSERT(da1->bus == rte_bus_find_by_name("pci") ||
> > > > > da1->bus == rte_bus_find_by_name("vdev"));
> > > > > RTE_ASSERT(da2->bus == rte_bus_find_by_name("pci") ||
> > > > > da2->bus == rte_bus_find_by_name("vdev"));
> > > > > da1->bus->parse(da1->name, &bindev1);
> > > > > da1->bus->parse(da2->name, &bindev2);
> > > > > if (memcmp(&bindev1, &bindev2, sizeof(bindev1)) == 0) {
> > > > > /* found the device */
> > > > > } else {
> > > > > /* not found */
> > > > > }
> > > > > }
> > > > >
> > > > > So, really, really ugly. Anyway.
> > > > >
> > > > Yes, ugly :) Thanks for this update!
> > > > Will keep the comparison by device->name.
> > > >
> > >
> > > Well as explained, above, the comparison by device->name only works
> > > with whitelisted devices.
> > >
> >
> > > So either implement something broken right now that you will need to
> > > update in 18.05, or implement it properly in 18.05 from the get go.
> > >
> > For the current needs it is enough.
> > We can also say that it is the user responsibility to pass to failsafe the
> > same
> names and same args as he passes for EAL(or default EAL names).
> > I think I emphasized it in documentation.
> >
>
> Okay, as you wish. Just be aware of this limitation.
>
> I think this functionality is good and useful, but it needs to be made clean.
> The proper function should be available soon, then this implementaion
> should be cleaned up.
Sure.
>
> > > > > <snip>
> > > > >
> > > > > > > > + /* Take control of device probed by EAL
> > > options. */
> > > > > > > > + DEBUG("Taking control of a probed sub
> > > device"
> > > > > > > > + " %d named %s", i, da->name);
> > > > > > >
> > > > > > > In this case, the devargs of the probed device must be
> > > > > > > copied within the sub- device definition and removed from
> > > > > > > the EAL using the proper rte_devargs API.
> > > > > > >
> > > > > > > Note that there is no rte_devargs copy function. You can use
> > > > > > > rte_devargs_parse instead, "parsing" again the original
> > > > > > > devargs into the sub- device one. It is necessary for
> > > > > > > complying with internal rte_devargs requirements (da->args
> > > > > > > being malloc-ed, at the moment,
> > > > > but may evolve).
> > > > > > >
> > > > > > > The rte_eal_devargs_parse function is not easy enough to use
> > > > > > > right now, you will have to build a devargs string (using
> > > > > > > snprintf) and
> > > submit it.
> > > > > > > I proposed a change this release for it but it will not make
> > > > > > > it for 18.02, that would have simplified your implementation.
> > > > > > >
> > > > > >
> > > > > > Got you. You right we need to remove the created devargs in
> > > > > > fail-safe
> > > > > parse level.
> > > > > > What do you think about checking it in the parse level and
> > > > > > avoid the new
> > > > > devargs creation?
> > > > > > Also to do the copy in parse level(same method as we are doing
> > > > > > in probe
> > > > > level)?
> > > > > >
> > > > >
> > > > > Not sure I follow here, but the new rte_devargs is part of the
> > > > > sub-device (it is not a pointer, but allocated alongside the
> sub_device).
> > > > >
> > > > > So keep everything here, it is the right place to deal with these
> > > > > things.
> > > > >
> > > > But it will prevent the double parsing and also saves the method:
> > > > If the device already parsed - copy its devargs and continue.
> > > > If the device already probed - copy the device pointer and continue.
> > > >
> > > > I think this is the right dealing, no?
> > > > Why to deal with parse level in probe level? Just keep all the
> > > > parse work to
> > > parse level and the probe work to probe level.
> > >
> > > After re-reading, I think we misunderstood each other.
> > > You cannot remove the rte_devargs created during parsing: it is
> > > allocated alongside the sub_device structure.
> > >
> > > You must only remove the rte_devargs allocated by the EAL (using
> > > rte_eal_devargs_remove()).
> > >
> >
> > Sure.
> >
> > > Before removing it, you must copy its content in the local
> > > sub_device rte_devargs structure. I only proposed a way to do this
> > > copy that would not deal with rte_devargs internals, as it is bound to
> evolve rather soon.
> > >
> > Yes.
> >
> > > Otherwise, no, I do not want to complicate the parsing operations,
> > > they are already too complicated and too criticals. Better to keep it all
> here.
> >
> > I think fs_parse_device function is not complicated and it is the natural
> place for devargs games.
> > For me this is the right place for the copy & remove devargs.
> > Are you insisting to put all in fs_bus_init?
>
> You would have to put fs_ethdev_portid_find in failsafe_args, which is
> mixing layers. Sorry but yes, please keep all these changes in this file.
>
OK, Thanks man!
> Thanks,
> --
> Gaëtan Rivet
> 6WIND
