Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 14 Dec 2023, Julien Grall wrote:
> Hi,
> 
> On 13/12/2023 14:02, Nicola Vetrini wrote:
> > On 2023-12-12 16:49, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 11/12/2023 12:32, Julien Grall wrote:
> > > > Hi,
> > > > 
> > > > On 11/12/2023 10:30, Nicola Vetrini wrote:
> > > > > The branches of the switch after a call to 'do_unexpected_trap'
> > > > > cannot return, but there is one path that may return, hence
> > > > > only some clauses are marked with ASSERT_UNREACHABLE().
> > > > I don't understand why this is necessary. The code should never be
> > > > reachable because do_unexpected_trap() is a noreturn().
> > >
> > > From the matrix discussion, it wasn't clear what was my position on this
> > > patch.
> > > 
> > > I would much prefer if the breaks are kept. I could accept:
> > > 
> > > ASSERT_UNREACHABLE();
> > > break;
> > > 
> > > But this solution is a Nack because if you are concerned about functions
> > > like do_unexpected_trap() to return by mistaken, then it needs to also be
> > > safe in production.
> > > 
> > > The current proposal is not safe.

I re-read the email thread. I also do not think that this is useful:

         do_unexpected_trap("SVE trap at EL2", regs);
-        break;
+        ASSERT_UNREACHABLE();

I also do not think that we should be concerned about functions like
do_unexpected_trap() to return by mistaken.

That said, what is the problem from MISRA point of view that we are
trying to fix? Is the only problem the presence of break; after the call
to a noreturn function?

If that's not allowed, I would suggest to do this:


         do_unexpected_trap("SVE trap at EL2", regs);
-        break;
+        /* break; */


Or deviate "break" globally as it doesn't seem to be a safety risk in my
opinion. If nothing else, it should make the code a bit safer because in
case of mistakes in do_unexpected_trap, at least we would continue to
follow a more reasonable code path rather than blindly falling through
the next switch case by accident.


> > Ok. I wonder whether the should be applied here in vcpreg.c:
> > 
> > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> > index 39aeda9dab62..089d2f03eb5e 100644
> > --- a/xen/arch/arm/vcpreg.c
> > +++ b/xen/arch/arm/vcpreg.c
> > @@ -707,7 +707,8 @@ void do_cp10(struct cpu_user_regs *regs, const union hsr
> > hsr)
> >           inject_undef_exception(regs, hsr);
> >           return;
> >       }
> > -
> > +
> > +    ASSERT_UNREACHABLE();
> >       advance_pc(regs, hsr);
> >   }
> > 
> > the rationale being that, should the switch somehow fail to return, the
> > advance_pc would be called, rather than doing nothing.
> 
> To clarify, advance_pc(regs, hsr) would still be called in production build.
> So if you are concerned about advance_pc() been called, then adding an
> ASSERT_UNREACHABLE() is not going to help.
> 
> It took me a little while to confirm that none of the path effectively returns
> due to the macros (in hindsight, it wasn't a good idea of mine to introduce
> them).
> 
> Depending on what we are trying to solve there are 3 possible approach:
>   1. Leave advance_pc(). This means we could potentially
>      a. Advance the PC twice (if it was already called) and therefore skipping
> an instruction
>      b. Advance the PC once without an emulation
>   2. Remove advance_pc(). If we already called the function, then there is no
> problem. Otherwise, we would trap in a loop effectively rendering the guest
> vCPU unusable.
>   3. Replace with domain_crash()
> 
> Here it feels, that 3 is more suitable as this gives a clear indication
> why/where the emulation gone wrong.
> 
> This may still need to be accompanied with a ASSERT_UNREACHABLE() to please
> MISRA.
> 
> Bertrand, Michal, Stefano, what do you think?

Yes, I would go with 3., replace advance_pc with domain_crash. Assuming
that it would also solve the violation in ECLAIR.