Re: [Qemu-devel] Help needed testing on ppc

[Tom Musta <tommusta@gmail.com>]

On 5/6/2014 6:17 PM, BALATON Zoltan wrote:
> On Tue, 6 May 2014, Tom Musta wrote:
>> On 5/6/2014 5:03 AM, BALATON Zoltan wrote:
>>> I'd appreciate some insight and help.
[snip]
>> (1) Why is MorphOS using this invalid instruction form?  Would it be easier to fix the OS rather than QEMU?
> 
> I don't know why is it used. I can ask the MorphOS developers but they did not seem to be too supportive so far and at least one of them expressed that they have no interest supporting other than their officially supported list of hardware at this time. So
> I assume it is easier to fix QEMU than MorphOS and if it works on a real Mac then it should also work on QEMU's emulation of that Mac hardware.
> 
>> Is there some undocumented processor behavior that the code is dependent upon (e.g. is it actually expected CR0 to be set?).
> 
> This is what the testing was supposed to find out but MorphOS seems to run better with the quoted patch so I don't think it depends on any other undocumented behaviour other than ignoring reserved bits but I have no definitive answer.
> 

It still seems to me that setting a reserved instruction bit is an strange thing to do.  It would be nice to at least
have a justification from MorphOS.  It is possible that no one even knows the answer.

>> (2) Your patch makes some store instructions compliant with the most recent ISAs but there are many other instructions that are not addressed by the patch.  I think fixing only some will be a future source of confusion.>>

Alex:  do you have an opinion on this?  Are you OK with changing masks for a few stores but not all instructions in general?

>> (3) The change risks breaking behavior on older designs which may very well have taken the illegal instruction interrupt.  Would it make more sense to leave the masks as-is and instead make a single, isolated change in the decoder
>> (gen_intermediate_code_internal).  This behavior could be made conditional (configuration item?  processor family specific flag?).  Unfortunately, the masks also catch some invalid forms that do not involve reserved fields (e.g. lq/stq to odd numbered
>> registers).
> 
> I don't know this code very well so not sure I can follow your suggestion. Are you proposing that the invalid masks could be ignored globally in gen_intermediate_code_internal (around target-ppc/traslate.c:11444) based on some condition for all opcodes?
> 

target-ppc/translate.c has a function named gen_intermediate_code_internal which does the decoding of the guest instructions.
Specifically it has this code:

 11434              }
 11435          } else {
 11436              uint32_t inval;
 11437
 11438              if (unlikely(handler->type & (PPC_SPE | PPC_SPE_SINGLE | PPC_SPE_DOUBLE) && Rc(ctx.opcode))) {
 11439                  inval = handler->inval2;
 11440              } else {
 11441                  inval = handler->inval1;
 11442              }
 11443
 11444              if (unlikely((ctx.opcode & inval) != 0)) {
 11445                  if (qemu_log_enabled()) {
 11446                      qemu_log("invalid bits: %08x for opcode: "
 11447                               "%02x - %02x - %02x (%08x) " TARGET_FMT_lx "\n",
 11448                               ctx.opcode & inval, opc1(ctx.opcode),
 11449                               opc2(ctx.opcode), opc3(ctx.opcode),
 11450                               ctx.opcode, ctx.nip - 4);
 11451                  }
 11452                  gen_inval_exception(ctxp, POWERPC_EXCP_INVAL_INVAL);
 11453                  break;
 11454              }
 11455          }

My observations are that (a) rather than fix individual masks (like your patch does), one could inhibit the detection of illegal bits
in one spot.  This behavior could be made dependent on something ... a configuration flag ... or it could be dependent on the processor
model.  So there is an opportunity to not change every PPC model because of an oddity in MorpOS.

> Since your quotes above show that QEMU does not implement the current specification and code relying on older behaviour would not run on newer processors so it's likely they will get fixed so I think the risk of breaking older designs is less than breaking
> software that rely on current specification so IMO it should be changed in QEMU if possible and only care about older designs when one is actually encountered.
> 

I agree with this argument except for the clause that says: "... and is being phased into the Embedded environment",
which still appears in the most recent ISA.  So the Book E folks my not be so ready to eliminate the reserved
bit masks.

>> (4) In general, modeling undefined behavior is a slippery slope.  I would much prefer to see the code fixed or justified before changing QEMU.
> 
> I can try to ask on the MorphOS list but their previous answer to another question was that it works on the hardware they officially support.

"Working" should not be confused for "correct"  :)