Re: [PATCH 1/3] KVM: selftests: Add exception handling support for aarch64

[Marc Zyngier <maz@kernel.org>]

AOn Thu, 29 Apr 2021 18:51:59 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Fri, Apr 23, 2021 at 09:58:24AM +0100, Marc Zyngier wrote:
> > Hi Ricardo,
> > 
> > Thanks for starting this.
> > 
> > On Fri, 23 Apr 2021 05:03:49 +0100,
> > Ricardo Koller <ricarkol@google.com> wrote:
> > > +.pushsection ".entry.text", "ax"
> > > +.balign 0x800
> > > +.global vectors
> > > +vectors:
> > > +.popsection
> > > +
> > > +/*
> > > + * Build an exception handler for vector and append a jump to it into
> > > + * vectors (while making sure that it's 0x80 aligned).
> > > + */
> > > +.macro HANDLER, el, label, vector
> > > +handler\()\vector:
> > > +	save_registers \el
> > > +	mov	x0, sp
> > > +	mov	x1, \vector
> > > +	bl	route_exception
> > > +	restore_registers \el
> > > +
> > > +.pushsection ".entry.text", "ax"
> > > +.balign 0x80
> > > +	b	handler\()\vector
> > > +.popsection
> > > +.endm
> > 
> > That's an interesting construct, wildly different from what we are
> > using elsewhere in the kernel, but hey, I like change ;-). It'd be
> > good to add a comment to spell out that anything that emits into
> > .entry.text between the declaration of 'vectors' and the end of this
> > file will break everything.
> > 
> > > +
> > > +.global ex_handler_code
> > > +ex_handler_code:
> > > +	HANDLER	1, sync, 0			// Synchronous EL1t
> > > +	HANDLER	1, irq, 1			// IRQ EL1t
> > > +	HANDLER	1, fiq, 2			// FIQ EL1t
> > > +	HANDLER	1, error, 3			// Error EL1t
> > 
> > Can any of these actually happen? As far as I can see, the whole
> > selftest environment seems to be designed around EL1h.
> >
> 
> They can happen. KVM defaults to use EL1h:

That's not a KVM decision. That's an architectural requirement. Reset
is an exception, exception use the handler mode.

> 
> 	#define VCPU_RESET_PSTATE_EL1   (PSR_MODE_EL1h | PSR_A_BIT | PSR_I_BIT | \
> 
> but then a guest can set the SPSel to 0:
> 
> 	asm volatile("msr spsel, #0");
> 
> and this happens:
> 
> 	  Unexpected exception guest (vector:0x0, ec:0x25)
> 
> I think it should still be a valid situation: some test might want to
> try it.

Sure, but that's not what this test (in patch #2) is doing, is it?
If, as I believe, this is an unexpected situation, why not handle it
separately? I'm not advocating one way or another, but it'd be good to
understand the actual scope of the exception handling in this
infrastructure.

If you plan to allow tests to run in the EL1t environment, where do
you decide to switch back to EL1t after taking the exception in EL1h?
Are the tests supposed to implement both stack layouts?

Overall, I'm worried that nobody is going to use this layout *unless*
it becomes mandated.

Thanks,

	M.

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