Message-ID: <ZNt3mQeGbql4oi55@FVFF77S0Q05N>
Date:   Tue, 15 Aug 2023 14:05:36 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     James Clark <james.clark@....com>
Cc:     Anshuman Khandual <anshuman.khandual@....com>,
        Will Deacon <will@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        catalin.marinas@....com, Mark Brown <broonie@...nel.org>,
        Rob Herring <robh@...nel.org>, Marc Zyngier <maz@...nel.org>,
        Suzuki Poulose <suzuki.poulose@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        linux-perf-users@...r.kernel.org
Subject: Re: [PATCH V13 - RESEND 02/10] arm64/perf: Add BRBE registers and
 fields

On Tue, Aug 15, 2023 at 11:17:19AM +0100, James Clark wrote:
> 
> 
> On 31/07/2023 10:06, Mark Rutland wrote:
> > On Mon, Jul 31, 2023 at 08:03:21AM +0530, Anshuman Khandual wrote:
> >>
> >>
> >> On 7/28/23 22:22, James Clark wrote:
> >>>
> >>>
> >>> On 28/07/2023 17:20, Will Deacon wrote:
> >>>> On Tue, Jul 11, 2023 at 01:54:47PM +0530, Anshuman Khandual wrote:
> >>>>> This adds BRBE related register definitions and various other related field
> >>>>> macros there in. These will be used subsequently in a BRBE driver which is
> >>>>> being added later on.
> >>>>>
> >>>>> Cc: Catalin Marinas <catalin.marinas@....com>
> >>>>> Cc: Will Deacon <will@...nel.org>
> >>>>> Cc: Marc Zyngier <maz@...nel.org>
> >>>>> Cc: Mark Rutland <mark.rutland@....com>
> >>>>> Cc: linux-arm-kernel@...ts.infradead.org
> >>>>> Cc: linux-kernel@...r.kernel.org
> >>>>> Tested-by: James Clark <james.clark@....com>
> >>>>> Reviewed-by: Mark Brown <broonie@...nel.org>
> >>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
> >>>>> ---
> >>>>>  arch/arm64/include/asm/sysreg.h | 103 +++++++++++++++++++++
> >>>>>  arch/arm64/tools/sysreg         | 158 ++++++++++++++++++++++++++++++++
> >>>>>  2 files changed, 261 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> >>>>> index b481935e9314..f95e30c13c8b 100644
> >>>>> --- a/arch/arm64/include/asm/sysreg.h
> >>>>> +++ b/arch/arm64/include/asm/sysreg.h
> >>>>> @@ -163,6 +163,109 @@
> >>>>>  #define SYS_DBGDTRTX_EL0		sys_reg(2, 3, 0, 5, 0)
> >>>>>  #define SYS_DBGVCR32_EL2		sys_reg(2, 4, 0, 7, 0)
> >>>>>  
> >>>>> +#define __SYS_BRBINFO(n)		sys_reg(2, 1, 8, ((n) & 0xf), ((((n) & 0x10)) >> 2 + 0))
> >>>>> +#define __SYS_BRBSRC(n)			sys_reg(2, 1, 8, ((n) & 0xf), ((((n) & 0x10)) >> 2 + 1))
> >>>>> +#define __SYS_BRBTGT(n)			sys_reg(2, 1, 8, ((n) & 0xf), ((((n) & 0x10)) >> 2 + 2))
> >>>>
> >>>> It's that time on a Friday but... aren't these macros busted? I think you
> >>>> need brackets before adding the offset, otherwise wouldn't, for example,
> >>>> target registers 0-15 all access info register 0 and __SYS_BRBTGT(16) would
> >>>> then start accessing source register 0?
> >>>>
> >>>> I'm surprised that the compiler doesn't warn about this, but even more
> >>>> surprised that you managed to test this.
> >>>>
> >>>> Please tell me I'm wrong!
> >>>>
> >>>> Will
> >>>
> >>> No I think you are right, it is wrong. Luckily there is already an
> >>> extraneous bracket so you you can fix it by moving one a place down:
> >>>
> >>>   sys_reg(2, 1, 8, ((n) & 0xf), ((((n) & 0x10) >> 2) + 2))
> >>>
> >>> It's interesting because the test [1] is doing quite a bit and looking
> >>> at the branch info, and that src and targets match up to function names.
> >>> I also manually looked at the branch buffers and didn't see anything
> >>> obviously wrong like things that looked like branch infos in the source
> >>> or target fields. Will have to take another look to see if it would be
> >>> possible for the test to catch this.
> >>>
> >>> James
> >>>
> >>> [1]:
> >>> https://gitlab.arm.com/linux-arm/linux-jc/-/commit/3a7ddce70c2daadb63fcc511de0a89055ca48b32
> >>
> >> ((((n) & 0x10)) >> 2 + 2) ---> ((((n) & 0x10) >> 2) + 2)
> >>
> >> The additional brackets are useful in explicitly telling the compiler but
> >> what it the compiler is just doing the right thing implicitly i.e computing
> >> the shifting operation before doing the offset addition.
> > 
> > No; that is not correct. In c, '+' has higher precedence than '>>'.
> > 
> > For 'a >> b + c' the compiler *must* treat that as 'a >> (b + c)', and not as
> > '(a >> b) + c'
> > 
> > That's trivial to test:
> > 
> > | [mark@...vadlaks:~]% cat shiftadd.c 
> > | #include <stdio.h>
> > | 
> > | unsigned long logshiftadd(unsigned long a,
> > |                           unsigned long b,
> > |                           unsigned long c)
> > | {
> > |         printf("%ld >> %ld + %ld is %ld\n",
> > |                a, b, c, a >> b + c);
> > | }
> > | 
> > | int main(int argc, char *argv)
> > | {
> > |         logshiftadd(0, 0, 0);
> > |         logshiftadd(0, 0, 1);
> > |         logshiftadd(0, 0, 2);
> > |         printf("\n");
> > |         logshiftadd(1024, 0, 0);
> > |         logshiftadd(1024, 0, 1);
> > |         logshiftadd(1024, 0, 2);
> > |         printf("\n");
> > |         logshiftadd(1024, 2, 0);
> > |         logshiftadd(1024, 2, 1);
> > |         logshiftadd(1024, 2, 2);
> > | 
> > |         return 0;
> > | }
> > | [mark@...vadlaks:~]% gcc shiftadd.c -o shiftadd
> > | [mark@...vadlaks:~]% ./shiftadd 
> > | 0 >> 0 + 0 is 0
> > | 0 >> 0 + 1 is 0
> > | 0 >> 0 + 2 is 0
> > | 
> > | 1024 >> 0 + 0 is 1024
> > | 1024 >> 0 + 1 is 512
> > | 1024 >> 0 + 2 is 256
> > | 
> > | 1024 >> 2 + 0 is 256
> > | 1024 >> 2 + 1 is 128
> > | 1024 >> 2 + 2 is 64
> > 
> >> During testing, all > those captured branch records looked alright.
> > 
> > I think we clearly need better testing here.
> > 
> > Thanks,
> > Mark.
> 
> Hi Will and Mark,
> 
> So I started looking into the test both with and without the fix,
> strangely I couldn't see any difference in the branch outputs, or
> anywhere in the driver where it would be flipping or filtering anything
> to make it only appear to be working. This was a bit confusing, but
> added up with the original point that the test was passing and it was
> actually doing something.
> 
> So I started going deeper and found what the issue (non-issue) is.
> 
> Firstly why is there no warning:
> 
> The expression is stringified and passed to the assembler, so it skips
> the C compiler warning settings. I can send a patch to fix this, but all
> we need to do is get the compiler to evaluate the argument and then
> throw it away, luckily there are no other issues found even with an
> allyesconfig, so BRBE was the only thing with this bug:
> 
>  #define read_sysreg_s(r) ({
>  	u64 __val;
> +	u32 __maybe_unused __check_r = (u32)(r);
>  	asm volatile(__mrs_s("%0", r) : "=r" (__val));
>  	__val;					
>  })
> 
> 
> Secondly, why does BRBE actually work:
> 
> Well the assembler (at least in my Clang toolchain) has a different
> order of operations to C. I put a minimal repro here:
> https://godbolt.org/z/YP9adh5xh
> 
> You can see the op2 should be a 0b100000 (0x20) for BRBSRC and it
> appears to be, you can also see that moving the bracket makes no
> difference in this case.
> 
> For some more evidence, the disassembler I have locally actually gives
> the correct register name, even when the bracket is wrong, and diffing
> the .o file gives no difference when moving the bracket:
> 
>   0000000000000008 <main>:
>    8:   d503245f        bti     c
>    c:   d503201f        nop
>   10:   d503201f        nop
>   14:   2a1f03e0        mov     w0, wzr
>   18:   d5318028        mrs     x8, brbsrc0_el1
>   1c:   d5318128        mrs     x8, brbsrc1_el1
>   20:   d65f03c0        ret
> 
> Seems completely crazy to me that this is actually the case. So maybe I
> am also wrong. Don't know if this counts as a compiler bug or it's just
> supposed to be like that.

>From a quick dig, it's supposed to be like that: the GNU assembler uses a
different operator precedence to C, and clang's assembler does the same for
compatibility. What a great.

Compare:

  https://ftp.gnu.org/old-gnu/Manuals/gas-2.9.1/html_chapter/as_6.html#SEC66

... with:

  https://en.wikipedia.org/wiki/Operators_in_C_and_C%2B%2B#Operator_precedence

Adding the brackets will make this work in either case, so I think that's the
right thing to do for now.

Thanks,
Mark.

Powered by blists - more mailing lists