Sean Christopherson <seanjc@xxxxxxxxxx> writes:
> 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.
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.
> 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@xxxxxxxxxxxxxx>
cheers
