[Qemu-devel] target-ppc: Problem with mtmsr emulation

[Qemu-devel] target-ppc: Problem with mtmsr emulation

[Thomas Huth]

 Hi all!

There seems to be a problem with the emulation of the mtmsr instruction:
According to the PowerISA spec, chapter Book III-S, the mtmsr opcode
has a so-called "L" field at bit position 15. Looking at the function
gen_mtmsr() in target-ppc/translate.c, the bit is taken into account
since the function checks for ctx->opcode & 0x00010000.
However, when looking at the GEN_HANDLER definition later in that file:

 GEN_HANDLER(mtmsr, 0x1F, 0x12, 0x04, 0x001FF801, PPC_MISC)

you can see that the bit is marked as invalid bit in the 0x001FF801
mask, thus if the bit is set, a program exception is generated instead
of executing the gen_mtmsr() function.

An easy way to fix this for Book III-S is to change the mask to
0x001EF801 (just like the mask for mtmsrd), but I am afraid that this
would break the Book III-E variant of mtmsr, since the embedded version
does not have this bit defined. Any suggestions how to fix this problem
in a proper way?

 Thomas

Re: [Qemu-devel] target-ppc: Problem with mtmsr emulation

[Alexander Graf]

> Am 28.03.2014 um 16:16 schrieb Thomas Huth <thuth@linux.vnet.ibm.com>:
> 
> 
> Hi all!
> 
> There seems to be a problem with the emulation of the mtmsr instruction:
> According to the PowerISA spec, chapter Book III-S, the mtmsr opcode
> has a so-called "L" field at bit position 15. Looking at the function
> gen_mtmsr() in target-ppc/translate.c, the bit is taken into account
> since the function checks for ctx->opcode & 0x00010000.
> However, when looking at the GEN_HANDLER definition later in that file:
> 
> GEN_HANDLER(mtmsr, 0x1F, 0x12, 0x04, 0x001FF801, PPC_MISC)
> 
> you can see that the bit is marked as invalid bit in the 0x001FF801
> mask, thus if the bit is set, a program exception is generated instead
> of executing the gen_mtmsr() function.
> 
> An easy way to fix this for Book III-S is to change the mask to
> 0x001EF801 (just like the mask for mtmsrd), but I am afraid that this
> would break the Book III-E variant of mtmsr, since the embedded version
> does not have this bit defined. Any suggestions how to fix this problem
> in a proper way?

Please check in the older isa versions whether that bit is declared reserved.

If it is, we need to make sure we only match it on newer ISA conformance. In general there are 2 ways to do this:

  1) disjunct feature bits
  2) manual check inside the translating function

We don't have any way to manually apply logic like "match if bit x is not present" or have duplicate entries with manual ordering. I remember that I manually created a runtime computed not-bit a while ago for something, but I can't check right now (no access to a real machine).

I certainly do not mind an open-coded check though - we've done this for the VSX code too.

Alex

> 
> Thomas
> 

Re: [Qemu-devel] target-ppc: Problem with mtmsr emulation

[Thomas Huth]

On Fri, 28 Mar 2014 18:25:02 +0800
Alexander Graf <agraf@suse.de> wrote:

> 
> 
> > Am 28.03.2014 um 16:16 schrieb Thomas Huth <thuth@linux.vnet.ibm.com>:
> > 
> > 
> > Hi all!
> > 
> > There seems to be a problem with the emulation of the mtmsr instruction:
> > According to the PowerISA spec, chapter Book III-S, the mtmsr opcode
> > has a so-called "L" field at bit position 15. Looking at the function
> > gen_mtmsr() in target-ppc/translate.c, the bit is taken into account
> > since the function checks for ctx->opcode & 0x00010000.
> > However, when looking at the GEN_HANDLER definition later in that file:
> > 
> > GEN_HANDLER(mtmsr, 0x1F, 0x12, 0x04, 0x001FF801, PPC_MISC)
> > 
> > you can see that the bit is marked as invalid bit in the 0x001FF801
> > mask, thus if the bit is set, a program exception is generated instead
> > of executing the gen_mtmsr() function.
> > 
> > An easy way to fix this for Book III-S is to change the mask to
> > 0x001EF801 (just like the mask for mtmsrd), but I am afraid that this
> > would break the Book III-E variant of mtmsr, since the embedded version
> > does not have this bit defined. Any suggestions how to fix this problem
> > in a proper way?
> 
> Please check in the older isa versions whether that bit is declared reserved.
> 
> If it is, we need to make sure we only match it on newer ISA conformance.

The oldest ISA version that I've found (version 2.01, from 2003) already
contains the L bit, so I assume it's always been there. So it's likely
just a Book III-S vs. Book III-E issue.

 Thomas

Re: [Qemu-devel] [Qemu-ppc] target-ppc: Problem with mtmsr emulation

[Tom Musta]

On 3/28/2014 7:02 AM, Thomas Huth wrote:
> On Fri, 28 Mar 2014 18:25:02 +0800
> Alexander Graf <agraf@suse.de> wrote:
> 
>>
>>
>>> Am 28.03.2014 um 16:16 schrieb Thomas Huth <thuth@linux.vnet.ibm.com>:
>>>

<snip>

>>> An easy way to fix this for Book III-S is to change the mask to
>>> 0x001EF801 (just like the mask for mtmsrd), but I am afraid that this
>>> would break the Book III-E variant of mtmsr, since the embedded version
>>> does not have this bit defined. Any suggestions how to fix this problem
>>> in a proper way?
>>
>> Please check in the older isa versions whether that bit is declared reserved.
>>
>> If it is, we need to make sure we only match it on newer ISA conformance.
> 
> The oldest ISA version that I've found (version 2.01, from 2003) already
> contains the L bit, so I assume it's always been there. So it's likely
> just a Book III-S vs. Book III-E issue.
> 

The L bit was not part of the original PowerPC ISA.  I checked both my 604 manual
((C) 1993) and the May, Silha, Simpson, Warren book ((C) 1994) ... neither contains
the L bit.  So the *actual* delineation is not as simple as Book III-S vs. Book
III-E.  I suspect the change was introduced in the mid-2000's.

To make matters worse, the change was incompatible with the previous versions of
the architecture -- The L=1 case is the old behavior (copy source register bits
verbatim, execution synchronizing) whereas L=0 is the new behavior (force external
interrupts and virtual address translation in user-state, context synchronizing).
And, the L=1 case on Book-IIIS is more like the L=0 case in Book-IIIE.

Also, I do not (yet) see the actual implementation of the Book-IIIS L=0 behavior
in the QEMU code.  This bug is probably masked by the fact that folks who use
mtmsr probably know what they are doing -- i.e. who would try to enable user-mode
and not enabled address translation?

Egads, what a mess.

I agree with Alex that a flags based approach could be used to support the L bit for
Book III-S models and to ignore the L bit for Book III-E models.  The question is
which flag(s) can we use?  Let me see if I can find out.

Re: [Qemu-devel] [Qemu-ppc] target-ppc: Problem with mtmsr emulation

[Tom Musta]

On 3/28/2014 9:36 AM, Tom Musta wrote:
> I agree with Alex that a flags based approach could be used to support the L bit for
> Book III-S models and to ignore the L bit for Book III-E models.  The question is
> which flag(s) can we use?  Let me see if I can find out.

I looked at this a bit more.  I do not see any good correlation with the existing
instruction flags.

I do see some good correlation with the exception flags. But there are problems: One
challenge that I see is that the exception flags are not (currently) available in the
context structure passed to the generators (DisasContext). The other problem is that
this has the same brittleness that Alex was observing about adding new models and
needing to remember all the little bits to be set (different thread yesterday).

Given this, I think a reasonable way to proceed would be to:

  (a) add an explicit instruction flag for L bit support (e.g. PPC2_MTMSR_L)
  (b) remove the bit from the reserved bits mask
  (c) add some additional logic to the mtmsr[d] generators to gate the L=1 path.
  (d) add the new flag to the more recent models that support Book III-S.

And I still think there might be a bug in the L=0 Book III-S path.