* [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 related [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 related [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;
as well as URLs for NNTP newsgroup(s).