From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E8FC0C433E0 for ; Tue, 9 Feb 2021 16:16:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A315564EAA for ; Tue, 9 Feb 2021 16:16:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232005AbhBIQQd (ORCPT ); Tue, 9 Feb 2021 11:16:33 -0500 Received: from elvis.franken.de ([193.175.24.41]:37111 "EHLO elvis.franken.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232412AbhBIQPh (ORCPT ); Tue, 9 Feb 2021 11:15:37 -0500 Received: from uucp (helo=alpha) by elvis.franken.de with local-bsmtp (Exim 3.36 #1) id 1l9VfC-0005wN-00; Tue, 09 Feb 2021 17:14:54 +0100 Received: by alpha.franken.de (Postfix, from userid 1000) id AB7E0C0E00; Tue, 9 Feb 2021 17:14:20 +0100 (CET) Date: Tue, 9 Feb 2021 17:14:20 +0100 From: Thomas Bogendoerfer To: "Maciej W. Rozycki" Cc: Jinyang He , Jiaxun Yang , linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org, Paul Burton , Jun-Ru Chang Subject: Re: [PATCH v2 2/4] MIPS: microMIPS: Fix the judgment of mm_jr16_op and mm_jalr_op Message-ID: <20210209161420.GA15428@alpha.franken.de> References: <1611207098-11381-1-git-send-email-hejinyang@loongson.cn> <1611207098-11381-3-git-send-email-hejinyang@loongson.cn> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Feb 07, 2021 at 10:31:38PM +0100, Maciej W. Rozycki wrote: > On Thu, 21 Jan 2021, Jinyang He wrote: > > > mm16_r5_format.rt is 5 bits, so directly judge the value if equal or not. > > mm_jalr_op requires 7th to 16th bits. These 10 which bits generated by > > The minor opcode extension field is comprised of bits 15:6, not 16:7 as > your description suggests. Please be accurate with statements. > > > shifting u_format.uimmediate by 6 may be affected by sign extension. > > Why? The `uimmediate' bit-field member is unsigned for a reason. No > sign-extension is made on unsigned data with the right-shift operation. > > > Thus, take out the 10 bits for comparison. > > > > Without this patch, errors may occur, such as these bits are all ones. > > How did you come to this conclusion? > > > diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c > > index d737234..74d7fd8 100644 > > --- a/arch/mips/kernel/process.c > > +++ b/arch/mips/kernel/process.c > > @@ -292,8 +292,8 @@ static inline int is_jump_ins(union mips_instruction *ip) > > * microMIPS is kind of more fun... > > */ > > if (mm_insn_16bit(ip->word >> 16)) { > > - if ((ip->mm16_r5_format.opcode == mm_pool16c_op && > > - (ip->mm16_r5_format.rt & mm_jr16_op) == mm_jr16_op)) > > + if (ip->mm16_r5_format.opcode == mm_pool16c_op && > > + ip->mm16_r5_format.rt == mm_jr16_op) > > return 1; > > return 0; > > } > > Code style changes should be submitted on their own as separate patches. > > > @@ -305,7 +305,7 @@ static inline int is_jump_ins(union mips_instruction *ip) > > if (ip->r_format.opcode != mm_pool32a_op || > > ip->r_format.func != mm_pool32axf_op) > > return 0; > > - return ((ip->u_format.uimmediate >> 6) & mm_jalr_op) == mm_jalr_op; > > + return ((ip->u_format.uimmediate >> 6) & GENMASK(9, 0)) == mm_jalr_op; > > You've now excluded JALR.HB, JALRS, and JALRS.HB instructions. The mask > was there for a reason. If you can't be bothered to verify microMIPS > changes say with QEMU, then at the very least please check documentation. > The intent of this code is clear and these instructions are even spelled > out explicitly in the comment at the top. > > Thomas, please revert this change as I can see you've already taken it. > It's plain wrong. It's now reverted in mips-next. Thomas. -- Crap can work. Given enough thrust pigs will fly, but it's not necessarily a good idea. [ RFC1925, 2.3 ]