On Thu, Apr 28, 2016 at 10:51:28AM +0200, Aurelien Jarno wrote:
> On 2016-04-27 23:21, James Hogan wrote:
> > Commit b00c72180c36 ("target-mips: add PC, XNP reg numbers to RDHWR")
> > changed the rdhwr helpers to use check_hwrena() to check the register
> > being accessed is enabled in CP0_HWREna when used from user mode. If
> > that check fails an EXCP_RI exception is raised at the host PC
> > calculated with GETPC().
> >
> > However check_hwrena() may not be fully inlined as the
> > do_raise_exception() part of it is common regardless of the arguments.
> > This causes GETPC() to calculate the address in the call in the helper
> > instead of the generated code calling the helper. No TB will be found
> > and the EPC reported with the resulting guest RI exception points to the
> > beginning of the TB instead of the RDHWR instruction.
> >
> > We can't reliably force check_hwrena() to be inlined, and converting it
> > to a macro would be ugly, so instead pass the host PC in as an argument,
> > with each rdhwr helper passing GETPC(). This should avoid any dependence
> > on compiler behaviour, and in practice seems to prevent the partial
> > inlining of check_hwrena() on x86_64.
> >
> > This issue causes failures when running a MIPS KVM (trap & emulate)
> > guest in a MIPS QEMU TCG guest, as the inner guest kernel will do a
> > RDHWR of counter, which is disabled in the outer guest's CP0_HWREna by
> > KVM so it can emulate the inner guest's counter. The emulation fails and
> > the RI exception is passed to the inner guest.
> >
> > Fixes: b00c72180c36 ("target-mips: add PC, XNP reg numbers to RDHWR")
> > Signed-off-by: James Hogan <james.ho...@imgtec.com>
> > Cc: Leon Alrae <leon.al...@imgtec.com>
> > Cc: Yongbok Kim <yongbok....@imgtec.com>
> > Cc: Aurelien Jarno <aurel...@aurel32.net>
> > ---
> > target-mips/op_helper.c | 16 ++++++++--------
> > 1 file changed, 8 insertions(+), 8 deletions(-)
>
> Thanks for the detailed analysis. The other solution would have been to
> declare the function as __attribute__((__always_inline__)), but I think
> your solution is even better.
Yeh, I did try this first but I got big fat warnings from GCC like this
one that probably rightly scared me off that approach:
CC mips64el-softmmu/target-mips/op_helper.o
target-mips/op_helper.c +2297 :48: error: always_inline function might not be
inlinable [-Werror=attributes]
static __attribute__((__always_inline__)) void check_hwrena(CPUMIPSState *env,
int reg)
^
>
> Reviewed-by: Aurelien Jarno <aurel...@aurel32.net>
Thanks!
Cheers
James
signature.asc
Description: Digital signature
