[Qemu-devel] [Patch] target-ppc mtcrf instruction not recognized

[Qemu-devel] [Patch] target-ppc mtcrf instruction not recognized

[Pierre d'Herbemont]

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

Hi,

I have been playing with ppc-darwin-user. And I have to say that qemu- 
ppc is too strict with ppc opcode validity:
On Mac OS X binary release called Tiger (or 10.4), the __bzero  
function [1] contains a mtcrf, which has been translated to  
0x7c901120, by Apple's as. Current qemu rejects it, saying it is  
invalid. According to the IBM Specification the mtcrf instruction  
[2], contains reserved flags, for which the values are unknown. qemu  
assumes that those are set to 1, which is obviously wrong. The  
attached patch provides a fix.

The mtcrf case might not be the only case in which such a situation  
occurs. That would need a special attention, though being strict is  
better than letting pass.

Pierre.

[1] http://fxr.watson.org/fxr/source/osfmk/ppc/commpage/bzero_32.s? 
v=DARWIN8#L98
[2] http://publibn.boulder.ibm.com/doc_link/en_US/a_doc_lib/aixassem/ 
alangref/mtcrf.htm


[-- Attachment #2: qemu_mtcrf.diff.txt --]
[-- Type: text/plain, Size: 562 bytes --]

Index: target-ppc/translate.c
===================================================================
RCS file: /cvsroot/qemu/qemu/target-ppc/translate.c,v
retrieving revision 1.31
diff -u -r1.31 translate.c
--- target-ppc/translate.c	12 May 2005 18:46:11 -0000	1.31
+++ target-ppc/translate.c	14 May 2005 17:14:35 -0000
@@ -2123,7 +2123,7 @@
 }
 
 /* mtcrf */
-GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x00100801, PPC_MISC)
+GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x00000000, PPC_MISC)
 {
     gen_op_load_gpr_T0(rS(ctx->opcode));
     gen_op_store_cr(CRM(ctx->opcode));

Re: [Qemu-devel] [Patch] target-ppc mtcrf instruction not recognized

[J. Mayer]

On Sat, 2005-05-14 at 19:20 +0200, Pierre d'Herbemont wrote:
> Hi,
> 
> I have been playing with ppc-darwin-user. And I have to say that qemu- 
> ppc is too strict with ppc opcode validity:
> On Mac OS X binary release called Tiger (or 10.4), the __bzero  
> function [1] contains a mtcrf, which has been translated to  
> 0x7c901120, by Apple's as. Current qemu rejects it, saying it is  
> invalid. According to the IBM Specification the mtcrf instruction  
> [2], contains reserved flags, for which the values are unknown. qemu  
> assumes that those are set to 1, which is obviously wrong. The  
> attached patch provides a fix.

Qemu assume all reserved fields are set to zero, not 1.
PowerPC specifications says: (section 8.1 of PEM)
"Some instructions fields are reserved or must contain a predefined
value as shown in the individual instruction layouts. If a reserved
field does not have all bits cleared, or if a field that must contain a
particular value does not contains that value, the instruction form is
invalid ..."
In section 4.1.3.2:
"Invalid forms result when a bit or openrands is coded incorrectly, for
example, or when a reserved bit (shown as '0') is coded as '1'.
and
"an attempt to execute an invalid form of an instruction either invokes
the illegal instruction error handler (or program exception) or yields
boundedly-undefined results.

In the case of mtcrf, the PowerPC specification says the bits 11, 21 and
31 (IBM notation) _must_ be zero.
This is what is described in 32 bits PEM as well as 64 bits PEM
(including the latest revision dated on 31/03/2005) and the 740/750
PowerPC user manual (which is the one currently emulated by Qemu).

It would be acceptable to relax the check if it would make MacOS X 10.4
boot.
But in this case, only the bit 11 (which causes the problem here) should
be relaxed.
Then, the bit mask becomes 0x00000801 (not 0x00000000).


> [2]
http://publibn.boulder.ibm.com/doc_link/en_US/a_doc_lib/aixassem/alangref/mtcrf.htm

This is the language reference for POWER & PowerPC.
Some forms described in this documentation are valid only for POWER
CPUs.
Please use PowerPC Environment Manual for PowerPC only specifications.

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

Re: [Qemu-devel] [Patch] target-ppc mtcrf instruction not recognized

[Pierre d'Herbemont]

On 14 mai 05, at 20:15, J. Mayer wrote:

> Qemu assume all reserved fields are set to zero, not 1.

> PowerPC specifications says: (section 8.1 of PEM)
> "Some instructions fields are reserved or must contain a predefined
> value as shown in the individual instruction layouts. If a reserved
> field does not have all bits cleared, or if a field that must  
> contain a
> particular value does not contains that value, the instruction form is
> invalid ..."
> In section 4.1.3.2:
> "Invalid forms result when a bit or openrands is coded incorrectly,  
> for
> example, or when a reserved bit (shown as '0') is coded as '1'.
> and
> "an attempt to execute an invalid form of an instruction either  
> invokes
> the illegal instruction error handler (or program exception) or yields
> boundedly-undefined results.
>
> In the case of mtcrf, the PowerPC specification says the bits 11,  
> 21 and
> 31 (IBM notation) _must_ be zero.
> This is what is described in 32 bits PEM as well as 64 bits PEM
> (including the latest revision dated on 31/03/2005) and the 740/750
> PowerPC user manual (which is the one currently emulated by Qemu).
>
> It would be acceptable to relax the check if it would make MacOS X  
> 10.4
> boot.
> But in this case, only the bit 11 (which causes the problem here)  
> should
> be relaxed.
> Then, the bit mask becomes 0x00000801 (not 0x00000000).

> http://publibn.boulder.ibm.com/doc_link/en_US/a_doc_lib/aixassem/ 
> alangref/mtcrf.htm
>
> This is the language reference for POWER & PowerPC.
> Some forms described in this documentation are valid only for POWER
> CPUs.
> Please use PowerPC Environment Manual for PowerPC only specifications.
>

In fact I was referring to my old PowerPC Spec book in french from  
IBM (titled Architecture PowerPC), this link was the first google  
returned. After looking again in my book, I found what you said. I am  
confused, I wrote this a bit quickly. But the fact remains, would you  
send a correct patch for that? It'll definitely be compulsory in  
order to boot Mac OS X 10.4, and for darwin-user.

Thanks,

Pierre.

Re: [Qemu-devel] [Patch] target-ppc mtcrf instruction not recognized

[J. Mayer]

On Sat, 2005-05-14 at 21:03 +0200, Pierre d'Herbemont wrote:
> On 14 mai 05, at 20:15, J. Mayer wrote:
> 
> > Qemu assume all reserved fields are set to zero, not 1.
> 
> > PowerPC specifications says: (section 8.1 of PEM)
[...]
> > In the case of mtcrf, the PowerPC specification says the bits 11,  
> > 21 and
> > 31 (IBM notation) _must_ be zero.
> > This is what is described in 32 bits PEM as well as 64 bits PEM
> > (including the latest revision dated on 31/03/2005) and the 740/750
> > PowerPC user manual (which is the one currently emulated by Qemu).
> >
> > It would be acceptable to relax the check if it would make MacOS X  
> > 10.4 boot.
> > But in this case, only the bit 11 (which causes the problem here)  
> > should
> > be relaxed.
> > Then, the bit mask becomes 0x00000801 (not 0x00000000).
> 
[...]
> In fact I was referring to my old PowerPC Spec book in french from  
> IBM (titled Architecture PowerPC), this link was the first google  
> returned. After looking again in my book, I found what you said. I am  
> confused, I wrote this a bit quickly. But the fact remains, would you  
> send a correct patch for that? It'll definitely be compulsory in  
> order to boot Mac OS X 10.4, and for darwin-user.

OK, I did put this in my working repository and I'll submit this to
Fabrice.
Please try to do this change to check if other bits need to be relax or
not:

Index: target-ppc/translate.c
===================================================================
RCS file: /cvsroot/qemu/qemu/target-ppc/translate.c,v
retrieving revision 1.31
diff -u -r1.31 translate.c
--- target-ppc/translate.c      12 May 2005 18:46:11 -0000      1.31
+++ target-ppc/translate.c      14 May 2005 17:14:35 -0000
@@ -2123,7 +2123,7 @@
 }
 
 /* mtcrf */
-GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x00100801, PPC_MISC)
+GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x00000801, PPC_MISC)
 {
     gen_op_load_gpr_T0(rS(ctx->opcode));
     gen_op_store_cr(CRM(ctx->opcode));

And maybe someone should fill an Apple bug report to tell them their as
is buggy ;-)

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

Re: [Qemu-devel] [Patch] target-ppc mtcrf instruction not recognized

[Benjamin Herrenschmidt]

> OK, I did put this in my working repository and I'll submit this to
> Fabrice.
> Please try to do this change to check if other bits need to be relax or
> not:
> 
> Index: target-ppc/translate.c
> ===================================================================
> RCS file: /cvsroot/qemu/qemu/target-ppc/translate.c,v
> retrieving revision 1.31
> diff -u -r1.31 translate.c
> --- target-ppc/translate.c      12 May 2005 18:46:11 -0000      1.31
> +++ target-ppc/translate.c      14 May 2005 17:14:35 -0000
> @@ -2123,7 +2123,7 @@
>  }
>  
>  /* mtcrf */
> -GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x00100801, PPC_MISC)
> +GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x00000801, PPC_MISC)
>  {
>      gen_op_load_gpr_T0(rS(ctx->opcode));
>      gen_op_store_cr(CRM(ctx->opcode));
> 
> And maybe someone should fill an Apple bug report to tell them their as
> is buggy ;-)

It is definitely a bug in Apple assembler I would say, so I suggest you
add a comment in the above patch in order to avoid somebody later on
"fixing" your mask :)

Ben.
 

Re: [Qemu-devel] [Patch] target-ppc mtcrf instruction not recognized

[Pierre d'Herbemont]

On 16 mai 05, at 09:54, Benjamin Herrenschmidt wrote:

>
>> OK, I did put this in my working repository and I'll submit this to
>> Fabrice.
>> Please try to do this change to check if other bits need to be relax 
>> or
>> not:
>>
>> Index: target-ppc/translate.c
>> ===================================================================
>> RCS file: /cvsroot/qemu/qemu/target-ppc/translate.c,v
>> retrieving revision 1.31
>> diff -u -r1.31 translate.c
>> --- target-ppc/translate.c      12 May 2005 18:46:11 -0000      1.31
>> +++ target-ppc/translate.c      14 May 2005 17:14:35 -0000
>> @@ -2123,7 +2123,7 @@
>>  }
>>
>>  /* mtcrf */
>> -GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x00100801, PPC_MISC)
>> +GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x00000801, PPC_MISC)
>>  {
>>      gen_op_load_gpr_T0(rS(ctx->opcode));
>>      gen_op_store_cr(CRM(ctx->opcode));
>>

I've tested it: it works fine as expected.

>> And maybe someone should fill an Apple bug report to tell them their 
>> as
>> is buggy ;-)
>
> It is definitely a bug in Apple assembler I would say, so I suggest you
> add a comment in the above patch in order to avoid somebody later on
> "fixing" your mask :)

I thought it was, but it seems to be not so buggy ;)

I have had a look at the Apple's as source code of Darwin 8.0 (10.4) 
[1] and I have found an interesting comment :
	    /*
	     * If this is the mtcrf opcode (0x7c000120) and val is not zero and
	     * has exactly one bit set then use the new form of the mtcrf
	     * opcode.  This has bit 0x00100000 set and the FXM field is a bit
	     * mask. Else use the old form without bit 0x00100000 set.
               */

I don't know what they are refering to, but that may have been 
introduced by the PowerPC 970 or an other new PPC, since it was not 
present on the Mac OS X 10.2's as [2].

Pierre.

  [1] http://darwinsource.opendarwin.org/10.4/cctools-576/as/ppc.c
  [2] http://darwinsource.opendarwin.org/10.2/cctools-435/as/ppc.c

Re: [Qemu-devel] [Patch] target-ppc mtcrf instruction not recognized

[J. Mayer]

On Mon, 2005-05-16 at 17:54 +1000, Benjamin Herrenschmidt wrote:
> > OK, I did put this in my working repository and I'll submit this to
> > Fabrice.
> > Please try to do this change to check if other bits need to be relax or
> > not:
> > 
> > Index: target-ppc/translate.c
> > ===================================================================
> > RCS file: /cvsroot/qemu/qemu/target-ppc/translate.c,v
> > retrieving revision 1.31
> > diff -u -r1.31 translate.c
> > --- target-ppc/translate.c      12 May 2005 18:46:11 -0000      1.31
> > +++ target-ppc/translate.c      14 May 2005 17:14:35 -0000
> > @@ -2123,7 +2123,7 @@
> >  }
> >  
> >  /* mtcrf */
> > -GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x00100801, PPC_MISC)
> > +GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x00000801, PPC_MISC)
> >  {
> >      gen_op_load_gpr_T0(rS(ctx->opcode));
> >      gen_op_store_cr(CRM(ctx->opcode));
> > 
> > And maybe someone should fill an Apple bug report to tell them their as
> > is buggy ;-)
> 
> It is definitely a bug in Apple assembler I would say, so I suggest you
> add a comment in the above patch in order to avoid somebody later on
> "fixing" your mask :)

Piotr Krysiuk pointed that it's, in fact, a new form of the instruction.
A new PowerPC specification has been released in january,
adding mtocrf and mfocrf ('o' like optimized).
Those new forms only transfer one set of flags (then it's like the
previous form when only one bit is set in the control register). The
specification also say that "old" form is supported as a micro-coded
instruction.

Then, I guess new PowerPC do implement this new instruction. But Apple
is then supposed to emulate it when running on a PowerPC that does not
support it, I guess.

I will implement the new form, so it most crf transfers can be
optimized.

The latest PowerPC specification is to be found here:
<http://www6.software.ibm.com/software/developer/library/PPC_Vers202_Book1_public.pdf>
<http://www6.software.ibm.com/software/developer/library/PPC_Vers202_Book2_public.pdf>
<http://www6.software.ibm.com/software/developer/library/PPC_Vers202_Book3_public.pdf>

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

Re: [Qemu-devel] [Patch] target-ppc mtcrf instruction not recognized

[Herbert Poetzl]

On Tue, May 17, 2005 at 11:10:52PM +0200, J. Mayer wrote:
> 
> I will implement the new form, so it most crf transfers can be
> optimized.
> 
> The latest PowerPC specification is to be found here:
> <http://www6.software.ibm.com/software/developer/library/PPC_Vers202_Book1_public.pdf>
> <http://www6.software.ibm.com/software/developer/library/PPC_Vers202_Book2_public.pdf>
> <http://www6.software.ibm.com/software/developer/library/PPC_Vers202_Book3_public.pdf>

you sure about that?

best,
Herbert

> -- 
> J. Mayer <l_indien@magic.fr>
> Never organized
> 
> 
> 
> _______________________________________________
> Qemu-devel mailing list
> Qemu-devel@nongnu.org
> http://lists.nongnu.org/mailman/listinfo/qemu-devel

Re: [Qemu-devel] [Patch] target-ppc mtcrf instruction not recognized

[J. Mayer]

On Wed, 2005-05-18 at 11:06 +0200, Herbert Poetzl wrote:
> On Tue, May 17, 2005 at 11:10:52PM +0200, J. Mayer wrote:
> > 
> > I will implement the new form, so it most crf transfers can be
> > optimized.
> > 
> > The latest PowerPC specification is to be found here:
[...]

> you sure about that?

No, sorry: Here are the right links:
<ftp://www6.software.ibm.com/software/developer/library/PPC_Vers202_Book1_public.pdf>
<ftp://www6.software.ibm.com/software/developer/library/PPC_Vers202_Book2_public.pdf>
<ftp://www6.software.ibm.com/software/developer/library/PPC_Vers202_Book3_public.pdf>

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

Re: [Qemu-devel] [Patch] target-ppc mtcrf instruction not recognized

[J. Mayer]

On Mon, 2005-05-16 at 19:06 +0100, Piotras wrote:
> You may also want to take a look at mtocrf (with "o" for "one") in the document:
>   ftp://www6.software.ibm.com/software/developer/library/es-ppcbook1.zip
> 

All right, there is a new (optional) form of this instruction.
However, what is currently done in Qemu is perfectly valid:
"On some processor that comply with versions of the architecture that
precede Version 2.00 the new form may be treated as follows:
mtocrf: may cause the system illegal instruction error handler to be
invoked
mfocrf: may copy the content of an SPR, possibly a priviledged SPR, into
register RT"

As I doubt Apple want its OS not to run on G3 & G4, they must have
handled those case in their kernel.

Of course, for performance reasons, and to be able to emulate new
PowerPCs, I will relax the check, but this check should not prevent Mac
OS X to run.

Regards.

> On 5/16/05, Piotras <piotras@gmail.com> wrote:
> > Hi!
> > 
> > I'm not sure if this is relevant, but please search for "new form of
> > the mtcrf" on:
> > http://developer.apple.com/technotes/tn/tn2087.html
> > http://www.opensource.apple.com/darwinsource/10.3.9/gas-573.1/as/ppc.c
> > 
> > There are also some references to "POWER4 optimised mtcrf" (try google).
> > 
> > Do not really have time to research this right now,
> > 
> > Piotr Krysiuk
> > 
> > On 5/15/05, J. Mayer <l_indien@magic.fr> wrote:
> > > On Sat, 2005-05-14 at 21:03 +0200, Pierre d'Herbemont wrote:
> > > > On 14 mai 05, at 20:15, J. Mayer wrote:
> > > >
> > > > > Qemu assume all reserved fields are set to zero, not 1.
> > > >
> > > > > PowerPC specifications says: (section 8.1 of PEM)
> > > [...]
> > > > > In the case of mtcrf, the PowerPC specification says the bits 11,
> > > > > 21 and
> > > > > 31 (IBM notation) _must_ be zero.
> > > > > This is what is described in 32 bits PEM as well as 64 bits PEM
> > > > > (including the latest revision dated on 31/03/2005) and the 740/750
> > > > > PowerPC user manual (which is the one currently emulated by Qemu).
> > > > >
> > > > > It would be acceptable to relax the check if it would make MacOS X
> > > > > 10.4 boot.
> > > > > But in this case, only the bit 11 (which causes the problem here)
> > > > > should
> > > > > be relaxed.
> > > > > Then, the bit mask becomes 0x00000801 (not 0x00000000).
> > > >
> > > [...]
> > > > In fact I was referring to my old PowerPC Spec book in french from
> > > > IBM (titled Architecture PowerPC), this link was the first google
> > > > returned. After looking again in my book, I found what you said. I am
> > > > confused, I wrote this a bit quickly. But the fact remains, would you
> > > > send a correct patch for that? It'll definitely be compulsory in
> > > > order to boot Mac OS X 10.4, and for darwin-user.
> > >
> > > OK, I did put this in my working repository and I'll submit this to
> > > Fabrice.
> > > Please try to do this change to check if other bits need to be relax or
> > > not:
> > >
> > > Index: target-ppc/translate.c
> > > ===================================================================
> > > RCS file: /cvsroot/qemu/qemu/target-ppc/translate.c,v
> > > retrieving revision 1.31
> > > diff -u -r1.31 translate.c
> > > --- target-ppc/translate.c      12 May 2005 18:46:11 -0000      1.31
> > > +++ target-ppc/translate.c      14 May 2005 17:14:35 -0000
> > > @@ -2123,7 +2123,7 @@
> > >  }
> > >
> > >  /* mtcrf */
> > > -GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x00100801, PPC_MISC)
> > > +GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x00000801, PPC_MISC)
> > >  {
> > >      gen_op_load_gpr_T0(rS(ctx->opcode));
> > >      gen_op_store_cr(CRM(ctx->opcode));
> > >
> > > And maybe someone should fill an Apple bug report to tell them their as
> > > is buggy ;-)
> > >
> > > --
> > > J. Mayer <l_indien@magic.fr>
> > > Never organized
> > >
> > > _______________________________________________
> > > Qemu-devel mailing list
> > > Qemu-devel@nongnu.org
> > > http://lists.nongnu.org/mailman/listinfo/qemu-devel
> > >
> >
-- 
J. Mayer <l_indien@magic.fr>
Never organized