[Qemu-devel] [PATCH] target-mips: Fix RDHWR exception host PC

[Qemu-devel] [PATCH] target-mips: Fix RDHWR exception host PC

[James Hogan]

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.hogan@imgtec.com>
Cc: Leon Alrae <leon.alrae@imgtec.com>
Cc: Yongbok Kim <yongbok.kim@imgtec.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
---
 target-mips/op_helper.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index 8ec1bef7d034..4417e6ba225f 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -2294,29 +2294,29 @@ void helper_deret(CPUMIPSState *env)
 }
 #endif /* !CONFIG_USER_ONLY */
 
-static inline void check_hwrena(CPUMIPSState *env, int reg)
+static inline void check_hwrena(CPUMIPSState *env, int reg, uintptr_t pc)
 {
     if ((env->hflags & MIPS_HFLAG_CP0) || (env->CP0_HWREna & (1 << reg))) {
         return;
     }
-    do_raise_exception(env, EXCP_RI, GETPC());
+    do_raise_exception(env, EXCP_RI, pc);
 }
 
 target_ulong helper_rdhwr_cpunum(CPUMIPSState *env)
 {
-    check_hwrena(env, 0);
+    check_hwrena(env, 0, GETPC());
     return env->CP0_EBase & 0x3ff;
 }
 
 target_ulong helper_rdhwr_synci_step(CPUMIPSState *env)
 {
-    check_hwrena(env, 1);
+    check_hwrena(env, 1, GETPC());
     return env->SYNCI_Step;
 }
 
 target_ulong helper_rdhwr_cc(CPUMIPSState *env)
 {
-    check_hwrena(env, 2);
+    check_hwrena(env, 2, GETPC());
 #ifdef CONFIG_USER_ONLY
     return env->CP0_Count;
 #else
@@ -2326,19 +2326,19 @@ target_ulong helper_rdhwr_cc(CPUMIPSState *env)
 
 target_ulong helper_rdhwr_ccres(CPUMIPSState *env)
 {
-    check_hwrena(env, 3);
+    check_hwrena(env, 3, GETPC());
     return env->CCRes;
 }
 
 target_ulong helper_rdhwr_performance(CPUMIPSState *env)
 {
-    check_hwrena(env, 4);
+    check_hwrena(env, 4, GETPC());
     return env->CP0_Performance0;
 }
 
 target_ulong helper_rdhwr_xnp(CPUMIPSState *env)
 {
-    check_hwrena(env, 5);
+    check_hwrena(env, 5, GETPC());
     return (env->CP0_Config5 >> CP0C5_XNP) & 1;
 }
 
-- 
2.4.10

Re: [Qemu-devel] [PATCH] target-mips: Fix RDHWR exception host PC

[Aurelien Jarno]

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.hogan@imgtec.com>
> Cc: Leon Alrae <leon.alrae@imgtec.com>
> Cc: Yongbok Kim <yongbok.kim@imgtec.com>
> Cc: Aurelien Jarno <aurelien@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.

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] target-mips: Fix RDHWR exception host PC

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 2672 bytes --]

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.hogan@imgtec.com>
> > Cc: Leon Alrae <leon.alrae@imgtec.com>
> > Cc: Yongbok Kim <yongbok.kim@imgtec.com>
> > Cc: Aurelien Jarno <aurelien@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 <aurelien@aurel32.net>

Thanks!

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] target-mips: Fix RDHWR exception host PC

[Leon Alrae]

On 27/04/16 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.hogan@imgtec.com>
> Cc: Leon Alrae <leon.alrae@imgtec.com>
> Cc: Yongbok Kim <yongbok.kim@imgtec.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  target-mips/op_helper.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Whoops, thanks for the fix. I'll send the pullreq soon, hopefully it's
not too late for 2.6.

Thanks,
Leon

Re: [Qemu-devel] [PATCH] target-mips: Fix RDHWR exception host PC

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 2424 bytes --]

On Thu, Apr 28, 2016 at 09:53:04AM +0100, Leon Alrae wrote:
> On 27/04/16 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.

Note also this sentence isn't actually very clear. preventing partial
inlining sounds like it stops inlining altogether, whereas actually I
meant that it seems to ensure full inlining of check_hwrena(). Perhaps
you could change it to ", and in practice seems to ensure the full
inlining of check_hwrena() on x86_64." when you apply it.

> > 
> > 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.hogan@imgtec.com>
> > Cc: Leon Alrae <leon.alrae@imgtec.com>
> > Cc: Yongbok Kim <yongbok.kim@imgtec.com>
> > Cc: Aurelien Jarno <aurelien@aurel32.net>
> > ---
> >  target-mips/op_helper.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> Whoops, thanks for the fix. I'll send the pullreq soon, hopefully it's
> not too late for 2.6.

Thanks

James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]