On Sat, Nov 28, 2020 at 05:27:48PM -0800, Alexei Starovoitov wrote:
> On Fri, Nov 27, 2020 at 05:57:33PM +0000, Brendan Jackman wrote:
> >
> > /* atomic op type fields (stored in immediate) */
> > -#define BPF_FETCH 0x01 /* fetch previous value into src reg */
> > +#define BPF_XCHG (0xe0 | BPF_FETCH) /* atomic exchange */
> > +#define BPF_CMPXCHG (0xf0 | BPF_FETCH) /* atomic compare-and-write */
> > +#define BPF_FETCH 0x01 /* fetch previous value into src reg or r0*/
>
> I think such comment is more confusing than helpful.
> I'd just say that the fetch bit is not valid on its own.
> It's used to build other instructions like cmpxchg and atomic_fetch_add.
OK sounds good.
> > + } else if (BPF_MODE(insn->code) == BPF_ATOMIC &&
> > + insn->imm == (BPF_CMPXCHG)) {
>
> redundant ().
Ack, thanks
> > + verbose(cbs->private_data, "(%02x) r0 = atomic%s_cmpxchg(*(%s *)(r%d %+d), r0, r%d)\n",
> > + insn->code,
> > + BPF_SIZE(insn->code) == BPF_DW ? "64" : "",
> > + bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
> > + insn->dst_reg, insn->off,
> > + insn->src_reg);
> > + } else if (BPF_MODE(insn->code) == BPF_ATOMIC &&
> > + insn->imm == (BPF_XCHG)) {
>
> redundant ().
Ack, thanks
