[Qemu-devel] qemu-system-ppc problem with PVR access from user space

[Qemu-devel] qemu-system-ppc problem with PVR access from user space

[Jason Wessel]

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

The typical kernel + user space I boot on the prep machine no longer
boots due to an issue accessing the PVR special purpose register.  When
the PVR is accessed from user space, it should generate an exception
with the PC set to the instruction that it occurred at when it saves to
the stack.  In the latest CVS, it is off by 4 bytes.  With out the fix
/sbin/init gets killed because the kernel's trap handler which does the
userspace emulation of the instruction does not clean up the trap.

I am using the attached patch to work around the problem, but I wonder
if there is a more generic problem that was introduced as a regression
with all ppc merges in the last month or so, given this used to work
fine through the generic handler.

Any insight into this would certainly be useful.

Thanks,
Jason.

[-- Attachment #2: ppc_pvr_access_from_user_space.patch --]
[-- Type: text/x-patch, Size: 1125 bytes --]


Work around the problem that the PC register is not saved with
the right address when taking a user space PVR access exception.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 target-ppc/translate_init.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -92,6 +92,13 @@ static void spr_write_clear (void *opaqu
 }
 #endif
 
+static void spr_read_generic_fault_user(void *opaque, int sprn)
+{
+	DisasContext *ctx = opaque;
+	ctx->nip -= 4;
+	GEN_EXCP_PRIVREG(ctx);
+}
+
 /* SPR common to all PowerPC */
 /* XER */
 static void spr_read_xer (void *opaque, int sprn)
@@ -5942,7 +5949,7 @@ static void init_ppc_proc (CPUPPCState *
     /* Register SPR common to all PowerPC implementations */
     gen_spr_generic(env);
     spr_register(env, SPR_PVR, "PVR",
-                 SPR_NOACCESS, SPR_NOACCESS,
+                 &spr_read_generic_fault_user, SPR_NOACCESS,
                  &spr_read_generic, SPR_NOACCESS,
                  def->pvr);
     /* PowerPC implementation specific initialisations (SPRs, timers, ...) */

Re: [Qemu-devel] qemu-system-ppc problem with PVR access from user space

[J. Mayer]

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


On Fri, 2007-11-02 at 08:04 -0500, Jason Wessel wrote:
> The typical kernel + user space I boot on the prep machine no longer
> boots due to an issue accessing the PVR special purpose register.  When
> the PVR is accessed from user space, it should generate an exception
> with the PC set to the instruction that it occurred at when it saves to
> the stack.  In the latest CVS, it is off by 4 bytes.  With out the fix
> /sbin/init gets killed because the kernel's trap handler which does the
> userspace emulation of the instruction does not clean up the trap.
> 
> I am using the attached patch to work around the problem, but I wonder
> if there is a more generic problem that was introduced as a regression
> with all ppc merges in the last month or so, given this used to work
> fine through the generic handler.
> 
> Any insight into this would certainly be useful.

Seems like I made a mistake for program exception generation while
fixing floating-point ones, I'm sorry. Your patch is incorrect but the
one attached should fix the problem. Could you please check it in your
case ?

-- 
J. Mayer <l_indien@magic.fr>
Never organized

[-- Attachment #2: ppc_excp.diff --]
[-- Type: text/x-patch, Size: 1134 bytes --]

Index: target-ppc/helper.c
===================================================================
RCS file: /sources/qemu/qemu/target-ppc/helper.c,v
retrieving revision 1.85
diff -u -d -d -p -r1.85 helper.c
--- target-ppc/helper.c	28 Oct 2007 00:55:05 -0000	1.85
+++ target-ppc/helper.c	2 Nov 2007 13:35:52 -0000
@@ -2146,10 +2145,9 @@ static always_inline void powerpc_excp (
                 new_msr |= (target_ulong)1 << MSR_HV;
 #endif
             msr |= 0x00100000;
-            if (msr_fe0 != msr_fe1) {
-                msr |= 0x00010000;
-                goto store_current;
-            }
+            if (msr_fe0 == msr_fe1)
+                goto store_next;
+            msr |= 0x00010000;
             break;
         case POWERPC_EXCP_INVAL:
 #if defined (DEBUG_EXCEPTIONS)
@@ -2187,7 +2185,7 @@ static always_inline void powerpc_excp (
                       env->error_code);
             break;
         }
-        goto store_next;
+        goto store_current;
     case POWERPC_EXCP_FPU:       /* Floating-point unavailable exception     */
         new_msr &= ~((target_ulong)1 << MSR_RI);
 #if defined(TARGET_PPC64H)

Re: [Qemu-devel] qemu-system-ppc problem with PVR access from user space

[Jason Wessel]

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

J. Mayer wrote:
> On Fri, 2007-11-02 at 08:04 -0500, Jason Wessel wrote:
>   
>> The typical kernel + user space I boot on the prep machine no longer
>> boots due to an issue accessing the PVR special purpose register.  When
>> the PVR is accessed from user space, it should generate an exception
>> with the PC set to the instruction that it occurred at when it saves to
>> the stack.  In the latest CVS, it is off by 4 bytes.  With out the fix
>> /sbin/init gets killed because the kernel's trap handler which does the
>> userspace emulation of the instruction does not clean up the trap.
>>
>> I am using the attached patch to work around the problem, but I wonder
>> if there is a more generic problem that was introduced as a regression
>> with all ppc merges in the last month or so, given this used to work
>> fine through the generic handler.
>>
>> Any insight into this would certainly be useful.
>>     
>
> Seems like I made a mistake for program exception generation while
> fixing floating-point ones, I'm sorry. Your patch is incorrect but the
> one attached should fix the problem. Could you please check it in your
> case ?
>   

That worked quite well.  Now my patch is back to normal.  I use the
attached patch to silence the warning about the privileged access else
it prints every time the glibc processor feature check is used.  The
only difference of course from the last one is that the PC no longer
needs to be adjusted, much like before.

The other option would be for you to remove the printf of the debug
information?  Perhaps that was something accidentally left behind?

Thanks,
Jason.

[-- Attachment #2: ppc_pvr_access_from_user_space.patch --]
[-- Type: text/x-patch, Size: 1113 bytes --]


Work around the problem that the PC register is not saved with
the right address when taking a user space PVR access exception.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 target-ppc/translate_init.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -92,6 +92,12 @@ static void spr_write_clear (void *opaqu
 }
 #endif
 
+static void spr_read_generic_fault_user(void *opaque, int sprn)
+{
+    DisasContext *ctx = opaque;
+    GEN_EXCP_PRIVREG(ctx);
+}
+
 /* SPR common to all PowerPC */
 /* XER */
 static void spr_read_xer (void *opaque, int sprn)
@@ -5942,7 +5948,7 @@ static void init_ppc_proc (CPUPPCState *
     /* Register SPR common to all PowerPC implementations */
     gen_spr_generic(env);
     spr_register(env, SPR_PVR, "PVR",
-                 SPR_NOACCESS, SPR_NOACCESS,
+                 &spr_read_generic_fault_user, SPR_NOACCESS,
                  &spr_read_generic, SPR_NOACCESS,
                  def->pvr);
     /* PowerPC implementation specific initialisations (SPRs, timers, ...) */

Re: [Qemu-devel] qemu-system-ppc problem with PVR access from user space

[Jocelyn Mayer]

On Fri, 2007-11-02 at 08:57 -0500, Jason Wessel wrote:
> J. Mayer wrote:
> > On Fri, 2007-11-02 at 08:04 -0500, Jason Wessel wrote:
> >   
> >> The typical kernel + user space I boot on the prep machine no longer
> >> boots due to an issue accessing the PVR special purpose register.  When
> >> the PVR is accessed from user space, it should generate an exception
> >> with the PC set to the instruction that it occurred at when it saves to
> >> the stack.  In the latest CVS, it is off by 4 bytes.  With out the fix
> >> /sbin/init gets killed because the kernel's trap handler which does the
> >> userspace emulation of the instruction does not clean up the trap.
> >>
> >> I am using the attached patch to work around the problem, but I wonder
> >> if there is a more generic problem that was introduced as a regression
> >> with all ppc merges in the last month or so, given this used to work
> >> fine through the generic handler.
> >>
> >> Any insight into this would certainly be useful.
> >>     
> >
> > Seems like I made a mistake for program exception generation while
> > fixing floating-point ones, I'm sorry. Your patch is incorrect but the
> > one attached should fix the problem. Could you please check it in your
> > case ?
> >   
> 
> That worked quite well.  Now my patch is back to normal.  I use the
> attached patch to silence the warning about the privileged access else
> it prints every time the glibc processor feature check is used.  The
> only difference of course from the last one is that the PC no longer
> needs to be adjusted, much like before.
> 
> The other option would be for you to remove the printf of the debug
> information?  Perhaps that was something accidentally left behind?

No, it's not accidental. An application accessing priviledged SPR,
including the PVR, is likely to be buggy. I checked in the kernel
(2.6.23), trapping the mfpvr instruction is a huge bug because it breaks
the virtualisation features of the PowerPC architecture. Application
like mol will suffer of this, not being able to pretend the virtualized
CPU is not the same as the host CPU. The PowerPC architecture has been
designed to be fully virtualisable but the vanilla Linux kernel breaks
this useful feature. The bug is then to be fixed in the kernel (and the
glibc if it really uses mfpvr).
The kernel may provide informations about the CPU features to the
userland (but this is absolutelly not PowerPC specific) but it's very
important that it never shows priviledge resources directly to user
programs, or the model is broken and virtualisation application won't be
able to run properly anymore.

-- 
Jocelyn Mayer <l_indien@magic.fr>

Re: [Qemu-devel] qemu-system-ppc problem with PVR access from user space

[Daniel Jacobowitz]

On Fri, Nov 02, 2007 at 05:23:59PM +0100, Jocelyn Mayer wrote:
> No, it's not accidental. An application accessing priviledged SPR,
> including the PVR, is likely to be buggy. I checked in the kernel
> (2.6.23), trapping the mfpvr instruction is a huge bug because it breaks
> the virtualisation features of the PowerPC architecture. Application
> like mol will suffer of this, not being able to pretend the virtualized
> CPU is not the same as the host CPU. The PowerPC architecture has been
> designed to be fully virtualisable but the vanilla Linux kernel breaks
> this useful feature. The bug is then to be fixed in the kernel (and the
> glibc if it really uses mfpvr).

I suggest you take this up with the PowerPC kernel maintainers, which
might work, instead of making QEMU noisy about it; the people using
QEMU don't care, and they'll just disable the warning.  It wasn't
an accidental decision on the kernel maintainers' part either.

I don't see the PVR read in current glibc, but I thought it was there;
I don't remember exactly what happened.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [Qemu-devel] qemu-system-ppc problem with PVR access from user space

[J. Mayer]

On Fri, 2007-11-02 at 16:46 -0400, Daniel Jacobowitz wrote:
> On Fri, Nov 02, 2007 at 05:23:59PM +0100, Jocelyn Mayer wrote:
> > No, it's not accidental. An application accessing priviledged SPR,
> > including the PVR, is likely to be buggy. I checked in the kernel
> > (2.6.23), trapping the mfpvr instruction is a huge bug because it breaks
> > the virtualisation features of the PowerPC architecture. Application
> > like mol will suffer of this, not being able to pretend the virtualized
> > CPU is not the same as the host CPU. The PowerPC architecture has been
> > designed to be fully virtualisable but the vanilla Linux kernel breaks
> > this useful feature. The bug is then to be fixed in the kernel (and the
> > glibc if it really uses mfpvr).
> 
> I suggest you take this up with the PowerPC kernel maintainers, which
> might work, instead of making QEMU noisy about it; the people using
> QEMU don't care, and they'll just disable the warning.  It wasn't
> an accidental decision on the kernel maintainers' part either.

You're absolutely right, it's a kernel problem: it would prevent any
attempt to enable a kqemu-like feature for the PowerPC, for example. And
it seems this behavior has been in the Linux kernel for a very long
time...
I will disable the warning in the PVR specific case, but this is ugly as
it will prevent detection of bugged PVR accesses when using OSes that
respect the PowerPC specifications.

> I don't see the PVR read in current glibc, but I thought it was there;
> I don't remember exactly what happened.

One thing is sure: any application which uses mfpvr is bugged. I guess
there might be some libraries that would like to do it to enable some
optimisations at run-time. Or applications like mplayer... But I don't
see why init should ever have any usage of knowing the CPU features...

-- 
J. Mayer <l_indien@magic.fr>
Never organized