Re: [Qemu-devel] RFC: Why does target/m68k RTE insn. use gen_exception

[Richard Henderson]

On 6/29/19 6:36 PM, Lucien Murray-Pitts wrote:
> On Sat, Jun 29, 2019 at 12:15:44PM +0200, Richard Henderson wrote:
>> On 6/28/19 5:50 PM, Lucien Murray-Pitts wrote:
>>>  op_helper.c
>>>    static void m68k_interrupt_all(CPUM68KState *env, int is_hw)
>>>    ...
>>>      if (cs->exception_index == EXCP_ACCESS) {
>>>       ...
>>>       do_stack_frame(env, &sp, 7, oldsr, 0, retaddr /*LMP: BROKEN - needs 
>>> PC NEXT*/);
>>>
>>> Actually according to the MC68000 manuals the "return address" (the PC 
>>> saved on
>>> the stack) can be upto 5 instructions later due to prefetch. So some pc_next
>>> would best be used here.
>>
>> The way I read it from the 68040 manual, it's "the pc of the instruction
>> executing at the time the fault was detected".  Well, we did in fact detect 
>> the
>> fault at "retaddr", so that seems to be the right answer.  The fact that real
>> hardware has a different pipeline and detects the fault later seems 
>> immaterial,
>> and largely irrelevant, since the programmer wasn't given any guarantees for
>> what sort of value appears in that slot.
>>
> 
> I was reading the 68000/68020, and based on your suggestion now the 68060 
> manual
> The 68000 is pretty rough, but I agree you could expect it to more likely be 
> the
> next or even upto 5 or so instructions away.
> 
> MC68000UM.pdf, 5.4.1 Bus Error Operation
>   ....the following information is placed on the supervisor stack:
>     1. Status register
>     2. Program counter (two words, which may be up to five words past the
>        instruction being executed)
> ....
> MC68000UM.pdf, 6.3.9.1 BUS ERROR.
>   ...value saved for the program counter is advanced 2–10 bytes beyond the
>   address of the first word of the instruction that made the reference causing
>   the bus error. If the bus error occurred during the fetch of the next
>   instruction, the saved program counter has a value in the vicinity of the
>   current instruction, even if the current instruction is a branch, a jump, or
>   a return instruction ...
> 
> MC68020UM.pdf, 6.1.2 Bus Error Exception
>   The saved PC value is the logical address of the instruction that was
>   executing at the time the fault was detected. This is not necessarily the
>   instruction that initiated the bus cycle since the processor overlaps
>   execution of instructions
>   (See 6.4 Bus Fault REcovery and 6.3.11 Return From Exception)
>   
> MC68060UM.pdf, 8.4.4.1 Program Counter (PC).
> On read access faults, the PC points to the instruction that caused the
> access error. This instruction is restarted when an RTE is executed, hence,
> the read cycle is re-executed. On read access errors on the second or later
> of misaligned reads, the read cycles that are successful prior to the access
> error are re-executed since the processor uses a restart model for recovery
> from exceptions.

Interesting.  Thanks for digging through the other manuals.

> In my case I think the original firmware expects to return after the
> faulting instruction, and the retry of the bus io is to be ignored
> (this is a memory map probe routine).

Oh my.  Ok, well, if there's software that relies on some advance, I guess we
can conditionally do that.  It'll take a bit of work...

In

  https://patchwork.ozlabs.org/patch/1072596/

I arrange for the length of the current instruction to be stored in qemu's
exception unwind data.  Within restore_state_to_opc, the "ilen" is stored into
an existing slot in s390's env structure; you'd have to add such a slot for 
m68k.

Given the length of the insn currently executing, you can reliably compute the
pc of the next instruction.  Which would let you advance the pc for this case,
for the appropriate cpu type.

> However for the m68k the do_transaction_failed function pointer field
> has not been implemented.
> 
> I implemented it in a rudamentary fashion copying from already existing
> m68k_cpu_unassigned_access.  I really dont know if this is the right way.
> 
>   void m68k_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
>                                   unsigned size, MMUAccessType access_type,
>                                   int mmu_idx, MemTxAttrs attrs,
>                                   MemTxResult response, uintptr_t retaddr)
>   {
>       cs->exception_index = EXCP_ACCESS;
>       cpu_loop_exit(cs);
>   }

At minimum that's going to need

  cpu_loop_exit_restore(cs, retaddr);

But usually store some of the addr and access_type data into fields so that you
can properly fill in the exception frame later.

> Then I reverted my device to the "mem with attributes" io with a return of
> MEMTX_ERROR and found that the behavior during single-step were the same as
> before.
> 
> I end up ISR+1 instruction stepped in.

So, earlier, I suggested adding a call to cpu_handle_debug_exception within
cpu_handle_exception.  Did you try that?


r~