On Tue, Jun 25, 2019 at 08:11:27AM +0200, Andrew Jones wrote: > 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 suppose we could also write the 0xf in terms of ARM_MAX_VQ, with a (ARM_MAX_VQ - 1), but that's getting into emulation implementation preferences, which I don't know anything about. So I'd leave that to Richard and Peter. > > > > > 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
