Re: [PATCH 4/4] KVM: selftests: aarch64: Test read-only PT memory regions

[Ricardo Koller <ricarkol@google.com>]

On Wed, Jan 25, 2023 at 12:26:01PM +0000, Marc Zyngier wrote:
> On Tue, 24 Jan 2023 19:54:57 +0000,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > On Tue, Jan 24, 2023 at 08:26:02AM -0800, Ricardo Koller wrote:
> > > On Mon, Jan 23, 2023 at 11:36:52PM +0000, Oliver Upton wrote:
> > > > On Tue, Jan 10, 2023 at 02:24:32AM +0000, Ricardo Koller wrote:
> > > > > Extend the read-only memslot tests in page_fault_test to test read-only PT
> > > > > (Page table) memslots. Note that this was not allowed before commit "KVM:
> > > > > arm64: Fix handling of S1PTW S2 fault on RO memslots" as all S1PTW faults
> > > > > were treated as writes which resulted in an (unrecoverable) exception
> > > > > inside the guest.
> > > > 
> > > > Do we need an additional test that the guest gets nuked if TCR_EL1.HA =
> > > > 0b1 and AF is clear in one of the stage-1 PTEs?
> > > > 
> > > 
> > > That should be easy to add. The only issue is whether that's also a case
> > > of checking for very specific KVM behavior that could change in the
> > > future.
> > 
> > From the perspective of the guest I believe this to match the
> > architecture. An external abort is appropriate if the hardware update to
> > a descriptor failed.
> > 
> > I believe that the current implementation of this in KVM is slightly
> > wrong, though. AFAICT, we encode the abort with an FSC of 0x10, which
> > indicates an SEA occurred outside of a table walk. The other nuance of
> > reporting SEAs due to a TTW is that the FSC encodes the level at which
> > the external abort occurred. Nonetheless, I think we can hide behind
> > R_BGPQR of DDI0487I.a and always encode a level of 0:
> > 
> > """
> >   If a synchronous External abort is generated due to a TLB or
> >   intermediate TLB caching structure, including parity or ECC errors,
> >   then all of the following are permitted:
> >    - If the PE cannot precisely determine the translation stage at which
> >      the error occurred, then it is reported and prioritized as a stage 1
> >      fault.
> >    - If the PE cannot precisely determine the lookup level at which the
> >      error occurred, then the lookup level is reported and prioritized
> >      as one of the following:
> >      - The lowest-numbered lookup level that could have caused the error.
> >      - If the PE cannot determine any information about the lookup level,
> >      then level 0.
> > """
> > 
> > Thoughts?
> 
> Indeed, the abort injection has always been on the dodgy side of
> things. I remember Christoffer and I writing this, saying that it was
> something we'd have to eventually fix. 10 years down the line, this
> code is, unsurprisingly, still dodgy.
> 
> My vote would be to slightly extend the API to take a set of
> KVM-specific flags to give context to the injection helpers (such as
> SEA during a TTW), and bring the KVM behaviour in line with the
> architecture.
> 
> Reporting 0 in the FSC is probably OK, but we should also be able to
> determine which level this fails at:
> 
> - Sample FAR_EL2[55] to derive which TTBR this translates from (n)
> - From TCR_EL1.{TnSZ,TGn}, you can determine the number of levels
> 
> There is a bunch of tables for this in the ARM ARM, and it is possible
> to come up with a decent formula that encompass all the possible
> combinations.
> 
> But as I said, 0 is probably fine... ;-)
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
> 

Thank you both.

So, what about the following? I can send a series after this that
includes a KVM fix to report level 0 in the FSC in this S1PTW case, and
an extra test that checks that the exception comes with some sane values
(like a sane level in the FSC). Then, getting the actual lookup level
can be added as an improvement (with less priority than the first fix).

Ricardo