On Wed, 27 Dec 2023 at 23:55, Richard Henderson
<richard.hender...@linaro.org> wrote:
>
> On 12/18/23 22:32, Peter Maydell wrote:
> > + if (s->nv && s->nv2 && ri->nv2_redirect_offset) {
>
> Again, s->nv test is redundant.
Fixed, thanks.
> > + /*
> > + * Some registers always redirect to memory; some only do so if
> > + * HCR_EL2.NV1 is 0, and some only if NV1 is 1 (these come in
> > + * pairs which share an offset; see the table in R_CSRPQ).
> > + */
> > + if (ri->nv2_redirect_offset & NV2_REDIR_NV1) {
> > + nv2_mem_redirect = s->nv1;
> > + } else if (ri->nv2_redirect_offset & NV2_REDIR_NO_NV1) {
> > + nv2_mem_redirect = !s->nv1;
> > + } else {
> > + nv2_mem_redirect = true;
> > + }
>
> I wondered if it would be clearer with the "both" case having both bits set.
> While I see
> that the first defined offset is 0x20, offset 0x00 is still reserved and
> *could* be used.
> At which point ri->nv2_redirect_offset would need a non-zero value for a zero
> offset.
>
> Maybe clearer as
>
> nv2_mem_redirect = (ri->nv2_redirect_offset &
> (s->nv1 ? NV2_REDIR_NV1_1 : NV2_REDIR_NV1_0));
>
> ?
>
> This is more verbose for the (common?) case of redirect regardless of nv1, so
> maybe not.
Yes, my motivation for the notation I used is that I wanted to
make the specification of the cpreg structs in the common case
simple and not too long-winded. If offset 0 does ever get
allocated, we'll have to come back and revisit this. But
new entries clearly seem to be being allocated at the other
end of the table, so I think our chances are good...
> > + if (s->nv2_mem_be) {
> > + mop |= MO_BE;
> > + }
>
> MO_BSWAP is host dependent -- needs
>
> mop |= (s->nv2_mem_be ? MO_BE : MO_LE);
Fixed.
With those two fixes, can I have a reviewed-by? This is the
only patch without one, and all the fixes seem to me like
very minor things not worth sending out a full v2 for.
thanks
-- PMM
