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@xxxxxxxxxx> 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@xxxxxxxxxx> 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
