Drop the vfio_file_iommu_group() stub and instead unconditionally declare
the function to fudge around a KVM wart where KVM tries to do symbol_get()
on vfio_file_iommu_group() (and other VFIO symbols) even if CONFIG_VFIO=n.
Ensuring the symbol is always declared fixes a PPC build error when
modules are also disabled, in which case symbol_get() simply points at the
address of the symbol (with some attributes shenanigans). Because KVM
does symbol_get() instead of directly depending on VFIO, the lack of a
fully defined symbol is not problematic (ugly, but "fine").
arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7:
error: attribute declaration must precede definition [-Werror,-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); })
^
include/linux/vfio.h:294:35: note: previous definition is here
static inline struct iommu_group *vfio_file_iommu_group(struct file *file)
^
arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7:
error: attribute declaration must precede definition [-Werror,-Wignored-attributes]
fn = symbol_get(vfio_file_iommu_group);
^
include/linux/module.h:805:65: note: expanded from macro 'symbol_get'
#define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })
^
include/linux/vfio.h:294:35: note: previous definition is here
static inline struct iommu_group *vfio_file_iommu_group(struct file *file)
^
2 errors generated.
Although KVM is firmly in the wrong (there is zero reason for KVM to build
virt/kvm/vfio.c when VFIO is disabled), fudge around the error in VFIO as
the stub is unnecessary and doesn't serve its intended purpose (KVM is the
only external user of vfio_file_iommu_group()), and there is an in-flight
series to clean up the entire KVM<->VFIO interaction, i.e. fixing this in
KVM would result in more churn in the long run, and the stub needs to go
away regardless.
Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]
Link: https://lore.kernel.org/all/[email protected]
Link: https://lore.kernel.org/all/[email protected]
Cc: Nick Desaulniers <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Tested-by: Michael Ellerman <[email protected]>
Fixes: c1cce6d079b8 ("vfio: Compile vfio_group infrastructure optionally")
Signed-off-by: Sean Christopherson <[email protected]>
---
include/linux/vfio.h | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
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;
base-commit: ae2667cd8a479bb5abd6e24c12fcc9ef5bc06d75
--
2.43.0.rc1.413.gea7ed67945-goog
On Wed, Nov 29, 2023 at 04:10:00PM -0800, Sean Christopherson wrote:
> Drop the vfio_file_iommu_group() stub and instead unconditionally declare
> the function to fudge around a KVM wart where KVM tries to do symbol_get()
> on vfio_file_iommu_group() (and other VFIO symbols) even if CONFIG_VFIO=n.
>
> Ensuring the symbol is always declared fixes a PPC build error when
> modules are also disabled, in which case symbol_get() simply points at the
> address of the symbol (with some attributes shenanigans). Because KVM
> does symbol_get() instead of directly depending on VFIO, the lack of a
> fully defined symbol is not problematic (ugly, but "fine").
>
> arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7:
> error: attribute declaration must precede definition [-Werror,-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); })
> ^
> include/linux/vfio.h:294:35: note: previous definition is here
> static inline struct iommu_group *vfio_file_iommu_group(struct file *file)
> ^
> arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7:
> error: attribute declaration must precede definition [-Werror,-Wignored-attributes]
> fn = symbol_get(vfio_file_iommu_group);
> ^
> include/linux/module.h:805:65: note: expanded from macro 'symbol_get'
> #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })
> ^
> include/linux/vfio.h:294:35: note: previous definition is here
> static inline struct iommu_group *vfio_file_iommu_group(struct file *file)
> ^
> 2 errors generated.
>
> Although KVM is firmly in the wrong (there is zero reason for KVM to build
> virt/kvm/vfio.c when VFIO is disabled), fudge around the error in VFIO as
> the stub is unnecessary and doesn't serve its intended purpose (KVM is the
> only external user of vfio_file_iommu_group()), and there is an in-flight
> series to clean up the entire KVM<->VFIO interaction, i.e. fixing this in
> KVM would result in more churn in the long run, and the stub needs to go
> away regardless.
>
> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]
> Link: https://lore.kernel.org/all/[email protected]
> Link: https://lore.kernel.org/all/[email protected]
> Cc: Nick Desaulniers <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Tested-by: Michael Ellerman <[email protected]>
> Fixes: c1cce6d079b8 ("vfio: Compile vfio_group infrastructure optionally")
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> include/linux/vfio.h | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
Reviewed-by: Jason Gunthorpe <[email protected]>
Jason
On Wed, 29 Nov 2023 16:10:00 -0800
Sean Christopherson <[email protected]> wrote:
> Drop the vfio_file_iommu_group() stub and instead unconditionally declare
> the function to fudge around a KVM wart where KVM tries to do symbol_get()
> on vfio_file_iommu_group() (and other VFIO symbols) even if CONFIG_VFIO=n.
>
> Ensuring the symbol is always declared fixes a PPC build error when
> modules are also disabled, in which case symbol_get() simply points at the
> address of the symbol (with some attributes shenanigans). Because KVM
> does symbol_get() instead of directly depending on VFIO, the lack of a
> fully defined symbol is not problematic (ugly, but "fine").
>
> arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7:
> error: attribute declaration must precede definition [-Werror,-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); })
> ^
> include/linux/vfio.h:294:35: note: previous definition is here
> static inline struct iommu_group *vfio_file_iommu_group(struct file *file)
> ^
> arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7:
> error: attribute declaration must precede definition [-Werror,-Wignored-attributes]
> fn = symbol_get(vfio_file_iommu_group);
> ^
> include/linux/module.h:805:65: note: expanded from macro 'symbol_get'
> #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })
> ^
> include/linux/vfio.h:294:35: note: previous definition is here
> static inline struct iommu_group *vfio_file_iommu_group(struct file *file)
> ^
> 2 errors generated.
>
> Although KVM is firmly in the wrong (there is zero reason for KVM to build
> virt/kvm/vfio.c when VFIO is disabled), fudge around the error in VFIO as
> the stub is unnecessary and doesn't serve its intended purpose (KVM is the
> only external user of vfio_file_iommu_group()), and there is an in-flight
> series to clean up the entire KVM<->VFIO interaction, i.e. fixing this in
> KVM would result in more churn in the long run, and the stub needs to go
> away regardless.
>
> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]
> Link: https://lore.kernel.org/all/[email protected]
> Link: https://lore.kernel.org/all/[email protected]
> Cc: Nick Desaulniers <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Tested-by: Michael Ellerman <[email protected]>
> Fixes: c1cce6d079b8 ("vfio: Compile vfio_group infrastructure optionally")
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> include/linux/vfio.h | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> 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;
>
> base-commit: ae2667cd8a479bb5abd6e24c12fcc9ef5bc06d75
I think we've squashed all the outstanding concerns in the other
thread, so I've pushed this to my for-linux branch for v6.7. Speak up
if I'm missing any unresolved issues. Thanks!
Alex