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

[Julien Grall <julien@xxxxxxx>]

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.
> >
> > Cheers,
> 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?

Cheers,

--
Julien Grall