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

[Peter Maydell]

On 28 December 2015 at 01:55, Michael Davidsaver  wrote:
> On 12/17/2015 10:38 AM, Peter Maydell wrote:
>> We could use a comment here (a) explaining what we're doing and (b)
>> mentioning that this isn't architecturally correct -- ideally we should
>> catch these exception exits on execution of the jump insn, not by
>> letting the jump execute and then trapping when we actually try to
>> execute at the magic addresses.
>
> I had an instructive little digression to investigate doing things the
> "right way" (in tcg).  I can see how it would be done by adding a
> conditional every time the PC could be updated.  To me the unassigned
> handler trick/hack seems simpler (less likely to add a bug) and avoids
> emitting more code for every ldm/pop instruction.

Yes, it's faster, which is why we do it this way. It is however
not what the hardware does (in a way which is visible to guest code
which is specifically looking for the difference), which is why it's
worth commenting on.

thanks
-- PMM

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

[Peter Maydell]

On 27 December 2015 at 20:22, Michael Davidsaver  wrote:
> On 12/17/2015 10:38 AM, Peter Maydell wrote:
>> On 3 December 2015 at 00:18, Michael Davidsaver  
>> 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
> 0x4000 -> 0x40ff 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] < 0xfff0)) {
>>> +) {
>> 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
   0xfffx, 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 0xfffx 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

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

[Michael Davidsaver]

On 12/17/2015 10:38 AM, Peter Maydell wrote:
> On 3 December 2015 at 00:18, Michael Davidsaver  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
0x4000 -> 0x40ff which completes all reads with zero and logs. 
Please advise on how this should be handled.

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 can see that the http server example spends some time attempting MII
operations on the NIC.  As these aren't modeled it spins and eventually
gives up.

> ...
> We could use a comment here (a) explaining what we're doing and (b)
> mentioning that this isn't architecturally correct -- ideally we
> should catch these exception exits on execution of the jump insn, not
> by letting the jump execute and then trapping when we actually try to
> execute at the magic addresses. 

Will do.

>> ...
>> @@ -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] < 0xfff0)) {
>> +) {
> 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.

Put more literally, gen_intermediate_code() is only called from
tb_gen_code() where it comes after a call to get_page_addr_code(),
wherein the unassigned handler calls cpu_loop_exit().

I've replaced the M case for EXCP_EXCEPTION_EXIT in
gen_intermediate_code() with an assert.  So far it hasn't failed.

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

[Michael Davidsaver]

On 12/17/2015 10:38 AM, Peter Maydell wrote:
> We could use a comment here (a) explaining what we're doing and (b)
> mentioning that this isn't architecturally correct -- ideally we should
> catch these exception exits on execution of the jump insn, not by
> letting the jump execute and then trapping when we actually try to
> execute at the magic addresses.

I had an instructive little digression to investigate doing things the
"right way" (in tcg).  I can see how it would be done by adding a
conditional every time the PC could be updated.  To me the unassigned
handler trick/hack seems simpler (less likely to add a bug) and avoids
emitting more code for every ldm/pop instruction.

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

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver  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 .

> ---
>  hw/arm/armv7m.c  |  8 
>  target-arm/cpu.c | 32 +---
>  2 files changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index a80d2ad..68146de 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -176,7 +176,6 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int 
> mem_size, int num_irq,
>  uint64_t entry;
>  uint64_t lowaddr;
>  int big_endian;
> -MemoryRegion *hack = g_new(MemoryRegion, 1);
>
>  if (cpu_model == NULL) {
> cpu_model = "cortex-m3";
> @@ -221,13 +220,6 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, 
> int mem_size, int num_irq,
>  }
>  }
>
> -/* Hack to map an additional page of ram at the top of the address
> -   space.  This stops qemu complaining about executing code outside RAM
> -   when returning from an exception.  */
> -memory_region_init_ram(hack, NULL, "armv7m.hack", 0x1000, _fatal);
> -vmstate_register_ram_global(hack);
> -memory_region_add_subregion(system_memory, 0xf000, hack);
> -
>  qemu_register_reset(armv7m_reset, cpu);
>  return nvic;
>  }
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 30739fc..728854f 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -280,6 +280,25 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int 
> interrupt_request)
>  }
>
>  #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
> +static void arm_v7m_unassigned_access(CPUState *cpu, hwaddr addr,
> +  bool is_write, bool is_exec, int 
> opaque,
> +  unsigned size)
> +{
> +ARMCPU *arm = ARM_CPU(cpu);
> +CPUARMState *env = >env;
> +
> +env->exception.vaddress = addr;
> +env->exception.fsr = is_write ? 0x808 : 0x8; /* Precise External Abort */
> +if (env->v7m.exception != 0 && addr >= 0xfff0 && !is_write) {
> +cpu->exception_index = EXCP_EXCEPTION_EXIT;

We could use a comment here (a) explaining what we're doing and (b)
mentioning that this isn't architecturally correct -- ideally we should
catch these exception exits on execution of the jump insn, not by
letting the jump execute and then trapping when we actually try to
execute at the magic addresses.

> +} else if (is_exec) {
> +cpu->exception_index = EXCP_PREFETCH_ABORT;
> +} else {
> +cpu->exception_index = EXCP_DATA_ABORT;
> +}
> +cpu_loop_exit(cpu);
> +}
> +
>  static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  {
>  CPUClass *cc = CPU_GET_CLASS(cs);
> @@ -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] < 0xfff0)) {
> +) {

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.

(This isn't what's causing my test image to not run, though.)

>  cs->exception_index = EXCP_IRQ;
>  cc->do_interrupt(cs);
>  ret = true;
> @@ -909,6 +918,7 @@ static void arm_v7m_class_init(ObjectClass *oc, void 
> *data)
>  cc->do_interrupt = arm_v7m_cpu_do_interrupt;
>  #endif
>
> +cc->do_unassigned_access = arm_v7m_unassigned_access;
>  cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
>  }

thanks
-- PMM

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

[Michael Davidsaver]

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.
---
 hw/arm/armv7m.c  |  8 
 target-arm/cpu.c | 32 +---
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index a80d2ad..68146de 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -176,7 +176,6 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int 
mem_size, int num_irq,
 uint64_t entry;
 uint64_t lowaddr;
 int big_endian;
-MemoryRegion *hack = g_new(MemoryRegion, 1);
 
 if (cpu_model == NULL) {
cpu_model = "cortex-m3";
@@ -221,13 +220,6 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int 
mem_size, int num_irq,
 }
 }
 
-/* Hack to map an additional page of ram at the top of the address
-   space.  This stops qemu complaining about executing code outside RAM
-   when returning from an exception.  */
-memory_region_init_ram(hack, NULL, "armv7m.hack", 0x1000, _fatal);
-vmstate_register_ram_global(hack);
-memory_region_add_subregion(system_memory, 0xf000, hack);
-
 qemu_register_reset(armv7m_reset, cpu);
 return nvic;
 }
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 30739fc..728854f 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -280,6 +280,25 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 }
 
 #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
+static void arm_v7m_unassigned_access(CPUState *cpu, hwaddr addr,
+  bool is_write, bool is_exec, int opaque,
+  unsigned size)
+{
+ARMCPU *arm = ARM_CPU(cpu);
+CPUARMState *env = >env;
+
+env->exception.vaddress = addr;
+env->exception.fsr = is_write ? 0x808 : 0x8; /* Precise External Abort */
+if (env->v7m.exception != 0 && addr >= 0xfff0 && !is_write) {
+cpu->exception_index = EXCP_EXCEPTION_EXIT;
+} else if (is_exec) {
+cpu->exception_index = EXCP_PREFETCH_ABORT;
+} else {
+cpu->exception_index = EXCP_DATA_ABORT;
+}
+cpu_loop_exit(cpu);
+}
+
 static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
 CPUClass *cc = CPU_GET_CLASS(cs);
@@ -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] < 0xfff0)) {
+) {
 cs->exception_index = EXCP_IRQ;
 cc->do_interrupt(cs);
 ret = true;
@@ -909,6 +918,7 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data)
 cc->do_interrupt = arm_v7m_cpu_do_interrupt;
 #endif
 
+cc->do_unassigned_access = arm_v7m_unassigned_access;
 cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
 }
 
-- 
2.1.4