[PATCH v6 08/12] KVM: arm64: Convert translation level parameter to s8

[Ard Biesheuvel]

On Tue, 28 Nov 2023 at 14:41, Marc Zyngier <maz at kernel.org> wrote:
>
> On Tue, 28 Nov 2023 12:23:38 +0000,
> Ryan Roberts <ryan.roberts at arm.com> wrote:
> >
> > On 28/11/2023 10:49, Ard Biesheuvel wrote:
> > > On Mon, 27 Nov 2023 at 12:18, Ryan Roberts <ryan.roberts at arm.com> wrote:
> > >>
> > >> With the introduction of FEAT_LPA2, the Arm ARM adds a new lel of
> > >> translation, level -1, so levels can now be in the range [-1;3]. 3 is
> > >> always the last level and the first level is determined based on the
> > >> number of VA bits in use.
> > >>
> > >> Convert level variables to use a signed type in preparation for
> > >> supporting this new level -1.
> > >>
> > >> Since the last level is always anchored at 3, and the first level varies
> > >> to suit the number of VA/IPA bits, take the opportunity to replace
> > >> KVM_PGTABLE_MAX_LEVELS with the 2 macros KVM_PGTABLE_FIRST_LEVEL and
> > >> KVM_PGTABLE_LAST_LEVEL. This removes the assumption from the code that
> > >> levels run from 0 to KVM_PGTABLE_MAX_LEVELS - 1, which will soon no
> > >> longer be true.
> > >>
> > >> Reviewed-by: Oliver Upton <oliver.upton at linux.dev>
> > >> Signed-off-by: Ryan Roberts <ryan.roberts at arm.com>
> > >> ---
> > >>  arch/arm64/include/asm/kvm_emulate.h  |  2 +-
> > >>  arch/arm64/include/asm/kvm_pgtable.h  | 31 +++++++------
> > >>  arch/arm64/include/asm/kvm_pkvm.h     |  5 +-
> > >>  arch/arm64/kvm/hyp/nvhe/mem_protect.c |  6 +--
> > >>  arch/arm64/kvm/hyp/nvhe/mm.c          |  4 +-
> > >>  arch/arm64/kvm/hyp/nvhe/setup.c       |  2 +-
> > >>  arch/arm64/kvm/hyp/pgtable.c          | 66 +++++++++++++++------------
> > >>  arch/arm64/kvm/mmu.c                  | 16 ++++---
> > >>  8 files changed, 71 insertions(+), 61 deletions(-)
> > >>
> > >> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > >> index 78a550537b67..13fd9dbf2d1d 100644
> > >> --- a/arch/arm64/include/asm/kvm_emulate.h
> > >> +++ b/arch/arm64/include/asm/kvm_emulate.h
> > >> @@ -409,7 +409,7 @@ static __always_inline u8 kvm_vcpu_trap_get_fault_type(const struct kvm_vcpu *vc
> > >>         return kvm_vcpu_get_esr(vcpu) & ESR_ELx_FSC_TYPE;
> > >>  }
> > >>
> > >> -static __always_inline u8 kvm_vcpu_trap_get_fault_level(const struct kvm_vcpu *vcpu)
> > >> +static __always_inline s8 kvm_vcpu_trap_get_fault_level(const struct kvm_vcpu *vcpu)
> > >>  {
> > >>         return kvm_vcpu_get_esr(vcpu) & ESR_ELx_FSC_LEVEL;
> > >>  }
> > >
> > > This helper is currently only used for permission faults, which don't
> > > exist at level -1. Also, there is only a single caller of this helper,
> > > which uses the result only to infer the size covered by the block
> > > entry that describes the mapping.
> > >
> > > So in my LPA2 series, I intend to remove this helper altogether, and
> > > just replace it with something along the lines of
> > >
> > > static inline
> > > u64 kvm_vcpu_trap_get_perm_fault_granule(const struct kvm_vcpu *vcpu)
> > > {
> > >     unsigned long esr = kvm_vcpu_get_esr(vcpu);
> > >
> > >     BUG_ON(!esr_is_permission_fault(esr));
> > >     return BIT(ARM64_HW_PGTABLE_LEVEL_SHIFT(esr & ESR_ELx_FSC_LEVEL));
> > > }
> > >
> > > to avoid having to reason about whether masking with ESR_ELx_FSC_LEVEL
> > > is appropriate for the fault type in question.
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/commit/?h=arm64-lpa2-v6-combined&id=26c3425ec73ca751c45848f6f3f2d96e02cb4327
> > > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/commit/?h=arm64-lpa2-v6-combined&id=d6a849d6b318e70bf2f80f9b18a933136520019a
> > >
> >
> > This would take me an afternoon to get educated enough to even be able to offer
> > an opinon. So I'll leave this to the bigger boys to discuss. :)
>
> Dunno who the big boys are (I'm rather small, myself).
>
> Looking at the first patch, I rather like that cleanup. It makes it
> clear (cue the fault_granule handling for permission fault) that we
> should consider splitting user_mem_abort() into two functions: one
> that deals with translation faults, and one that is solely concerned
> with permissions faults.
>
> Ard, if you want to split the KVM stuff from the core arch code in
> that patch and post the result, I'd be happy to take it for a ride in
> -next.
>

Sure. The only core arch code that it touches is mm/fault.c, beyond
adding the esr_is_*_fault() helpers, which the KVM code depends on.

So i'll just keep those in asm/esr.h, unless you prefer to add them to
a kvm/arm include now and move them later?