Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1515019 > unrolled thread

[PATCH] vfio: Fix build break when SPAPR_TCE_IOMMU=n

Started byMichael Ellerman <mpe@ellerman.id.au>
First post2016-11-04 06:50 +0100
Last post2016-11-08 01:10 +0100
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] vfio: Fix build break when SPAPR_TCE_IOMMU=n Michael Ellerman <mpe@ellerman.id.au> - 2016-11-04 06:50 +0100
    Re: [PATCH] vfio: Fix build break when SPAPR_TCE_IOMMU=n Paolo Bonzini <pbonzini@redhat.com> - 2016-11-04 11:10 +0100
      Re: [PATCH] vfio: Fix build break when SPAPR_TCE_IOMMU=n Michael Ellerman <mpe@ellerman.id.au> - 2016-11-07 09:40 +0100
        Re: [PATCH] vfio: Fix build break when SPAPR_TCE_IOMMU=n Alex Williamson <alex.williamson@redhat.com> - 2016-11-07 17:30 +0100
          Re: [PATCH] vfio: Fix build break when SPAPR_TCE_IOMMU=n Paolo Bonzini <pbonzini@redhat.com> - 2016-11-07 17:40 +0100
          Re: [PATCH] vfio: Fix build break when SPAPR_TCE_IOMMU=n Michael Ellerman <mpe@ellerman.id.au> - 2016-11-08 00:50 +0100
            Re: [PATCH] vfio: Fix build break when SPAPR_TCE_IOMMU=n Alex Williamson <alex.williamson@redhat.com> - 2016-11-08 01:10 +0100

#1515019 — [PATCH] vfio: Fix build break when SPAPR_TCE_IOMMU=n

FromMichael Ellerman <mpe@ellerman.id.au>
Date2016-11-04 06:50 +0100
Subject[PATCH] vfio: Fix build break when SPAPR_TCE_IOMMU=n
Message-ID<szFy1-5RC-1@gated-at.bofh.it>
Currently the kconfig logic for VFIO_IOMMU_SPAPR_TCE and VFIO_SPAPR_EEH
is broken when SPAPR_TCE_IOMMU=n. Leading to:

    warning: (VFIO) selects VFIO_IOMMU_SPAPR_TCE which has unmet direct dependencies (VFIO && SPAPR_TCE_IOMMU)
    warning: (VFIO) selects VFIO_IOMMU_SPAPR_TCE which has unmet direct dependencies (VFIO && SPAPR_TCE_IOMMU)
    drivers/vfio/vfio_iommu_spapr_tce.c:113:8: error: implicit declaration of function 'mm_iommu_find'

This stems from the fact that VFIO selects VFIO_IOMMU_SPAPR_TCE, and
although it has an if clause, the condition is not correct.

We could fix it by doing select VFIO_IOMMU_SPAPR_TCE if SPAPR_TCE_IOMMU,
but the cleaner fix is to drop the selects and tie VFIO_IOMMU_SPAPR_TCE
to the value of VFIO, and express the dependencies in only once place.

Do the same for VFIO_SPAPR_EEH.

The end result is that the values of VFIO_IOMMU_SPAPR_TCE and
VFIO_SPAPR_EEH follow the value of VFIO, except when SPAPR_TCE_IOMMU=n
and/or EEH=n. Which is exactly what we want to happen.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 drivers/vfio/Kconfig | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index da6e2ce77495..6b51a4ebed8a 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -6,12 +6,12 @@ config VFIO_IOMMU_TYPE1
 config VFIO_IOMMU_SPAPR_TCE
 	tristate
 	depends on VFIO && SPAPR_TCE_IOMMU
-	default n
+	default VFIO
 
 config VFIO_SPAPR_EEH
 	tristate
 	depends on EEH && VFIO_IOMMU_SPAPR_TCE
-	default n
+	default VFIO
 
 config VFIO_VIRQFD
 	tristate
@@ -22,8 +22,6 @@ menuconfig VFIO
 	tristate "VFIO Non-Privileged userspace driver framework"
 	depends on IOMMU_API
 	select VFIO_IOMMU_TYPE1 if (X86 || S390 || ARM_SMMU || ARM_SMMU_V3)
-	select VFIO_IOMMU_SPAPR_TCE if (PPC_POWERNV || PPC_PSERIES)
-	select VFIO_SPAPR_EEH if (PPC_POWERNV || PPC_PSERIES)
 	select ANON_INODES
 	help
 	  VFIO provides a framework for secure userspace device drivers.
-- 
2.7.4

[toc] | [next] | [standalone]

#1515101

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-11-04 11:10 +0100
Message-ID<szJBE-hL-13@gated-at.bofh.it>
In reply to#1515019

On 04/11/2016 06:48, Michael Ellerman wrote:
> Currently the kconfig logic for VFIO_IOMMU_SPAPR_TCE and VFIO_SPAPR_EEH
> is broken when SPAPR_TCE_IOMMU=n. Leading to:
> 
>     warning: (VFIO) selects VFIO_IOMMU_SPAPR_TCE which has unmet direct dependencies (VFIO && SPAPR_TCE_IOMMU)
>     warning: (VFIO) selects VFIO_IOMMU_SPAPR_TCE which has unmet direct dependencies (VFIO && SPAPR_TCE_IOMMU)
>     drivers/vfio/vfio_iommu_spapr_tce.c:113:8: error: implicit declaration of function 'mm_iommu_find'
> 
> This stems from the fact that VFIO selects VFIO_IOMMU_SPAPR_TCE, and
> although it has an if clause, the condition is not correct.
> 
> We could fix it by doing select VFIO_IOMMU_SPAPR_TCE if SPAPR_TCE_IOMMU,
> but the cleaner fix is to drop the selects and tie VFIO_IOMMU_SPAPR_TCE
> to the value of VFIO, and express the dependencies in only once place.
> 
> Do the same for VFIO_SPAPR_EEH.
> 
> The end result is that the values of VFIO_IOMMU_SPAPR_TCE and
> VFIO_SPAPR_EEH follow the value of VFIO, except when SPAPR_TCE_IOMMU=n
> and/or EEH=n. Which is exactly what we want to happen.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  drivers/vfio/Kconfig | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> index da6e2ce77495..6b51a4ebed8a 100644
> --- a/drivers/vfio/Kconfig
> +++ b/drivers/vfio/Kconfig
> @@ -6,12 +6,12 @@ config VFIO_IOMMU_TYPE1
>  config VFIO_IOMMU_SPAPR_TCE
>  	tristate
>  	depends on VFIO && SPAPR_TCE_IOMMU
> -	default n
> +	default VFIO

No need to depend on VFIO since you already have it in "default".  (I
assume you cannot use "default y" because "depends on" doesn't downgrade
"y" to "m" when VFIO is a module.

A shorthand is

	def_tristate VFIO && SPAPR_TCE_IOMMU

>  config VFIO_SPAPR_EEH
>  	tristate
>  	depends on EEH && VFIO_IOMMU_SPAPR_TCE
> -	default n
> +	default VFIO

Likewise:

	def_tristate VFIO_IOMMU_SPAPR_TCE && EEH

Thanks,

Paolo

>  config VFIO_VIRQFD
>  	tristate
> @@ -22,8 +22,6 @@ menuconfig VFIO
>  	tristate "VFIO Non-Privileged userspace driver framework"
>  	depends on IOMMU_API
>  	select VFIO_IOMMU_TYPE1 if (X86 || S390 || ARM_SMMU || ARM_SMMU_V3)
> -	select VFIO_IOMMU_SPAPR_TCE if (PPC_POWERNV || PPC_PSERIES)
> -	select VFIO_SPAPR_EEH if (PPC_POWERNV || PPC_PSERIES)
>  	select ANON_INODES
>  	help
>  	  VFIO provides a framework for secure userspace device drivers.
>

[toc] | [prev] | [next] | [standalone]

#1515905

FromMichael Ellerman <mpe@ellerman.id.au>
Date2016-11-07 09:40 +0100
Message-ID<sANDb-tf-17@gated-at.bofh.it>
In reply to#1515101
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 04/11/2016 06:48, Michael Ellerman wrote:
>> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
>> index da6e2ce77495..6b51a4ebed8a 100644
>> --- a/drivers/vfio/Kconfig
>> +++ b/drivers/vfio/Kconfig
>> @@ -6,12 +6,12 @@ config VFIO_IOMMU_TYPE1
>>  config VFIO_IOMMU_SPAPR_TCE
>>  	tristate
>>  	depends on VFIO && SPAPR_TCE_IOMMU
>> -	default n
>> +	default VFIO
>
> No need to depend on VFIO since you already have it in "default".

True, I can take that out.

> (I assume you cannot use "default y" because "depends on" doesn't downgrade
> "y" to "m" when VFIO is a module.

Correct.

> A shorthand is
>
> 	def_tristate VFIO && SPAPR_TCE_IOMMU

Yep. My experience though is that a lot of folks don't really know what
that means. So I prefer to spell it out with an explicit type, depends
and default.

But I'll respin it that way if Alex prefers the shorter style.

cheers

[toc] | [prev] | [next] | [standalone]

#1516295

FromAlex Williamson <alex.williamson@redhat.com>
Date2016-11-07 17:30 +0100
Message-ID<sAUY3-5vP-45@gated-at.bofh.it>
In reply to#1515905
On Mon, 07 Nov 2016 19:34:42 +1100
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Paolo Bonzini <pbonzini@redhat.com> writes:
> > On 04/11/2016 06:48, Michael Ellerman wrote:  
> >> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> >> index da6e2ce77495..6b51a4ebed8a 100644
> >> --- a/drivers/vfio/Kconfig
> >> +++ b/drivers/vfio/Kconfig
> >> @@ -6,12 +6,12 @@ config VFIO_IOMMU_TYPE1
> >>  config VFIO_IOMMU_SPAPR_TCE
> >>  	tristate
> >>  	depends on VFIO && SPAPR_TCE_IOMMU
> >> -	default n
> >> +	default VFIO  
> >
> > No need to depend on VFIO since you already have it in "default".  

depends and defaults are different beasts though, if VFIO is not
enabled and we're not on a powerpc system with SPAPR,
VFIO_IOMMU_SPAPR_TCE should not be selectable, not just default to 'n'.

> 
> True, I can take that out.
> 
> > (I assume you cannot use "default y" because "depends on" doesn't downgrade
> > "y" to "m" when VFIO is a module.  
> 
> Correct.
> 
> > A shorthand is
> >
> > 	def_tristate VFIO && SPAPR_TCE_IOMMU  
> 
> Yep. My experience though is that a lot of folks don't really know what
> that means. So I prefer to spell it out with an explicit type, depends
> and default.
> 
> But I'll respin it that way if Alex prefers the shorter style.

Perhaps I'm one of those people.  Non-powerpc archs should not have an
option to select this, which is why the depends is there, AIUI.  So
long as we don't start exposing options that aren't relevant to a
platform, I'm flexible on what shorthands we use, but you may need to
teach me about them first.  Thanks,

Alex

[toc] | [prev] | [next] | [standalone]

#1516304

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-11-07 17:40 +0100
Message-ID<sAV7I-5z9-21@gated-at.bofh.it>
In reply to#1516295

On 07/11/2016 17:25, Alex Williamson wrote:
> On Mon, 07 Nov 2016 19:34:42 +1100
> Michael Ellerman <mpe@ellerman.id.au> wrote:
> 
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>> On 04/11/2016 06:48, Michael Ellerman wrote:  
>>>> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
>>>> index da6e2ce77495..6b51a4ebed8a 100644
>>>> --- a/drivers/vfio/Kconfig
>>>> +++ b/drivers/vfio/Kconfig
>>>> @@ -6,12 +6,12 @@ config VFIO_IOMMU_TYPE1
>>>>  config VFIO_IOMMU_SPAPR_TCE
>>>>  	tristate
>>>>  	depends on VFIO && SPAPR_TCE_IOMMU
>>>> -	default n
>>>> +	default VFIO  
>>>
>>> No need to depend on VFIO since you already have it in "default".  
> 
> depends and defaults are different beasts though, if VFIO is not
> enabled and we're not on a powerpc system with SPAPR,
> VFIO_IOMMU_SPAPR_TCE should not be selectable, not just default to 'n'.

AFAIU without a prompt nothing is selectable anyway (hence my preference
for a shorthand).

Paolo

>>
>> True, I can take that out.
>>
>>> (I assume you cannot use "default y" because "depends on" doesn't downgrade
>>> "y" to "m" when VFIO is a module.  
>>
>> Correct.
>>
>>> A shorthand is
>>>
>>> 	def_tristate VFIO && SPAPR_TCE_IOMMU  
>>
>> Yep. My experience though is that a lot of folks don't really know what
>> that means. So I prefer to spell it out with an explicit type, depends
>> and default.
>>
>> But I'll respin it that way if Alex prefers the shorter style.
> 
> Perhaps I'm one of those people.  Non-powerpc archs should not have an
> option to select this, which is why the depends is there, AIUI.  So
> long as we don't start exposing options that aren't relevant to a
> platform, I'm flexible on what shorthands we use, but you may need to
> teach me about them first.  Thanks,
> 
> Alex
>

[toc] | [prev] | [next] | [standalone]

#1516678

FromMichael Ellerman <mpe@ellerman.id.au>
Date2016-11-08 00:50 +0100
Message-ID<sB1PP-1Bj-9@gated-at.bofh.it>
In reply to#1516295
Alex Williamson <alex.williamson@redhat.com> writes:
> On Mon, 07 Nov 2016 19:34:42 +1100
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> > On 04/11/2016 06:48, Michael Ellerman wrote:  
>> >> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
>> >> index da6e2ce77495..6b51a4ebed8a 100644
>> >> --- a/drivers/vfio/Kconfig
>> >> +++ b/drivers/vfio/Kconfig
>> >> @@ -6,12 +6,12 @@ config VFIO_IOMMU_TYPE1
>> >>  config VFIO_IOMMU_SPAPR_TCE
>> >>  	tristate
>> >>  	depends on VFIO && SPAPR_TCE_IOMMU
>> >> -	default n
>> >> +	default VFIO  
>> >
>> > No need to depend on VFIO since you already have it in "default".  
>
> depends and defaults are different beasts though, if VFIO is not
> enabled and we're not on a powerpc system with SPAPR,
> VFIO_IOMMU_SPAPR_TCE should not be selectable, not just default to 'n'.

Right. But dropping VFIO from the depends won't change that. In fact it
has no effect because the entire directory is not built if VFIO=n.

See drivers/Makefile:

obj-$(CONFIG_VFIO)		+= vfio/


So we don't need the depends on VFIO there.

>> > A shorthand is
>> >
>> > 	def_tristate VFIO && SPAPR_TCE_IOMMU  
>> 
>> Yep. My experience though is that a lot of folks don't really know what
>> that means. So I prefer to spell it out with an explicit type, depends
>> and default.
>> 
>> But I'll respin it that way if Alex prefers the shorter style.
>
> Perhaps I'm one of those people.  Non-powerpc archs should not have an
> option to select this, which is why the depends is there, AIUI.  So
> long as we don't start exposing options that aren't relevant to a
> platform, I'm flexible on what shorthands we use, but you may need to
> teach me about them first.  Thanks,

Using the def_tristate trick won't expose the option to users, because
it has no description it's not user selectable. But it does make the
symbol exist on an x86 build, and it appears in the .config.

eg, using def_tristate you get:

# CONFIG_VFIO_IOMMU_SPAPR_TCE is not set
# CONFIG_VFIO is not set

Whereas using depends all you get is:

# CONFIG_VFIO is not set


So using def_tristate in this case is not entirely equivalent.

cheers

[toc] | [prev] | [next] | [standalone]

#1516692

FromAlex Williamson <alex.williamson@redhat.com>
Date2016-11-08 01:10 +0100
Message-ID<sB29b-1Xi-1@gated-at.bofh.it>
In reply to#1516678
On Tue, 08 Nov 2016 10:49:35 +1100
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Alex Williamson <alex.williamson@redhat.com> writes:
> > On Mon, 07 Nov 2016 19:34:42 +1100
> > Michael Ellerman <mpe@ellerman.id.au> wrote:  
> >> Paolo Bonzini <pbonzini@redhat.com> writes:  
> >> > On 04/11/2016 06:48, Michael Ellerman wrote:    
> >> >> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> >> >> index da6e2ce77495..6b51a4ebed8a 100644
> >> >> --- a/drivers/vfio/Kconfig
> >> >> +++ b/drivers/vfio/Kconfig
> >> >> @@ -6,12 +6,12 @@ config VFIO_IOMMU_TYPE1
> >> >>  config VFIO_IOMMU_SPAPR_TCE
> >> >>  	tristate
> >> >>  	depends on VFIO && SPAPR_TCE_IOMMU
> >> >> -	default n
> >> >> +	default VFIO    
> >> >
> >> > No need to depend on VFIO since you already have it in "default".    
> >
> > depends and defaults are different beasts though, if VFIO is not
> > enabled and we're not on a powerpc system with SPAPR,
> > VFIO_IOMMU_SPAPR_TCE should not be selectable, not just default to 'n'.  
> 
> Right. But dropping VFIO from the depends won't change that. In fact it
> has no effect because the entire directory is not built if VFIO=n.
> 
> See drivers/Makefile:
> 
> obj-$(CONFIG_VFIO)		+= vfio/
> 
> 
> So we don't need the depends on VFIO there.
> 
> >> > A shorthand is
> >> >
> >> > 	def_tristate VFIO && SPAPR_TCE_IOMMU    
> >> 
> >> Yep. My experience though is that a lot of folks don't really know what
> >> that means. So I prefer to spell it out with an explicit type, depends
> >> and default.
> >> 
> >> But I'll respin it that way if Alex prefers the shorter style.  
> >
> > Perhaps I'm one of those people.  Non-powerpc archs should not have an
> > option to select this, which is why the depends is there, AIUI.  So
> > long as we don't start exposing options that aren't relevant to a
> > platform, I'm flexible on what shorthands we use, but you may need to
> > teach me about them first.  Thanks,  
> 
> Using the def_tristate trick won't expose the option to users, because
> it has no description it's not user selectable. But it does make the
> symbol exist on an x86 build, and it appears in the .config.
> 
> eg, using def_tristate you get:
> 
> # CONFIG_VFIO_IOMMU_SPAPR_TCE is not set
> # CONFIG_VFIO is not set
> 
> Whereas using depends all you get is:
> 
> # CONFIG_VFIO is not set
> 
> 
> So using def_tristate in this case is not entirely equivalent.

I get a surprising number of users trying to manually hack on
their .config file, so I'd prefer it not appear in the .config unless
CONFIG_VFIO is y/m _and_ the user is actually building to a config
where SPAPR is relevant.  I'm sure it will save me at least a few
questions in the future.  Thanks,

Alex

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web