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
