From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf35.google.com (mail-qv1-xf35.google.com [IPv6:2607:f8b0:4864:20::f35]) by sourceware.org (Postfix) with ESMTPS id B32D038582A1 for ; Wed, 14 Feb 2024 16:36:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B32D038582A1 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 B32D038582A1 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::f35 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1707928617; cv=none; b=NxL+6EiD0w8MMaxahdcJ9hx7q2KAIPXq5Hr5CGTWzM3l2PmP82nVy3LHeJoB0FyGq77L9cncvhDEdltJJhYIwhm6Pelz06mtRti9q8Om5xEew1AjH5hkcXefr327W18m9mebpx3TR1QHmVxu1HfP0/9ryG3SArEk8xOND5T3f6c= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1707928617; c=relaxed/simple; bh=hPS7DiLpBE5QlrlhGSOtiGxQ/bEVyvFrruUAYdJtS10=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=UaEX8uQvMe2WvVnBPllaqOLLoCDGyf3dUzuOsV1Yy/LGWvQjTyH21p/AxkH2ViszBelX3S7wwpWbwkTM4oKCSqpdVhjDT9l1LpIN2EcUAxt9Zcv1AyJ/uVHboImpDt6Bcacv2a01tdjg58Y8VdqcwVgI5PIUhx+0asFgzNCANzo= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-qv1-xf35.google.com with SMTP id 6a1803df08f44-68ee2c0a237so6602966d6.1 for ; Wed, 14 Feb 2024 08:36:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=obs-cr.20230601.gappssmtp.com; s=20230601; t=1707928615; x=1708533415; 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=QQdqQc4hob3iMh9AeSOJ6wgGUHn5A8iAz2G/RQ7upOM=; b=ValsDuEoPIHu09sQ38TiU0BY3j8TuVxrpJ3ivYVRrb67eAA3M6VlDUIMVyQZcKqd6w hTLtM1khT0rE4PiRR4EEYAZOPqgeCRiRf3S0bYqdpVz50U8sVQOS9amSJHQRlsfZBm/x wlNJI0q4XeeWoEefp1uJuo11anqdRs97CQoFut4OZnaLQqZKKnq475vAD9A7OnKvdV1D 7Y1c2kzNZ/FMpd8GYp1EifkZtmj+nkllGYP8G9vIVm0DyAgM5KMcDMBww+X/xVGoKnhl oksN1/rKJusccyn3Q+gv9aOuQQt/d3dmXxGUQaxTnDEsD2PSY2/jpQdcAErE3qxPypFn T+3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707928615; x=1708533415; 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=QQdqQc4hob3iMh9AeSOJ6wgGUHn5A8iAz2G/RQ7upOM=; b=n7QEPSlD7OYCzISA1nSDErrhvzvOza4ERjcoMoscz0LNJ9SX1pu1cZud2UGh3pc+XB Pi1UizVDK5gIYJaIe7oSUQ1Zy7D8Q/+7GD16S6yr3jaklGdL/Q2GKEyoZlLl3KeBNs8N E+v5EEGE5uyTfq0q09+2jYdLrd/1bnqbhR6y/TTgg6DoWMNlVvFldHGIG/o/ytNIYnto jmCLc7LDp5aAOLm0c3pZvGL/nh3GhTs0FCOq/4ilETifs/7p4GUFFq78AnQxgiX4wWvG w6lpTNg7kNopMnj29ah8MKgstneTgcxZNn6ik4XRGmcCevDI/+1qof7RdHom8VknNkGO VlQw== X-Gm-Message-State: AOJu0YwbUnYhzFdpyua+zZo/1lBuC/gnFfhE3bD0nBsmKmui1oa3FeuE rNuIu36a+2HzzOcJ0S611Q2+Yk3VyUoHb+Fo680BwOaqi6dfhblVVYwf5QQcLUSc504Qg1hJwIN 7SJcsOFExzKId4r8F5uGQ8tlHBnHiKu4ZPmAKxQ== X-Google-Smtp-Source: AGHT+IFJ7MGMBEUQkZTd5ZZsKxaJHXxwtaYET8Qw6ROn9PwS01DL3QTWQLVOHdAL3wx6lkSxsM32b2ZmVOqMQY0Oxxs= X-Received: by 2002:a0c:f049:0:b0:68c:ce3b:8aa4 with SMTP id b9-20020a0cf049000000b0068cce3b8aa4mr3565909qvl.7.1707928615025; Wed, 14 Feb 2024 08:36:55 -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: <87bk8jknyo.fsf@oracle.com> From: Will Hawkins Date: Wed, 14 Feb 2024 11:36:44 -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=-7.0 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: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_CAL= LX > > * (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/testsuite/= 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 is > about matching some llvm test, IMO consistency between binutils and llvm > 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/testsuite/= 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/bpf/= 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/bpf/= 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! S= orry! 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%d= 16", > > 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",