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

2023-11-30 Thread Jason Gunthorpe
On Wed, Nov 29, 2023 at 06:02:08PM -0800, Sean Christopherson wrote:

> > > 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  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

Oh wow that is some pretty dark magic there :|

Jason


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

2023-11-29 Thread Michael Ellerman
Sean Christopherson  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  writes:
>> > > > On Fri, Nov 10, 2023, Michael Ellerman wrote:
>> > > >> Jason Gunthorpe  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  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)
>0x810396c5 <+5>:   mov$0x81829230,%rax
>0x810396cc <+12>:  test   %rax,%rax
>
> whereas VFIO=n gets
>
> fn = symbol_get(vfio_file_is_valid);
> if (!fn)
>0x810396c5 <+5>:   mov$0x0,%rax
>0x810396cc <+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


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

2023-11-29 Thread Sean Christopherson
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  writes:
> > > > On Fri, Nov 10, 2023, Michael Ellerman wrote:
> > > >> Jason Gunthorpe  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  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)
   0x810396c5 <+5>: mov$0x81829230,%rax
   0x810396cc <+12>:test   %rax,%rax

whereas VFIO=n gets

fn = symbol_get(vfio_file_is_valid);
if (!fn)
   0x810396c5 <+5>: mov$0x0,%rax
   0x810396cc <+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. 


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

2023-11-29 Thread Jason Gunthorpe
On Wed, Nov 29, 2023 at 05:07:45PM -0800, Sean Christopherson wrote:
> On Wed, Nov 29, 2023, Michael Ellerman wrote:
> > Sean Christopherson  writes:
> > > On Fri, Nov 10, 2023, Michael Ellerman wrote:
> > >> Jason Gunthorpe  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  when non-modular turning this into a
link failure without the kconfig part?

Jason


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

2023-11-29 Thread Sean Christopherson
On Wed, Nov 29, 2023, Michael Ellerman wrote:
> Sean Christopherson  writes:
> > On Fri, Nov 10, 2023, Michael Ellerman wrote:
> >> Jason Gunthorpe  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.


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

2023-11-29 Thread Alex Williamson
On Wed, 29 Nov 2023 10:31:03 -0800
Sean Christopherson  wrote:

> On Wed, Nov 29, 2023, Jason Gunthorpe wrote:
> > On Tue, Nov 28, 2023 at 06:21:42PM -0800, Sean Christopherson wrote:  
> > > 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;
> > >   
> > 
> > So you symbol get on a symbol that can never be defined? Still says to
> > me the kconfig needs fixing :|  
> 
> Yeah, I completely agree, and if KVM didn't already rely on this horrific
> behavior and there wasn't a more complete overhaul in-flight, I wouldn't 
> suggest
> this.
> 
> I'll send the KVM Kconfig/Makefile cleanups from my "Hide KVM internals from 
> others"
> series separately (which is still the bulk of the series) so as to prioritize
> getting the cleanups landed.
> 

Seems we have agreement and confirmation of the fix above as an
interim, do you want to post it formally and I can pick it up for
v6.7-rc?  Thanks,

Alex



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

2023-11-29 Thread Sean Christopherson
On Wed, Nov 29, 2023, Jason Gunthorpe wrote:
> On Tue, Nov 28, 2023 at 06:21:42PM -0800, Sean Christopherson wrote:
> > 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;
> > 
> 
> So you symbol get on a symbol that can never be defined? Still says to
> me the kconfig needs fixing :|

Yeah, I completely agree, and if KVM didn't already rely on this horrific
behavior and there wasn't a more complete overhaul in-flight, I wouldn't suggest
this.

I'll send the KVM Kconfig/Makefile cleanups from my "Hide KVM internals from 
others"
series separately (which is still the bulk of the series) so as to prioritize
getting the cleanups landed.


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

2023-11-29 Thread Jason Gunthorpe
On Tue, Nov 28, 2023 at 06:21:42PM -0800, Sean Christopherson wrote:
> 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;
> 

So you symbol get on a symbol that can never be defined? Still says to
me the kconfig needs fixing :|

But sure, as a small patch it looks fine

Thanks,
Jason


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

2023-11-29 Thread Michael Ellerman
Sean Christopherson  writes:
> On Fri, Nov 10, 2023, Michael Ellerman wrote:
>> Jason Gunthorpe  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 


cheers


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

2023-11-28 Thread Sean Christopherson
On Fri, Nov 10, 2023, Michael Ellerman wrote:
> Jason Gunthorpe  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;



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

2023-11-09 Thread Michael Ellerman
Jason Gunthorpe  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.
>
> Reported-by: kernel test robot 
> Closes: 
> https://lore.kernel.org/oe-kbuild-all/202308251949.5iiav0sz-...@intel.com/
> Closes: 
> https://lore.kernel.org/oe-kbuild-all/202309030741.82alacdg-...@intel.com/
> Closes: 
> https://lore.kernel.org/oe-kbuild-all/202309110914.qlh0lu6l-...@intel.com/
> Cc: Nick Desaulniers 
> Fixes: c1cce6d079b8 ("vfio: Compile vfio_group infrastructure optionally")
> Signed-off-by: Jason Gunthorpe 
> ---
>  arch/arm64/kvm/Kconfig   | 2 +-
>  arch/powerpc/kvm/Kconfig | 2 +-
>  arch/s390/kvm/Kconfig| 2 +-
>  arch/x86/kvm/Kconfig | 2 +-
>  virt/kvm/Kconfig | 7 ++-
>  5 files changed, 10 insertions(+), 5 deletions(-)
>
> Sean's large series will also address this:
>
> https://lore.kernel.org/kvm/20230916003118.2540661-7-sea...@google.com/
>
> I don't know if it is sever enough to fix in the rc cycle, but here is the
> patch.
>
> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> index 83c1e09be42e5b..7c43eaea51ce05 100644
> --- a/arch/arm64/kvm/Kconfig
> +++ b/arch/arm64/kvm/Kconfig
> @@ -28,7 +28,7 @@ menuconfig KVM
>   select KVM_MMIO
>   select KVM_GENERIC_DIRTYLOG_READ_PROTECT
>   select KVM_XFER_TO_GUEST_WORK
> - select KVM_VFIO
> + select HAVE_KVM_ARCH_VFIO
>   select HAVE_KVM_EVENTFD
>   select HAVE_KVM_IRQFD
>   select HAVE_KVM_DIRTY_RING_ACQ_REL
> diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig
> index 902611954200df..b64824e4cbc1eb 100644
> --- a/arch/powerpc/kvm/Kconfig
> +++ b/arch/powerpc/kvm/Kconfig
> @@ -22,7 +22,7 @@ config KVM
>   select PREEMPT_NOTIFIERS
>   select HAVE_KVM_EVENTFD
>   select HAVE_KVM_VCPU_ASYNC_IOCTL
> - select KVM_VFIO
> + select HAVE_KVM_ARCH_VFIO
>   select IRQ_BYPASS_MANAGER
>   select HAVE_KVM_IRQ_BYPASS
>   select INTERVAL_TREE
> diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
> index 45fdf2a9b2e326..d206ad3a777d5d 100644
> --- a/arch/s390/kvm/Kconfig
> +++ b/arch/s390/kvm/Kconfig
> @@ -31,7 +31,7 @@ config KVM
>   select HAVE_KVM_IRQ_ROUTING
>   select HAVE_KVM_INVALID_WAKEUPS
>   select HAVE_KVM_NO_POLL
> - select KVM_VFIO
> + select HAVE_KVM_ARCH_VFIO
>   select INTERVAL_TREE
>   select MMU_NOTIFIER
>   help
> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
> index ed90f148140dfe..8e70e693f90e30 100644
> --- a/arch/x86/kvm/Kconfig
> +++ b/arch/x86/kvm/Kconfig
> @@ -45,7 +45,7 @@ config KVM
>   select HAVE_KVM_NO_POLL
>   select KVM_XFER_TO_GUEST_WORK
>   select KVM_GENERIC_DIRTYLOG_READ_PROTECT
> - select KVM_VFIO
> + select HAVE_KVM_ARCH_VFIO
>   select INTERVAL_TREE
>   select HAVE_KVM_PM_NOTIFIER if PM
>   select KVM_GENERIC_HARDWARE_ENABLING
> diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
> index 484d0873061ca5..0bf34809e1bbfe 100644
> --- a/virt/kvm/Kconfig
> +++ b/virt/kvm/Kconfig
> @@ -59,9 +59,14 @@ config HAVE_KVM_MSI
>  config HAVE_KVM_CPU_RELAX_INTERCEPT
> bool
>  
> -config KVM_VFIO
> +config HAVE_KVM_ARCH_VFIO
> bool
>  
> +config KVM_VFIO
> +   def_bool y
> +   depends on HAVE_KVM_ARCH_VFIO
> +   depends on VFIO
> +
>  config HAVE_KVM_INVALID_WAKEUPS
> bool
>  
>
> base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
> -- 
> 2.42.0