From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0D1B2C3DA6F for ; Fri, 25 Aug 2023 18:50:09 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 93DE72800CA; Fri, 25 Aug 2023 14:50:08 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8EE572800C6; Fri, 25 Aug 2023 14:50:08 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 78E862800CA; Fri, 25 Aug 2023 14:50:08 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 67A3B2800C6 for ; Fri, 25 Aug 2023 14:50:08 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 25CD81C9B02 for ; Fri, 25 Aug 2023 18:50:08 +0000 (UTC) X-FDA: 81163516896.29.0A5CDEA Received: from mail-qv1-f52.google.com (mail-qv1-f52.google.com [209.85.219.52]) by imf29.hostedemail.com (Postfix) with ESMTP id 4AB4912001A for ; Fri, 25 Aug 2023 18:50:05 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=6iuloOTX; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf29.hostedemail.com: domain of ndesaulniers@google.com designates 209.85.219.52 as permitted sender) smtp.mailfrom=ndesaulniers@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1692989406; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=h9OpAPTbZ3wNNM0mxwWe5mRvai82387mVIXj9OVtNdk=; b=ZgGHn7eP6tIL+EsZpTKOK1ArZHYiQEKaaWdvxISCk+ciJezxHIMVHwHJDKOJE7Gya3MKmc LQTRPXy9MrA4gL8ZHIZS9zV2e3gtEgkgVteBo8A7VJIEypz8tf4/VU5DpYOgHn2737WrRQ MJNLQXJVARDu+LFWVh2ezgR2wtNzheU= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=6iuloOTX; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf29.hostedemail.com: domain of ndesaulniers@google.com designates 209.85.219.52 as permitted sender) smtp.mailfrom=ndesaulniers@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1692989406; a=rsa-sha256; cv=none; b=jPzT4hzvP0Tc0Ay+c13Vk5HJROxXJ2YVP4TFEQcfPjwZyVSLbgMqReeOrqd+46R6fFGkPW C1qCM08t6pEVY3UAWM+C9NT7q18+5+1c0k4vGBbxqR7cQDzoQdRbgzmwsGbEZXy0pOFsoF XaTkfffy4jRW1Yu9cbaq8GOvxhYs4Qs= Received: by mail-qv1-f52.google.com with SMTP id 6a1803df08f44-649a11843b3so7593126d6.0 for ; Fri, 25 Aug 2023 11:50:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1692989405; x=1693594205; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=h9OpAPTbZ3wNNM0mxwWe5mRvai82387mVIXj9OVtNdk=; b=6iuloOTXDJ8ZCqCBNofRfCNpRB/xEoBcGKnt7i5XEFPJbB6KQagZqwHtNTVoTBMm12 qdAKGIc1fdSFd37Ad5yCkAkAnLKYZHlV/Ii969cnuICosCa0ZCI8ZeZaVnpga5qkI2eZ he+0zhe5qWfjtgbU4HObfbl+6Mvw+jBbtsDT/iYtErhMVbfbseScSKTHbYRi52+n1wjy tyGPCAf/RlQuX5sAaV7Wf8S1zHKu9aPe3gvHiC86ArBd3qSrgi5nRk4D02Y/LRU5buTj 8by0CqLpNJC4cK9oXpnH7nhns2Avv4IYFPTg+Gx07ZZk3X+RkxJLfMwkn32XSIxleeIw eMOw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1692989405; x=1693594205; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=h9OpAPTbZ3wNNM0mxwWe5mRvai82387mVIXj9OVtNdk=; b=gjQF9r3UQ3onmijnjqeIH2/snyyk5EbYS9SmuYnT/PxER6GGjUYsDF3hO9tUsZEKcD I//qd1m7RuWRIc46XP1RGoCmghBEdTIRt8ge64k6OGkRl7IVCD+daqDe9IJZReLfqwZ5 PzDb7peus9yp9AQfrwVV2xzIBkMTisX7ejVDmA0fdoj7w3STOTTtzGBweIyheKtqYFj4 W8CMsYjnXzJiCxeEn/5weGwZN57W+hoY1zrqZ/A7GynOn34bUsBVfKWAI9BSt8SnXhWz nJGwdPxZ9TfEa7k0fvraToOlgR6fqUaPBfFdmXuSC8B9Ya05kGB+veGJG1Afx0Ntlw6E WA0A== X-Gm-Message-State: AOJu0YxPAjKxBP/CAXYJuY8Iyo1bUji88/FkREG5JBNWTk3kTofW8xhi z4U0IykcHIy3BwLeh6A6w8jylbr5DqWsrJ8KwsF6Vw== X-Google-Smtp-Source: AGHT+IFq+roUlNyt9X1sD8n8JL+O82NlCWsAGoBXHT60lEuPonY8dvkPphl1k7OLoVkpKekR8n1wvi0vOjqY/DcDoJc= X-Received: by 2002:a0c:c984:0:b0:64f:6d93:3cbe with SMTP id b4-20020a0cc984000000b0064f6d933cbemr6207479qvk.47.1692989405367; Fri, 25 Aug 2023 11:50:05 -0700 (PDT) MIME-Version: 1.0 References: <202308251949.5IiaV0sz-lkp@intel.com> In-Reply-To: From: Nick Desaulniers Date: Fri, 25 Aug 2023 11:49:53 -0700 Message-ID: Subject: Re: [linux-next:master 3581/12910] arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: error: attribute declaration must precede definition To: Jason Gunthorpe , Yi Liu Cc: kernel test robot , llvm@lists.linux.dev, oe-kbuild-all@lists.linux.dev, Linux Memory Management List , Alex Williamson , Ard Biesheuvel , Jessica Yu , Kees Cook , Will Deacon , Nathan Chancellor Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: z66yj9hofj6urrfhdnxgiw9t5haqstyn X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 4AB4912001A X-HE-Tag: 1692989405-525196 X-HE-Meta: U2FsdGVkX1/2cR9stiOdmv3VDD2Cc7JOgxnt7Ij3EH9Fsw7ifPGBSbj1rPOAjxw9IOycW3/p7D840VCxNb/8XitZQKfnOm7KIGrnHfzpX5EAzII1Hjev+qyZeiaBOdLqjA5zdIKR3YIijnFCO9PpcELh2C9eFXxMmlge1AzrlUomPTOc+zpCc8yeT2rUe1f4TD/5vUtsGl8Uwo2zXAhDJgelVeybyB7PHbQ8XcxfjkjgU++6UTeFPa8woJ5Yi8/vHMXTKNv2ARxsaBbsPaVkxMvrcrhPJGHdOzNZPDOKQyGwxkOp2Fnn5iFE/fKDcgIgXX1o+PShROAwpSw4uYov7DQM/h9L+EVYOeRkHbBT84/5mvkdbZhfZZBVcpyD69eeLAbrPPbhh0ETTNAvUdQD1dwVy28Lr9cnIAUeXdnLD8OlOR+ibKXqNAeCRXfqHlyIs6oy6d5beELyyRQNmzfgrgZrx0eqt1/mWAO3krM/871aAQR/Db9IV52wGCVTTW9a2dSes5T8RJuqBNu+TPddLDsxULRJ83AuW/tnCChttUuHK2Wv4NSI/239MorZmhnqMKfENHM+RJmeH/V1JZaidgFStFpeO1TBCq58XFTfUPbB9BJQs549ZlyZbapyzJy9368mOfUzRuD1YL88NbSLxxp+R6zNsaY7scbBjTfn3S2G/HmTy4zS6gPskjlJsi0hQ7sXc4ZdD95E2esJSCT/3M+NmsIbcUUa7rUO1nF053cEpZ+zAaov9gtaPWKKZ7pM3euw3TTPlhBFLs9tSy10ZzRi40OTDHmEaT1Tn7MBO1aNbnaER4MuWZH4+TUGU+vaHEq1FKHN9II4NuSOVIZMYh+19xz4ktgGVItXXh4ijACTqP+sIjjiTwZ6fDn+ZOetdGYDZPqwc7oL0RKz6NPHX8B1fKKUX7DHUEWKF0VQcQHZ3mAztieI0paWQzM4/ryLiD2ho9o5JDmKRLf6BQE UfvBlhjK Kfx29nbjXqzSKrcaARUTxCLgmjW3GZ4TY8mbuMbrP6/0iDrl48XGtLQT0sWAZ209y1n7Ui2FvltJuFjII3WHtnBiWBcUHb42m9PSM+qeX83mSHl5EuawlesULDY/pJ8NibbB7QLaQIQ1hWST21BCSHBPfdz4Yhp4+kAF2GtIpRrr/nAmodTrtIAOzl9cCVu7GTJRHTRPiWbr+2yn0zHXp9eAXrGCJP3oZLvw1ATTC+d1Z1e5TvrGNjt6ywLzGgkceXadxUGuFTDT3zEZg/b1jSOtHbfyFz2VocOuIFsmS/VxE+Bjkuen1bHORO47d3547F4IbIvn5Oo/kNCifTKP3F1J/argRtq3UpzpIKOZA68wcuIpVC9IS6OXDDR/FuVVJ+FuHdvFvpcXREhPS7ya7UnbnKSClmClUWtKsBjjDJfWl3y76fJz+H5+06x3QqC2tOVVHEN3q+yZ1OfcdPxOxMM5DSUO2ulT4VutF X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Aug 25, 2023 at 5:12=E2=80=AFAM Jason Gunthorpe wr= ote: > > 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: Com= pile vfio_group infrastructure optionally > > config: powerpc64-randconfig-r001-20230825 (https://download.01.org/0da= y-ci/archive/20230825/202308251949.5IiaV0sz-lkp@intel.com/config) > > compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.gi= t ae42196bc493ffe877a7e3dff8be32035dea4d07) > > reproduce: (https://download.01.org/0day-ci/archive/20230825/2023082519= 49.5IiaV0sz-lkp@intel.com/reproduce) > > > > If you fix the issue in a separate patch/commit (i.e. not just a new ve= rsion of > > the same patch/commit), kindly add following tags > > | Reported-by: kernel test robot > > | Closes: https://lore.kernel.org/oe-kbuild-all/202308251949.5IiaV0sz-l= kp@intel.com/ > > > > All errors (new ones prefixed by >>): > > > > >> arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7: error: attribute dec= laration must precede definition [-Werror,-Wignored-attributes] > > fn =3D 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,visi= bility("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 dec= laration must precede definition [-Werror,-Wignored-attributes] > > fn =3D 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,visi= bility("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 > 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 configuratio= ns > 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/1034688d58783779168d59b47d2b3e897= ad869c6/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 =3D symbol_get(vfio_file_iommu_group); + fn =3D 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.5IiaV0= sz-lkp@intel.com/config -O .config $ make LLVM=3D1 ARCH=3Dpowerpc -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=3Dn 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 =3D 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=3Dn Perhaps something is wrong with that, and randconfig is able to tickle IOMMUFD=3Dn 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=3Dy 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/32a9db9c4a4= b4950407dfceb4bd4c36bf7a6ac4e/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 --=20 Thanks, ~Nick Desaulniers