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

[Marc Zyngier <maz@kernel.org>]

On Wed, 25 Jan 2023 14:02:18 +0000,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> 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).

Works for me. You could also fold the level-0 fix in this series, and
only add the lookup level fix later, if ever.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.