Sean Christopherson <sea...@google.com> writes:
> 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 <sea...@google.com> writes:
>> > > > On Fri, Nov 10, 2023, Michael Ellerman wrote:
>> > > >> Jason Gunthorpe <j...@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.
>> >
>> > 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.9a2f72c...@lists.samba.org/
cheers
