linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Ard Biesheuvel <ardb@kernel.org>
To: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	 Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Sudeep Holla <sudeep.holla@arm.com>,
	 Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v1 2/2] arm64: smccc: Support SMCCC v1.3 SVE register saving hint
Date: Tue, 18 May 2021 18:53:59 +0200	[thread overview]
Message-ID: <CAMj1kXGxmX0eGaux_H=_N2a5SPit-WkdvCmYv2SxqiG6tiDchw@mail.gmail.com> (raw)
In-Reply-To: <87pmxotirl.wl-maz@kernel.org>

On Tue, 18 May 2021 at 12:55, Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Mark,
>
> On Tue, 18 May 2021 11:09:19 +0100,
> Mark Brown <broonie@kernel.org> wrote:
> >
> > SMCCC v1.2 requires that all SVE state be preserved over SMC calls which
> > introduces substantial overhead in the common case where there is no SVE
> > state in the registers. To avoid this SMCCC v1.3 introduces a flag which
> > allows the caller to say that there is no state that needs to be preserved
> > in the registers. Make use of this flag, setting it if the SMCCC version
> > indicates support for it and the TIF_ flags indicate that there is no live
> > SVE state in the registers, this avoids placing any constraints on when
> > SMCCC calls can be done or triggering extra saving and reloading of SVE
> > register state in the kernel.
> >
> > This would be straightforward enough except for the rather entertaining
> > inline assembly we use to do SMCCC v1.1 calls to allow us to take advantage
> > of the limited number of registers it clobbers. Deal with this by having a
> > slightly non-standard function which we call immediately before issuing the
> > SMCCC call to make our checks. This causes an extra function call on any
> > system built with SVE support, and extra checks when SVE is detected at
> > runtime, but these costs are expected to be reasonable in the context of
> > doing a SMCCC call in the first place.
> >
> > Signed-off-by: Mark Brown <broonie@kernel.org>
> > ---
> >  arch/arm64/kernel/smccc-call.S | 39 ++++++++++++++++++++++++++++++++++
> >  drivers/firmware/smccc/smccc.c |  4 ++++
> >  include/linux/arm-smccc.h      | 22 +++++++++++++++++--
> >  3 files changed, 63 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/smccc-call.S b/arch/arm64/kernel/smccc-call.S
> > index d62447964ed9..217bfb03a6c9 100644
> > --- a/arch/arm64/kernel/smccc-call.S
> > +++ b/arch/arm64/kernel/smccc-call.S
> > @@ -7,8 +7,47 @@
> >
> >  #include <asm/asm-offsets.h>
> >  #include <asm/assembler.h>
> > +#include <asm/thread_info.h>
> > +
> > +/*
> > + * If we have SMCCC v1.3 and (as is likely) no SVE state in
> > + * the registers then set the SMCCC hint bit to say there's no
> > + * need to preserve it.  Do this by directly adjusting the SMCCC
> > + * function value which is already stored in x0 ready to be called.
> > + *
> > + * Since we need scratch registers but wish to avoid having to handle
> > + * the stack we expect the caller to preserve x15 and x16 if needed,
> > + * the only callers are expected to be the call below and the inline
> > + * asm in linux/arm-smccc.h for SMCCC 1.1 and later calls.
> > + */
> > +SYM_CODE_START(__smccc_sve_check)
> > +     BTI_C
> > +
> > +alternative_if ARM64_SVE
> > +
> > +     adrp    x15, smccc_has_sve_hint
>
> The adrp instruction will give you a 4kB aligned address, which
> results in 1 chance out of 512 to point to the right location. The
> adr_l macro is probably what you want.
>

... or use ldr_l, in which case the ldr below can be omitted as well.


> > +     ldr     x15, [x15]
> > +     cbz     x15, 1f
> > +
> > +     get_current_task x15
> > +     ldr     x15, [x15, #TSK_TI_FLAGS]
> > +     and     x16, x15, #TIF_FOREIGN_FPSTATE  // Any live FP state?
> > +     cbnz    x16, 1f
> > +     mov     x16, #TIF_SVE                   // Does that state include SVE?
> > +     and     x16, x15, x16
> > +     cbnz    x16, 2f
> > +
> > +1:   orr     x0, x0, ARM_SMCCC_1_3_SVE_HINT
> > +alternative_else_nop_endif
> > +
> > +2:   ret
> > +SYM_CODE_END(__smccc_sve_check)
> > +EXPORT_SYMBOL(__smccc_sve_check)
> >
> >       .macro SMCCC instr
> > +alternative_if ARM64_SVE
> > +     bl      __smccc_sve_check
> > +alternative_else_nop_endif
> >       \instr  #0
> >       ldr     x4, [sp]
> >       stp     x0, x1, [x4, #ARM_SMCCC_RES_X0_OFFS]
> > diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
> > index 028f81d702cc..9f937b125ab0 100644
> > --- a/drivers/firmware/smccc/smccc.c
> > +++ b/drivers/firmware/smccc/smccc.c
> > @@ -15,6 +15,7 @@ static u32 smccc_version = ARM_SMCCC_VERSION_1_0;
> >  static enum arm_smccc_conduit smccc_conduit = SMCCC_CONDUIT_NONE;
> >
> >  bool __ro_after_init smccc_trng_available = false;
> > +u64 __ro_after_init smccc_has_sve_hint = false;
> >
> >  void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
> >  {
> > @@ -22,6 +23,9 @@ void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
> >       smccc_conduit = conduit;
> >
> >       smccc_trng_available = smccc_probe_trng();
> > +     if (IS_ENABLED(CONFIG_ARM64_SVE) &&
> > +         smccc_version >= ARM_SMCCC_VERSION_1_3)
> > +             smccc_has_sve_hint = true;
> >  }
> >
> >  enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void)
> > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> > index 6861489a1890..611f3bc9c131 100644
> > --- a/include/linux/arm-smccc.h
> > +++ b/include/linux/arm-smccc.h
> > @@ -63,6 +63,9 @@
> >  #define ARM_SMCCC_VERSION_1_0                0x10000
> >  #define ARM_SMCCC_VERSION_1_1                0x10001
> >  #define ARM_SMCCC_VERSION_1_2                0x10002
> > +#define ARM_SMCCC_VERSION_1_3                0x10003
> > +
> > +#define ARM_SMCCC_1_3_SVE_HINT               0x10000
> >
> >  #define ARM_SMCCC_VERSION_FUNC_ID                                    \
> >       ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,                         \
> > @@ -216,6 +219,8 @@ u32 arm_smccc_get_version(void);
> >
> >  void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit);
> >
> > +extern u64 smccc_has_sve_hint;
> > +
> >  /**
> >   * struct arm_smccc_res - Result from SMC/HVC call
> >   * @a0-a3 result values from registers 0 to 3
> > @@ -297,6 +302,18 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
> >
> >  #endif
> >
> > +#ifdef CONFIG_ARM64_SVE
> > +
> > +#define SMCCC_SVE_CHECK "bl __smccc_sve_check \n"
> > +#define smccc_sve_clobbers "x15", "x16", "lr",
> > +
> > +#else
> > +
> > +#define SMCCC_SVE_CHECK
> > +#define smccc_sve_clobbers
> > +
> > +#endif
> > +
> >  #define ___count_args(_0, _1, _2, _3, _4, _5, _6, _7, _8, x, ...) x
> >
> >  #define __count_args(...)                                            \
> > @@ -364,7 +381,7 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
> >
> >  #define ___constraints(count)                                                \
> >       : __constraint_read_ ## count                                   \
> > -     : "memory"
> > +     : smccc_sve_clobbers "memory"
> >  #define __constraints(count) ___constraints(count)
> >
> >  /*
> > @@ -379,7 +396,8 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
> >               register unsigned long r2 asm("r2");                    \
> >               register unsigned long r3 asm("r3");                    \
> >               __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
> > -             asm volatile(inst "\n" :                                \
> > +             asm volatile(SMCCC_SVE_CHECK                            \
>
> What happens when this is called from a context where
> __smccc_sve_check isn't mapped, nor does "current" mean anything? See
> arch/arm64/kvm/hyp/nvhe/psci-relay.c for an example.
>
> > +                          inst "\n" :                                \
> >                            "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3) \
> >                            __constraints(__count_args(__VA_ARGS__))); \
> >               if (___res)                                             \
>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  parent reply	other threads:[~2021-05-18 16:55 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-18 10:09 [PATCH v1 0/2] arm64: Implement SMCCC v1.3 SVE register saving hint Mark Brown
2021-05-18 10:09 ` [PATCH v1 1/2] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE Mark Brown
2021-05-18 10:09 ` [PATCH v1 2/2] arm64: smccc: Support SMCCC v1.3 SVE register saving hint Mark Brown
2021-05-18 10:53   ` Marc Zyngier
2021-05-18 11:53     ` Mark Brown
2021-05-18 12:14       ` Marc Zyngier
2021-05-18 13:01         ` Mark Brown
2021-05-18 16:53     ` Ard Biesheuvel [this message]
2021-05-18 16:43   ` Sudeep Holla
2021-05-18 16:55     ` Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMj1kXGxmX0eGaux_H=_N2a5SPit-WkdvCmYv2SxqiG6tiDchw@mail.gmail.com' \
    --to=ardb@kernel.org \
    --cc=broonie@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=maz@kernel.org \
    --cc=sudeep.holla@arm.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).