Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 07 Dec 2020 13:04:23 -0800
From:   John Fastabend <john.fastabend@gmail.com>
To:     Brendan Jackman <jackmanb@google.com>, bpf@vger.kernel.org
Cc:     Alexei Starovoitov <ast@kernel.org>, Yonghong Song <yhs@fb.com>,
        Daniel Borkmann <daniel@iogearbox.net>,
        KP Singh <kpsingh@chromium.org>,
        Florent Revest <revest@chromium.org>,
        linux-kernel@vger.kernel.org, Jann Horn <jannh@google.com>,
        Brendan Jackman <jackmanb@google.com>
Message-ID: <5fce98d7d860e_5a9620833@john-XPS-13-9370.notmuch>
In-Reply-To: <20201207160734.2345502-2-jackmanb@google.com>
References: <20201207160734.2345502-1-jackmanb@google.com>
 <20201207160734.2345502-2-jackmanb@google.com>
Subject: RE: [PATCH bpf-next v4 01/11] bpf: x86: Factor out emission of ModR/M
 for *(reg + off)
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Brendan Jackman wrote:
> The case for JITing atomics is about to get more complicated. Let's
> factor out some common code to make the review and result more
> readable.
> 
> NB the atomics code doesn't yet use the new helper - a subsequent
> patch will add its use as a side-effect of other changes.
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---

Small nit on style preference below.

Acked-by: John Fastabend <john.fastabend@gmail.com>

[...]

>  
> @@ -1240,11 +1250,7 @@ st:			if (is_imm8(insn->off))
>  			goto xadd;
>  		case BPF_STX | BPF_XADD | BPF_DW:
>  			EMIT3(0xF0, add_2mod(0x48, dst_reg, src_reg), 0x01);
> -xadd:			if (is_imm8(insn->off))
> -				EMIT2(add_2reg(0x40, dst_reg, src_reg), insn->off);
> -			else
> -				EMIT1_off32(add_2reg(0x80, dst_reg, src_reg),
> -					    insn->off);
> +xadd:			emit_modrm_dstoff(&prog, dst_reg, src_reg, insn->off);

I at least prefer the xadd on its own line above the emit_*(). That seems
more consistent with the rest of the code in this file. The only other
example like this is st:.