On Fri, Nov 10, 2023, Michael Ellerman wrote:
> Jason Gunthorpe <jgg@xxxxxxxxxx> writes:
> > There are a bunch of reported randconfig failures now because of this,
> > something like:
> >
> >>> 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
>
> > 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;
