From patchwork Wed Dec 13 15:45:03 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: James Morse X-Patchwork-Id: 10110353 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id D6A72602B3 for ; Wed, 13 Dec 2017 15:47:10 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id BEA9628D05 for ; Wed, 13 Dec 2017 15:47:10 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B321628D0C; Wed, 13 Dec 2017 15:47:10 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 44FC028D05 for ; Wed, 13 Dec 2017 15:47:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753332AbdLMPrI (ORCPT ); Wed, 13 Dec 2017 10:47:08 -0500 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:59204 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753132AbdLMPrH (ORCPT ); Wed, 13 Dec 2017 10:47:07 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id EC7441435; Wed, 13 Dec 2017 07:47:06 -0800 (PST) Received: from [10.1.207.55] (melchizedek.cambridge.arm.com [10.1.207.55]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 047A93F4FF; Wed, 13 Dec 2017 07:47:04 -0800 (PST) Message-ID: <5A314AFF.60300@arm.com> Date: Wed, 13 Dec 2017 15:45:03 +0000 From: James Morse User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.6.0 MIME-Version: 1.0 To: Marc Zyngier CC: linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, Christoffer Dall , Mark Rutland , Catalin Marinas , Will Deacon , Steve Capper Subject: Re: [PATCH v2 07/19] arm64: insn: Add encoder for bitwise operations using litterals References: <20171211144937.4537-1-marc.zyngier@arm.com> <20171211144937.4537-8-marc.zyngier@arm.com> <5A3020DA.8010309@arm.com> <07971a66-1b36-d2cd-e590-2b7b65d14b97@arm.com> In-Reply-To: <07971a66-1b36-d2cd-e590-2b7b65d14b97@arm.com> Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Marc, On 13/12/17 14:32, Marc Zyngier wrote: > On 12/12/17 18:32, James Morse wrote: >> On 11/12/17 14:49, Marc Zyngier wrote: >>> We lack a way to encode operations such as AND, ORR, EOR that take >>> an immediate value. Doing so is quite involved, and is all about >>> reverse engineering the decoding algorithm described in the >>> pseudocode function DecodeBitMasks(). >>> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c >>> index 7e432662d454..326b17016485 100644 >>> --- a/arch/arm64/kernel/insn.c >>> +++ b/arch/arm64/kernel/insn.c >> >>> +static u32 aarch64_encode_immediate(u64 imm, >>> + enum aarch64_insn_variant variant, >>> + u32 insn) >>> +{ >>> + unsigned int immr, imms, n, ones, ror, esz, tmp; >>> + u64 mask; >> >> [...] >> >>> + /* Compute the rotation */ >>> + if (range_of_ones(imm)) { >>> + /* >>> + * Pattern: 0..01..10..0 >>> + * >>> + * Compute how many rotate we need to align it right >>> + */ >>> + ror = ffs(imm) - 1; >> >> (how come range_of_ones() uses __ffs64() on the same value?) > > News flash: range_of_ones is completely buggy. It will fail on the > trivial value 1 (__ffs64(1) = 0; 0 - 1 = -1; val >> -1 is... ermmmm). > I definitely got mixed up between the two. They do different things!? Aaaaaahhhh.... [ ...] >> Unless I've gone wrong, I think the 'Trim imm to the element size' code needs to >> move up into the esz-reducing loop so it doesn't happen for a 64bit immediate. > Yup. I've stashed the following patch: > > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > index b8fb2d89b3a6..e58be1c57f18 100644 > --- a/arch/arm64/kernel/insn.c > +++ b/arch/arm64/kernel/insn.c > @@ -1503,8 +1503,7 @@ pstate_check_t * const aarch32_opcode_cond_checks[16] = { > static bool range_of_ones(u64 val) > { > /* Doesn't handle full ones or full zeroes */ > - int x = __ffs64(val) - 1; > - u64 sval = val >> x; > + u64 sval = val >> __ffs64(val); > > /* One of Sean Eron Anderson's bithack tricks */ > return ((sval + 1) & (sval)) == 0; > @@ -1515,7 +1514,7 @@ static u32 aarch64_encode_immediate(u64 imm, > u32 insn) > { > unsigned int immr, imms, n, ones, ror, esz, tmp; > - u64 mask; > + u64 mask = ~0UL; > > /* Can't encode full zeroes or full ones */ > if (!imm || !~imm) > @@ -1543,8 +1542,12 @@ static u32 aarch64_encode_immediate(u64 imm, > for (tmp = esz; tmp > 2; tmp /= 2) { > u64 emask = BIT(tmp / 2) - 1; > > - if ((imm & emask) != ((imm >> (tmp / 2)) & emask)) > + if ((imm & emask) != ((imm >> (tmp / 2)) & emask)) { > + /* Trim imm to the element size */ > + mask = BIT(esz - 1) - 1; > + imm &= mask; Won't this still lose the top bit? It generates 0x7fffffff for esz=32, and for esz=32 we run through here when the two 16bit values are different. This still runs for a 64bit immediate. The 0xf80000000fffffff example compares 0xf8000000 with 0fffffff then breaks here on the first iteration of this loop. With this change it still attempts to generate a 64bit mask. I was thinking of something like [0]. That only runs when we know the two tmp:halves match, it just keeps the bottom tmp:half for the next run and never runs for a 64bit immediate. > break; > + } > > esz = tmp; > } > @@ -1552,10 +1555,6 @@ static u32 aarch64_encode_immediate(u64 imm, > /* N is only set if we're encoding a 64bit value */ > n = esz == 64; > > - /* Trim imm to the element size */ > - mask = BIT(esz - 1) - 1; > - imm &= mask; > - > /* That's how many ones we need to encode */ > ones = hweight64(imm); > > I really need to run this against gas in order to make sure > I get the same parameters for all the possible values. Sounds good, Thanks, James [0] Not even built: diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c index 12d3ec2154c2..d9fbdea7b18d 100644 --- a/arch/arm64/kernel/insn.c +++ b/arch/arm64/kernel/insn.c @@ -1529,15 +1529,15 @@ static u32 aarch64_encode_immediate(u64 imm, break; esz = tmp; + + /* Trim imm to the element size */ + mask = BIT(esz) - 1; + imm &= mask; } /* N is only set if we're encoding a 64bit value */ n = esz == 64; - /* Trim imm to the element size */ - mask = BIT(esz - 1) - 1; - imm &= mask; - /* That's how many ones we need to encode */ ones = hweight64(imm);