From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x22d.google.com (mail-oi1-x22d.google.com [IPv6:2607:f8b0:4864:20::22d]) by sourceware.org (Postfix) with ESMTPS id D409C3860763 for ; Wed, 14 Feb 2024 22:13:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D409C3860763 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=obs.cr Authentication-Results: sourceware.org; spf=none smtp.mailfrom=obs.cr ARC-Filter: OpenARC Filter v1.0.0 sourceware.org D409C3860763 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::22d ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1707948830; cv=none; b=xIf598frdyOBKTTjTdhUJtFpAwi/D/b0gdgEXtN7savcd22SbAUi4TeK4McUiBACHUEJs2w4ZdHPFfEz0N7eVPMCQalUGUMbQ4TWJSLXFCi+U0L0FNolIN8L6WxDWOFQjpL/lADFtjTJESEmg9OLnnLEiOm+MeGxvEKLqGTN2U0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1707948830; c=relaxed/simple; bh=mu1qYGUGG3KsM3bEDvcils+WozFly0E51ume1pAaUBw=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=ZXpdM06Dhb5weNYA62zTcVa/j6AvWwt6sdOU6CKYlZ/PBrZ6U3NKMYpw0swYvmnarMEkOHdMK9tJodJQFMX0pwV28HTl2hog5XZu8gktS30L5Z49bMLS259Ht5vvLi2DfIadpuJdN6O8h+gYOzaYIOD0XVVS4oQRfbDBjGb6FbE= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-oi1-x22d.google.com with SMTP id 5614622812f47-3c0a36077a5so208556b6e.0 for ; Wed, 14 Feb 2024 14:13:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=obs-cr.20230601.gappssmtp.com; s=20230601; t=1707948827; x=1708553627; darn=sourceware.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=lL+3onmFrjaOcWEog1RdiFdTm0EOldPNwnr0FJz3AR0=; b=WJG3T2Vcpo3DgzwiM23AHXywrZa+A+4ZFz0UVUDSSHVKx4fKH2Vz8HBMgpCKCr+c8/ Djg/Z9yJMG1NJi4aY8amXSJkoRaSQWbyT17rVdfPMQ+O2ToDtRjNtg+/uWB5rp9MK+cT Jdx9z1d78TAfPeevnQ2SoYFKnPuh5wVRuO/n6g/BlNjjeYNOHvKdjFSpAuiOSRYkyJmC aAYHnT6AFZGBo72uFSUFBYzmZLYm2u/F/aMQ+dVKca7b8PURN81EhBzZ9M70M+ug2W8r /cCc9PKRLG7c56ffD8Udcv7+vakCksYRfIPA9ujp2dg3eLnxlZZFUSlmFDNSjD+zZK7l 2DTw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707948827; x=1708553627; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=lL+3onmFrjaOcWEog1RdiFdTm0EOldPNwnr0FJz3AR0=; b=j7CR6JXRdiVduZ6memd8rzBpPLAybwQNG4lp2h2u7rXpbP00bdliXj5OGv3rW5d3/y F2HxYIVapSPg/QDdMByYZwxBUpDsJL7ESn0Rgv7E+2b3Rr8soOXHXpgjQtUP2pwbbA7q 4gvPjEcjjy8U7BHD7HCauy3RJYHVwFBEBy1MQomOX0zQrO+gT8PcR62BSkZb8+sHgD2T R1C0HYR4MzdAmNufSGj1M+bFkp2MYqExsvO9eLOa2CmLTVo0eqiPohR810pPEULaEVGX 59ctfSQQwroyBWECqDbhrfJlmPZq+UtSyj1KLlpS7x9gX7BIhNqp2F+RTPKq5JJyPNev L3Pw== X-Gm-Message-State: AOJu0YxSNgA7NmqbumY+ARsnZd49bunaK+h8Z0ZGDdJ6yVnF1qDUxyNp z5yFHeNrkjM+U7D2EN7ucZwOZnZbuItYHBVXBMpsNwu0FHETuW4uWlywYKyfMWVMqlXL7uJxLMM bQubvWGeMTIXO/rY45up4S3hj60JRUtIPxNVU3g== X-Google-Smtp-Source: AGHT+IH5DDYJAITDzY+koSzBQvzMIxT2VmmUK3v6Kvg/xSVVQeRcQGFvB/aOL6MTmZbpV+b7j5LOUYZmId/bp2adEWo= X-Received: by 2002:aca:280f:0:b0:3c0:2fd9:5158 with SMTP id 15-20020aca280f000000b003c02fd95158mr117772oix.49.1707948827125; Wed, 14 Feb 2024 14:13:47 -0800 (PST) MIME-Version: 1.0 References: <20240214160303.869180-1-hawkinsw@obs.cr> <20240214160303.869180-2-hawkinsw@obs.cr> <87bk8jknyo.fsf@oracle.com> In-Reply-To: From: Will Hawkins Date: Wed, 14 Feb 2024 17:13:36 -0500 Message-ID: Subject: Re: [PATCH v3 1/1] objdump, as: Add callx support for BPF CPU v1 To: "Jose E. Marchesi" Cc: binutils@sourceware.org, yonghong.song@linux.dev Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,GIT_PATCH_0,KAM_NUMSUBJECT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Wed, Feb 14, 2024 at 11:36=E2=80=AFAM Will Hawkins wro= te: > > On Wed, Feb 14, 2024 at 11:30=E2=80=AFAM Jose E. Marchesi > wrote: > > > > > > Hi Will. > > Thanks for the patch. Please see some comments below. > > > > > Add support for (dis)assembling the callx instruction back to CPU v1. > > > > > > gas/ChangeLog: > > > > > > * testsuite/gas/bpf/indcall-1-pseudoc.d: Refactor tests ... > > > * testsuite/gas/bpf/indcall-1-pseudoc.s: ... to visually match = ... > > > * testsuite/gas/bpf/indcall-1.d: ... equivalent test in ... > > > * testsuite/gas/bpf/indcall-1.s: ... clang/llvm. > > > > > > include/ChangeLog: > > > > > > * opcode/bpf.h (enum bpf_insn_id): BPF_INSN_CALLR to BPF_INSN_C= ALLX > > > * (for consistency) and add it to the v1 ISA variant. > > > > > > opcodes/ChangeLog: > > > > > > * bpf-opc.c: Use BPF_INSN_CALLX instead of BPF_INSN_CALLR. > > > > > > Signed-off-by: Will Hawkins > > > --- > > > gas/testsuite/gas/bpf/indcall-1-pseudoc.d | 8 ++++---- > > > gas/testsuite/gas/bpf/indcall-1-pseudoc.s | 6 +++--- > > > gas/testsuite/gas/bpf/indcall-1.d | 10 +++++----- > > > gas/testsuite/gas/bpf/indcall-1.s | 6 +++--- > > > include/opcode/bpf.h | 2 +- > > > opcodes/bpf-opc.c | 4 ++-- > > > 6 files changed, 18 insertions(+), 18 deletions(-) > > > > > > diff --git a/gas/testsuite/gas/bpf/indcall-1-pseudoc.d b/gas/testsuit= e/gas/bpf/indcall-1-pseudoc.d > > > index 7a95bad8e65..12f9d6a9d49 100644 > > > --- a/gas/testsuite/gas/bpf/indcall-1-pseudoc.d > > > +++ b/gas/testsuite/gas/bpf/indcall-1-pseudoc.d > > > @@ -1,4 +1,4 @@ > > > -#as: -EL -mdialect=3Dpseudoc -misa-spec=3Dxbpf > > > +#as: -EL -mdialect=3Dpseudoc -misa-spec=3Dv1 > > > > I think this test can now omit -misa-spec entirely. > > > > > #objdump: -M xbpf,pseudoc,dec -dr > > > #source: indcall-1-pseudoc.s > > > #name: BPF indirect call 1, pseudoc syntax > > > @@ -10,11 +10,11 @@ Disassembly of section \.text: > > > 0000000000000000
: > > > 0: b7 00 00 00 01 00 00 00 r0=3D1 > > > 8: b7 01 00 00 01 00 00 00 r1=3D1 > > > - 10: b7 02 00 00 02 00 00 00 r2=3D2 > > > - 18: 18 06 00 00 38 00 00 00 r6=3D56 ll > > > + 10: b7 03 00 00 03 00 00 00 r3=3D3 > > > + 18: 18 02 00 00 38 00 00 00 r2=3D56 ll > > > 20: 00 00 00 00 00 00 00 00[ ]* > > > 18: R_BPF_64_64 .text > > > - 28: 8d 06 00 00 00 00 00 00 callx r6 > > > + 28: 8d 02 00 00 00 00 00 00 callx r2 > > > 30: 95 00 00 00 00 00 00 00 exit > > > > I don't see any reason for changing the test bodies like this. If it i= s > > about matching some llvm test, IMO consistency between binutils and llv= m > > testsuites is not a general goal we can (or should) aim to. > > > > Same applies to the other tests changed in the thunk below. > > As I said, I am happy to do whatever you like! I did it just so that I > could visually check that the two matched and then I left it. In v4 I > will revert this particular piece! > > > > > > 0000000000000038 : > > > diff --git a/gas/testsuite/gas/bpf/indcall-1-pseudoc.s b/gas/testsuit= e/gas/bpf/indcall-1-pseudoc.s > > > index 2042697f15b..5639e288869 100644 > > > --- a/gas/testsuite/gas/bpf/indcall-1-pseudoc.s > > > +++ b/gas/testsuite/gas/bpf/indcall-1-pseudoc.s > > > @@ -4,9 +4,9 @@ > > > main: > > > r0 =3D 1 > > > r1 =3D 1 > > > - r2 =3D 2 > > > - r6 =3D bar ll > > > - callx r6 > > > + r3 =3D 3 > > > + r2 =3D bar ll > > > + callx r2 > > > exit > > > bar: > > > r0 =3D 0 > > > diff --git a/gas/testsuite/gas/bpf/indcall-1.d b/gas/testsuite/gas/bp= f/indcall-1.d > > > index 51103bba2a1..1a2c36999b1 100644 > > > --- a/gas/testsuite/gas/bpf/indcall-1.d > > > +++ b/gas/testsuite/gas/bpf/indcall-1.d > > > @@ -1,5 +1,5 @@ > > > -#as: -EL -misa-spec=3Dxbpf > > > -#objdump: -dr -M xbpf,dec > > > +#as: -EL -misa-spec=3Dv1 > > > +#objdump: -dr -M v1,dec > > > > Likewise, I think this test can just omit the -M v1 and -ima-spec=3Dv1 > > arguments now. > > > > Ack! > > > > #source: indcall-1.s > > > #name: BPF indirect call 1, normal syntax > > > > > > @@ -10,11 +10,11 @@ Disassembly of section \.text: > > > 0000000000000000
: > > > 0: b7 00 00 00 01 00 00 00 mov %r0,1 > > > 8: b7 01 00 00 01 00 00 00 mov %r1,1 > > > - 10: b7 02 00 00 02 00 00 00 mov %r2,2 > > > - 18: 18 06 00 00 38 00 00 00 lddw %r6,56 > > > + 10: b7 03 00 00 03 00 00 00 mov %r3,3 > > > + 18: 18 02 00 00 38 00 00 00 lddw %r2,56 > > > 20: 00 00 00 00 00 00 00 00[ ]* > > > 18: R_BPF_64_64 .text > > > - 28: 8d 06 00 00 00 00 00 00 call %r6 > > > + 28: 8d 02 00 00 00 00 00 00 call %r2 > > > 30: 95 00 00 00 00 00 00 00 exit > > > > > > 0000000000000038 : > > > diff --git a/gas/testsuite/gas/bpf/indcall-1.s b/gas/testsuite/gas/bp= f/indcall-1.s > > > index 5d49e41040a..7fbeeeb9a38 100644 > > > --- a/gas/testsuite/gas/bpf/indcall-1.s > > > +++ b/gas/testsuite/gas/bpf/indcall-1.s > > > @@ -4,9 +4,9 @@ > > > main: > > > mov %r0, 1 > > > mov %r1, 1 > > > - mov %r2, 2 > > > - lddw %r6, bar > > > - call %r6 > > > + mov %r3, 3 > > > + lddw %r2, bar > > > + call %r2 > > > exit > > > > > > bar: > > > diff --git a/include/opcode/bpf.h b/include/opcode/bpf.h > > > index df1e3bd0918..97e25053175 100644 > > > --- a/include/opcode/bpf.h > > > +++ b/include/opcode/bpf.h > > > @@ -202,7 +202,7 @@ enum bpf_insn_id > > > BPF_INSN_JAR, BPF_INSN_JEQR, BPF_INSN_JGTR, BPF_INSN_JSGTR, > > > BPF_INSN_JGER, BPF_INSN_JSGER, BPF_INSN_JLTR, BPF_INSN_JSLTR, > > > BPF_INSN_JSLER, BPF_INSN_JLER, BPF_INSN_JSETR, BPF_INSN_JNER, > > > - BPF_INSN_CALLR, BPF_INSN_CALL, BPF_INSN_EXIT, > > > + BPF_INSN_CALLX, BPF_INSN_CALL, BPF_INSN_EXIT, > > > > I don't think it is a good idea to rename BPF_INSN_CALLR to > > BPF_INSN_CALLX. > > > > At the moment the assembler uses the `callx' mnemonic for it and GCC > > uses the same name because that is what clang does, but if/when the > > corresponding instruction gets incorporated into BPF, I indend to push > > for a better name, certainly not something as unmemorable and > > indescriptive as `callx'. > > I will drop this change, as well! I know that Dave is working on this > naming now on the standardization mailing list. I look forward to > watching that argument! (I am definitely going to get butter with the > popcorn!). > > I will have a v4 later today -- the day job calls for the next few hours!= Sorry! > Will Sent! Sorry for the delay! Will > > > > > > > /* Compare-and-jump instructions (reg OP imm.) */ > > > BPF_INSN_JEQI, BPF_INSN_JGTI, BPF_INSN_JSGTI, > > > BPF_INSN_JGEI, BPF_INSN_JSGEI, BPF_INSN_JLTI, BPF_INSN_JSLTI, > > > diff --git a/opcodes/bpf-opc.c b/opcodes/bpf-opc.c > > > index 19e096501a2..23473fc0cd9 100644 > > > --- a/opcodes/bpf-opc.c > > > +++ b/opcodes/bpf-opc.c > > > @@ -272,8 +272,8 @@ const struct bpf_opcode bpf_opcodes[] =3D > > > BPF_V1, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_JSET|BPF_SRC_X}, > > > {BPF_INSN_JNER, "jne%W%dr , %sr , %d16", "if%w%dr !=3D %sr%wgoto%w= %d16", > > > BPF_V1, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_JNE|BPF_SRC_X}, > > > - {BPF_INSN_CALLR, "call%W%dr", "callx%w%dr", > > > - BPF_XBPF, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_CALL|BPF_SRC_X}, > > > + {BPF_INSN_CALLX, "call%W%dr", "callx%w%dr", > > > + BPF_V1, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_CALL|BPF_SRC_X}, > > > {BPF_INSN_CALL, "call%W%d32", "call%w%d32", > > > BPF_V1, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_CALL|BPF_SRC_K}, > > > {BPF_INSN_EXIT, "exit", "exit",