On Wed, 27 Dec 2023 at 23:55, Richard Henderson
<richard.henderson@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.