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

[Alex Williamson]

On Tue, 08 Nov 2016 10:49:35 +1100
Michael Ellerman <mpe@xxxxxxxxxxxxxx> wrote:

> Alex Williamson <alex.williamson@xxxxxxxxxx> writes:
> > On Mon, 07 Nov 2016 19:34:42 +1100
> > Michael Ellerman <mpe@xxxxxxxxxxxxxx> wrote:  
> >> Paolo Bonzini <pbonzini@xxxxxxxxxx> 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