Re: [bootwrapper PATCH v3 01/15] aarch64: correct ZCR_EL3.LEN initialization

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Jan 25, 2022 at 05:44:47PM +0000, Mark Brown wrote:
> On Tue, Jan 25, 2022 at 04:33:39PM +0000, Mark Rutland wrote:
> > On Tue, Jan 25, 2022 at 03:59:38PM +0000, Mark Brown wrote:
> > > On Tue, Jan 25, 2022 at 03:00:43PM +0000, Mark Rutland wrote:
> 
> > > > As the same time, rename ZCR_EL3_LEN_MASK to ZCR_EL3_LEN_MAX, as it is
> > > > used as a value rather than as a bit mask.
> 
> > > Well, it's both and we do call it a mask in both the kernel and TF-A
> > > too...
> 
> > Here specifically in the boot-wrapper I'm just using it as a value, so I don't
> > see that those are relevant?
> 
> That's also a bunch of the other usage - the kernel uses it to set the
> maximum available value by virtue of setting every possible bit in the
> bitfield, though it does also use it as a mask in places.
> 
> > The idea was to be consistent with the <register>_<field>_<valuename> naming,
> > and avoiding the "why are you programming a mask as the value" questions from
> > last time around, where the unambiguous options are:
> 
> I've not seen that - I did see that there had been some discussion as to
> why the code had been including the reserved bits but hadn't seen any
> discussion of confusion between mask and max.

In:

  https://lore.kernel.org/all/20220117143104.28db5001@donnerap.cambridge.arm.com/#t

w.r.t. the use of ZCR_EL3_LEN, Andre said:

| So when comparing this to the other uses of XXX_EL3_YYY, they typically
| describe a mask, but here we seems to abuse this as a value?
 
... which begat the rename. In this cleanup I've tried to use
<register>_<field> for masks, and <register>_<field>_<valuename> for specific
values to program in, and hence I added ZCR_EL3_LEN_MAX.

I'm going to stick with ZCR_EL3_LEN_MAX.

In that same paragraph he said:

| the existing code writes 0x1ff into that register, presumable to cover future
| vector length extensions 

... which begat the conversation of which length to use.

I later noted that TF-A currently uses 0xf, and so I aligned with that and plan
to stick with it in the absence of any published architectural guidance that we
*should* set the RAZ/WI bits. We can always add that in future when necessary
if/when the field grows, and until then it's less surprising to only be setting
the bits that are documented as needing to be set in this manner.

> > a) Use <register>_<field>_<valuename> definition, as here.
> 
> > b) Use <register>_<field>_MASK, and add comments at each usage as to why we use
> >    a mask as a value, to explain why that isn't a bug.
> 
> > c) Have both <register>_<field>_MASK and some value definition, and use some
> >    insertion helper to insert the value.
> 
> > ... and I went with (a) because it was the simplest.
> 
> > Is there a problem with this?
> 
> My concern is continuity of the enumeration algorithm between the
> various implementations we have, it's something we're not currently
> great with and this creates a separation between the kernel and the boot
> wrapper implementations.  There's no change in what's actually being
> done but it creates some additional effort to figure out why we're
> setting a maximum here and not trying to set all the bits as we do in
> the kernel.

TBH, I'd argue if we're setting those bits in the kernel it's probably a bug,
because we don't know *exactly* what effect they'll have when allocated in the
future.

Thanks,
Mark.

> TF-A is doing something else again (actually two different
> things if you include SME which uses the same algorithm for the
> streaming vector length), for SVE it uses a maximum length too but it
> uses a lower length than the maximum representable one that's used here.
> 
> It may be that the best thing to do is to make this change but also
> update the kernel to have separate but identical mask and maximum
> definitions, though TBH I'd rather not actually have that update happen
> while the SME series is pending.


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