Re: [PATCH v2 2/2] arm64: smccc: Support SMCCC v1.3 SVE register saving hint

From: Mark Brown <broonie@kernel.org>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: 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>,
	Marc Zyngier <maz@kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2 2/2] arm64: smccc: Support SMCCC v1.3 SVE register saving hint
Date: Wed, 19 May 2021 14:07:59 +0100	[thread overview]
Message-ID: <20210519130759.GC4224@sirena.org.uk> (raw)
In-Reply-To: <CAMj1kXFLU0hndmr_G1Zq=3JDP1FpLx7-r3R3gXwThVuejgPVkw@mail.gmail.com>

[-- Attachment #1.1: Type: text/plain, Size: 1952 bytes --]

On Wed, May 19, 2021 at 09:05:41AM +0200, Ard Biesheuvel wrote:
> On Tue, 18 May 2021 at 20:35, Mark Brown <broonie@kernel.org> wrote:

> > + * 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,

> You should use x16 and x17 here. The calling convention dictates that
> the linker may use x16 and x17 in a veneer (or PLT lookup sequence) if
> the branch to  __smccc_sve_check() goes out of range, and even if our
> PLT code only uses x16 at this time, it is better to follow the AAPCS
> here.

Oh, of course - not sure why I didn't use x17 TBH.

> > + * 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

> Any reason not to use SYM_FUNC_START() here?

Not with the changes you've suggested, 

> > +alternative_if ARM64_SVE
> > +
> > +       ldr_l   x15, smccc_has_sve_hint
> > +       cbz     x15, 2f
> > +
> > +       get_current_task x15
> > +       ldr     x15, [x15, #TSK_TI_FLAGS]
> > +       and     x16, x15, #_TIF_FOREIGN_FPSTATE // Any live FP state?
> > +       cbnz    x16, 1f

> You could use tbnz here, with the actual shift in TIF_FOREIGN_FPSTATE

Ah, yes - I'd misremembered what tbnz uses and thought it didn't apply.
That does help a lot.

> >         .macro SMCCC instr
> > +alternative_if ARM64_SVE
> > +       bl      __smccc_sve_check
> > +alternative_else_nop_endif

> Do we need the redundant alternatives here? Could we use an
> ALTERNATIVE() below instead, and drop the one in __smccc_sve_check?

We do need the one right there, but with the use of ALTERNATIVE() in the
inline asm we'd be able to drop the one in __smccc_sve_check.  Or for
that matter we could not bother with alternatives at all if we're happy
that the overhead were OK.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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