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
