From: Daniel Borkmann <daniel@iogearbox•net>
To: Alexei Starovoitov <alexei.starovoitov@gmail•com>,
Y Song <ys114321@gmail•com>
Cc: Edward Cree <ecree@solarflare•com>,
David Miller <davem@davemloft•net>,
netdev <netdev@vger•kernel.org>,
Jiong Wang <jiong.wang@netronome•com>,
Jakub Kicinski <jakub.kicinski@netronome•com>
Subject: Re: [PATCH net-next] bpf/verifier: improve disassembly of BPF_END instructions
Date: Mon, 25 Sep 2017 23:44:02 +0200 [thread overview]
Message-ID: <59C978A2.6070405@iogearbox.net> (raw)
In-Reply-To: <20170924055016.w6x5tj6kjxjbocpl@ast-mbp>
On 09/24/2017 07:50 AM, Alexei Starovoitov wrote:
> On Fri, Sep 22, 2017 at 09:49:10PM -0700, Y Song wrote:
>> On Fri, Sep 22, 2017 at 9:23 AM, Edward Cree <ecree@solarflare•com> wrote:
>>> On 22/09/17 16:16, Alexei Starovoitov wrote:
>>>> looks like we're converging on
>>>> "be16/be32/be64/le16/le32/le64 #register" for BPF_END.
>>>> I guess it can live with that. I would prefer more C like syntax
>>>> to match the rest, but llvm parsing point is a strong one.
>>> Yep, agreed. I'll post a v2 once we've settled BPF_NEG.
>>>> For BPG_NEG I prefer to do it in C syntax like interpreter does:
>>>> ALU_NEG:
>>>> DST = (u32) -DST;
>>>> ALU64_NEG:
>>>> DST = -DST;
>>>> Yonghong, does it mean that asmparser will equally suffer?
>>> Correction to my earlier statements: verifier will currently disassemble
>>> neg as:
>>> (87) r0 neg 0
>>> (84) (u32) r0 neg (u32) 0
>>> because it pretends 'neg' is a compound-assignment operator like +=.
>>> The analogy with be16 and friends would be to use
>>> neg64 r0
>>> neg32 r0
>>> whereas the analogy with everything else would be
>>> r0 = -r0
>>> r0 = (u32) -r0
>>> as Alexei says.
>>> I'm happy to go with Alexei's version if it doesn't cause problems for llvm.
>>
>> I got some time to do some prototyping in llvm and it looks like that
>> I am able to
>> resolve the issue and we are able to use more C-like syntax. That is:
>> for bswap:
>> r1 = (be16) (u16) r1
>> or
>> r1 = (be16) r1
>> or
>> r1 = be16 r1
>> for neg:
>> r0 = -r0
>> (for 32bit support, llvm may output "w0 = -w0" in the future. But
>> since it is not
>> enabled yet, you can continue to output "r0 = (u32) -r0".)
>>
>> Not sure which syntax is best for bswap. The "r1 = (be16) (u16) r1" is most
>> explicit in its intention.
>>
>> Attaching my llvm patch as well and cc'ing Jiong and Jakub so they can see my
>> implementation and the relative discussion here. (In this patch, I did
>> not implement
>> bswap for little endian yet.) Maybe they can provide additional comments.
>
> This is awesome. In such case I'd like to swing back to the C syntax for bpf_end :)
> Any of these
> r1 = (be16) (u16) r1
> or
> r1 = (be16) r1
> or
> r1 = be16 r1
> are better than just
> be16 r1
> I like 1st the most, since it's explicit in terms of what happens with upper bits,
> but 2nd is also ok. 3rd is not quite C-like.
But above cast to be16 also doesn't seem quite C-like in terms
of what we're actually doing... 3rd option would be my personal
preference even if it doesn't look C-like, but otoh we also have
'call' etc which is neither.
next prev parent reply other threads:[~2017-09-25 21:44 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-21 15:09 [PATCH net-next] bpf/verifier: improve disassembly of BPF_END instructions Edward Cree
2017-09-21 15:52 ` Alexei Starovoitov
2017-09-21 16:24 ` Edward Cree
2017-09-21 16:40 ` Alexei Starovoitov
2017-09-21 16:40 ` Y Song
2017-09-21 16:58 ` Edward Cree
2017-09-21 19:29 ` Daniel Borkmann
2017-09-21 19:44 ` Alexei Starovoitov
2017-09-21 19:58 ` Edward Cree
2017-09-21 23:11 ` Y Song
2017-09-22 13:46 ` Edward Cree
2017-09-22 14:11 ` Y Song
2017-09-22 14:27 ` Y Song
2017-09-22 15:16 ` Alexei Starovoitov
2017-09-22 16:23 ` Edward Cree
2017-09-23 4:49 ` Y Song
2017-09-24 5:50 ` Alexei Starovoitov
2017-09-25 21:44 ` Daniel Borkmann [this message]
2017-09-26 1:33 ` Alexei Starovoitov
2017-09-26 14:37 ` Edward Cree
2017-09-21 16:30 ` Y Song
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=59C978A2.6070405@iogearbox.net \
--to=daniel@iogearbox$(echo .)net \
--cc=alexei.starovoitov@gmail$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=ecree@solarflare$(echo .)com \
--cc=jakub.kicinski@netronome$(echo .)com \
--cc=jiong.wang@netronome$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=ys114321@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox