Re: [Xen-devel] [PATCH v3 4/4] xen/arm: Don't crash the domain on invalid HVC immediate

[Julien Grall <julien.grall@xxxxxxx>]

On 02/02/18 14:37, Andre Przywara wrote:
> Hi,
Hi,

> On 02/02/18 10:14, Julien Grall wrote:
> > domain_crash_synchronous() should only be used when something went wrong
> > in Xen. It is better to inject to the guest as it will be in a better
> > position to provide helpful information (stack trace...).
> >
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >
> > ---
> >
> >      We potentially want to return -1 instead. This would make Xen more
> >      future-proof if we decide to implement the other HVC immediate.
> >
> >      Changes in v2:
> >          - Add Stefano's reviewed-by
> > ---
> >   xen/arch/arm/traps.c | 13 ++++++++-----
> >   1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 1e85f99ec1..1cba7e584d 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1471,14 +1471,17 @@ static void do_debug_trap(struct cpu_user_regs *regs, 
> > unsigned int code)
> >   #endif
> >   
> >   static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
> > -                              unsigned long iss)
> > +                              const union hsr hsr)
> >   {
> >       arm_hypercall_fn_t call = NULL;
> >   
> >       BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) );
> >   
> > -    if ( iss != XEN_HYPERCALL_TAG )
> > -        domain_crash_synchronous();
> > +    if ( hsr.iss != XEN_HYPERCALL_TAG )
> > +    {
> > +        gprintk(XENLOG_WARNING, "Invalid HVC imm 0x%x\n", hsr.iss);
> > +        return inject_undef_exception(regs, hsr);
> Why is this UNDEF?
> UNDEF is the exception used when either HVC is disabled or if it's
> called from EL0.
> I couldn't find anything that talks about how non-implemented immediates
> should be handled, I guess they would just be ignored?
> Do you have any sources that recommend UNDEF?
Per the SMCCC, all nonzero immediate for HVC are designated for use by 
the hypervisors. So I take as we can implement the way we want.
We have few solutions here:
	1) Ignore. This means that a guest may wrongly call an HVC and will 
never notice it until implemented. This would lead to some trouble with 
introduce new ABI.
	2) Return a value. We could return -1 (or -ENOSYS). But that would need 
to be defined.
        3) Undefined. The guest should not be allow to touch it, so it makes 
sense.

1# is not a solution because catching them wrong usage on old Xen would 
be a nightmare.
2# might be doable, thought I am not sure we want to commit on a return 
value now. But old Xen would see crash the domain.
So I choose #3 which still crash the domain but by injecting an undefined.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel