Re: [Qemu-devel] [PATCH v2 02/26] armv7m: Undo armv7m.hack

[Peter Maydell]

On 27 December 2015 at 20:22, Michael Davidsaver <address@hidden> wrote:
> On 12/17/2015 10:38 AM, Peter Maydell wrote:
>> On 3 December 2015 at 00:18, Michael Davidsaver <address@hidden> wrote:
>>> Add CPU unassigned access handler in place of special
>>> MemoryRegion to catch exception returns.
>>>
>>> The unassigned handler will signal other faults as either
>>> prefetch or data exceptions, with the FSR code 0x8 to
>>> distinguish them from memory translation faults (0xd).
>>> Future code will make use of this distinction when
>>> deciding to raise BusFault or MemManage exceptions.
>> This patch breaks my Stellaris test image -- instead of starting
>> it just sits there with a black screen.
>>
>> I've put a copy of that test image up at
>>   http://people.linaro.org/~peter.maydell/stellaris.tgz
>> You can run it with path/to/stellaris/runme path/to/qemu-system-arm .
>
> There were several issues.  Two bugs (wrong IRQ enabled and systick not
> enabled) and a "feature" (access to unimplemented registers for a PWM
> controller is now a BusFault).
>
> As a workaround for the "feature" I add a low priority MemoryRegion from
> 0x40000000 -> 0x40ffffff which completes all reads with zero and logs.
> Please advise on how this should be handled.

We should probably at least identify what particular devices are
supposed to be here and put in dummy versions, rather than just
having a single memory region which does RAZ/WI.

> With these changes both test programs appear to run correctly, although
> the http server example has painfully slow load times and seems to hit
> an out of memory condition if I look at it wrong.  Is this expected?
> (and the blub on the buttons page about "xml technology" is priceless)

I don't run the HTTP example often. The basic requirement is
"should not get any worse as a result of the patchset". Problems
that were already there before need not be addressed.

>>> @@ -294,19 +313,9 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, 
>>> int interrupt_request)
>>>          cc->do_interrupt(cs);
>>>          ret = true;
>>>      }
>>> -    /* ARMv7-M interrupt return works by loading a magic value
>>> -     * into the PC.  On real hardware the load causes the
>>> -     * return to occur.  The qemu implementation performs the
>>> -     * jump normally, then does the exception return when the
>>> -     * CPU tries to execute code at the magic address.
>>> -     * This will cause the magic PC value to be pushed to
>>> -     * the stack if an interrupt occurred at the wrong time.
>>> -     * We avoid this by disabling interrupts when
>>> -     * pc contains a magic address.
>>> -     */
>>>      if (interrupt_request & CPU_INTERRUPT_HARD
>>>          && !(env->daif & PSTATE_I)
>>> -        && (env->regs[15] < 0xfffffff0)) {
>>> +            ) {
>> Can we really drop this change? The thing it's guarding against
>> (interrupt comes in while the PC is this not-really-an-address
>> value) can still happen whether we catch the attempt to execute
>> in translate.c or via the unassigned-access hook.
>
> I don't think the M-profile case in gen_intermediate_code() in
> translate.c can ever be reached without first hitting the unassigned
> memory handler.  Before the code can be translated, the page containing
> it must be loaded.  Such loads no longer succeed.

Yes, but the code you've deleted here may be called after
we have set the PC to a magic value but before we have tried
to do the address load for it:
 1 translation block A (with the pop or ldm) sets PC to
   0xfffffffx, and execution leaves this TB and returns to
   the top level loop
 2 normally, we would then try to execute at the magic address,
   which would result in our trying to translate a TB for that
   address, which immediately causes us to run the code in the
   unassigned-access hook. That will cause the PC to be set
   to the appropriate value for having returned from the
   interrupt handler
 3 however, it is possible that an interrupt has been raised
   which means that between steps 1 and 2 we will say "actually,
   need to take an interrupt now". Since between steps 1 and 2
   the value in env->regs[15] is the magic 0xfffffffx value,
   we will end up stacking the magic value as part of the
   interrupt entry process. This is wrong, and the reason for
   the condition above is to avoid this problem.

Changing the handling of "PC == magic value" from translate.c
to the unassigned-access hook does not close the window where
env->regs[15] is a value the guest should not see as an
interrupted PC.

thanks
-- PMM