Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Date:   Tue, 31 Oct 2023 13:06:39 -0700
From:   Charlie Jenkins <charlie@rivosinc.com>
To:     Andreas Schwab <schwab@linux-m68k.org>
Cc:     Emil Renner Berthing <emil.renner.berthing@canonical.com>,
        linux-riscv@lists.infradead.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Eric Biederman <ebiederm@xmission.com>,
        Kees Cook <keescook@chromium.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Samuel Holland <samuel.holland@sifive.com>,
        Nelson Chu <nelson@rivosinc.com>,
        Emil Renner Berthing <kernel@esmil.dk>
Subject: Re: [PATCH v7 1/3] riscv: Avoid unaligned access when relocating
 modules
Message-ID: <ZUFeTzStw1qaM0m+@ghost>
References: <20231031-module_relocations-v7-0-6f4719b64bf7@rivosinc.com>
 <20231031-module_relocations-v7-1-6f4719b64bf7@rivosinc.com>
 <CAJM55Z-v0EwrZp876DdLSzad2u55nJV_uBs_+MUJDqFW5MTPkA@mail.gmail.com>
 <878r7iiwb6.fsf@igel.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <878r7iiwb6.fsf@igel.home>
Precedence: bulk

On Tue, Oct 31, 2023 at 05:35:57PM +0100, Andreas Schwab wrote:
> On Okt 31 2023, Emil Renner Berthing wrote:
> 
> >> +static int riscv_insn_rmw(void *location, u32 keep, u32 set)
> >> +{
> >> +	u16 *parcel = location;
> >> +	u32 insn = (u32)le16_to_cpu(parcel[0]) | (u32)le16_to_cpu(parcel[1]) << 16;
> >> +
> >> +	insn &= keep;
> >> +	insn |= set;
> >> +
> >> +	parcel[0] = cpu_to_le32(insn);
> >
> > Why cpu_to_le32(insn)? Unless I've misunderstood something downcasting unsigned
> > to unsigned values in C (eg. from u32 to u16) is defined to always discard the
> > most signifcant bits, so cpu_to_le16(insn) should be fine.
> 
> cpu_to_le32(insn) can't be right here anyway, since it also swaps the
> two u16 halves and would be the same as cpu_to_le16(insn >> 16) on big
> endian.

Yes, not sure why I did that... I will fix that up.

- Charlie

> 
> -- 
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."