linux-kernel - Re: [PATCH] vfio: Fix build break when SPAPR_TCE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87wpgeluk0.fsf@concordia.ellerman.id.au>
Date:   Tue, 08 Nov 2016 10:49:35 +1100
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, linuxppc-dev@...abs.org,
        aik@...abs.ru, gwshan@...ux.vnet.ibm.com
Subject: Re: [PATCH] vfio: Fix build break when SPAPR_TCE_IOMMU=n

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