Re: [linux-next:master 3581/12910] arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: error: attribute declaration must precede definition

[Jason Gunthorpe <jgg@nvidia.com>]

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