[PATCH rc] kvm: Prevent compiling virt/kvm/vfio.c unless VFIO is selected

From: Jason Gunthorpe <jgg@nvidia.com>
Date: 2023-09-20 18:19:23

There are a bunch of reported randconfig failures now because of this,
something like:

quoted

arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: warning: attribute declaration must precede definition [-Wignored-attributes]

           fn = symbol_get(vfio_file_iommu_group);
                ^
   include/linux/module.h:805:60: note: expanded from macro 'symbol_get'
   #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })

It happens because the arch forces KVM_VFIO without knowing if VFIO is
even enabled.

Split the kconfig so the arch selects the usual HAVE_KVM_ARCH_VFIO and
then KVM_VFIO is only enabled if the arch wants it and VFIO is turned on.

Reported-by: kernel test robot <redacted>
Closes: https://lore.kernel.org/oe-kbuild-all/202308251949.5IiaV0sz-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202309030741.82aLACDG-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202309110914.QLH0LU6L-lkp@intel.com/
Cc: Nick Desaulniers <ndesaulniers@google.com>
Fixes: c1cce6d079b8 ("vfio: Compile vfio_group infrastructure optionally")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 arch/arm64/kvm/Kconfig   | 2 +-
 arch/powerpc/kvm/Kconfig | 2 +-
 arch/s390/kvm/Kconfig    | 2 +-
 arch/x86/kvm/Kconfig     | 2 +-
 virt/kvm/Kconfig         | 7 ++++++-
 5 files changed, 10 insertions(+), 5 deletions(-)

Sean's large series will also address this:

https://lore.kernel.org/kvm/20230916003118.2540661-7-seanjc@google.com/

I don't know if it is sever enough to fix in the rc cycle, but here is the
patch.

diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index 83c1e09be42e5b..7c43eaea51ce05 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig

@@ -28,7 +28,7 @@ menuconfig KVM
 	select KVM_MMIO
 	select KVM_GENERIC_DIRTYLOG_READ_PROTECT
 	select KVM_XFER_TO_GUEST_WORK
-	select KVM_VFIO
+	select HAVE_KVM_ARCH_VFIO
 	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_IRQFD
 	select HAVE_KVM_DIRTY_RING_ACQ_REL

diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig
index 902611954200df..b64824e4cbc1eb 100644
--- a/arch/powerpc/kvm/Kconfig
+++ b/arch/powerpc/kvm/Kconfig

@@ -22,7 +22,7 @@ config KVM
 	select PREEMPT_NOTIFIERS
 	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_VCPU_ASYNC_IOCTL
-	select KVM_VFIO
+	select HAVE_KVM_ARCH_VFIO
 	select IRQ_BYPASS_MANAGER
 	select HAVE_KVM_IRQ_BYPASS
 	select INTERVAL_TREE

diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
index 45fdf2a9b2e326..d206ad3a777d5d 100644
--- a/arch/s390/kvm/Kconfig
+++ b/arch/s390/kvm/Kconfig

@@ -31,7 +31,7 @@ config KVM
 	select HAVE_KVM_IRQ_ROUTING
 	select HAVE_KVM_INVALID_WAKEUPS
 	select HAVE_KVM_NO_POLL
-	select KVM_VFIO
+	select HAVE_KVM_ARCH_VFIO
 	select INTERVAL_TREE
 	select MMU_NOTIFIER
 	help

diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index ed90f148140dfe..8e70e693f90e30 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig

@@ -45,7 +45,7 @@ config KVM
 	select HAVE_KVM_NO_POLL
 	select KVM_XFER_TO_GUEST_WORK
 	select KVM_GENERIC_DIRTYLOG_READ_PROTECT
-	select KVM_VFIO
+	select HAVE_KVM_ARCH_VFIO
 	select INTERVAL_TREE
 	select HAVE_KVM_PM_NOTIFIER if PM
 	select KVM_GENERIC_HARDWARE_ENABLING

diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
index 484d0873061ca5..0bf34809e1bbfe 100644
--- a/virt/kvm/Kconfig
+++ b/virt/kvm/Kconfig

@@ -59,9 +59,14 @@ config HAVE_KVM_MSI
 config HAVE_KVM_CPU_RELAX_INTERCEPT
        bool
 
-config KVM_VFIO
+config HAVE_KVM_ARCH_VFIO
        bool
 
+config KVM_VFIO
+       def_bool y
+       depends on HAVE_KVM_ARCH_VFIO
+       depends on VFIO
+
 config HAVE_KVM_INVALID_WAKEUPS
        bool

base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
-- 
2.42.0

Re: [PATCH rc] kvm: Prevent compiling virt/kvm/vfio.c unless VFIO is selected

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2023-09-22 00:01:36

Jason Gunthorpe [off-list ref] writes:

There are a bunch of reported randconfig failures now because of this,
something like:

quoted

arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: warning: attribute declaration must precede definition [-Wignored-attributes]

           fn = symbol_get(vfio_file_iommu_group);
                ^
   include/linux/module.h:805:60: note: expanded from macro 'symbol_get'
   #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })

It happens because the arch forces KVM_VFIO without knowing if VFIO is
even enabled.

Split the kconfig so the arch selects the usual HAVE_KVM_ARCH_VFIO and
then KVM_VFIO is only enabled if the arch wants it and VFIO is turned on.

Reported-by: kernel test robot <redacted>
Closes: https://lore.kernel.org/oe-kbuild-all/202308251949.5IiaV0sz-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202309030741.82aLACDG-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202309110914.QLH0LU6L-lkp@intel.com/
Cc: Nick Desaulniers <ndesaulniers@google.com>
Fixes: c1cce6d079b8 ("vfio: Compile vfio_group infrastructure optionally")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 arch/arm64/kvm/Kconfig   | 2 +-
 arch/powerpc/kvm/Kconfig | 2 +-
 arch/s390/kvm/Kconfig    | 2 +-
 arch/x86/kvm/Kconfig     | 2 +-
 virt/kvm/Kconfig         | 7 ++++++-
 5 files changed, 10 insertions(+), 5 deletions(-)

Sean's large series will also address this:

https://lore.kernel.org/kvm/20230916003118.2540661-7-seanjc@google.com/

I don't know if it is sever enough to fix in the rc cycle, but here is the
patch.

Thanks for debugging this, I had seen it but hadn't got around to it.

I think it's definitely worth fixing now. It's a pretty simple patch and
it's still early in the rc cycle.

Tested-by: Michael Ellerman <mpe@ellerman.id.au>

cheers

quoted hunk

diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index 83c1e09be42e5b..7c43eaea51ce05 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig

@@ -28,7 +28,7 @@ menuconfig KVM
 	select KVM_MMIO
 	select KVM_GENERIC_DIRTYLOG_READ_PROTECT
 	select KVM_XFER_TO_GUEST_WORK
-	select KVM_VFIO
+	select HAVE_KVM_ARCH_VFIO
 	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_IRQFD
 	select HAVE_KVM_DIRTY_RING_ACQ_REL

diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig
index 902611954200df..b64824e4cbc1eb 100644
--- a/arch/powerpc/kvm/Kconfig
+++ b/arch/powerpc/kvm/Kconfig

@@ -22,7 +22,7 @@ config KVM
 	select PREEMPT_NOTIFIERS
 	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_VCPU_ASYNC_IOCTL
-	select KVM_VFIO
+	select HAVE_KVM_ARCH_VFIO
 	select IRQ_BYPASS_MANAGER
 	select HAVE_KVM_IRQ_BYPASS
 	select INTERVAL_TREE

diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
index 45fdf2a9b2e326..d206ad3a777d5d 100644
--- a/arch/s390/kvm/Kconfig
+++ b/arch/s390/kvm/Kconfig

@@ -31,7 +31,7 @@ config KVM
 	select HAVE_KVM_IRQ_ROUTING
 	select HAVE_KVM_INVALID_WAKEUPS
 	select HAVE_KVM_NO_POLL
-	select KVM_VFIO
+	select HAVE_KVM_ARCH_VFIO
 	select INTERVAL_TREE
 	select MMU_NOTIFIER
 	help

diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index ed90f148140dfe..8e70e693f90e30 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig

@@ -45,7 +45,7 @@ config KVM
 	select HAVE_KVM_NO_POLL
 	select KVM_XFER_TO_GUEST_WORK
 	select KVM_GENERIC_DIRTYLOG_READ_PROTECT
-	select KVM_VFIO
+	select HAVE_KVM_ARCH_VFIO
 	select INTERVAL_TREE
 	select HAVE_KVM_PM_NOTIFIER if PM
 	select KVM_GENERIC_HARDWARE_ENABLING

diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
index 484d0873061ca5..0bf34809e1bbfe 100644
--- a/virt/kvm/Kconfig
+++ b/virt/kvm/Kconfig

@@ -59,9 +59,14 @@ config HAVE_KVM_MSI
 config HAVE_KVM_CPU_RELAX_INTERCEPT
        bool
 
-config KVM_VFIO
+config HAVE_KVM_ARCH_VFIO
        bool
 
+config KVM_VFIO
+       def_bool y
+       depends on HAVE_KVM_ARCH_VFIO
+       depends on VFIO
+
 config HAVE_KVM_INVALID_WAKEUPS
        bool

base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
-- 
2.42.0

Ping? Re: [PATCH rc] kvm: Prevent compiling virt/kvm/vfio.c unless VFIO is selected

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2023-11-10 06:09:46

Jason Gunthorpe [off-list ref] writes:

There are a bunch of reported randconfig failures now because of this,
something like:

quoted

arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: warning: attribute declaration must precede definition [-Wignored-attributes]

           fn = symbol_get(vfio_file_iommu_group);
                ^
   include/linux/module.h:805:60: note: expanded from macro 'symbol_get'
   #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })

It happens because the arch forces KVM_VFIO without knowing if VFIO is
even enabled.

This is still breaking some builds. Can we get this fix in please?

cheers

quoted hunk

Split the kconfig so the arch selects the usual HAVE_KVM_ARCH_VFIO and
then KVM_VFIO is only enabled if the arch wants it and VFIO is turned on.

Reported-by: kernel test robot <redacted>
Closes: https://lore.kernel.org/oe-kbuild-all/202308251949.5IiaV0sz-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202309030741.82aLACDG-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202309110914.QLH0LU6L-lkp@intel.com/
Cc: Nick Desaulniers <ndesaulniers@google.com>
Fixes: c1cce6d079b8 ("vfio: Compile vfio_group infrastructure optionally")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 arch/arm64/kvm/Kconfig   | 2 +-
 arch/powerpc/kvm/Kconfig | 2 +-
 arch/s390/kvm/Kconfig    | 2 +-
 arch/x86/kvm/Kconfig     | 2 +-
 virt/kvm/Kconfig         | 7 ++++++-
 5 files changed, 10 insertions(+), 5 deletions(-)

Sean's large series will also address this:

https://lore.kernel.org/kvm/20230916003118.2540661-7-seanjc@google.com/

I don't know if it is sever enough to fix in the rc cycle, but here is the
patch.

diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index 83c1e09be42e5b..7c43eaea51ce05 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig

@@ -28,7 +28,7 @@ menuconfig KVM
 	select KVM_MMIO
 	select KVM_GENERIC_DIRTYLOG_READ_PROTECT
 	select KVM_XFER_TO_GUEST_WORK
-	select KVM_VFIO
+	select HAVE_KVM_ARCH_VFIO
 	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_IRQFD
 	select HAVE_KVM_DIRTY_RING_ACQ_REL

diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig
index 902611954200df..b64824e4cbc1eb 100644
--- a/arch/powerpc/kvm/Kconfig
+++ b/arch/powerpc/kvm/Kconfig

@@ -22,7 +22,7 @@ config KVM
 	select PREEMPT_NOTIFIERS
 	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_VCPU_ASYNC_IOCTL
-	select KVM_VFIO
+	select HAVE_KVM_ARCH_VFIO
 	select IRQ_BYPASS_MANAGER
 	select HAVE_KVM_IRQ_BYPASS
 	select INTERVAL_TREE

diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
index 45fdf2a9b2e326..d206ad3a777d5d 100644
--- a/arch/s390/kvm/Kconfig
+++ b/arch/s390/kvm/Kconfig

@@ -31,7 +31,7 @@ config KVM
 	select HAVE_KVM_IRQ_ROUTING
 	select HAVE_KVM_INVALID_WAKEUPS
 	select HAVE_KVM_NO_POLL
-	select KVM_VFIO
+	select HAVE_KVM_ARCH_VFIO
 	select INTERVAL_TREE
 	select MMU_NOTIFIER
 	help

diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index ed90f148140dfe..8e70e693f90e30 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig

@@ -45,7 +45,7 @@ config KVM
 	select HAVE_KVM_NO_POLL
 	select KVM_XFER_TO_GUEST_WORK
 	select KVM_GENERIC_DIRTYLOG_READ_PROTECT
-	select KVM_VFIO
+	select HAVE_KVM_ARCH_VFIO
 	select INTERVAL_TREE
 	select HAVE_KVM_PM_NOTIFIER if PM
 	select KVM_GENERIC_HARDWARE_ENABLING

diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
index 484d0873061ca5..0bf34809e1bbfe 100644
--- a/virt/kvm/Kconfig
+++ b/virt/kvm/Kconfig

@@ -59,9 +59,14 @@ config HAVE_KVM_MSI
 config HAVE_KVM_CPU_RELAX_INTERCEPT
        bool
 
-config KVM_VFIO
+config HAVE_KVM_ARCH_VFIO
        bool
 
+config KVM_VFIO
+       def_bool y
+       depends on HAVE_KVM_ARCH_VFIO
+       depends on VFIO
+
 config HAVE_KVM_INVALID_WAKEUPS
        bool

base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
-- 
2.42.0

Re: Ping? Re: [PATCH rc] kvm: Prevent compiling virt/kvm/vfio.c unless VFIO is selected

From: Sean Christopherson <seanjc@google.com>
Date: 2023-11-29 02:22:20

On Fri, Nov 10, 2023, Michael Ellerman wrote:

Jason Gunthorpe [off-list ref] writes:

quoted

There are a bunch of reported randconfig failures now because of this,
something like:

quoted

arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: warning: attribute declaration must precede definition [-Wignored-attributes]

           fn = symbol_get(vfio_file_iommu_group);
                ^
   include/linux/module.h:805:60: note: expanded from macro 'symbol_get'
   #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })

It happens because the arch forces KVM_VFIO without knowing if VFIO is
even enabled.

This is still breaking some builds. Can we get this fix in please?

cheers

quoted

Split the kconfig so the arch selects the usual HAVE_KVM_ARCH_VFIO and
then KVM_VFIO is only enabled if the arch wants it and VFIO is turned on.

Heh, so I was trying to figure out why things like vfio_file_set_kvm() aren't
problematic, i.e. why the existing mess didn't cause failures.  I can't repro the
warning (requires clang-16?), but IIUC the reason only the group code is problematic
is that vfio.h creates a stub for vfio_file_iommu_group() and thus there's no symbol,
whereas vfio.h declares vfio_file_set_kvm() unconditionally.

Because KVM is doing symbol_get() and not taking a direct dependency, the lack of
an exported symbol doesn't cause problems, i.e. simply declaring the symbol makes
the compiler happy.

Given that the vfio_file_iommu_group() stub shouldn't exist (KVM is the only user,
and so if I'm correct the stub is worthless), what about this as a temporary "fix"?

I'm 100% on-board with fixing KVM properly, my motivation is purely to minimize
the total amount of churn.  E.g. if this works, then the only extra churn is to
move the declaration of vfio_file_iommu_group() back under the #if, versus having
to churn all of the KVM Kconfigs twice (once now, and again for the full cleanup).

diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 454e9295970c..a65b2513f8cd 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h

@@ -289,16 +289,12 @@ void vfio_combine_iova_ranges(struct rb_root_cached *root, u32 cur_nodes,
 /*
  * External user API
  */
-#if IS_ENABLED(CONFIG_VFIO_GROUP)
 struct iommu_group *vfio_file_iommu_group(struct file *file);
+
+#if IS_ENABLED(CONFIG_VFIO_GROUP)
 bool vfio_file_is_group(struct file *file);
 bool vfio_file_has_dev(struct file *file, struct vfio_device *device);
 #else
-static inline struct iommu_group *vfio_file_iommu_group(struct file *file)
-{
-       return NULL;
-}
-
 static inline bool vfio_file_is_group(struct file *file)
 {
        return false;

Re: Ping? Re: [PATCH rc] kvm: Prevent compiling virt/kvm/vfio.c unless VFIO is selected

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2023-11-29 09:39:47

Sean Christopherson [off-list ref] writes:

On Fri, Nov 10, 2023, Michael Ellerman wrote:

quoted

Jason Gunthorpe [off-list ref] writes:

quoted

There are a bunch of reported randconfig failures now because of this,
something like:

quoted

arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: warning: attribute declaration must precede definition [-Wignored-attributes]

           fn = symbol_get(vfio_file_iommu_group);
                ^
   include/linux/module.h:805:60: note: expanded from macro 'symbol_get'
   #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })

It happens because the arch forces KVM_VFIO without knowing if VFIO is
even enabled.

This is still breaking some builds. Can we get this fix in please?

cheers

quoted

Split the kconfig so the arch selects the usual HAVE_KVM_ARCH_VFIO and
then KVM_VFIO is only enabled if the arch wants it and VFIO is turned on.

Heh, so I was trying to figure out why things like vfio_file_set_kvm() aren't
problematic, i.e. why the existing mess didn't cause failures.  I can't repro the
warning (requires clang-16?), but IIUC the reason only the group code is problematic
is that vfio.h creates a stub for vfio_file_iommu_group() and thus there's no symbol,
whereas vfio.h declares vfio_file_set_kvm() unconditionally.

That warning I'm unsure about.

But the final report linked in Jason's mail shows a different one:

   In file included from arch/powerpc/kvm/../../../virt/kvm/vfio.c:17:
   include/linux/vfio.h: In function 'kvm_vfio_file_iommu_group':
   include/linux/vfio.h:294:35: error: weak declaration of 'vfio_file_iommu_group' being applied to a already existing, static definition
     294 | static inline struct iommu_group *vfio_file_iommu_group(struct file *file)
         |                                   ^~~~~~~~~~~~~~~~~~~~~

Which is simple to reproduce, just build ppc64le_defconfig and then turn
off CONFIG_MODULES (I'm using GCC 13, the report is for GCC 12).

Because KVM is doing symbol_get() and not taking a direct dependency, the lack of
an exported symbol doesn't cause problems, i.e. simply declaring the symbol makes
the compiler happy.

Given that the vfio_file_iommu_group() stub shouldn't exist (KVM is the only user,
and so if I'm correct the stub is worthless), what about this as a temporary "fix"?

I'm 100% on-board with fixing KVM properly, my motivation is purely to minimize
the total amount of churn.  E.g. if this works, then the only extra churn is to
move the declaration of vfio_file_iommu_group() back under the #if, versus having
to churn all of the KVM Kconfigs twice (once now, and again for the full cleanup).

Fine by me.

quoted hunk

diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 454e9295970c..a65b2513f8cd 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h

@@ -289,16 +289,12 @@ void vfio_combine_iova_ranges(struct rb_root_cached *root, u32 cur_nodes,
 /*
  * External user API
  */
-#if IS_ENABLED(CONFIG_VFIO_GROUP)
 struct iommu_group *vfio_file_iommu_group(struct file *file);
+
+#if IS_ENABLED(CONFIG_VFIO_GROUP)
 bool vfio_file_is_group(struct file *file);
 bool vfio_file_has_dev(struct file *file, struct vfio_device *device);
 #else
-static inline struct iommu_group *vfio_file_iommu_group(struct file *file)
-{
-       return NULL;
-}
-
 static inline bool vfio_file_is_group(struct file *file)
 {
        return false;

That fixes the build for me.

Tested-by: Michael Ellerman <mpe@ellerman.id.au>


cheers

Re: Ping? Re: [PATCH rc] kvm: Prevent compiling virt/kvm/vfio.c unless VFIO is selected

From: Sean Christopherson <seanjc@google.com>
Date: 2023-11-30 01:08:18

On Wed, Nov 29, 2023, Michael Ellerman wrote:

Sean Christopherson [off-list ref] writes:

quoted

On Fri, Nov 10, 2023, Michael Ellerman wrote:

quoted

Jason Gunthorpe [off-list ref] writes:

quoted

There are a bunch of reported randconfig failures now because of this,
something like:

quoted

arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: warning: attribute declaration must precede definition [-Wignored-attributes]

           fn = symbol_get(vfio_file_iommu_group);
                ^
   include/linux/module.h:805:60: note: expanded from macro 'symbol_get'
   #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })

It happens because the arch forces KVM_VFIO without knowing if VFIO is
even enabled.

This is still breaking some builds. Can we get this fix in please?

cheers

quoted

Split the kconfig so the arch selects the usual HAVE_KVM_ARCH_VFIO and
then KVM_VFIO is only enabled if the arch wants it and VFIO is turned on.

Heh, so I was trying to figure out why things like vfio_file_set_kvm() aren't
problematic, i.e. why the existing mess didn't cause failures.  I can't repro the
warning (requires clang-16?), but IIUC the reason only the group code is problematic
is that vfio.h creates a stub for vfio_file_iommu_group() and thus there's no symbol,
whereas vfio.h declares vfio_file_set_kvm() unconditionally.

That warning I'm unsure about.

Ah, it's the same warning, I just missed the CONFIG_MODULES=n requirement.

Re: Ping? Re: [PATCH rc] kvm: Prevent compiling virt/kvm/vfio.c unless VFIO is selected

From: Jason Gunthorpe <jgg@nvidia.com>
Date: 2023-11-30 01:17:47

On Wed, Nov 29, 2023 at 05:07:45PM -0800, Sean Christopherson wrote:

On Wed, Nov 29, 2023, Michael Ellerman wrote:

quoted

Sean Christopherson [off-list ref] writes:

quoted

On Fri, Nov 10, 2023, Michael Ellerman wrote:

quoted

Jason Gunthorpe [off-list ref] writes:

quoted

There are a bunch of reported randconfig failures now because of this,
something like:

quoted

arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: warning: attribute declaration must precede definition [-Wignored-attributes]

           fn = symbol_get(vfio_file_iommu_group);
                ^
   include/linux/module.h:805:60: note: expanded from macro 'symbol_get'
   #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })

It happens because the arch forces KVM_VFIO without knowing if VFIO is
even enabled.

This is still breaking some builds. Can we get this fix in please?

cheers

quoted

Split the kconfig so the arch selects the usual HAVE_KVM_ARCH_VFIO and
then KVM_VFIO is only enabled if the arch wants it and VFIO is turned on.

Heh, so I was trying to figure out why things like vfio_file_set_kvm() aren't
problematic, i.e. why the existing mess didn't cause failures.  I can't repro the
warning (requires clang-16?), but IIUC the reason only the group code is problematic
is that vfio.h creates a stub for vfio_file_iommu_group() and thus there's no symbol,
whereas vfio.h declares vfio_file_set_kvm() unconditionally.

That warning I'm unsure about.

Ah, it's the same warning, I just missed the CONFIG_MODULES=n requirement.

Oh, wait, doesn't that mean the approach won't work? IIRC doesn't
symbol_get turn into just &fn when non-modular turning this into a
link failure without the kconfig part?

Jason

Re: Ping? Re: [PATCH rc] kvm: Prevent compiling virt/kvm/vfio.c unless VFIO is selected

From: Sean Christopherson <seanjc@google.com>
Date: 2023-11-30 02:02:37

On Wed, Nov 29, 2023, Jason Gunthorpe wrote:

On Wed, Nov 29, 2023 at 05:07:45PM -0800, Sean Christopherson wrote:

quoted

On Wed, Nov 29, 2023, Michael Ellerman wrote:

quoted

Sean Christopherson [off-list ref] writes:

quoted

On Fri, Nov 10, 2023, Michael Ellerman wrote:

quoted

Jason Gunthorpe [off-list ref] writes:

quoted

There are a bunch of reported randconfig failures now because of this,
something like:

quoted

arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: warning: attribute declaration must precede definition [-Wignored-attributes]

           fn = symbol_get(vfio_file_iommu_group);
                ^
   include/linux/module.h:805:60: note: expanded from macro 'symbol_get'
   #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })

It happens because the arch forces KVM_VFIO without knowing if VFIO is
even enabled.

This is still breaking some builds. Can we get this fix in please?

cheers

quoted

Split the kconfig so the arch selects the usual HAVE_KVM_ARCH_VFIO and
then KVM_VFIO is only enabled if the arch wants it and VFIO is turned on.

Heh, so I was trying to figure out why things like vfio_file_set_kvm() aren't
problematic, i.e. why the existing mess didn't cause failures.  I can't repro the
warning (requires clang-16?), but IIUC the reason only the group code is problematic
is that vfio.h creates a stub for vfio_file_iommu_group() and thus there's no symbol,
whereas vfio.h declares vfio_file_set_kvm() unconditionally.

That warning I'm unsure about.

Ah, it's the same warning, I just missed the CONFIG_MODULES=n requirement.

Oh, wait, doesn't that mean the approach won't work? IIRC doesn't
symbol_get turn into just &fn when non-modular turning this into a
link failure without the kconfig part?

Yes, but it doesn't cause linker errors.  IIUC, because the extern declaration
is tagged "weak", a dummy default is used.  E.g. on x86, this is what is generated
with VFIO=y

                fn = symbol_get(vfio_file_is_valid);
                if (!fn)
   0xffffffff810396c5 <+5>:	mov    $0xffffffff81829230,%rax
   0xffffffff810396cc <+12>:	test   %rax,%rax

whereas VFIO=n gets

                fn = symbol_get(vfio_file_is_valid);
                if (!fn)
   0xffffffff810396c5 <+5>:	mov    $0x0,%rax
   0xffffffff810396cc <+12>:	test   %rax,%rax

I have no idea if the fact that symbol_get() generates '0', i.e. the !NULL checks
work as expected, is intentional or if KVM works by sheer dumb luck.  I'm not
entirely sure I want to find out, though it's definitely extra motiviation to get
the KVM mess fixed sooner than later.

Re: Ping? Re: [PATCH rc] kvm: Prevent compiling virt/kvm/vfio.c unless VFIO is selected

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2023-11-30 06:39:25

Sean Christopherson [off-list ref] writes:

On Wed, Nov 29, 2023, Jason Gunthorpe wrote:

quoted

On Wed, Nov 29, 2023 at 05:07:45PM -0800, Sean Christopherson wrote:

quoted

On Wed, Nov 29, 2023, Michael Ellerman wrote:

quoted

Sean Christopherson [off-list ref] writes:

quoted

On Fri, Nov 10, 2023, Michael Ellerman wrote:

quoted

Jason Gunthorpe [off-list ref] writes:

quoted

There are a bunch of reported randconfig failures now because of this,
something like:

quoted

arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: warning: attribute declaration must precede definition [-Wignored-attributes]

           fn = symbol_get(vfio_file_iommu_group);
                ^
   include/linux/module.h:805:60: note: expanded from macro 'symbol_get'
   #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })

It happens because the arch forces KVM_VFIO without knowing if VFIO is
even enabled.

This is still breaking some builds. Can we get this fix in please?

cheers

quoted

Split the kconfig so the arch selects the usual HAVE_KVM_ARCH_VFIO and
then KVM_VFIO is only enabled if the arch wants it and VFIO is turned on.

Heh, so I was trying to figure out why things like vfio_file_set_kvm() aren't
problematic, i.e. why the existing mess didn't cause failures.  I can't repro the
warning (requires clang-16?), but IIUC the reason only the group code is problematic
is that vfio.h creates a stub for vfio_file_iommu_group() and thus there's no symbol,
whereas vfio.h declares vfio_file_set_kvm() unconditionally.

That warning I'm unsure about.

Ah, it's the same warning, I just missed the CONFIG_MODULES=n requirement.

Oh, wait, doesn't that mean the approach won't work? IIRC doesn't
symbol_get turn into just &fn when non-modular turning this into a
link failure without the kconfig part?

It does build.

I haven't boot tested it, but TBH I don't really care as long as the
build is green, I don't think anyone's actually using this weird
combination of config options.

Yes, but it doesn't cause linker errors.  IIUC, because the extern declaration
is tagged "weak", a dummy default is used.  E.g. on x86, this is what is generated
with VFIO=y

                fn = symbol_get(vfio_file_is_valid);
                if (!fn)
   0xffffffff810396c5 <+5>:	mov    $0xffffffff81829230,%rax
   0xffffffff810396cc <+12>:	test   %rax,%rax

whereas VFIO=n gets

                fn = symbol_get(vfio_file_is_valid);
                if (!fn)
   0xffffffff810396c5 <+5>:	mov    $0x0,%rax
   0xffffffff810396cc <+12>:	test   %rax,%rax

I have no idea if the fact that symbol_get() generates '0', i.e. the !NULL checks
work as expected, is intentional or if KVM works by sheer dumb luck.

I think it's intentional:

  https://lore.kernel.org/all/20030117045054.9A2F72C073@lists.samba.org/

cheers

Re: Ping? Re: [PATCH rc] kvm: Prevent compiling virt/kvm/vfio.c unless VFIO is selected

From: Jason Gunthorpe <jgg@nvidia.com>
Date: 2023-11-30 12:08:27

On Wed, Nov 29, 2023 at 06:02:08PM -0800, Sean Christopherson wrote:

quoted

Ah, it's the same warning, I just missed the CONFIG_MODULES=n requirement.

Oh, wait, doesn't that mean the approach won't work? IIRC doesn't
symbol_get turn into just &fn when non-modular turning this into a
link failure without the kconfig part?

Yes, but it doesn't cause linker errors.  IIUC, because the extern declaration
is tagged "weak", a dummy default is used.  E.g. on x86, this is what is generated
with VFIO=y

Oh wow that is some pretty dark magic there :|

Jason

Re: Ping? Re: [PATCH rc] kvm: Prevent compiling virt/kvm/vfio.c unless VFIO is selected

From: Jason Gunthorpe <jgg@nvidia.com>
Date: 2023-11-29 12:48:57

On Tue, Nov 28, 2023 at 06:21:42PM -0800, Sean Christopherson wrote:

quoted hunk

diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 454e9295970c..a65b2513f8cd 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h

@@ -289,16 +289,12 @@ void vfio_combine_iova_ranges(struct rb_root_cached *root, u32 cur_nodes,
 /*
  * External user API
  */
-#if IS_ENABLED(CONFIG_VFIO_GROUP)
 struct iommu_group *vfio_file_iommu_group(struct file *file);
+
+#if IS_ENABLED(CONFIG_VFIO_GROUP)
 bool vfio_file_is_group(struct file *file);
 bool vfio_file_has_dev(struct file *file, struct vfio_device *device);
 #else
-static inline struct iommu_group *vfio_file_iommu_group(struct file *file)
-{
-       return NULL;
-}
-
 static inline bool vfio_file_is_group(struct file *file)
 {
        return false;

So you symbol get on a symbol that can never be defined? Still says to
me the kconfig needs fixing :|

But sure, as a small patch it looks fine

Thanks,
Jason

Re: Ping? Re: [PATCH rc] kvm: Prevent compiling virt/kvm/vfio.c unless VFIO is selected

From: Sean Christopherson <seanjc@google.com>
Date: 2023-11-29 18:31:36

On Wed, Nov 29, 2023, Jason Gunthorpe wrote:

On Tue, Nov 28, 2023 at 06:21:42PM -0800, Sean Christopherson wrote:

quoted

diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 454e9295970c..a65b2513f8cd 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h

@@ -289,16 +289,12 @@ void vfio_combine_iova_ranges(struct rb_root_cached *root, u32 cur_nodes,
 /*
  * External user API
  */
-#if IS_ENABLED(CONFIG_VFIO_GROUP)
 struct iommu_group *vfio_file_iommu_group(struct file *file);
+
+#if IS_ENABLED(CONFIG_VFIO_GROUP)
 bool vfio_file_is_group(struct file *file);
 bool vfio_file_has_dev(struct file *file, struct vfio_device *device);
 #else
-static inline struct iommu_group *vfio_file_iommu_group(struct file *file)
-{
-       return NULL;
-}
-
 static inline bool vfio_file_is_group(struct file *file)
 {
        return false;

So you symbol get on a symbol that can never be defined? Still says to
me the kconfig needs fixing :|

Yeah, I completely agree, and if KVM didn't already rely on this horrific
behavior and there wasn't a more complete overhaul in-flight, I wouldn't suggest
this.

I'll send the KVM Kconfig/Makefile cleanups from my "Hide KVM internals from others"
series separately (which is still the bulk of the series) so as to prioritize
getting the cleanups landed.

Re: Ping? Re: [PATCH rc] kvm: Prevent compiling virt/kvm/vfio.c unless VFIO is selected

From: Alex Williamson <hidden>
Date: 2023-11-29 21:46:08

On Wed, 29 Nov 2023 10:31:03 -0800
Sean Christopherson [off-list ref] wrote:

On Wed, Nov 29, 2023, Jason Gunthorpe wrote:

quoted

On Tue, Nov 28, 2023 at 06:21:42PM -0800, Sean Christopherson wrote:

quoted

diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 454e9295970c..a65b2513f8cd 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h

@@ -289,16 +289,12 @@ void vfio_combine_iova_ranges(struct rb_root_cached *root, u32 cur_nodes,
 /*
  * External user API
  */
-#if IS_ENABLED(CONFIG_VFIO_GROUP)
 struct iommu_group *vfio_file_iommu_group(struct file *file);
+
+#if IS_ENABLED(CONFIG_VFIO_GROUP)
 bool vfio_file_is_group(struct file *file);
 bool vfio_file_has_dev(struct file *file, struct vfio_device *device);
 #else
-static inline struct iommu_group *vfio_file_iommu_group(struct file *file)
-{
-       return NULL;
-}
-
 static inline bool vfio_file_is_group(struct file *file)
 {
        return false;

So you symbol get on a symbol that can never be defined? Still says to
me the kconfig needs fixing :|

Yeah, I completely agree, and if KVM didn't already rely on this horrific
behavior and there wasn't a more complete overhaul in-flight, I wouldn't suggest
this.

I'll send the KVM Kconfig/Makefile cleanups from my "Hide KVM internals from others"
series separately (which is still the bulk of the series) so as to prioritize
getting the cleanups landed.

Seems we have agreement and confirmation of the fix above as an
interim, do you want to post it formally and I can pick it up for
v6.7-rc?  Thanks,

Alex

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help