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

[Marc Zyngier <maz@kernel.org>]

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.