Re: [dpdk-dev] [RFC v1 1/1] vfio: set vf token and gain vf device access

[Vamsi Krishna Attunuru <vattunuru@marvell.com>]

Hi Vipin,

Please see inline.

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Varghese, Vipin
> Sent: Wednesday, April 1, 2020 8:45 AM
> To: Vamsi Krishna Attunuru <vattunuru@marvell.com>; dev@dpdk.org
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> alex.williamson@redhat.com; thomas@monjalon.net;
> david.marchand@redhat.com
> Subject: Re: [dpdk-dev] [RFC v1 1/1] vfio: set vf token and gain vf device
> access
> 
> Snipped
> > diff --git a/lib/librte_eal/linux/eal/eal_vfio.c
> > b/lib/librte_eal/linux/eal/eal_vfio.c
> > index 01b5ef3..e2fdd35 100644
> > --- a/lib/librte_eal/linux/eal/eal_vfio.c
> > +++ b/lib/librte_eal/linux/eal/eal_vfio.c
> 
> DPDK supports freebsd where `/dev/pci` can be probed like Linux
> `/sys/bus/pci`. Will you be adding the functions in
> `lib/librte_eal/freebsd/eal/` folder or implement as `NOT supported`?
> 
> What about window ` lib/librte_eal/windows/eal/`?

If other kernel (freebsd & windows) drivers also support this vf token scheme, functions can be added in respective eal implementations.

> 
> snipped
> > +	snprintf(linkname, sizeof(linkname),
> > +			 "%s/%s/physfn", sysfs_base, dev_addr);
> 
> Would you like to check the pointer sysfs_base or dev_addr?

Physfn pointer is being checked here.
> 
> >  int
> >  rte_vfio_setup_device(const char *sysfs_base, const char *dev_addr,
> >  		int *vfio_dev_fd, struct vfio_device_info *device_info) @@ -
> 
> Snipped
> 
> > +		ret = is_vf_token_required(sysfs_base, dev_addr);
> 
> The return value from the above function is 0 or -1.

Function also returns 1.
> 
> > +		/* if negative, something failed */
> > +		if (ret < 0)
> > +			return -1;
> > +
> > +		if (ret == 0) {
> 
> Will it be ok to skip this check?
No, it's a valid case.
> 
> > +			/* vf_token required to open device file descriptor */
> > +			rte_uuid_unparse(uuid_token,
> > +					 vf_token, sizeof(vf_token));
> > +			snprintf(dev, sizeof(dev),
> > +				 "%s vf_token=%s", dev_addr, vf_token);
> > +		}
> 
> snipped
> 
> general comment `can we use EAL LOG for info, warn, error`?