Re: [PATCH 09/27] arm64/sve: Signal frame and context structure definition

[Dave Martin <Dave dot Martin at arm dot com>]

On Tue, Aug 22, 2017 at 02:53:49PM +0100, Alex Bennée wrote:
> 
> Dave Martin <Dave.Martin@arm.com> writes:
> 
> > On Tue, Aug 22, 2017 at 11:22:44AM +0100, Alex Bennée wrote:
> >>
> >> Dave Martin <Dave.Martin@arm.com> writes:

[...]

> >> > +/*
> >> > + * The SVE architecture leaves space for future expansion of the
> >> > + * vector length beyond its initial architectural limit of 2048 bits
> >> > + * (16 quadwords).
> >> > + */
> >> > +#define SVE_VQ_MIN		1
> >> > +#define SVE_VQ_MAX		0x200
> >> > +
> >> > +#define SVE_VL_MIN		(SVE_VQ_MIN * 0x10)
> >> > +#define SVE_VL_MAX		(SVE_VQ_MAX * 0x10)
> >> > +
> >> > +#define SVE_NUM_ZREGS		32
> >> > +#define SVE_NUM_PREGS		16
> >> > +
> >> > +#define sve_vl_valid(vl) \
> >> > +	((vl) % 0x10 == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
> >> > +#define sve_vq_from_vl(vl)	((vl) / 0x10)
> >> > +#define sve_vl_from_vq(vq)	((vq) * 0x10)
> >>
> >> I got a little confused first time through over what VQ and VL where.
> >> Maybe it would make sense to expand a little more from first principles?
> >>
> >>   /*
> >>    * The SVE architecture defines vector registers as a multiple of 128
> >>    * bit quadwords. The current architectural limit is 2048 bits (16
> >>    * quadwords) but there is room for future expansion beyond that.
> >>     */
> >
> > This comes up in several places and so I didn't want to comment it
> > repeatedly everywhere.
> >
> > Instead, I wrote up something in section 2 (Vector length terminology)
> > of Documentation/arm64/sve.txt -- see patch 25.  Can you take a look and
> > see whether that's adequate?
> 
> Ahh, I hadn't got to that yet. I'm unsure to the order the kernel likes
> to put things but I like to put design documents at the front of the

I don't have a strong opinion on that -- I had preferred not to add a
document describing stuff that doesn't exist at the time of commit.
I could commit a stub document at the start of the series, and then
commit the real document later.

Either way, it seemed overkill.

Perhaps I should have drawn more attention to the documentation in the
cover letter, and encouraged reviewers to look at it first.  My
experience is that people don't often read cover letters...

Now the series is posted, I'm minded to keep the order as-is, unless you
think it's a big issue.

Adding a reference to the document seems a reasonable thing to do,
so I could add that.

> patch queue as they are useful primers and saves you having to patch a:
> 
> modified   arch/arm64/include/uapi/asm/sigcontext.h
> @@ -132,19 +132,24 @@ struct sve_context {
>  /*
>   * The SVE architecture leaves space for future expansion of the
>   * vector length beyond its initial architectural limit of 2048 bits
> - * (16 quadwords).
> + * (16 quadwords). See Documentation/arm64/sve.txt for a summary of
> + * the terminology of Vector Quads (VQ) and Vector Lengths (VL).
>   */
> +
> +#define SVE_VQ_BITS             128      /* 128 bits in one quadword */
> +#define SVE_VQ_BYTES            (SVE_VQ_BITS / 8)
> +

I was trying to keep extraneous #defines to a minimum, since this is a
uapi header, and people may depend on anything defined here.

I think SVE_VQ_BYTES is reasonable to have, and this allows us to
rewrite a few hard-coded 0x10s and 16s symbolically which is probably a
good idea -- I'll add this.

SVE_VQ_BITS looks redundant to me though.  It wouldn't be used for any
purpose other than defining SVE_VQ_BYTES.

>  #define SVE_VQ_MIN		1
>  #define SVE_VQ_MAX		0x200
> 
> -#define SVE_VL_MIN		(SVE_VQ_MIN * 0x10)
> -#define SVE_VL_MAX		(SVE_VQ_MAX * 0x10)
> +#define SVE_VL_MIN		(SVE_VQ_MIN * SVE_VQ_BYTES)
> +#define SVE_VL_MAX		(SVE_VQ_MAX * SVE_VQ_BYTES)
> 
>  #define SVE_NUM_ZREGS		32
>  #define SVE_NUM_PREGS		16
> 
>  #define sve_vl_valid(vl) \
> -	((vl) % 0x10 == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
> +	((vl) % SVE_VQ_BYTES == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
>  #define sve_vq_from_vl(vl)	((vl) / 0x10)
>  #define sve_vl_from_vq(vq)	((vq) * 0x10)

[...]

Cheers
---Dave