Re: [PATCH v3 14/18] KVM: arm64: nv: Add SW walker for AT S1 emulation

[Marc Zyngier <maz@kernel.org>]

On Fri, 16 Aug 2024 12:02:37 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> On Fri, Aug 16, 2024 at 11:37:24AM +0100, Marc Zyngier wrote:
> > Hi Alex,
> > 
> > On Fri, 16 Aug 2024 10:22:43 +0100,
> > Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> > > 
> > > Hi Marc,
> > > 
> > > On Thu, Aug 15, 2024 at 07:28:41PM +0100, Marc Zyngier wrote:
> > > > 
> > > > Hi Alex,
> > > > 
> > > > On Thu, 15 Aug 2024 17:44:02 +0100,
> > > > Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> > 
> > [...]
> > 
> > > > > > +static bool par_check_s1_perm_fault(u64 par)
> > > > > > +{
> > > > > > +	u8 fst = FIELD_GET(SYS_PAR_EL1_FST, par);
> > > > > > +
> > > > > > +	return  ((fst & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_PERM &&
> > > > > > +		 !(par & SYS_PAR_EL1_S));
> > > > > 
> > > > > ESR_ELx_FSC_PERM = 0x0c is a permission fault, level 0, which Arm ARM says can
> > > > > only happen when FEAT_LPA2. I think the code should check that the value for
> > > > > PAR_EL1.FST is in the interval (ESR_ELx_FSC_PERM_L(0), ESR_ELx_FSC_PERM_L(3)].
> > > > 
> > > > I honestly don't want to second-guess the HW. If it reports something
> > > > that is the wrong level, why should we trust the FSC at all?
> > > 
> > > Sorry, I should have been clearer.
> > > 
> > > It's not about the hardware reporting a fault on level 0 of the translation
> > > tables, it's about the function returning false if the hardware reports a
> > > permission fault on levels 1, 2 or 3 of the translation tables.
> > > 
> > > For example, on a permssion fault on level 3, PAR_EL1. FST = 0b001111 = 0x0F,
> > > which means that the condition:
> > > 
> > > (fst & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_PERM (which is 0x0C) is false and KVM
> > > will fall back to the software walker.
> > > 
> > > Does that make sense to you?
> > 
> > I'm afraid I still don't get it.
> > 
> > From the kernel source:
> > 
> > #define ESR_ELx_FSC_TYPE	(0x3C)
> > 
> > This is a mask covering all fault types.
> > 
> > #define ESR_ELx_FSC_PERM	(0x0C)
> > 
> > This is the value for a permission fault, not encoding a level.
> > 
> > Taking your example:
> > 
> > (fst & ESR_ELx_FSC_TYPE) == (0x0F & 0x3C) == 0x0C == ESR_ELx_FSC_PERM
> > 
> > As I read it, the condition is true, as it catches a permission fault
> > on any level between 0 and 3.
> > 
> > You're obviously seeing something I don't, and I'm starting to
> > question my own sanity...
> 
> No, no, sorry for leading you on a wild goose chase, I read 0x3F for
> ESR_ELx_FSC_TYPE, which the value for the variable directly above it, instead of
> 0x3C :(
> 
> My bad, the code is correct!

Ah, glad we agree, I was starting to worry!

Thanks,

	M.

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