[dpdk-dev,15/15] vfio: change VFIO init to be extendable

  We can now just OR the vfio_enabled sequentially and so adding new VFIO
subsystems (vfio_platform) is possible.

Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
---
 lib/librte_eal/linuxapp/eal/eal.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
  

Hi Jan,

> We can now just OR the vfio_enabled sequentially and so adding new VFIO
> subsystems (vfio_platform) is possible.
> 
> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index 92225cf..1549fe5 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -727,12 +727,14 @@ rte_eal_iopl_init(void)  #ifdef VFIO_PRESENT  static
> int rte_eal_vfio_setup(void)  {
> -	if (internal_config.no_pci)
> -		return 0;
> +	int vfio_enabled = 0;
> 
> -	pci_vfio_enable();
> +	if (!internal_config.no_pci) {
> +		pci_vfio_enable();
> +		vfio_enabled |= pci_vfio_is_enabled();
> +	}

Could there be a situation where we need to know if a particular VFIO subsystem is enabled? Do you think it's worth adding (e.g. vfio_enabled |= VFIO_PCI_ENABLED or something)? (I don't imply it is necessary, just asking)

Thanks,
Anatoly
  

Hello Anatoly,

On Tue, 10 May 2016 11:50:23 +0000
"Burakov, Anatoly" <anatoly.burakov@intel.com> wrote:

> Hi Jan,
> 
> > We can now just OR the vfio_enabled sequentially and so adding new VFIO
> > subsystems (vfio_platform) is possible.
> > 
> > Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> > ---
> >  lib/librte_eal/linuxapp/eal/eal.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> > index 92225cf..1549fe5 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal.c
> > @@ -727,12 +727,14 @@ rte_eal_iopl_init(void)  #ifdef VFIO_PRESENT  static
> > int rte_eal_vfio_setup(void)  {
> > -	if (internal_config.no_pci)
> > -		return 0;
> > +	int vfio_enabled = 0;
> > 
> > -	pci_vfio_enable();
> > +	if (!internal_config.no_pci) {
> > +		pci_vfio_enable();
> > +		vfio_enabled |= pci_vfio_is_enabled();
> > +	}  
> 
> Could there be a situation where we need to know if a particular VFIO subsystem is enabled? Do you think it's worth adding (e.g. vfio_enabled |= VFIO_PCI_ENABLED or something)? (I don't imply it is necessary, just asking)

Well... the semantics are quite tricky here.

There are: vfio-pci, vfio-platform and vfio-amba in the Linux Kernel. I
ignore vfio-amba because I don't know any considerable device supported
by this.

With respect to the VFIO, if you have both PCI and SoC infra enabled,
you have to check for both vfio-pci and vfio-platform.

If only PCI is there you have to check only for vfio-pci.
But, if you check for vfio-platform as well and it is not there then
the EAL init would fail without a reason (because you don't need
vfio-platform). Vice-versa for SoC.

This patch improves the check to cover those situations. Later on,
I'll just append something like:

if (!internal_config.no_soc) {
	soc_vfio_enable();
	vfio_enabled |= soc_vfio_is_enabled();
}

and the EAL init would be happy... I hope ;).

The thing is that we don't know which devices will bind to VFIO and we
don't know which of those devices would use vfio-pci and vfio-platform.
So, if both PCI and SoC are enabled and only vfio-platform is there we
cannot say here "failed because vfio-pci is missing".

Jan

> 
> Thanks,
> Anatoly
  

> > Could there be a situation where we need to know if a particular VFIO
> > subsystem is enabled? Do you think it's worth adding (e.g.
> > vfio_enabled |= VFIO_PCI_ENABLED or something)? (I don't imply it is
> > necessary, just asking)
> 
> Well... the semantics are quite tricky here.
> 
> There are: vfio-pci, vfio-platform and vfio-amba in the Linux Kernel. I ignore
> vfio-amba because I don't know any considerable device supported by this.
> 
> With respect to the VFIO, if you have both PCI and SoC infra enabled, you
> have to check for both vfio-pci and vfio-platform.
> 
> If only PCI is there you have to check only for vfio-pci.
> But, if you check for vfio-platform as well and it is not there then the EAL init
> would fail without a reason (because you don't need vfio-platform). Vice-
> versa for SoC.
> 
> This patch improves the check to cover those situations. Later on, I'll just
> append something like:
> 
> if (!internal_config.no_soc) {
> 	soc_vfio_enable();
> 	vfio_enabled |= soc_vfio_is_enabled();
> }
> 
> and the EAL init would be happy... I hope ;).
> 
> The thing is that we don't know which devices will bind to VFIO and we don't
> know which of those devices would use vfio-pci and vfio-platform.
> So, if both PCI and SoC are enabled and only vfio-platform is there we cannot
> say here "failed because vfio-pci is missing".

OK fair enough :)

I haven't yet tested the patchset itself, I'll report back if I get any issues.

Thanks,
Anatoly