Patchwork [4/4] tcg-hppa: Compute is_write in cpu_signal_handler.

login
register
mail settings
Submitter Richard Henderson
Date March 12, 2010, 2:58 p.m.
Message ID <14f1f6f787a15f6d6a5c3ce7f49a2e682aa339e7.1268754659.git.rth@twiddle.net>
Download mbox | patch
Permalink /patch/47867/
State New
Headers show

Comments

Richard Henderson - March 12, 2010, 2:58 p.m.
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 cpu-exec.c |   38 +++++++++++++++++++++++++++++++-------
 1 files changed, 31 insertions(+), 7 deletions(-)
Stuart Brady - March 17, 2010, 2:10 a.m.
On Fri, Mar 12, 2010 at 03:58:08PM +0100, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <rth@twiddle.net>

Acked-by: Stuart Brady <stuart.brady@gmail.com>

Argh.  It just seems mind bogglingly silly that is_write doesn't seem to
be included in the siginfo on archs where doing so would make sense...

It's at least something I'd have hoped libc could take care of in the case
where the hardware doesn't indicate whether the fault was due to a read
or a write...

(Right up there with the use of our own caching flushing code instead of
calls to mprotect(), etc. and the rather dubious use of qemu-lock.h...)

> ---
>  cpu-exec.c |   38 +++++++++++++++++++++++++++++++-------
>  1 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index bcfcda2..14204f4 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -1193,15 +1193,39 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>  {
>      struct siginfo *info = pinfo;
>      struct ucontext *uc = puc;
> -    unsigned long pc;
> -    int is_write;
> +    unsigned long pc = uc->uc_mcontext.sc_iaoq[0];
> +    uint32_t insn = *(uint32_t *)pc;
> +    int is_write = 0;
> +
> +    /* XXX: need kernel patch to get write flag faster.  */
> +    switch (insn >> 26) {
> +    case 0x1a: /* STW */
> +    case 0x19: /* STH */
> +    case 0x18: /* STB */
> +    case 0x1b: /* STWM */
> +        is_write = 1;
> +        break;
> +
> +    case 0x09: /* CSTWX, FSTWX, FSTWS */
> +    case 0x0b: /* CSTDX, FSTDX, FSTDS */
> +        /* Distinguish from coprocessor load ... */
> +        is_write = (insn >> 9) & 1;
> +        break;
> +
> +    case 0x03:
> +        switch ((insn >> 6) & 15) {
> +        case 0xa: /* STWS */
> +        case 0x9: /* STHS */
> +        case 0x8: /* STBS */
> +        case 0xe: /* STWAS */
> +        case 0xc: /* STBYS */
> +            is_write = 1;
> +        }
> +        break;
> +    }
>  
> -    pc = uc->uc_mcontext.sc_iaoq[0];
> -    /* FIXME: compute is_write */
> -    is_write = 0;
>      return handle_cpu_signal(pc, (unsigned long)info->si_addr, 
> -                             is_write,
> -                             &uc->uc_sigmask, puc);
> +                             is_write, &uc->uc_sigmask, puc);
>  }
>  
>  #else
> -- 
> 1.6.6.1
> 
>
Stuart Brady - March 17, 2010, 2:23 a.m.
On Wed, Mar 17, 2010 at 02:10:43AM +0000, Stuart Brady wrote:
> Argh.  It just seems mind bogglingly silly that is_write doesn't seem to
> be included in the siginfo on archs where doing so would make sense...

> It's at least something I'd have hoped libc could take care of in the case
> where the hardware doesn't indicate whether the fault was due to a read
> or a write...

Oh, actually, I guess you'd just test the equivalent of
'(info->si_isr >> 33) & 1' or 'DSISR_sig(uc) & 0x00800000)' on such
an arch, but it still seems a shame that this isn't done for us on
archs without that ability...

Cheers,
Richard Henderson - March 17, 2010, 2:49 p.m.
On 03/16/2010 07:23 PM, Stuart Brady wrote:
> On Wed, Mar 17, 2010 at 02:10:43AM +0000, Stuart Brady wrote:
>> Argh.  It just seems mind bogglingly silly that is_write doesn't seem to
>> be included in the siginfo on archs where doing so would make sense...
> 
>> It's at least something I'd have hoped libc could take care of in the case
>> where the hardware doesn't indicate whether the fault was due to a read
>> or a write...
> 
> Oh, actually, I guess you'd just test the equivalent of
> '(info->si_isr >> 33) & 1' or 'DSISR_sig(uc) & 0x00800000)' on such
> an arch, but it still seems a shame that this isn't done for us on
> archs without that ability...

Unfortunately there's no reserved space in the hppa sigcontext, which
is what would need to be expanded in order to store the additional info
from the kernel.  So in addition to a kernel patch, userland would need
to change its abi.  All kinda nasty, that.


r~

Patch

diff --git a/cpu-exec.c b/cpu-exec.c
index bcfcda2..14204f4 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -1193,15 +1193,39 @@  int cpu_signal_handler(int host_signum, void *pinfo,
 {
     struct siginfo *info = pinfo;
     struct ucontext *uc = puc;
-    unsigned long pc;
-    int is_write;
+    unsigned long pc = uc->uc_mcontext.sc_iaoq[0];
+    uint32_t insn = *(uint32_t *)pc;
+    int is_write = 0;
+
+    /* XXX: need kernel patch to get write flag faster.  */
+    switch (insn >> 26) {
+    case 0x1a: /* STW */
+    case 0x19: /* STH */
+    case 0x18: /* STB */
+    case 0x1b: /* STWM */
+        is_write = 1;
+        break;
+
+    case 0x09: /* CSTWX, FSTWX, FSTWS */
+    case 0x0b: /* CSTDX, FSTDX, FSTDS */
+        /* Distinguish from coprocessor load ... */
+        is_write = (insn >> 9) & 1;
+        break;
+
+    case 0x03:
+        switch ((insn >> 6) & 15) {
+        case 0xa: /* STWS */
+        case 0x9: /* STHS */
+        case 0x8: /* STBS */
+        case 0xe: /* STWAS */
+        case 0xc: /* STBYS */
+            is_write = 1;
+        }
+        break;
+    }
 
-    pc = uc->uc_mcontext.sc_iaoq[0];
-    /* FIXME: compute is_write */
-    is_write = 0;
     return handle_cpu_signal(pc, (unsigned long)info->si_addr, 
-                             is_write,
-                             &uc->uc_sigmask, puc);
+                             is_write, &uc->uc_sigmask, puc);
 }
 
 #else