From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id EF9F938582A6 for ; Tue, 13 Feb 2024 11:57:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EF9F938582A6 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org EF9F938582A6 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1707825476; cv=none; b=A6gjofVn4ywYxxd0PNjcw2L8qIyuKUofNoNLgNF50b+RoZE/aapNDctheLZs1LfRBAQNtMdaMLb2rX7b06+abF8tfxEM/KHO/hWNY3+1e0FMN7GWqAwIhzSMVw4Mcv1U6BRrJ5hrTkx+ODwvUqAMqqLCqVoQKvtUi2vosO5S/v0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1707825476; c=relaxed/simple; bh=F8EtfxmQZa56FMcgQIKpbs58imZAqVKHnvaCEG+zEFI=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=QAkPgChFl0dYZAGAiTnkUEYUbuiy2yoVgLhO/BMD9phxbPZG27krYEfzRNPiYjbc33oIhSBCx7GKGao2e86c5G8VvABUdav5d4sjoIUo+B08JmomPPmPDN63N3fgwKRKiH9b0rr0EzjLAI9ULS1L8N5SQOOODyObigCCFFsG62A= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1707825474; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:autocrypt:autocrypt; bh=0Ttbgv7yPNl56MM52Q1TdCEHXDz8jghwSbkWUcKWzqY=; b=cSYOEjd+zvAXHk8UGMHTpJzV/eSdb3yp7iuMx46UAHu0xEhmPCz4I5twJskI/vAat6dUqs dSkDxM42Y0QnEWB7IfGTZyY8mBoBrwncXpAy4wa86d9FJeMPG9p06CzzwSNxB+4Wlflvbu noFpVzFBlIsPisuVFKxGNpk7L6SBynU= Received: from mail-lj1-f199.google.com (mail-lj1-f199.google.com [209.85.208.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-253-6XnPQDi0OMC8wggBXy1jqw-1; Tue, 13 Feb 2024 06:57:53 -0500 X-MC-Unique: 6XnPQDi0OMC8wggBXy1jqw-1 Received: by mail-lj1-f199.google.com with SMTP id 38308e7fff4ca-2d0d408ec2eso40205701fa.3 for ; Tue, 13 Feb 2024 03:57:53 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707825471; x=1708430271; h=content-transfer-encoding:in-reply-to:autocrypt:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=0Ttbgv7yPNl56MM52Q1TdCEHXDz8jghwSbkWUcKWzqY=; b=PNbbgkYFVIgldYP3hDQ4QMTeos7fDXvhA73w9g9kxdayUUkky70xF5Eh1sLnVjbaNe 2tAtInQf83/5glBKYxu0Qx0dtscxFhTEqJYisrsAy2eFLrcAJB/tbgWorzgtm2PudNdD Gz0z6PdJn8Uvbba4JKfSRXLb8F0NYnxg2X+1yGy3+ilxrJlw3sQ40gwD4c1OUZCK3aCr 0Y5yq8FxwTjTtb0jmCQ3S7iC7scUfkpBXqJElisvaGzU/sK12rMXexHldQbGxYqQEqym +Vk9GEDeeY+lOkl4CPvFOB0d0n5ciu/LISt4mWPPCGgcaU89GaKU554hslhXWzh870kf JG5w== X-Forwarded-Encrypted: i=1; AJvYcCW68i12F2rRoiLp4bIBvmN7DGUYI2kA6CLOtcLNCRwOBIWGrwBrC1aBHCpJuSRuiAt8sUOs7lvG8DwcNp9kYpjjy3SVHTqPzg== X-Gm-Message-State: AOJu0YxS3Ut16ZhcTvyRwAB9tZU5YLteE5E8O+1QUrkb2j+e/FC1f5xR j4xffNX0eJ1P/WoQG9miC+LV461RV3xalo3oZdxRMnLafctiBJusHQyLW+l4zMyUDR/KYXHBRGP m7rJmo7PLVW8e4NCfa+GadNkoDzINkNVRd55N5MXXMNEyfU2mS9l6s6S3qN0df38= X-Received: by 2002:a2e:3806:0:b0:2d0:a71f:5eab with SMTP id f6-20020a2e3806000000b002d0a71f5eabmr7025991lja.23.1707825471484; Tue, 13 Feb 2024 03:57:51 -0800 (PST) X-Google-Smtp-Source: AGHT+IHjOjLmSfIdGVNBAsX3cIZbD2IUGw319FNf6pErO0ziVk9XC1N539oPbIFzsTg0Gir5OekzaQ== X-Received: by 2002:a2e:3806:0:b0:2d0:a71f:5eab with SMTP id f6-20020a2e3806000000b002d0a71f5eabmr7025977lja.23.1707825471077; Tue, 13 Feb 2024 03:57:51 -0800 (PST) X-Forwarded-Encrypted: i=1; AJvYcCXspXpOV4smZINwsT4d0ZaJNGD75VSWBrFX0a7Nqs1arJje64PHqkQoHpw01v4HWSdmR5VZLPhFKj7cNRT9UkQ833asYlhtDA== Received: from [192.168.1.5] ([79.123.79.31]) by smtp.gmail.com with ESMTPSA id az4-20020a05600c600400b00411ce6def3fsm472077wmb.38.2024.02.13.03.57.50 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 13 Feb 2024 03:57:50 -0800 (PST) Message-ID: <82116e8c-390b-4389-8d04-744839785f2c@redhat.com> Date: Tue, 13 Feb 2024 11:57:50 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 1/1] objdump, as: Add callx support for BPF CPU v1 To: Will Hawkins , binutils@sourceware.org References: <20240212174209.620310-1-hawkinsw@obs.cr> <20240212174209.620310-2-hawkinsw@obs.cr> From: Nick Clifton Autocrypt: addr=nickc@redhat.com; keydata= xsFNBFm/2cUBEADkvRqMWfAryJ52T4J/640Av5cam9ojdFih9MjcX7QWFxIzJfTFYq2z+nb4 omdfZosdCJL2zGcn6C0AxpHNvxR9HMDkEyFHKrjDh4xWU+pH4z9azQEqJh331X7UzbZldqQo 16VkuVavgsTJaHcXm+nGIBTcUbl2oiTtHhmuaYxx6JTMcFjC7vyO5mLBw78wt52HBYweJ0Nj HBvvH/JxbAAULSPRUC61K0exlO49VFbFETQNG1hZTKEji95fPbre7PpXQ0ewQShUgttEE/J3 UA4jYaF9lOcZgUzbA27xTV//KomP0D30yr4e4EJEJYYNKa3hofTEHDXeeNgM25tprhBUMdbV RZpf2Keuk2uDVwc+EiOVri48rb1NU+60sOXvoGO6Ks81+mhAGmrBrlgLhAp8K1HPHI4MG4gH nrMqX2rEGUGRPFjC3qqVVlPm8H05PnosNqDLQ1Pf7C0pVgsCx6hKQB7Y1qBui7aoj9zeFaQg pYef+CEERIKEcWwrjaOJwK3pi9HFdxS0NNWYZj8HPzz/AsgTTQdsbulPlVq2SsctmOnL42CZ OCTppGYwl53CG/EqVY+UQBzFzJBaY8TJRFFYVEy5/HH4H11rMoZwqIkk71EOGU3X6mWlANRi kR3M4GhVITRzuaV69Fed+OeXcCmP94ASLfuhBR2uynmcHpBKpwARAQABzTtOaWNrIENsaWZ0 b24gKENoaWVmIEJpbnV0aWxzIE1haW50YWluZXIpIDxuaWNrY0ByZWRoYXQuY29tPsLBeAQT AQIAIgUCWb/ZxQIbAwYLCQgHAwIGFQgCCQoLBBYCAwECHgECF4AACgkQE/zvid2ePE9cOxAA 3cX1bdDaTFttTqukdPXLCtD2aNwJos4vB4LYPSgugLkYaHIQH9d1NQPhS0TlUeovnFNESLaV soihv0YmBUCyL4jE52FRoTjE6fUhYkFNqIWN2HYwkVrSap2UUJFquRVoVbPkbSup8P+D8eyd BbdxsY6f+5E8Rtz5ibVnPZTib7CyqnFokJITWjzGdIP0Gn+JWVa6jtHTImWx1MtqiuVRDapU hrIoUIjf98HQn9/N5ylEFYQTw7tzaJNWeGUoGYS8+8n/0sNbuYQUU/zwMVY9wpJcrXaas6yZ XGpF/tua59t9LFCct+07YAUSWyaBXqBW3PKQz7QP+oE8yje91XrhOQam04eJhPIBLO88g6/U rdKaY7evBB8bJ76Zpn1yqsYOXwAxifD0gDcRTQcB2s5MYXYmizn2GoUm1MnCJeAfQCi/YMob R+c8xEEkRU83Tnnw3pmAbRU6OcPihEFuK/+SOMKIuV1QWmjkbAr4g9XeXvaN+TRJ9Hl/k1k/ sj+uOfyGIaFzM/fpaLmFk8vHeej4i2/C6cL4mnahwYBDHAfHO65ZUIBAssdA6AeJ+PGsYeYh qs6zkpaA2b0wT4f9s7BPSqi0Veky8bUYYY7WpjzDcHnj1gEeIU55EhOQ42dnEfv7WrIAXanO P8SjhgqAUkb3R88azZCpEMTHiCE4bFxzOmjOwU0EWb/ZxQEQALaJE/3u23rTvPLkitaTJFqK kwPVylzkwmKdvd2qeEFk1qys2J3tACTMyYVnYTSXy5EJH2zJyhUfLnhLp8jJZF4oU5QehOaJ PcMmzI/CZS1AmH+jnm6pukdZAowTzJyt4IKSapr+7mxcxX1YQ2XewMnFYpLkAA2dHaChLSU/ EHJXe3+O4DgEURTFMa3SRN/J4GNMBacKXnMSSYylI5DcIOZ/v0IGa5MAXHrP1Hwm1rBmloIc gmzexczBf+IcWgCLThyFPffv+2pfLK1XaS82OzBC7fS01pB/eDOkjQuKy16sKZX6Rt57vud4 0uE5a0lpyItC2P7u7QWL4yT5pMF+oS8bm3YWgEntV380RyZpqgJGZTZLNq2T4ZgfiaueEV4J zOnG2/QRGjOUrNQaYzKy5V127CTnRg4BYF/uLEmizLcI3O3U1+mEz6h48wkAojO1B6AZ8Lm+ JuxOW5ouGcrkTEuIG56GcDwMWS/Pw/vNsDyNmOCjy9eEKWJgmMmLaq59HpfTd8IOeaYyuAQH AsYt/zzKy0giMgjhCQtuc99E4nQE9KZ44DKsnqRabK9s3zYE3PIkCFIEZcUiJXSXWWOIdJ43 j+YyFHU5hqXfECM6rzKGBeBUGTzyWcOX6YwRM4LzQDVJwYG8cVfth+v4/ImcXR43D4WVxxBE AjKag02b+1yfABEBAAHCwV8EGAECAAkFAlm/2cUCGwwACgkQE/zvid2ePE/dqQ/6ApUwgsZz tps0MOdRddjPwz44pWXS5MG45irMQXELGQyxkrafc8lwHeABYstoK8dpopTcJGE3dZGL3JNz 1YWxQ5AV4uyqBn5N8RubcA8NzR6DQP+OGPIwzMketvVC/cbbKDZqf0uTDy3jP65OFhSkTEIy nYv1Mb4JJl3Sq+haUbfWLAV5nboSuHmiZE6Bz2+TjdoVkNwHBfpqxu6MlWka+P98SUcmY8iV hPy9QC1XFOGdFDFf1kYgHW27mFwds35NQhNARgftAVz9FZXruW6tFIIfisjr3rVjD9R8VgL7 l5vMr9ylOFpepnI6+wd2X1566HW7F1Zw1DIrY2NHL7kL5635bHrJY4n7o/n7Elk/Ca/MAqzd IZxz6orfXeImsqZ6ODn4Y47PToS3Tr3bMNN9N6tmOPQZkJGHDBExbhAi/Jp8fpWxMmpVCUl6 c85cOBCR4s8tZsvGYOjR3CvqKrX4bb8GElrhOvAJa6DdmZXc7AyoVMaTvhpq3gJYKmC64oqt 7zwIHwaCxTbP6C6oUp9ENRV7nHnXN3BlvIgCo4QEs6HkDzkmgYlCEOKBiDyVMSkPDZdsspa+ K4GlU2Swi/BDJMjtDxyo+K0M81LXXxOeRfEIfPtZ3ddxBKPva1uSsuz+pbN9d1JY8Ko5T/h1 6susi2ReUyNJEJaSnjO5z13TQ1U= In-Reply-To: <20240212174209.620310-2-hawkinsw@obs.cr> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-GB Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_NUMSUBJECT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,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: Hi Will, > Add support for (dis)assembling the callx instruction back to CPU v1. I am not going to comment on the callx instruction itself, since I am not a BPF expert and it seems like you are already working on that with Jose and YYonghong. Instead I thought that I would comment on the patch instead... > Signed-off-by: Will Hawkins Just to be clear: your sign off here does mean that you agree to the Developer Certificate of Origin (v1.1) right ? [I am being paranoid...:-] > @@ -935,7 +937,13 @@ encode_insn (struct bpf_insn *insn, char *bytes, > if (immediate_overflow (imm, 32)) > as_bad (_("immediate out of range, shall fit in 32 bits")); > else > - encode_int32 (insn->imm32.X_add_number, bytes + 4); > + encode_int32 (insn->imm32.X_add_number, bytes + 4); > + } > + > + if (insn->has_rmm32) > + { > + int64_t imm = insn->rmm32; > + encode_int32 (imm, bytes + 4); > } > > if (insn->has_disp32 && insn->disp32.X_op == O_constant) This frag struck me as strange since it appears that the first change is to replace a line with itself. Looking a bit closer I saw that you are removing some unnecessary spaces at the end of the line. Which is good and not a problem. It just looked odd when I was reviewing the patch. The second part of the frag also seems a little odd. Given that encode_int32() takes an int32_t as its first parameter, why do you use int64_t as the type for "imm" ? In fact why have a local variable at all ? Wouldn't it be simpler to just have: if (insn->has_rmm32) encode_int32 (insn->rmm32, bytes + 4); > @@ -1610,6 +1618,21 @@ md_assemble (char *str ATTRIBUTE_UNUSED) > insn.has_imm32 = 1; > p += 4; > } > + else if (strncmp (p, "%r32", 4) == 0) This is not a criticism, but merely a comment. I dislike the presence of "magic" numbers in code, numbers whose value may or may not be obvious to the reader and which may appear more than once. So for example the number 4 in the code line above is "magic" to my mind, and a potential source of problems. Imagine that at some point in the future the name of the register was changed to "%foo32". Most coders would realise that the 4 is the length of the "%r32" string and so change it to 5, but would they also realise that the 4 is reused later on to adjust the "p" pointer: > + p += 4; Now you are following the code style that is already present in the tc-bpf.c file, so there is no fault there. But, in my opinion, the style is wrong. I would rather see something like this: #define RMM32_REG_NAME "%r32" #define RMM32_REG_LEN strlen (RMM32_REG_NAME) [...] else if (strncmp (p, RMM32_REG_NAEM, RMM32_REG_LEN) == 0) { [...] p += RMM32_REG_LEN; } That way if the name is ever changed, all the other adjustments would happen automatically. Plus if those defines were in a BPF specific header file then they could be reused elsewhere, eg in the disassembler. Alternatively ... there is a function called startswith() which is used in the assembler in lots of places, and you could use this to make the if-statement simpler, ie: [...] else if (startswith (p, "%r32") { [...] This does not resolve the use of the magic value 4 later on, but still makes the code look cleaner. Just my opnion of course. > diff --git a/opcodes/bpf-dis.c b/opcodes/bpf-dis.c > index d4020c259fb..50e714cb74b 100644 > --- a/opcodes/bpf-dis.c > +++ b/opcodes/bpf-dis.c > @@ -251,6 +251,12 @@ print_insn_bpf (bfd_vma pc, disassemble_info *info) > imm32); > p += 4; > } > + else if (strncmp (p, "%r32", 4) == 0) > + { > + int32_t imm32 = bpf_extract_imm32 (word, endian); > + print_register (info, p, imm32); > + p += 4; > + } See the comments above. :-) > diff --git a/include/opcode/bpf.h b/include/opcode/bpf.h > @@ -254,6 +254,7 @@ struct bpf_opcode > %d16 - 16-bit signed displacement (in 64-bit words minus one.) > %o16 - 16-bit signed offset (in bytes.) > %i32 - 32-bit signed immediate. > + %r32 - 32-bit signed immediate indicating a register. > %I32 - Like %i32. > %i64 - 64-bit signed immediate. > %w - expect zero or more white spaces and print a single space. Why is %r32 signed ? Does the BPF format support negative register numbers ? > diff --git a/sim/bpf/bpf-sim.c b/sim/bpf/bpf-sim.c Just FYI - the simulator is part of the GDB project, not the Binutils project, so you will need to submit the patch to bpf-sim.c to them, not us. The email address to use is: gdb-patches@sourceware.org Cheers Nick PS. Please don't think that my comments are meant to be criticisms of your code. The code is good. I am merely trying to provide some suggestions to make it even better. :-)