Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: bpf: handling non BPF register names in inline assembly with
 -target bpf
To:     Arnaldo Carvalho de Melo <acme@redhat.com>
CC:     Alexei Starovoitov <ast@fb.com>,
        Daniel Borkmann <daniel@iogearbox.net>,
        David Miller <davem@davemloft.net>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>, <acme@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <c8b0b3c0-6687-1be2-66d0-95b20662145d@fb.com>
 <20180411183902.GC12166@redhat.com>
From:   Yonghong Song <yhs@fb.com>
Message-ID: <613f0a0d-c433-8f4d-dcc1-c9889deae39e@fb.com>
Date:   Wed, 11 Apr 2018 12:22:37 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:52.0)
 Gecko/20100101 Thunderbird/52.7.0
MIME-Version: 1.0
In-Reply-To: <20180411183902.GC12166@redhat.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Received-SPF: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Apr 2018 19:22:43.0019 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 870045c4-9d51-47ae-cd53-08d59fe199eb
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLUPR15MB0065
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-04-11_08:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 4/11/18 11:39 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 11, 2018 at 09:37:46AM -0700, Yonghong Song escreveu:
>> Hi, Arnaldo,
>   
>> When I studied the bpf compilation issue with latest linus/net-next
>> kernel (https://patchwork.kernel.org/patch/10333829/), an alternative
>> approach I tried is to use __BPF__ macro.
> 
> You mean you used an alternative approach that does _not_ use the
> __BPF__ macro, right? I looked at the patch and yeah, looks sane as

Right.

> well, since the kernel build process already defines that
> CC_HAVE_ASM_GOTO, checking if gcc has that feature, etc.
> 
>> The following patch introduced "#ifndef __BPF__" in
>> arch/x86/include/asm/asm.h for some inline assembly related to x86
>> "esp" register name.
>   
> 
>> ==========
>>   commit ca26cffa4e4aaeb09bb9e308f95c7835cb149248
>> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Date:   Mon Dec 4 13:08:47 2017 -0300
>>
>>      x86/asm: Allow again using asm.h when building for the 'bpf'
>> clang target
>>
>>      Up to f5caf621ee35 ("x86/asm: Fix inline asm call constraints
>> for Clang")
>>      we were able to use x86 headers to build to the 'bpf' clang target, as
>>      done by the BPF code in tools/perf/.
>>   ...
>> diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
>> index 219faae..386a690 100644
>> --- a/arch/x86/include/asm/asm.h
>> +++ b/arch/x86/include/asm/asm.h
>> @@ -136,6 +136,7 @@
>>   #endif
>>
>>   #ifndef __ASSEMBLY__
>> +#ifndef __BPF__
>>   /*
>>    * This output constraint should be used for any inline asm which
>> has a "call"
>>    * instruction.  Otherwise the asm may be inserted before the frame
>> pointer
>> @@ -145,5 +146,6 @@
>>   register unsigned long current_stack_pointer asm(_ASM_SP);
>>   #define ASM_CALL_CONSTRAINT "+r" (current_stack_pointer)
>>   #endif
>> +#endif
>> ...
>> ==========
>>
>> I just landed a clang patch (clang 7.0.0 trunk)
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_rL329823&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=1D1Hmeg3QWPTsbr6rIHgNpk8Fiqk1jkAvrluQQsjk2Y&s=P5EaOqBvaqpDpx3di0Qm1W2fMKaooJJ7b6IkiGD6NU8&e=
>> which will permit bpf clang target to accept ANY register
>> names. In this case, the inline assembly will be accepted by clang
>> and will be thrown away since variable current_stack_pointer is
>> not used in bpf programs.
> 
> Ok, then that ifndef __BPF__ above will not be needed anymore, but only
> people with clang > that version will be able to build tools/perf/

Yes.

I have a suggestion, not sure whether it works for you or not.
The whole issue you needs to introduce this __BPF__ is because
you included ptrace.h with "clang -target bpf ...". Typically,
when people include ptrace.h, they use "clang <native_arch> ..."
and "llc -march=bpf ..." since ptrace.h and its dependent header
files added a lot of assembly codes and asm code related constructs or 
maybe other arch-spec #define as well, which bpf target cannot handle.

Look at test bpf-script-test-kbuild.c, I think you can drop
uapi/asm/ptrace.h from include file list. This way, you do not need
__BPF__ in x86/include/asm/asm.h. At the same time, you can
remove __BPF__ as well.

The clang compiler change I had is just a bonus. It intends to
help "clang -target bpf ..." users just in case their bpf program
header files contains some host-specific insn asm codes with host-arch
register names, for which I expect most users won't hit this.

>   
>> If the inline assembly is indeed for BPF program, later llc
>> AsmParser will do syntax and semantics checking again.
>>
>> With the above clang patch, the above "#ifndef __BPF__" can be removed.
>> You can decide when is the appropriate time to use latest clang compiler
>> and remove the above "#ifndef __BPF__".
> 
> So are you proposing that we have something similar to that
> CC_HAVE_ASM_GOTO check in the kernel main Makefile, to define something
> like CC_HAVE_ASM_REGS (or some better name), i.e. something like:
> 
> # check for 'asm(_ASM_SP)'
> ifeq ($(call shell-cached,$(CONFIG_SHELL) $(srctree)/scripts/cc-asm-regs.sh
> $(CC) $(KBUILD_CFLAGS)), y)
>    CC_HAVE_ASM_REGS := 1
>    KBUILD_CFLAGS += -DCC_HAVE_ASM_REGS
>    KBUILD_AFLAGS += -DCC_HAVE_ASM_REGS
> endif

No, I am not suggesting a kernel change like this.
If you can change bpf-script-test-kbuild.c file and get rid of __BPF__,
it will be good. Otherwise, we can leave __BPF__ there for a while
until later clang 7.0 becomes mainstream and then we can remove it.
I just want to you to be aware that there is a clang feature to solve
your problem.

Thanks!

Yonghong

> ?
> 
> - Arnaldo
>