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

[Michael Ellerman <mpe@ellerman.id.au>]

Sean Christopherson <seanjc@google.com> writes:
> On Fri, Nov 10, 2023, Michael Ellerman wrote:
>> Jason Gunthorpe <jgg@nvidia.com> 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@ellerman.id.au>


cheers