Message ID | 20231130001000.543240-1-seanjc@google.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | vfio: Drop vfio_file_iommu_group() stub to fudge around a KVM wart | expand |
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 <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202308251949.5IiaV0sz-lkp@intel.com > Closes: https://lore.kernel.org/oe-kbuild-all/202309030741.82aLACDG-lkp@intel.com > Closes: https://lore.kernel.org/oe-kbuild-all/202309110914.QLH0LU6L-lkp@intel.com > Link: https://lore.kernel.org/all/0-v1-08396538817d+13c5-vfio_kvm_kconfig_jgg@nvidia.com > Link: https://lore.kernel.org/all/20230916003118.2540661-1-seanjc@google.com > Cc: Nick Desaulniers <ndesaulniers@google.com> > Cc: Jason Gunthorpe <jgg@nvidia.com> > Tested-by: Michael Ellerman <mpe@ellerman.id.au> > Fixes: c1cce6d079b8 ("vfio: Compile vfio_group infrastructure optionally") > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > include/linux/vfio.h | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Wed, 29 Nov 2023 16:10:00 -0800 Sean Christopherson <seanjc@google.com> 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 <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202308251949.5IiaV0sz-lkp@intel.com > Closes: https://lore.kernel.org/oe-kbuild-all/202309030741.82aLACDG-lkp@intel.com > Closes: https://lore.kernel.org/oe-kbuild-all/202309110914.QLH0LU6L-lkp@intel.com > Link: https://lore.kernel.org/all/0-v1-08396538817d+13c5-vfio_kvm_kconfig_jgg@nvidia.com > Link: https://lore.kernel.org/all/20230916003118.2540661-1-seanjc@google.com > Cc: Nick Desaulniers <ndesaulniers@google.com> > Cc: Jason Gunthorpe <jgg@nvidia.com> > Tested-by: Michael Ellerman <mpe@ellerman.id.au> > Fixes: c1cce6d079b8 ("vfio: Compile vfio_group infrastructure optionally") > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > 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
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;