Re: [PATCH v2 06/26] arm64: Add debug registers affected by HDFGxTR_EL2

[Marc Zyngier <maz@kernel.org>]

On Thu, 03 Aug 2023 01:00:52 +0100,
Miguel Luis <miguel.luis@oracle.com> wrote:
> 
> Hi Marc,
> 
> > On 2 Aug 2023, at 17:52, Marc Zyngier <maz@kernel.org> wrote:
> > 
> > On Mon, 31 Jul 2023 17:41:41 +0100,
> > Miguel Luis <miguel.luis@oracle.com> wrote:
> >> 
> >> Hi Marc,
> >> 
> >> A few comments on this one, please see below.
> >> 
> >>> On 28 Jul 2023, at 08:29, Marc Zyngier <maz@kernel.org> wrote:
> >>> 
> >>> The HDFGxTR_EL2 registers trap a (huge) set of debug and trace
> >>> related registers. Add their encodings (and only that, because
> >>> we really don't care about what these registers actually do at
> >>> this stage).
> >>> 
> >>> Signed-off-by: Marc Zyngier <maz@kernel.org>
> >>> ---
> >>> arch/arm64/include/asm/sysreg.h | 78 +++++++++++++++++++++++++++++++++
> >>> 1 file changed, 78 insertions(+)
> >>> 
> >>> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> >>> index 76289339b43b..9dfd127be55a 100644
> >>> --- a/arch/arm64/include/asm/sysreg.h
> >>> +++ b/arch/arm64/include/asm/sysreg.h
> >>> @@ -194,6 +194,84 @@
> >>> #define SYS_DBGDTRTX_EL0 sys_reg(2, 3, 0, 5, 0)
> >>> #define SYS_DBGVCR32_EL2 sys_reg(2, 4, 0, 7, 0)
> >>> 
> >>> +#define SYS_BRBINF_EL1(n) sys_reg(2, 1, 8, (n & 15), (((n & 16) >> 2) | 0))
> >>> +#define SYS_BRBINFINJ_EL1 sys_reg(2, 1, 9, 1, 0)
> >>> +#define SYS_BRBSRC_EL1(n) sys_reg(2, 1, 8, (n & 15), (((n & 16) >> 2) | 1))
> >>> +#define SYS_BRBSRCINJ_EL1 sys_reg(2, 1, 9, 1, 1)
> >>> +#define SYS_BRBTGT_EL1(n) sys_reg(2, 1, 8, (n & 15), (((n & 16) >> 2) | 2))
> >>> +#define SYS_BRBTGTINJ_EL1 sys_reg(2, 1, 9, 1, 2)
> >>> +#define SYS_BRBTS_EL1 sys_reg(2, 1, 9, 0, 2)
> >>> +
> >>> +#define SYS_BRBCR_EL1 sys_reg(2, 1, 9, 0, 0)
> >>> +#define SYS_BRBFCR_EL1 sys_reg(2, 1, 9, 0, 1)
> >>> +#define SYS_BRBIDR0_EL1 sys_reg(2, 1, 9, 2, 0)
> >>> +
> >>> +#define SYS_TRCITECR_EL1 sys_reg(3, 0, 1, 2, 3)
> >>> +#define SYS_TRCITECR_EL1 sys_reg(3, 0, 1, 2, 3)
> >> 
> >> SYS_TRCITECR_EL1 shows up twice.
> > 
> > Ah, nice one. Too many registers.
> > 
> >> 
> >>> +#define SYS_TRCACATR(m) sys_reg(2, 1, 2, ((m & 7) << 1), (2 | (m >> 3)))
> >> 
> >> Besides m’s restrictions it could be sanitised in op2 to consider only bit m[3].
> >> Suggestion for op2: (2 | ((m & 8) >> 3)))
> > 
> > It is fully expected that 'm' will be in the 0-15 range, as per the
> > architecture (D19.4.8), and the tables only use that exact range.
> > 
> > Do you see an actual bug, or is this defensive programming?
> > 
> 
> I was seeing a problem when m[5]=1 then Op2 of 0b01:m[3] isn’t guaranteed
> anymore overridden with 0b11:m[3].

But that'd be wrong anyway. Now, we'd have an encoding that possibly
aliases to something else, and we don't know about it.

> Clearly ‘m’ would be outside the range but not messing with Op2 fixed bits 0b01.
> Not a problem for patch 21 though.

All these macros have as the basis for their use that you use *valid*
input.

> Due to the uncertainty if this can bite later, hence the suggestion and also
> open to advices.

If you really want some defensive programming on that front, then you
should make sure we detect the issue at compile time, rather than
silently changing the encoding.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.