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: [PATCH v2] x86/cpufeature: guard asm_volatile_goto usage with
 NO_BPF_WORKAROUND
From:   Yonghong Song <yhs@fb.com>
To:     Peter Zijlstra <peterz@infradead.org>
CC:     <mingo@kernel.org>, <ast@fb.com>, <daniel@iogearbox.net>,
        <linux-kernel@vger.kernel.org>, <x86@kernel.org>,
        <kernel-team@fb.com>
References: <20180415042738.1788215-1-yhs@fb.com>
 <20180420081936.GD4064@hirez.programming.kicks-ass.net>
 <fd14097e-31c5-0c0e-cbe5-d399299ca489@fb.com>
 <20180423105209.GX4064@hirez.programming.kicks-ass.net>
 <3a6e6348-fb23-c2f9-5bd8-928b056b4722@fb.com>
 <8bf96497-2fc3-8d11-1c40-cb027eae44a6@fb.com>
Message-ID: <0bcea2d8-c088-f8f5-ae76-4ce5f974e291@fb.com>
Date:   Wed, 2 May 2018 07:01:18 -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: <8bf96497-2fc3-8d11-1c40-cb027eae44a6@fb.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Received-SPF: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 May 2018 14:01:31.6850 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: a045ef54-38bd-49e4-03e9-08d5b035361b
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO2PR15MB0075
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-05-02_05:,,
 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


Hi, Peter,

Ping again. Did you get chances to think about this issue again?

Thanks!

Yonghong

On 4/27/18 9:34 AM, Yonghong Song wrote:
> Hi, Peter,
> 
> Just wanted to ping again so that you did not miss the email below.
> Please let me know your opinion.
> 
> Thanks!
> 
> Yonghong
> 
> 
> On 4/23/18 9:50 AM, Yonghong Song wrote:
>> Hi, Peter,
>>
>> Please see comments below.
>>
>> On 4/23/18 3:52 AM, Peter Zijlstra wrote:
>>> On Fri, Apr 20, 2018 at 11:06:03AM -0700, Yonghong Song wrote:
>>>> On 4/20/18 1:19 AM, Peter Zijlstra wrote:
>>>
>>>>> Hurm, so adding __BPF__ for BPF compiles isn't an option? It seems 
>>>>> to me
>>>>> having a CPP flag to identify BPF compile context might be useful in
>>>>> general.
>>>>
>>>> With "clang -target bpf", we already have __BPF__ defined.
>>>> For tracing, esp. ptrace.h is included, "clang -target 
>>>> <native_arch>" where
>>>> "-target <native_arch>" can be omitted, is typically used.
>>>
>>>> The reason is the native architecture header files typically
>>>> include a lot of various asm related stuff where "-target bpf" cannot
>>>> really handle. We relay on native clang to flush out all these
>>>> asm constructs and only bpf program needed stuff survives
>>>> reach to backend compiler.
>>>
>>> So because 'clang -target bpf' is 'broken', you do a work-around using
>>
>> 'clang -target bpf' is 'broken' in this case because the x86 arch has
>> a lot of inline asm's in the header file where bpf target cannot handle.
>> For most networking related bpf programs where `asm` is rarely involved,
>> `clang -target bpf` works fine most of time.
>>
>>> 'clang -target <native_arch>'. But because that doesn't set __BPF__ you
>>
>> `clang -target <native_arch>` should work, regardless of whether __BPF__
>> macro is setup or not. The reason it doesn't work now is due to its 
>> lacking asm-goto support. So to use `clang -target <native_arch>` is not
>> really a workaround for `target bpf`. It by itself should work.
>>
>>> want to add NO_BPF_WORKAROUND to the kernel instead of adding __BPF__ to
>>> your build rules to better mimick -target bpf, which you should be
>>> using.
>>>
>>> How is that sane? Why not use 'clang -target <native_arch> -D__BPF__'
>>
>> To workaround the asm-goto issue, the suggested macro __BPF__ can be 
>> added to user space and kernel. But note that `clang -target 
>> <native_arch>` will not define the macro __BPF__, so this requires
>> user space change.
>>
>> Also, to make sure people understand that this is a WORKAROUND for
>> asm-goto issue and is not a lasting thing we want to support. I have
>> the following change for cpufeature.h:
>>
>> diff --git a/arch/x86/include/asm/cpufeature.h 
>> b/arch/x86/include/asm/cpufeature.h
>> index b27da9602a6d..c832118defa1 100644
>> --- a/arch/x86/include/asm/cpufeature.h
>> +++ b/arch/x86/include/asm/cpufeature.h
>> @@ -140,6 +140,7 @@ extern void clear_cpu_cap(struct cpuinfo_x86 *c, 
>> unsigned int bit);
>>
>>   #define setup_force_cpu_bug(bit) setup_force_cpu_cap(bit)
>>
>> +#ifndef __BPF_WORKAROUND__
>>   /*
>>    * Static testing of CPU features.  Used the same as boot_cpu_has().
>>    * These will statically patch the target code for additional
>> @@ -195,6 +196,9 @@ static __always_inline __pure bool 
>> _static_cpu_has(u16 bit)
>>                  boot_cpu_has(bit) :                             \
>>                  _static_cpu_has(bit)                            \
>>   )
>> +#else
>> +#define static_cpu_has(bit)            boot_cpu_has(bit)
>> +#endif
>>
>>   #define cpu_has_bug(c, bit)            cpu_has(c, (bit))
>>   #define set_cpu_bug(c, bit)            set_cpu_cap(c, (bit))
>> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
>> index 4d6a6edd4bf6..b229e5090e4a 100644
>>
>> As mentioned above, user space needs to add this new macro definition.
>> Specifically for kernel/samples/bpf:
>> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
>> index 4d6a6edd4bf6..b229e5090e4a 100644
>> --- a/samples/bpf/Makefile
>> +++ b/samples/bpf/Makefile
>> @@ -255,7 +255,7 @@ $(obj)/tracex5_kern.o: $(obj)/syscall_nrs.h
>>   $(obj)/%.o: $(src)/%.c
>>          $(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) 
>> -I$(obj) \
>>                  -I$(srctree)/tools/testing/selftests/bpf/ \
>> -               -D__KERNEL__ -Wno-unused-value -Wno-pointer-sign \
>> +               -D__KERNEL__ -D__BPF_WORKAROUND__ -Wno-unused-value 
>> -Wno-pointer-sign \
>>                  -D__TARGET_ARCH_$(ARCH) 
>> -Wno-compare-distinct-pointer-types \
>>                  -Wno-gnu-variable-sized-type-not-at-end \
>>                  -Wno-address-of-packed-member 
>> -Wno-tautological-compare \
>>
>> Please let me know whether this approach is okay to you or not,
>> whether the name __BPF_WORKAROUND__ is better than __BPF__ or not, or 
>> we could use the earlier approach which does not require user space 
>> change.
>>
>> Thanks!