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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5A000C27C75 for ; Tue, 11 Jun 2024 17:49:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=xoU4bus5Y4kIl3eNA453GkViawRptQhmd5JkGHvADms=; b=Cjw8kNGtl7VE48oSf2/7fVKj17 em45UzR8CuxNBYXqIGE8UK0yDGoL4mYdk+hIoca2Bz7rRPhs2IvmIXMNFGzGRDZ6Ot8C7Rr1m85kg 6UGerqpJs/q5IK4mZoGet/pqUXpbMzUr/he5IuLiy9totXr35zDbICXnw88M6D7WfSCPQO1me20mS CZjh/DBZRvAqhrMuhoCPerYwgrwWzwILd4/nCQlXFlaaVGwyWkMHGsjlf26+oS2nXCBiM2cqVlvLV VWTedYc2SJLQHIsX2tC5xaG3+nhnjYyXu2FDeicczjeG72h+4ega/OjmK79S8BUyHcxBsSMwq4+ZC p2qiqQ6w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sH5cD-00000009j09-1Wp1; Tue, 11 Jun 2024 17:49:01 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sH5c9-00000009iyp-2tyj for linux-arm-kernel@lists.infradead.org; Tue, 11 Jun 2024 17:48:59 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id BD344152B; Tue, 11 Jun 2024 10:49:19 -0700 (PDT) Received: from J2N7QTR9R3 (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 06D643F73B; Tue, 11 Jun 2024 10:48:52 -0700 (PDT) Date: Tue, 11 Jun 2024 18:48:47 +0100 From: Mark Rutland To: Linus Torvalds Cc: Peter Anvin , Ingo Molnar , Borislav Petkov , Thomas Gleixner , Rasmus Villemoes , Josh Poimboeuf , Catalin Marinas , Will Deacon , Linux Kernel Mailing List , the arch/x86 maintainers , linux-arm-kernel@lists.infradead.org, linux-arch Subject: Re: [PATCH 4/7] arm64: add 'runtime constant' support Message-ID: References: <20240610204821.230388-1-torvalds@linux-foundation.org> <20240610204821.230388-5-torvalds@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240611_104857_880336_A5DD224A X-CRM114-Status: GOOD ( 29.52 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Jun 11, 2024 at 09:56:17AM -0700, Linus Torvalds wrote: > On Tue, 11 Jun 2024 at 07:29, Mark Rutland wrote: > > > > Do we expect to use this more widely? If this only really matters for > > d_hash() it might be better to handle this via the alternatives > > framework with callbacks and avoid the need for new infrastructure. > > Hmm. The notion of a callback for alternatives is intriguing and would > be very generic, but we don't have anything like that right now. > > Is anybody willing to implement something like that? Because while I > like the idea, it sounds like a much bigger change. Fair enough if that's a pain on x86, but we already have them on arm64, and hence using them is a smaller change there. We already have a couple of cases which uses MOVZ;MOVK;MOVK;MOVK sequence, e.g. // in __invalidate_icache_max_range() asm volatile(ALTERNATIVE_CB("movz %0, #0\n" "movk %0, #0, lsl #16\n" "movk %0, #0, lsl #32\n" "movk %0, #0, lsl #48\n", ARM64_ALWAYS_SYSTEM, kvm_compute_final_ctr_el0) : "=r" (ctr)); ... which is patched via the callback: void kvm_compute_final_ctr_el0(struct alt_instr *alt, __le32 *origptr, __le32 *updptr, int nr_inst) { generate_mov_q(read_sanitised_ftr_reg(SYS_CTR_EL0), origptr, updptr, nr_inst); } ... where the generate_mov_q() helper does the actual instruction generation. So if we only care about a few specific constants, we could give them their own callbacks, like kvm_compute_final_ctr_el0() above. [...] > > We have some helpers for instruction manipulation, and we can use > > aarch64_insn_encode_immediate() here, e.g. > > > > #include > > > > static inline void __runtime_fixup_16(__le32 *p, unsigned int val) > > { > > u32 insn = le32_to_cpu(*p); > > insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_16, insn, val); > > *p = cpu_to_le32(insn); > > } > > Ugh. I did that, and then noticed that it makes the generated code > about ten times bigger. > > That interface looks positively broken. > > There is absolutely nobody who actually wants a dynamic argument, so > it would have made both the callers and the implementation *much* > simpler had the "AARCH64_INSN_IMM_16" been encoded in the function > name the way I did it for my instruction rewriting. > > It would have made the use of it simpler, it would have avoided all > the "switch (type)" garbage, and it would have made it all generate > much better code. Oh, completely agreed. FWIW, I have better versions sat in my arm64/insn/rework branch, but I haven't had the time to get all the rest of the insn framework cleanup sorted: https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/commit/?h=arm64/insn/rework&id=9cf0ec088c9d5324c60933bf3924176fea0a4d0b I can go prioritise getting that bit out if it'd help, or I can clean this up later. Those allow the compiler to do much better, including compile-time (or runtime) checks that immediates fit. For example: void encode_imm16(__le32 *p, u16 imm) { u32 insn = le32_to_cpu(*p); // Would warn if 'imm' were u32. // As u16 always fits, no warning BUILD_BUG_ON(!aarch64_insn_try_encode_unsigned_imm16(&insn, imm)); *p = cpu_to_le32(insn); } ... compiles to: : ldr w2, [x0] bfi w2, w1, #5, #16 str w2, [x0] ret ... which I think is what you want? > So I did that change you suggested, and then undid it again. > > Because that whole aarch64_insn_encode_immediate() thing is an > abomination, and should be burned at the stake. It's misdesigned in > the *worst* possible way. > > And no, this code isn't performance-critical, but I have some taste, > and the code I write will not be using that garbage. Fair enough. Mark.