On Wed, Nov 29, 2023, Jason Gunthorpe wrote: > On Wed, Nov 29, 2023 at 05:07:45PM -0800, Sean Christopherson wrote: > > On Wed, Nov 29, 2023, Michael Ellerman wrote: > > > 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. > > > > 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.