This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 11/27] arm64/sve: Core task context handling
- From: Dave Martin <Dave dot Martin at arm dot com>
- To: Ard Biesheuvel <ard dot biesheuvel at linaro dot org>
- Cc: "linux-arch at vger dot kernel dot org" <linux-arch at vger dot kernel dot org>, libc-alpha at sourceware dot org, Szabolcs Nagy <szabolcs dot nagy at arm dot com>, Catalin Marinas <catalin dot marinas at arm dot com>, Will Deacon <will dot deacon at arm dot com>, Richard Sandiford <richard dot sandiford at arm dot com>, "kvmarm at lists dot cs dot columbia dot edu" <kvmarm at lists dot cs dot columbia dot edu>, "linux-arm-kernel at lists dot infradead dot org" <linux-arm-kernel at lists dot infradead dot org>
- Date: Thu, 17 Aug 2017 17:42:36 +0100
- Subject: Re: [PATCH 11/27] arm64/sve: Core task context handling
- Authentication-results: sourceware.org; auth=none
- References: <1502280338-23002-1-git-send-email-Dave.Martin@arm.com> <1502280338-23002-12-git-send-email-Dave.Martin@arm.com> <CAKv+Gu-Rei0LP9q77+KHtD4JqM-6j4XvQjG7oCLrjHx2M5DA7g@mail.gmail.com>
On Tue, Aug 15, 2017 at 06:31:05PM +0100, Ard Biesheuvel wrote:
> Hi Dave,
>
> On 9 August 2017 at 13:05, Dave Martin <Dave.Martin@arm.com> wrote:
> > This patch adds the core support for switching and managing the SVE
> > architectural state of user tasks.
[...]
> > +static u64 sve_cpacr_trap_on(u64 cpacr)
> > +{
> > + return cpacr & ~(u64)CPACR_EL1_ZEN_EL0EN;
> > +}
> > +
> > +static u64 sve_cpacr_trap_off(u64 cpacr)
> > +{
> > + return cpacr | CPACR_EL1_ZEN_EL0EN;
> > +}
> > +
> > +static void change_cpacr(u64 old, u64 new)
> > +{
> > + if (old != new)
> > + write_sysreg(new, CPACR_EL1);
> > +}
[...]
> > +static void task_fpsimd_load(void)
> > +{
> > + if (system_supports_sve() && test_thread_flag(TIF_SVE)) {
> > + unsigned int vl = current->thread.sve_vl;
> > +
> > + BUG_ON(!sve_vl_valid(vl));
> > + sve_load_state(sve_pffr(current),
> > + ¤t->thread.fpsimd_state.fpsr,
> > + sve_vq_from_vl(vl) - 1);
> > + } else
> > + fpsimd_load_state(¤t->thread.fpsimd_state);
> > +
>
> Please use braces consistently on all branches of an if ()
>
> > + if (system_supports_sve()) {
> > + u64 cpacr = read_sysreg(CPACR_EL1);
> > + u64 new_cpacr;
> > +
> > + /* Toggle SVE trapping for userspace if needed */
> > + if (test_thread_flag(TIF_SVE))
> > + new_cpacr = sve_cpacr_trap_off(cpacr);
> > + else
> > + new_cpacr = sve_cpacr_trap_on(cpacr);
> > +
> > + change_cpacr(cpacr, new_cpacr);
>
> I understand you want to avoid setting CPACR to the same value, but
> this does look a bit clunky IMO. Wouldn't it be much better to have a
> generic accessor with a mask and a value that encapsulates this?
For this I now have:
static void change_cpacr(u64 val, u64 mask)
{
u64 cpacr = read_sysreg(CPACR_EL1);
u64 new = (cpacr & ~mask) | val;
if (new != cpacr)
write_sysreg(new, CPACR_EL1);
}
static void sve_cpacr_trap_on(void)
{
change_cpacr(0, CPACR_EL1_ZEN_EL0EN);
}
static void sve_cpacr_trap_off(void)
{
change_cpacr(CPACR_EL1_ZEN_EL0EN, CPACR_EL1_ZEN_EL0EN);
}
This is stilla little verbose, but fairly clean. Possibly this was the
sort of thing you meant by a generic accessor.
What do you think?
[...]
Cheers
---Dave