Re: [Qemu-devel] [PATCH v2 05/14] target/arm/helper: zcr: Add build bug next to value range assumption

[Andrew Jones]

On Mon, Jun 24, 2019 at 05:03:08PM +0100, Dave Martin wrote:
> On Mon, Jun 24, 2019 at 12:30:37PM +0100, Andrew Jones wrote:
> > On Mon, Jun 24, 2019 at 12:05:07PM +0100, Dave Martin wrote:
> > > On Fri, Jun 21, 2019 at 05:34:13PM +0100, Andrew Jones wrote:
> > > 
> > > The purpose of this check should probably at least be described in a
> > > comment -- i.e., what actually depends on this?
> > 
> > I was thinking the already present "Bits other than [3:0] are RAZ/WI."
> > explained that, but how about this for an improvement?
> > 
> > /*
> >  * Only the lowest 4 bits of ZCR_ELx may be used to constrain the vector
> >  * length, the rest of the bits are RAZ/WI. Since the vector length of
> >  * 128-bits (1 in quadwords) is represented as zero in ZCR_ELx, and all
> >  * vector lengths are represented as their length in quadwords minus 1,
> >  * then four bits allow up to quadword 16 to be selected.
> >  */
> 
> No, maybe the existing comment is enough.
> 
> I thought there might be more code elsewhere that assumes that checks
> sve_max_vq <= ARM_MAX_VQ then then assumes that sve_max_vq <= 16.  But
> if not, we probably don't need an additional comment here.

I suppose there is some assumption that if sve_max_vq > 0 then it is
also <= ARM_MAX_VQ elsewhere in QEMU. However here in zcr_write I don't
think that assumption is being used. Here we're simply enforcing a limit
of 16 within the emulation, without checking sve_max_vq at all. So I like
the suggestion for a build bug like the one this patch adds, because
otherwise we have 16 in two separate places; the ARM_MAX_VQ definition
and the '& 0xf'.

> 
> I haven't tried to understand all the code in the series beyond the
> user/kernel interactions, so maybe I was just paranoid.

Paranoia is good for the soul. Or something like that...

Thanks,
drew