* [linux-next:master 3581/12910] arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: error: attribute declaration must precede definition @ 2023-08-25 11:23 kernel test robot 2023-08-25 12:12 ` Jason Gunthorpe 0 siblings, 1 reply; 6+ messages in thread From: kernel test robot @ 2023-08-25 11:23 UTC (permalink / raw) To: Yi Liu Cc: llvm, oe-kbuild-all, Linux Memory Management List, Alex Williamson, Jason Gunthorpe tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master head: 6269320850097903b30be8f07a5c61d9f7592393 commit: c1cce6d079b875396c9a7c6838fc5b024758e540 [3581/12910] vfio: Compile vfio_group infrastructure optionally config: powerpc64-randconfig-r001-20230825 (https://download.01.org/0day-ci/archive/20230825/202308251949.5IiaV0sz-lkp@intel.com/config) compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) reproduce: (https://download.01.org/0day-ci/archive/20230825/202308251949.5IiaV0sz-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308251949.5IiaV0sz-lkp@intel.com/ All errors (new ones prefixed by >>): >> 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. vim +89 arch/powerpc/kvm/../../../virt/kvm/vfio.c 4b22ef042d6f54 Jason Gunthorpe 2022-10-07 82 4b22ef042d6f54 Jason Gunthorpe 2022-10-07 83 #ifdef CONFIG_SPAPR_TCE_IOMMU 50d63b5bbfd122 Jason Gunthorpe 2022-05-04 84 static struct iommu_group *kvm_vfio_file_iommu_group(struct file *file) 121f80ba68f1a5 Alexey Kardashevskiy 2017-03-22 85 { 50d63b5bbfd122 Jason Gunthorpe 2022-05-04 86 struct iommu_group *(*fn)(struct file *file); 50d63b5bbfd122 Jason Gunthorpe 2022-05-04 87 struct iommu_group *ret; 121f80ba68f1a5 Alexey Kardashevskiy 2017-03-22 88 50d63b5bbfd122 Jason Gunthorpe 2022-05-04 @89 fn = symbol_get(vfio_file_iommu_group); 121f80ba68f1a5 Alexey Kardashevskiy 2017-03-22 90 if (!fn) 50d63b5bbfd122 Jason Gunthorpe 2022-05-04 91 return NULL; 121f80ba68f1a5 Alexey Kardashevskiy 2017-03-22 92 50d63b5bbfd122 Jason Gunthorpe 2022-05-04 93 ret = fn(file); 121f80ba68f1a5 Alexey Kardashevskiy 2017-03-22 94 50d63b5bbfd122 Jason Gunthorpe 2022-05-04 95 symbol_put(vfio_file_iommu_group); 121f80ba68f1a5 Alexey Kardashevskiy 2017-03-22 96 121f80ba68f1a5 Alexey Kardashevskiy 2017-03-22 97 return ret; 121f80ba68f1a5 Alexey Kardashevskiy 2017-03-22 98 } 121f80ba68f1a5 Alexey Kardashevskiy 2017-03-22 99 :::::: The code at line 89 was first introduced by commit :::::: 50d63b5bbfd12262069ad062611cd5e69c5e9e05 vfio: Change vfio_external_user_iommu_id() to vfio_file_iommu_group() :::::: TO: Jason Gunthorpe <jgg@nvidia.com> :::::: CC: Alex Williamson <alex.williamson@redhat.com> -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [linux-next:master 3581/12910] arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: error: attribute declaration must precede definition 2023-08-25 11:23 [linux-next:master 3581/12910] arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: error: attribute declaration must precede definition kernel test robot @ 2023-08-25 12:12 ` Jason Gunthorpe 2023-08-25 18:49 ` Nick Desaulniers 0 siblings, 1 reply; 6+ messages in thread From: Jason Gunthorpe @ 2023-08-25 12:12 UTC (permalink / raw) To: kernel test robot Cc: Yi Liu, llvm, oe-kbuild-all, Linux Memory Management List, Alex Williamson, Ard Biesheuvel, Jessica Yu, Kees Cook, Nick Desaulniers, Will Deacon On Fri, Aug 25, 2023 at 07:23:29PM +0800, kernel test robot wrote: > tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master > head: 6269320850097903b30be8f07a5c61d9f7592393 > commit: c1cce6d079b875396c9a7c6838fc5b024758e540 [3581/12910] vfio: Compile vfio_group infrastructure optionally > config: powerpc64-randconfig-r001-20230825 (https://download.01.org/0day-ci/archive/20230825/202308251949.5IiaV0sz-lkp@intel.com/config) > compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) > reproduce: (https://download.01.org/0day-ci/archive/20230825/202308251949.5IiaV0sz-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202308251949.5IiaV0sz-lkp@intel.com/ > > All errors (new ones prefixed by >>): > > >> 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' This VFIO code is fine.. > #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. Clang is complaining about this line Which is from: commit 13150bc5416f45234c955e5bed91623d178c6117 Author: Ard Biesheuvel <ardb@kernel.org> Date: Tue Oct 27 16:11:32 2020 +0100 module: use hidden visibility for weak symbol references Geert reports that commit be2881824ae9eb92 ("arm64/build: Assert for unwanted sections") results in build errors on arm64 for configurations that have CONFIG_MODULES disabled. I assume some tweaking there or a clang change is needed (BTW does clang actually work on power, I tried it a bit ago and it didn't get very far) Thanks, Jason ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [linux-next:master 3581/12910] arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: error: attribute declaration must precede definition 2023-08-25 12:12 ` Jason Gunthorpe @ 2023-08-25 18:49 ` Nick Desaulniers 2023-08-25 19:40 ` Jason Gunthorpe 0 siblings, 1 reply; 6+ messages in thread From: Nick Desaulniers @ 2023-08-25 18:49 UTC (permalink / raw) To: Jason Gunthorpe, Yi Liu Cc: kernel test robot, llvm, oe-kbuild-all, Linux Memory Management List, Alex Williamson, Ard Biesheuvel, Jessica Yu, Kees Cook, Will Deacon, Nathan Chancellor On Fri, Aug 25, 2023 at 5:12 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Fri, Aug 25, 2023 at 07:23:29PM +0800, kernel test robot wrote: > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master > > head: 6269320850097903b30be8f07a5c61d9f7592393 > > commit: c1cce6d079b875396c9a7c6838fc5b024758e540 [3581/12910] vfio: Compile vfio_group infrastructure optionally > > config: powerpc64-randconfig-r001-20230825 (https://download.01.org/0day-ci/archive/20230825/202308251949.5IiaV0sz-lkp@intel.com/config) > > compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) > > reproduce: (https://download.01.org/0day-ci/archive/20230825/202308251949.5IiaV0sz-lkp@intel.com/reproduce) > > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > > the same patch/commit), kindly add following tags > > | Reported-by: kernel test robot <lkp@intel.com> > > | Closes: https://lore.kernel.org/oe-kbuild-all/202308251949.5IiaV0sz-lkp@intel.com/ > > > > All errors (new ones prefixed by >>): > > > > >> 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' > > This VFIO code is fine.. > > > #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. > > Clang is complaining about this line > > Which is from: > > commit 13150bc5416f45234c955e5bed91623d178c6117 > Author: Ard Biesheuvel <ardb@kernel.org> > Date: Tue Oct 27 16:11:32 2020 +0100 > > module: use hidden visibility for weak symbol references > > Geert reports that commit be2881824ae9eb92 ("arm64/build: Assert for > unwanted sections") results in build errors on arm64 for configurations > that have CONFIG_MODULES disabled. > > I assume some tweaking there or a clang change is needed In my experience, how attributes are "merged" upon redeclaration is kind of a disaster fest. Adding new attributes to clang has a fair amount of boilerplate because "what should happen when something is redeclared w/ AND w/o the attribute" is a question that the code reviewers (who also generally happens to be the clang code owner who had ISO WG14 standardize attributes for C23) force authors to think about. The semantics for some must match the compiler that initially implemented them for compatibility. This is the method in clang that's emitting that diagnostic: https://github.com/llvm/llvm-project/blob/1034688d58783779168d59b47d2b3e897ad869c6/clang/lib/Sema/SemaDecl.cpp#L3133 As you can see above the line I highlighted, there are different rules for different attributes, some even dependent on the language mode (C vs C++). And that's not all of the logic for function attribute "merging" upon redeclaration that's alluded to in comments of that function. I think this example demonstrates a little clearer what's going on: https://godbolt.org/z/9d6scv1hE So based on the warning, it seems like symbol_get() can only be used before the parameter it's passed is defined, otherwise the "weak" and "visibility(hidden)" attributes are ignored. These attributes are merge-able upon redeclaration up until the first definition. If the definition of vfio_file_iommu_group() is visible to virt/kvm/vfio.c, do we even need this weak+hidden redeclaration? ``` diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c index ca24ce120906..b497b762ddba 100644 --- a/virt/kvm/vfio.c +++ b/virt/kvm/vfio.c @@ -86,7 +86,7 @@ static struct iommu_group *kvm_vfio_file_iommu_group(struct file *file) struct iommu_group *(*fn)(struct file *file); struct iommu_group *ret; - fn = symbol_get(vfio_file_iommu_group); + fn = vfio_file_iommu_group; if (!fn) return NULL; ``` Fixes the error for me in -next (can't repro on mainline): $ wget https://download.01.org/0day-ci/archive/20230825/202308251949.5IiaV0sz-lkp@intel.com/config -O .config $ make LLVM=1 ARCH=powerpc -j128 olddefconfig arch/powerpc/kvm/../../../virt/kvm/vfio.o The vmlinux target also builds with that change for that randconfig the bot unearthed. The all target hits https://github.com/ClangBuiltLinux/linux/issues/1601 (see below). But I'll admit I don't fully understand the implications of that change. Whenever I see weak symbols I get suspicious, especially for a kernel that's heavily configurable; usually providing a definition per config can accomplish what we sometimes use weak linkage for. At the least, it makes it obvious at build time what definition will be observed at runtime; not so for weak symbols. Also, vfio_file_iommu_group() has two different declarations based on CONFIG_VFIO_GROUP, which is probably how the bot's randconfig dug this up. Should my diff above be preprocessor-conditional on that config? Or should that function be doing something else in case CONFIG_VFIO_GROUP is not set? $ grep -rn CONFIG_VFIO_GROUP .config $ echo $? 1 So the bot's randconfig is reporting an issue specific to CONFIG_VFIO_GROUP=n where a static inline definition is provided as the declaration, hence the warning. The following does not work; the diagnostics are still observed. ``` diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c index ca24ce120906..fd6046a63605 100644 --- a/virt/kvm/vfio.c +++ b/virt/kvm/vfio.c @@ -86,6 +86,9 @@ static struct iommu_group *kvm_vfio_file_iommu_group(struct file *file) struct iommu_group *(*fn)(struct file *file); struct iommu_group *ret; + if (!IS_ENABLED(CONFIG_VFIO_GROUP)) + return NULL; + fn = symbol_get(vfio_file_iommu_group); if (!fn) return NULL; ``` This also builds: ``` diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c index ca24ce120906..f76c26f2ee77 100644 --- a/virt/kvm/vfio.c +++ b/virt/kvm/vfio.c @@ -80,7 +80,7 @@ static bool kvm_vfio_file_is_valid(struct file *file) return ret; } -#ifdef CONFIG_SPAPR_TCE_IOMMU +#if defined(CONFIG_SPAPR_TCE_IOMMU) && defined(CONFIG_VFIO_GROUP) static struct iommu_group *kvm_vfio_file_iommu_group(struct file *file) { struct iommu_group *(*fn)(struct file *file); @@ -207,7 +207,7 @@ static int kvm_vfio_file_del(struct kvm_device *dev, unsigned int fd) list_del(&kvf->node); kvm_arch_end_assignment(dev->kvm); -#ifdef CONFIG_SPAPR_TCE_IOMMU +#if defined(CONFIG_SPAPR_TCE_IOMMU) && defined(CONFIG_VFIO_GROUP) kvm_spapr_tce_release_vfio_group(dev->kvm, kvf); #endif kvm_vfio_file_set_kvm(kvf->file, NULL); @@ -226,7 +226,7 @@ static int kvm_vfio_file_del(struct kvm_device *dev, unsigned int fd) return ret; } -#ifdef CONFIG_SPAPR_TCE_IOMMU +#if defined(CONFIG_SPAPR_TCE_IOMMU) && defined(CONFIG_VFIO_GROUP) static int kvm_vfio_file_set_spapr_tce(struct kvm_device *dev, void __user *arg) { @@ -288,7 +288,7 @@ static int kvm_vfio_set_file(struct kvm_device *dev, long attr, return -EFAULT; return kvm_vfio_file_del(dev, fd); -#ifdef CONFIG_SPAPR_TCE_IOMMU +#if defined(CONFIG_SPAPR_TCE_IOMMU) && defined(CONFIG_VFIO_GROUP) case KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE: return kvm_vfio_file_set_spapr_tce(dev, arg); #endif @@ -335,7 +335,7 @@ static void kvm_vfio_release(struct kvm_device *dev) struct kvm_vfio_file *kvf, *tmp; list_for_each_entry_safe(kvf, tmp, &kv->file_list, node) { -#ifdef CONFIG_SPAPR_TCE_IOMMU +#if defined(CONFIG_SPAPR_TCE_IOMMU) && defined(CONFIG_VFIO_GROUP) kvm_spapr_tce_release_vfio_group(dev->kvm, kvf); #endif kvm_vfio_file_set_kvm(kvf->file, NULL); ``` Thought that then makes me think that perhaps even better would be some sort of Kconfig dependency expressed between CONFIG_SPAPR_TCE_IOMMU and CONFIG_VFIO_GROUP. Though I do see: drivers/vfio/Kconfig 7: select VFIO_GROUP if SPAPR_TCE_IOMMU || IOMMUFD=n Perhaps something is wrong with that, and randconfig is able to tickle IOMMUFD=n and still set VFIO_GROUP without SPAPR_TCE_IOMMU. $ grep -rn -e CONFIG_VFIO_GROUP -e CONFIG_SPAPR_TCE_IOMMU -e CONFIG_VFIO_GROUP .config 4678:CONFIG_SPAPR_TCE_IOMMU=y Yi, any thoughts on the above? > > (BTW does clang actually work on power, I tried it a bit ago and it > didn't get very far) So it looks like in CI we build+boot test the following configs: https://github.com/ClangBuiltLinux/continuous-integration2/blob/32a9db9c4a4b4950407dfceb4bd4c36bf7a6ac4e/generator.yml#L2686 - ppc44x_defconfig - ppc64_guest_defconfig - allmodconfig - fedora's ppc config - suse's ppc config I just tried simply defconfig and ran into some hermiticity issues with the kbuild rules for their vdso: https://github.com/ClangBuiltLinux/linux/issues/1601 -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [linux-next:master 3581/12910] arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: error: attribute declaration must precede definition 2023-08-25 18:49 ` Nick Desaulniers @ 2023-08-25 19:40 ` Jason Gunthorpe 2023-08-25 20:04 ` Nick Desaulniers 0 siblings, 1 reply; 6+ messages in thread From: Jason Gunthorpe @ 2023-08-25 19:40 UTC (permalink / raw) To: Nick Desaulniers Cc: Yi Liu, kernel test robot, llvm, oe-kbuild-all, Linux Memory Management List, Alex Williamson, Ard Biesheuvel, Jessica Yu, Kees Cook, Will Deacon, Nathan Chancellor On Fri, Aug 25, 2023 at 11:49:53AM -0700, Nick Desaulniers wrote: > I think this example demonstrates a little clearer what's going on: > https://godbolt.org/z/9d6scv1hE > > So based on the warning, it seems like symbol_get() can only be used > before the parameter it's passed is defined, otherwise the "weak" and > "visibility(hidden)" attributes are ignored. These attributes are > merge-able upon redeclaration up until the first definition. No, that doesn't make sense. The macro does: #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); }) x must be defined for typeof(x) to work. The commit I referenced explains what this is trying to do, it is deliberately trying to change the attributes of an already forward declared function. It seems to me from your godbolt output that the compiler is refusing to allow this if the function already has a body. In this case because it is a static inline. Due to a nonsensical CONFIG combination. Making a static inline a hidden weak references seems kind of crazy in the first place. It looks like the hidden was added to make up for strange things weak does, but I have no idea why weak would be needed here. It predates the git history. > If the definition of vfio_file_iommu_group() is visible to > virt/kvm/vfio.c, do we even need this weak+hidden redeclaration? > ``` > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c > index ca24ce120906..b497b762ddba 100644 > --- a/virt/kvm/vfio.c > +++ b/virt/kvm/vfio.c > @@ -86,7 +86,7 @@ static struct iommu_group > *kvm_vfio_file_iommu_group(struct file *file) > struct iommu_group *(*fn)(struct file *file); > struct iommu_group *ret; > > - fn = symbol_get(vfio_file_iommu_group); > + fn = vfio_file_iommu_group; Yes, the prototype already exists in all cases. The point is that vfio_file_iommu_group is usually not an inline. > But I'll admit I don't fully understand the implications of that > change. Well, it doesn't work for what it is supposed to do at all :) > Thought that then makes me think that perhaps even better would be > some sort of Kconfig dependency expressed between > CONFIG_SPAPR_TCE_IOMMU and CONFIG_VFIO_GROUP. Yeah, there are different layers. An iommu driver should not depend on a VFIO symbol.. > Though I do see: > drivers/vfio/Kconfig > 7: select VFIO_GROUP if SPAPR_TCE_IOMMU || IOMMUFD=n > Perhaps something is wrong with that, and randconfig is able to tickle > IOMMUFD=n and still set VFIO_GROUP without SPAPR_TCE_IOMMU. That should be fine.. > $ grep -rn -e CONFIG_VFIO_GROUP -e CONFIG_SPAPR_TCE_IOMMU -e > CONFIG_VFIO_GROUP .config > 4678:CONFIG_SPAPR_TCE_IOMMU=y I think the issue is that: # CONFIG_VFIO is not set CONFIG_KVM_VFIO=y And kvm-$(CONFIG_KVM_VFIO) += $(KVM)/vfio.o Which is a combination that doesn't make any sense. Looks like CONFIG_KVM_VFIO should probably be called CONFIG_KVM_ARCH_VFIO And then config KVM_VFIO bool depends on KVM_ARCH_VFIO depends on VFIO (or similar) So we don't even attempt to compile kvm/vfio.c if we don't have VFIO support turned on. > > (BTW does clang actually work on power, I tried it a bit ago and it > > didn't get very far) > > So it looks like in CI we build+boot test the following configs: > https://github.com/ClangBuiltLinux/continuous-integration2/blob/32a9db9c4a4b4950407dfceb4bd4c36bf7a6ac4e/generator.yml#L2686 > - ppc44x_defconfig > - ppc64_guest_defconfig > - allmodconfig > - fedora's ppc config > - suse's ppc config > > I just tried simply defconfig and ran into some hermiticity issues > with the kbuild rules for their vdso: > https://github.com/ClangBuiltLinux/linux/issues/1601 That looks familiar.. I just tried again and it seems to have built with a warning: ld.lld-15: warning: address (0xc000000000000100) of section .text is not a multiple of alignment (4096) So that's nice! Thanks, Jason ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [linux-next:master 3581/12910] arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: error: attribute declaration must precede definition 2023-08-25 19:40 ` Jason Gunthorpe @ 2023-08-25 20:04 ` Nick Desaulniers 2023-08-30 17:19 ` Jason Gunthorpe 0 siblings, 1 reply; 6+ messages in thread From: Nick Desaulniers @ 2023-08-25 20:04 UTC (permalink / raw) To: Jason Gunthorpe Cc: Yi Liu, kernel test robot, llvm, oe-kbuild-all, Linux Memory Management List, Alex Williamson, Ard Biesheuvel, Jessica Yu, Kees Cook, Will Deacon, Nathan Chancellor On Fri, Aug 25, 2023 at 12:40 PM Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Fri, Aug 25, 2023 at 11:49:53AM -0700, Nick Desaulniers wrote: > > I think this example demonstrates a little clearer what's going on: > > https://godbolt.org/z/9d6scv1hE > > > > So based on the warning, it seems like symbol_get() can only be used > > before the parameter it's passed is defined, otherwise the "weak" and > > "visibility(hidden)" attributes are ignored. These attributes are > > merge-able upon redeclaration up until the first definition. > > No, that doesn't make sense. The macro does: > > #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); }) > > x must be defined for typeof(x) to work. x must be //declared// for typeof(x) to work, but x doesn't need to be //defined//. https://godbolt.org/z/qv6zPWf9K But I think it's kind of moot, given the kconfig discussion below. > > I think the issue is that: > > # CONFIG_VFIO is not set > CONFIG_KVM_VFIO=y > > And > > kvm-$(CONFIG_KVM_VFIO) += $(KVM)/vfio.o > > Which is a combination that doesn't make any sense. > > Looks like CONFIG_KVM_VFIO should probably be called CONFIG_KVM_ARCH_VFIO > > And then > > config KVM_VFIO > bool > depends on KVM_ARCH_VFIO > depends on VFIO > > (or similar) > > So we don't even attempt to compile kvm/vfio.c if we don't have VFIO > support turned on. Do we need to split the Kconfig? ``` diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig index 484d0873061c..e5ebf89de855 100644 --- a/virt/kvm/Kconfig +++ b/virt/kvm/Kconfig @@ -61,6 +61,7 @@ config HAVE_KVM_CPU_RELAX_INTERCEPT config KVM_VFIO bool + depends on VFIO config HAVE_KVM_INVALID_WAKEUPS bool ``` $ ./scripts/config -e VFIO $ make LLVM=1 ARCH=powerpc -j128 olddefconfig arch/powerpc/kvm/../../../virt/kvm/vfio.o # # configuration written to .config # SYNC include/config/auto.conf.cmd CALL scripts/checksyscalls.sh CC arch/powerpc/kvm/../../../virt/kvm/vfio.o $ -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [linux-next:master 3581/12910] arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: error: attribute declaration must precede definition 2023-08-25 20:04 ` Nick Desaulniers @ 2023-08-30 17:19 ` Jason Gunthorpe 0 siblings, 0 replies; 6+ messages in thread From: Jason Gunthorpe @ 2023-08-30 17:19 UTC (permalink / raw) To: Nick Desaulniers Cc: Yi Liu, kernel test robot, llvm, oe-kbuild-all, Linux Memory Management List, Alex Williamson, Ard Biesheuvel, Jessica Yu, Kees Cook, Will Deacon, Nathan Chancellor On Fri, Aug 25, 2023 at 01:04:31PM -0700, Nick Desaulniers wrote: > > I think the issue is that: > > > > # CONFIG_VFIO is not set > > CONFIG_KVM_VFIO=y > > > > And > > > > kvm-$(CONFIG_KVM_VFIO) += $(KVM)/vfio.o > > > > Which is a combination that doesn't make any sense. > > > > Looks like CONFIG_KVM_VFIO should probably be called CONFIG_KVM_ARCH_VFIO > > > > And then > > > > config KVM_VFIO > > bool > > depends on KVM_ARCH_VFIO > > depends on VFIO > > > > (or similar) > > > > So we don't even attempt to compile kvm/vfio.c if we don't have VFIO > > support turned on. > > Do we need to split the Kconfig? > > ``` > diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig > index 484d0873061c..e5ebf89de855 100644 > --- a/virt/kvm/Kconfig > +++ b/virt/kvm/Kconfig > @@ -61,6 +61,7 @@ config HAVE_KVM_CPU_RELAX_INTERCEPT > > config KVM_VFIO > bool > + depends on VFIO .. I don't know these details about kconfig very well, I assume there is are reason most things are done with the HAVE_xx approach? Jason ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-30 17:19 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-08-25 11:23 [linux-next:master 3581/12910] arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: error: attribute declaration must precede definition kernel test robot 2023-08-25 12:12 ` Jason Gunthorpe 2023-08-25 18:49 ` Nick Desaulniers 2023-08-25 19:40 ` Jason Gunthorpe 2023-08-25 20:04 ` Nick Desaulniers 2023-08-30 17:19 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox