[dpdk-dev] [PATCH v12 2/2] eal: support for VFIO-PCI VF token

[Wang, Haiyue]

Hi Anatoly,

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov at intel.com>
> Sent: Wednesday, May 6, 2020 18:27
> To: Wang, Haiyue <haiyue.wang at intel.com>; dev at dpdk.org; thomas at monjalon.net; jerinj at marvell.com;
> david.marchand at redhat.com
> Subject: Re: [PATCH v12 2/2] eal: support for VFIO-PCI VF token
> 
> On 05-May-20 11:34 AM, Haiyue Wang wrote:
> > The kernel module vfio-pci introduces the VF token to enable SR-IOV
> > support since 5.7.
> >
> > The VF token can be set by a vfio-pci based PF driver and must be known
> > by the vfio-pci based VF driver in order to gain access to the device.
> >
> > Signed-off-by: Haiyue Wang <haiyue.wang at intel.com>
> > ---
> 
> <snip>
> 
> > diff --git a/lib/librte_eal/freebsd/eal.c b/lib/librte_eal/freebsd/eal.c
> > index 540b7d38c..00b02855e 100644
> > --- a/lib/librte_eal/freebsd/eal.c
> > +++ b/lib/librte_eal/freebsd/eal.c
> > @@ -1002,6 +1002,11 @@ rte_eal_vfio_intr_mode(void)
> >   	return RTE_INTR_MODE_NONE;
> >   }
> >
> > +void rte_eal_vfio_vf_token(rte_uuid_t vf_token)
> > +{
> > +	memset(vf_token, 0, sizeof(rte_uuid_t));
> > +}
> 
> What's the purpose of memset(0) here? Presumably, if the API is not
> supposed to be supported, the function should have no effect?
> 

Yes, originally I meant to no parse the input, then return zero UUID.
I changed it to the same as Linux: 
	rte_uuid_copy(vf_token, internal_config.vfio_vf_token);
Since it is inited to zero.

> > +
> >   int rte_vfio_setup_device(__rte_unused const char *sysfs_base,
> >   		      __rte_unused const char *dev_addr,
> >   		      __rte_unused int *vfio_dev_fd,
> > diff --git a/lib/librte_eal/include/rte_eal.h b/lib/librte_eal/include/rte_eal.h
> > index 2f9ed298d..d9040ce14 100644
> > --- a/lib/librte_eal/include/rte_eal.h
> > +++ b/lib/librte_eal/include/rte_eal.h
> > @@ -21,6 +21,7 @@
> >   #include <rte_bus.h>
> >
> >   #include <rte_pci_dev_feature_defs.h>
> > +#include <rte_uuid.h>
> >
> >   #ifdef __cplusplus
> >   extern "C" {
> > @@ -438,6 +439,17 @@ int rte_eal_create_uio_dev(void);
> >    */
> >   enum rte_intr_mode rte_eal_vfio_intr_mode(void);
> >
> > +
> > +/**
> > + * The user-configured vfio VF token.
> 
> The description is unclear. Suggested rewording:
> 
> Copy the user-configured vfio VF token.
> 

+1.

> > + *
> > + * @param vf_token
> > + *   vfio VF token configured with the command line is copied
> > + *   into this parameter, zero uuid by default.
> > + */
> > +__rte_experimental
> > +void rte_eal_vfio_vf_token(rte_uuid_t vf_token);
> 
> Maybe rte_eal_vfio_get_vf_token()? I imagine this would be a more
> descriptive name for this API.
> 

Yes, this looks better. Fixed.

> Once all of the above is addressed (or explained),
> 
> Acked-by: Anatoly Burakov <anatoly.burakov at intel.com>
> 
> --
> Thanks,
> Anatoly