From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex =?utf-8?Q?Benn=C3=A9e?= Subject: Re: [PATCH 11/27] arm64/sve: Core task context handling Date: Tue, 22 Aug 2017 19:39:48 +0100 Message-ID: <87ziara1l7.fsf@linaro.org> References: <1502280338-23002-1-git-send-email-Dave.Martin@arm.com> <1502280338-23002-12-git-send-email-Dave.Martin@arm.com> <87378jbmkg.fsf@linaro.org> <20170822171959.GX6321@e103592.cambridge.arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Received: from mail-wr0-f169.google.com ([209.85.128.169]:36958 "EHLO mail-wr0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752229AbdHVSjv (ORCPT ); Tue, 22 Aug 2017 14:39:51 -0400 Received: by mail-wr0-f169.google.com with SMTP id z91so128146523wrc.4 for ; Tue, 22 Aug 2017 11:39:51 -0700 (PDT) In-reply-to: <20170822171959.GX6321@e103592.cambridge.arm.com> Sender: linux-arch-owner@vger.kernel.org List-ID: To: Dave Martin Cc: linux-arch@vger.kernel.org, libc-alpha@sourceware.org, Ard Biesheuvel , Szabolcs Nagy , Catalin Marinas , Will Deacon , Richard Sandiford , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Dave Martin writes: > On Tue, Aug 22, 2017 at 05:21:19PM +0100, Alex Bennée wrote: >> >> Dave Martin writes: > > [...] > >> > --- a/arch/arm64/include/asm/processor.h >> > +++ b/arch/arm64/include/asm/processor.h >> > @@ -85,6 +85,8 @@ struct thread_struct { >> > unsigned long tp2_value; >> > #endif >> > struct fpsimd_state fpsimd_state; >> > + void *sve_state; /* SVE registers, if any */ >> > + u16 sve_vl; /* SVE vector length */ >> >> sve_vl is implicitly cast to unsigned int bellow - it should be >> consistent. > > Agreed -- I think this can just be unsigned int here, which is the type > I use everywhere except when encoding the vector length in the signal > frame and ptrace data. > > During development, there was an additional flags field here (now > merged into the thread_info flags). u16 was big enough for the largest > architectural vector length, so it seemed "tidy" to make both u16s. > Elsewhere, this created a risk of overflow if you try to compute say > the size of the whole register file from a u16, so I generally used > unsigned int for local variables in functions that handle the vl. > >> Given the allocation functions rely on sve_vl being valid it might be >> worth noting where this is set/live from? > > Agreed, I need to look more closely at exactly how to justify the > soundness of this in order to thin out BUG_ONs. > > If you need any pointers, please ping me. In the meantime, I would > rather not colour your judgement by infecting you with my assumptions ;) > >> > unsigned long fault_address; /* fault info */ >> > unsigned long fault_code; /* ESR_EL1 value */ >> > struct debug_info debug; /* debugging */ >> > diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h >> > index 46c3b93..1a4b30b 100644 >> > --- a/arch/arm64/include/asm/thread_info.h >> > +++ b/arch/arm64/include/asm/thread_info.h >> >> >> And I see there are other comments from Ard. > > Yup, I've already picked those up. > > I'll be posting a v2 with feedback applied once I've reviewed the > existing BUG_ON()s -- should happen sometime this week. We shall see how far I get tomorrow - you won't get any more from me after that until I get back from holiday so don't delay v2 on my account ;-) > > Cheers > ---Dave From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f169.google.com ([209.85.128.169]:36958 "EHLO mail-wr0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752229AbdHVSjv (ORCPT ); Tue, 22 Aug 2017 14:39:51 -0400 Received: by mail-wr0-f169.google.com with SMTP id z91so128146523wrc.4 for ; Tue, 22 Aug 2017 11:39:51 -0700 (PDT) References: <1502280338-23002-1-git-send-email-Dave.Martin@arm.com> <1502280338-23002-12-git-send-email-Dave.Martin@arm.com> <87378jbmkg.fsf@linaro.org> <20170822171959.GX6321@e103592.cambridge.arm.com> From: Alex =?utf-8?Q?Benn=C3=A9e?= Subject: Re: [PATCH 11/27] arm64/sve: Core task context handling In-reply-to: <20170822171959.GX6321@e103592.cambridge.arm.com> Date: Tue, 22 Aug 2017 19:39:48 +0100 Message-ID: <87ziara1l7.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-arch-owner@vger.kernel.org List-ID: To: Dave Martin Cc: linux-arch@vger.kernel.org, libc-alpha@sourceware.org, Ard Biesheuvel , Szabolcs Nagy , Catalin Marinas , Will Deacon , Richard Sandiford , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Message-ID: <20170822183948.zmAr-AWg4IB6NpHB0uf_wbv89Yh1_sagDFGZtscSjr8@z> Dave Martin writes: > On Tue, Aug 22, 2017 at 05:21:19PM +0100, Alex Bennée wrote: >> >> Dave Martin writes: > > [...] > >> > --- a/arch/arm64/include/asm/processor.h >> > +++ b/arch/arm64/include/asm/processor.h >> > @@ -85,6 +85,8 @@ struct thread_struct { >> > unsigned long tp2_value; >> > #endif >> > struct fpsimd_state fpsimd_state; >> > + void *sve_state; /* SVE registers, if any */ >> > + u16 sve_vl; /* SVE vector length */ >> >> sve_vl is implicitly cast to unsigned int bellow - it should be >> consistent. > > Agreed -- I think this can just be unsigned int here, which is the type > I use everywhere except when encoding the vector length in the signal > frame and ptrace data. > > During development, there was an additional flags field here (now > merged into the thread_info flags). u16 was big enough for the largest > architectural vector length, so it seemed "tidy" to make both u16s. > Elsewhere, this created a risk of overflow if you try to compute say > the size of the whole register file from a u16, so I generally used > unsigned int for local variables in functions that handle the vl. > >> Given the allocation functions rely on sve_vl being valid it might be >> worth noting where this is set/live from? > > Agreed, I need to look more closely at exactly how to justify the > soundness of this in order to thin out BUG_ONs. > > If you need any pointers, please ping me. In the meantime, I would > rather not colour your judgement by infecting you with my assumptions ;) > >> > unsigned long fault_address; /* fault info */ >> > unsigned long fault_code; /* ESR_EL1 value */ >> > struct debug_info debug; /* debugging */ >> > diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h >> > index 46c3b93..1a4b30b 100644 >> > --- a/arch/arm64/include/asm/thread_info.h >> > +++ b/arch/arm64/include/asm/thread_info.h >> >> >> And I see there are other comments from Ard. > > Yup, I've already picked those up. > > I'll be posting a v2 with feedback applied once I've reviewed the > existing BUG_ON()s -- should happen sometime this week. We shall see how far I get tomorrow - you won't get any more from me after that until I get back from holiday so don't delay v2 on my account ;-) > > Cheers > ---Dave -- Alex Bennée