Re: [PATCH 2/4] arm64: mte: Add asynchronous mode support

[Andrey Konovalov]

On Thu, Jan 7, 2021 at 6:25 PM Vincenzo Frascino
<vincenzo.frascino@xxxxxxx> wrote:
>
> Hi Andrey,
>
> On 1/7/21 4:29 PM, Andrey Konovalov wrote:
> > On Wed, Jan 6, 2021 at 12:56 PM Vincenzo Frascino
> > <vincenzo.frascino@xxxxxxx> wrote:
> >>
> >> MTE provides an asynchronous mode for detecting tag exceptions. In
> >> particular instead of triggering a fault the arm64 core updates a
> >> register which is checked by the kernel at the first entry after the tag
> >> exception has occurred.
> >>
> >> Add support for MTE asynchronous mode.
> >>
> >> The exception handling mechanism will be added with a future patch.
> >>
> >> Note: KASAN HW activates async mode via kasan.mode kernel parameter.
> >> The default mode is set to synchronous.
> >>
> >> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> >> Cc: Will Deacon <will.deacon@xxxxxxx>
> >> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@xxxxxxx>
> >> ---
> >>  arch/arm64/kernel/mte.c | 31 +++++++++++++++++++++++++++++--
> >>  1 file changed, 29 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> >> index 24a273d47df1..5d992e16b420 100644
> >> --- a/arch/arm64/kernel/mte.c
> >> +++ b/arch/arm64/kernel/mte.c
> >> @@ -153,8 +153,35 @@ void mte_init_tags(u64 max_tag)
> >>
> >>  void mte_enable_kernel(enum kasan_arg_mode mode)
> >>  {
> >> -       /* Enable MTE Sync Mode for EL1. */
> >> -       sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
> >> +       const char *m;
> >> +
> >> +       /* Preset parameter values based on the mode. */
> >> +       switch (mode) {
> >> +       case KASAN_ARG_MODE_OFF:
> >> +               return;
> >> +       case KASAN_ARG_MODE_LIGHT:
> >> +               /* Enable MTE Async Mode for EL1. */
> >> +               sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_ASYNC);
> >> +               m = "asynchronous";
> >> +               break;
> >> +       case KASAN_ARG_MODE_DEFAULT:
> >> +       case KASAN_ARG_MODE_PROD:
> >> +       case KASAN_ARG_MODE_FULL:
> >> +               /* Enable MTE Sync Mode for EL1. */
> >> +               sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
> >> +               m = "synchronous";
> >> +               break;
> >> +       default:
> >> +               /*
> >> +                * kasan mode should be always set hence we should
> >> +                * not reach this condition.
> >> +                */
> >> +               WARN_ON_ONCE(1);
> >> +               return;
> >> +       }
> >> +
> >> +       pr_info_once("MTE: enabled in %s mode at EL1\n", m);
> >> +
> >>         isb();
> >>  }
> >>
> >> --
> >> 2.29.2
> >>
> >
> > Hi Vincenzo,
> >
> > It would be cleaner to pass a bool to mte_enable_kernel() and have it
> > indicate sync/async mode. This way you don't have to pull all these
> > KASAN constants into the arm64 code.
> >
>
> Boolean arguments are generally bad for legibility, hence I tend to avoid them.
> In this case exposing the constants does not seem a big issue especially because
> the only user of this code is "KASAN_HW_TAGS" and definitely improves its
> legibility hence I would prefer to keep it as is.

I don't like that this spills KASAN internals to the arm64 code. Let's
add another enum with two values and pass it as an argument then.
Something like:

enum mte_mode {
  ARM_MTE_SYNC,
  ARM_MTE_ASYNC
}