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

[Ricardo Koller <ricarkol@google.com>]

On Thu, Apr 29, 2021 at 08:59:14PM +0100, Marc Zyngier wrote:
> 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.
> 

That makes sense, thanks for the clarification.

> > 
> > 	#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.

Got it, I see your point. Yes, I'm definitely not planning to use it.
Will just treat those vectors as "invalid".

Thanks again,
Ricardo