Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7cb9ebd2-5ee1-f00e-5d8e-0fe454f975a0@fb.com>
Date:   Mon, 12 Sep 2022 18:29:33 +0100
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.13.0
Subject: Re: [PATCH v2] libbpf: define bpf_tail_call_static when __clang__ is
 not defined
Content-Language: en-US
To:     James Hilliard <james.hilliard1@gmail.com>, bpf@vger.kernel.org
Cc:     Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Tom Rix <trix@redhat.com>, linux-kernel@vger.kernel.org,
        llvm@lists.linux.dev
References: <20220912063514.2824432-1-james.hilliard1@gmail.com>
From:   Yonghong Song <yhs@fb.com>
In-Reply-To: <20220912063514.2824432-1-james.hilliard1@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cHNvbnB2L1NSWnZZc0lNVkNIMUFEZzlHT0hNK0R2MWhsNG1hdVBJeXQybW84?=
 =?utf-8?B?dDQwVFlqdUJxeldZQUhyTGRQZXFyT3c0OFZnZDVKck93TUVxZmRsQXlMRzZE?=
 =?utf-8?B?RE11Zyt0ZUdRQWJUVVhPb3BDb0VNNFppWVY1bjF6dkRoUVF3ZHBxOE1XL3ps?=
 =?utf-8?B?aHBMVGxORmZhcEdodHN6YVIxN2M5UFpBd0pZRnJQV0VMVitWL25yMCtnK2s3?=
 =?utf-8?B?d2Y3TGE2Vjlua2RLeUZNVjRiU0c3dC9GT1pGRDlIMmtpZXZTS1crTExOaW5w?=
 =?utf-8?B?eEduZXl3MlpBWjVFWmlLZWh4WTZGVVNYWEVqRkJzME9rOURteHh4NzJaRnNT?=
 =?utf-8?B?N25BcnoxZTRRUllNZU9KM1dDd05iN09DdFRzT1dVc2hBSjd1UHIwNXNQYVlv?=
 =?utf-8?B?U3ROTXRmY0NlOXhlZWovSmZHdElPRGJYaXZLY3lKRkZTajlMMklKeWZ1QXc5?=
 =?utf-8?B?cHVNR1YwYzNDQ01rZXljVm1jZ3dNUlZ1Myt3SmFvaVY3K2RFSW5XTkVQcnR0?=
 =?utf-8?B?YWg2cHBZUWFFZmE1UGFQRklLRklUZ3JIZWpjN0ZaNnFwWEVaUFhuU1ZicWJX?=
 =?utf-8?B?clc4ZnpoSzdHd0pGLzdWL1VFVmxQM0JtZHJTaVlhRlR6VE5vUzRGa0htQ096?=
 =?utf-8?B?N0VDakhqOEcxTFo0MFg2enhNRTFqQWNqN0FndUxJWjJsd2duc3pwMldKeERu?=
 =?utf-8?B?dVQwZDlJcUNDbmQ0bXpSRlNrWjRMaldlMTl0VmtYa0VyZ014MUVubUE3VGE0?=
 =?utf-8?B?R1U2dXNjdUVNeW9sSFpVZHhFVDcyWldaSjd4SVJGTHRKa2tCZ2VKNHlYdEk5?=
 =?utf-8?B?MzFIdndFMk4weFVVOVBXM1VqT0FGTmczelNGTVBqbHNhZnlqV21kK0tlZWJU?=
 =?utf-8?B?aUs5LzhwSTVodE4yY09IL29HeTFuRkRHckJCdmYvSmtBeTg2UHB1QlBWNXJ6?=
 =?utf-8?B?d3ozYkVFUEdsdVNGMThodEM0cnNSanRoSDRHK0c3TFlKaVlyWTQxWkV2b3Ey?=
 =?utf-8?B?MTZzcHpZMEJ2UzZDYlhJaUxXV2Jva2dXejk0VkNKdmpid3hacXRBQTQrcW83?=
 =?utf-8?B?U3hkYzh2M3RacTlIZWtxYUZlczRMenNWZnB3a0N0WVdQaXlXTzNmRGNqT3ZI?=
 =?utf-8?B?dTQwVkw5L2xmdVN4VkZhakNrblJFczVTL3hpTlJUdVQyaEtGQ1pkbEVvZ0xq?=
 =?utf-8?B?bUFQZkFIVDRWVGZzczlPNnU3ZEtJOGVTK2piV2FMRGdrek1TbnFSa1pGQkM3?=
 =?utf-8?B?bDNIaENMcE55VStWQVl2VCtjQ0JMMEpZamh4aTN5R1ZRSXJST0dzekVwYmI2?=
 =?utf-8?B?RlAvNGtyUDk5ZEh0R0hNY1FHQUlkU2hhdWR5UEZqeEVDUFZsUkM2Rm85ZUh6?=
 =?utf-8?B?VUNqZG90bHlzT1dqRVlobUZFdXo1MmxQTXBxZlMweDNwNVpERUNOeEl4SDBt?=
 =?utf-8?B?S2diWldNTS9JLzNKYXErNWJzVmdBSTVzaHhzMkVIMUpEWXZQcWt3Z0V0dGxr?=
 =?utf-8?B?VERXNG1tSXdiRGhzQVcyM28xblppTk80bzRzWElWMW14SmtINkRvWnVNelJE?=
 =?utf-8?B?TkZVeTZuWFIwbndNSUNNbkZKWEIvMnJxeDlIQzV4MmFiZlUwSnQrY3VJYzdJ?=
 =?utf-8?B?NG1zMEd3TjUzaHZ6c0IrckhWQ1MzTUkyc1lEZ0tib3VXUXBta0IyWDQyMjlM?=
 =?utf-8?B?dy9WMkxES1VuTWx3UGJOaTZ1VEZvTzV4YTE0S0FhRWswVG5vamFkM0pJNGZx?=
 =?utf-8?B?ckJYYXd5b3ZwOWt1Rkx0MU5VNDVUK080ekJqS09BNVpsUXhlWTFySE44U1pi?=
 =?utf-8?B?VUI4cWl1TjRWZmRyaG0rNnNQZjZWZTg5bVAwSEhHeXp1ZTErMnVzcnNJZ2FT?=
 =?utf-8?B?c1V1MkJVaHc4bm5EbTlac2pkcmU1VnVxd1VHUTlZOXhxYjBzYmM4bVc2Wm1W?=
 =?utf-8?B?WWk3a1Y4VVhZRDJIOUlxL2FRQ1NWM1V5R2g3eU5uOUF1U3JkQ0NmZ0l2cWdQ?=
 =?utf-8?B?bmxLRWY1Q2ZvbHFnVEp1eUx1eUFDYmJuaHljcnl4T1hpQVdTdHRUV3hoYXNa?=
 =?utf-8?B?SnhZbUE0c0tvcWRZdFY3ZGJsT1c1dEduL1dKZzBLVjdIZjlhMGx3dzluWCtV?=
 =?utf-8?Q?RAJqiJsBo5ykpAyT2u+ER98VK?=
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fbef7fc5-75e7-4fd2-ae6e-08da94e45f8c
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Sep 2022 17:29:40.4105
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: +IwIjXc/N3pG2ZW+LUdv8NMXSZsOtyuWOuT+6uFnmg5V1VdG2nQ7dimyXz7mEc96
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR15MB2741
X-Proofpoint-GUID: ErESJmRdkgGJiV8c56XQXRzRlskc0FUr
X-Proofpoint-ORIG-GUID: ErESJmRdkgGJiV8c56XQXRzRlskc0FUr
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-12_12,2022-09-12_02,2022-06-22_01
X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 9/12/22 7:35 AM, James Hilliard wrote:
> The bpf_tail_call_static function is currently not defined unless
> using clang >= 8.
> 
> To support bpf_tail_call_static on GAS we can check if __clang__ is
> not defined to enable bpf_tail_call_static.
> 
> We need to use a GAS assembly syntax check so that the assembler
> is provided GAS compatible assembly as well.
> 
> We can use gasversion to provide a migration path to llvm syntax
> for GAS once llvm syntax is natively supported.

I didn't see a gasversion comparison in asm code.
Is it possible that we compare gasversion to a known
gas version which supports new syntax? If the gasversion
is supported, use the same syntax as llvm. If the
gasversion is not supported, output an illegal insn
and it would be even better if some error information
is printed out on the screen.

> 
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
> Changes v1 -> v2:
>    - use gasversion to detect assembly variant
> ---
>   tools/lib/bpf/bpf_helpers.h | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index 7349b16b8e2f..5b98f5506798 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -131,7 +131,7 @@
>   /*
>    * Helper function to perform a tail call with a constant/immediate map slot.
>    */
> -#if __clang_major__ >= 8 && defined(__bpf__)
> +#if (!defined(__clang__) || __clang_major__ >= 8) && defined(__bpf__)
>   static __always_inline void
>   bpf_tail_call_static(void *ctx, const void *map, const __u32 slot)
>   {
> @@ -139,8 +139,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot)
>   		__bpf_unreachable();
>   
>   	/*
> -	 * Provide a hard guarantee that LLVM won't optimize setting r2 (map
> -	 * pointer) and r3 (constant map index) from _different paths_ ending
> +	 * Provide a hard guarantee that the compiler won't optimize setting r2
> +	 * (map pointer) and r3 (constant map index) from _different paths_ ending
>   	 * up at the _same_ call insn as otherwise we won't be able to use the
>   	 * jmpq/nopl retpoline-free patching by the x86-64 JIT in the kernel
>   	 * given they mismatch. See also d2e4c1e6c294 ("bpf: Constant map key
> @@ -148,12 +148,18 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot)
>   	 *
>   	 * Note on clobber list: we need to stay in-line with BPF calling
>   	 * convention, so even if we don't end up using r0, r4, r5, we need
> -	 * to mark them as clobber so that LLVM doesn't end up using them
> -	 * before / after the call.
> +	 * to mark them as clobber so that the compiler doesn't end up using
> +	 * them before / after the call.
>   	 */
> -	asm volatile("r1 = %[ctx]\n\t"
> +	asm volatile(".ifdef .gasversion.\n\t"
> +		     "mov %%r1,%[ctx]\n\t"
> +		     "mov %%r2,%[map]\n\t"
> +		     "mov %%r3,%[slot]\n\t"
> +		     ".else\n\t"
> +		     "r1 = %[ctx]\n\t"
>   		     "r2 = %[map]\n\t"
>   		     "r3 = %[slot]\n\t"
> +		     ".endif\n\t"
>   		     "call 12"
>   		     :: [ctx]"r"(ctx), [map]"r"(map), [slot]"i"(slot)
>   		     : "r0", "r1", "r2", "r3", "r4", "r5");