On Tue, Aug 22, 2017 at 05:21:19PM +0100, Alex Bennée wrote: > > Dave Martin <Dave.Martin@xxxxxxx> 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 > <snip> > > 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. Cheers ---Dave