linux-kernel - Re: [RFC PATCH 0/6] Deduplicating RISCV cmpxchg.h macros

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <18e07d0a-7e21-46d0-9166-6ee07817352a@spud>
Date:   Tue, 21 Mar 2023 07:48:01 +0000
From:   Conor Dooley <conor.dooley@...rochip.com>
To:     Leonardo Brás <leobras@...hat.com>
CC:     Conor Dooley <conor@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>, Guo Ren <guoren@...nel.org>,
        <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 0/6] Deduplicating RISCV cmpxchg.h macros

On Tue, Mar 21, 2023 at 03:30:35AM -0300, Leonardo Brás wrote:
> Hello Conor, thanks for the feedback!
> 
> 
> On Sun, 2023-03-19 at 20:35 +0000, Conor Dooley wrote:
> > On Sat, Mar 18, 2023 at 05:00:54AM -0300, Leonardo Bras wrote:
> > > While studying riscv's cmpxchg.h file, I got really interested in 
> > > understanding how RISCV asm implemented the different versions of 
> > > {cmp,}xchg.
> > > 
> > > When I understood the pattern, it made sense for me to remove the 
> > > duplications and create macros to make it easier to understand what exactly 
> > > changes between the versions: Instruction sufixes & barriers.
> > > 
> > > I split those changes in 3 levels for each cmpxchg and xchg, resulting a 
> > > total of 6 patches. I did this so it becomes easier to review and remove 
> > > the last levels if desired, but I have no issue squashing them if it's 
> > > better.
> > > 
> > > Please provide comments.
> > > 
> > > Thanks!
> > > Leo
> > > 
> > > Leonardo Bras (6):
> > >   riscv/cmpxchg: Deduplicate cmpxchg() asm functions
> > >   riscv/cmpxchg: Deduplicate cmpxchg() macros
> > >   riscv/cmpxchg: Deduplicate arch_cmpxchg() macros
> > 
> > >   riscv/cmpxchg: Deduplicate xchg() asm functions
> > 
> > FWIW, this patch seems to break the build pretty badly:
> > https://patchwork.kernel.org/project/linux-riscv/patch/20230318080059.1109286-5-leobras@redhat.com/
> 
> Thanks for pointing out!
> 
> It was an intermediary error:
> Sufix for amoswap on acquire version was "d.aqrl" instead of the
> correct".d.aqrl", and that caused the fail.
> 
> I did not notice anything because the next commit made it more general, and thus
> removed this line of code. I will send a v2-RFC shortly.
> 
> I see that patch 4/6 has 5 fails, but on each one of them I can see:
> "I: build output in /ci/workspace/[...]", or
> ""I: build output in /tmp/[...]".

I don't push the built artifacts anywhere, just the brief logs -
although the "failed to build" log isn't very helpful other than telling
you the build broke.
That seems like a bug w.r.t. where tuxmake prints its output. I'll try
to fix that.

> I could not find any reference to where this is saved, though.
> Could you point where can I find the error output, just for the sake of further
> debugging?
> 
> > 
> > Patches 1 & 5 also add quite a lot of sparse issues (like 1000), but I
> > think that may be more of an artifact of the testing process as opposed
> > to something caused by this patchset.
> 
> For those I can see the build output diffs. Both added error lines to
> conchuod/build_rv64_gcc_allmodconfig.
> I tried to mimic this with [make allmodconfig + gcc build with
> CROSS_COMPILE=riscv64-linux-gnu- ] but I could not get any error in any patch.

If you can't replicate them, it's probably because they come from
sparse. I only really mentioned it here in case you went looking at the
other patches in the series and were wondering why things were so amiss.

> For patch 1/6 it removes as much error lines (-) as it adds (+), and the error
> messages are mostly the same, apart for an line number.

I don't see a line number difference, but rather an increase in the
number of times the same error lines have appeared in the output.
I don't find the single-line output from sparse to really be very
helpful, I should probably have a bit of a think how to present that
information better.

> For patch 5/6 it actually adds many more lines, but tracking (some of) the
> errors gave me no idea why.

Probably just sparse being unhappy with some way the macros were
changed - but some of it ("Should it be static?" bits) look very much
like the patch causing things to be rebuilt only for the "after the
patch" build, but somehow not for the "before" build.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)