| Message ID | 1478238521-32203-1-git-send-email-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Michael Ellerman <mpe@ellerman.id.au> writes: > 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. Ping? There was a bit of discussion on this patch but I think we decided it was correct in the end. cheers
On Thu, 02 Feb 2017 20:50:48 +1100 Michael Ellerman <mpe@ellerman.id.au> wrote: > Michael Ellerman <mpe@ellerman.id.au> writes: > > > 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. > > Ping? > > There was a bit of discussion on this patch but I think we decided it > was correct in the end. If there are no other comments or objections, I'll queue this for v4.11. Based on this last comment: 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 I prefer prefer the solution here that doesn't leave extra unselected config entries when CONFIG_VFIO is also not selected. Thanks, Alex
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.
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(-)