Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Richard Henderson]

On 10/26/21 9:38 AM, Alexander Graf wrote:
> > On 10/26/21 12:12 AM, Alexander Graf wrote:
> > > +        if (cm) {
> > > +            /* We don't cache MMIO regions */
> > > +            advance_pc = true;
> > > +            break;
> > > +        }
> > > +
> > >           assert(isv);
> >
> > The assert should come first.  If the "iss valid" bit is not set, then nothing 
> > else in the word is defined.
> >
> > Otherwise,
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> Yes, but isv=0 for cm=1. And even in other isv=0 situations most other fields 
> are valid (post-idx provides the correct va/pa too for example).
>
> Does cm=1 really give you isv=1 on other hardware?

Ah hah.  From 0487G.a, page D13-3191:

# For other faults reported in ESR_EL2, ISV is 0 except
# for the following stage 2 aborts...

(which incidentally sounds like documenting around a historic chip bug, since both EL1 and 
EL3 do get ISV set).  And of course HVF would be passing along the EL2 version, being the 
hypervisor.

I guess the assert can stand where it is, because everything below should be one of those 
"following stage 2 aborts".   The only thing that could be improved is a bit of commentary 
there at the assert.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~