[Qemu-devel] [PATCH v1 0/3] Some armv7m fixes

[Qemu-devel] [PATCH v1 0/3] Some armv7m fixes

[KONRAD Frederic]

Hi,

While playing with armv7m, I found two little bugs:
  - When there is an alias @0x00000000 to a flash memory the cpu state isn't
    reset correctly which leads later to an exception as ARM instruction-set is
    used. Presumably this bug might be present with the netduino2 board.
  - If the developer omits to set system_clock_rate we later go in a livelock
    when systick is triggered. Better aborting before to avoid the pain chasing
    the livelock.

Thanks,
Fred

KONRAD Frederic (3):
  add memory_region_get_offset_within_address_space
  arm: fix the armv7m reset state
  armv7m_systick: abort instead of locking on a bad rate

 hw/timer/armv7m_systick.c |  3 +++
 include/exec/memory.h     | 10 ++++++++++
 memory.c                  | 22 ++++++++++++++++++++--
 target/arm/cpu.c          | 14 ++++++++++++++
 4 files changed, 47 insertions(+), 2 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v1 1/3] add memory_region_get_offset_within_address_space

[KONRAD Frederic]

This is helpful in the next patch to know if a rom is pointed by an alias.

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
---
 include/exec/memory.h | 10 ++++++++++
 memory.c              | 22 ++++++++++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 8503685..e342412 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1270,6 +1270,16 @@ void memory_region_set_size(MemoryRegion *mr, uint64_t size);
 void memory_region_set_alias_offset(MemoryRegion *mr,
                                     hwaddr offset);
 
+/*
+ * memory_region_get_offset_within_address_space: get the offset of a region
+ *
+ * Returns the offset of a region within its address space. @mr must be mapped
+ * to an #AddressSpace.
+ *
+ * @mr: the #MemoryRegion to check.
+ */
+hwaddr memory_region_get_offset_within_address_space(MemoryRegion *mr);
+
 /**
  * memory_region_present: checks if an address relative to a @container
  * translates into #MemoryRegion within @container
diff --git a/memory.c b/memory.c
index 1044bba..2b7439b 100644
--- a/memory.c
+++ b/memory.c
@@ -598,11 +598,18 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
     return r;
 }
 
-static AddressSpace *memory_region_to_address_space(MemoryRegion *mr)
+static AddressSpace *memory_region_to_address_space(MemoryRegion *mr,
+                                                    hwaddr *offset)
 {
     AddressSpace *as;
 
+    if (offset) {
+        *offset = 0;
+    }
     while (mr->container) {
+        if (offset) {
+            *offset += mr->addr;
+        }
         mr = mr->container;
     }
     QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
@@ -613,6 +620,17 @@ static AddressSpace *memory_region_to_address_space(MemoryRegion *mr)
     return NULL;
 }
 
+hwaddr memory_region_get_offset_within_address_space(MemoryRegion *mr)
+{
+    hwaddr offset;
+    AddressSpace *as;
+
+    as = memory_region_to_address_space(mr, &offset);
+    assert(as);
+
+    return offset;
+}
+
 /* Render a memory region into the global view.  Ranges in @view obscure
  * ranges in @mr.
  */
@@ -2251,7 +2269,7 @@ static MemoryRegionSection memory_region_find_rcu(MemoryRegion *mr,
         addr += root->addr;
     }
 
-    as = memory_region_to_address_space(root);
+    as = memory_region_to_address_space(root, NULL);
     if (!as) {
         return ret;
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH v1 2/3] arm: fix the armv7m reset state

[KONRAD Frederic]

This fixes an odd bug when a ROM is present somewhere and an alias @0x00000000
is pointing to the ROM. The "if (rom)" test fails and we don't get a valid reset
state. QEMU later crashes with an exception because the ARMv7-M starts with the
ARM instruction set. (eg: PC & 0x01 is 0).

This patch uses memory_region_get_offset_within_address_space introduced before
to check if an alias doesn't point to a flash somewhere.

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
---
 target/arm/cpu.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 28a9141..b8afd97 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -201,6 +201,20 @@ static void arm_cpu_reset(CPUState *s)
 
         /* Load the initial SP and PC from the vector table at address 0 */
         rom = rom_ptr(0);
+
+        if (!rom) {
+            /* Sometimes address 0x00000000 is an alias to a flash which
+             * actually have a ROM.
+             */
+            MemoryRegionSection section;
+            hwaddr offset = 0;
+
+            section = memory_region_find(s->as->root, 0, 8);
+            offset = memory_region_get_offset_within_address_space(section.mr);
+            memory_region_unref(section.mr);
+            rom = rom_ptr(offset);
+        }
+
         if (rom) {
             /* Address zero is covered by ROM which hasn't yet been
              * copied into physical memory.
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v1 2/3] arm: fix the armv7m reset state

[Peter Maydell]

On 29 June 2017 at 10:28, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
> This fixes an odd bug when a ROM is present somewhere and an alias @0x00000000
> is pointing to the ROM. The "if (rom)" test fails and we don't get a valid reset
> state. QEMU later crashes with an exception because the ARMv7-M starts with the
> ARM instruction set. (eg: PC & 0x01 is 0).
>
> This patch uses memory_region_get_offset_within_address_space introduced before
> to check if an alias doesn't point to a flash somewhere.
>
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>

This is awkward, because in the "we have a ROM but it's not been
copied into memory yet" case, the only thing we have is the
rom->addr, which is the address which the user's ROM blob said
it ought to be loaded in at. If the user didn't actually provide
a ROM blob that loads at 0 that seems a bit like a user error,
and I don't think this patch will catch all the cases of that
sort of mistake. For instance if address 0 is real flash and the
high address alias is modelled by having the high address be the
alias, then if the user passes us an ELF file saying "load to
the high address" then this change won't catch that I think
(because doing the memory_region_find/get_offset_within_address_space
will return 0, which has already been tried). You'd need to
somehow have a way to say "find all the addresses within this
AS where this MR is mapped" and try them all...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 2/3] arm: fix the armv7m reset state

[KONRAD Frederic]

On 06/29/2017 05:14 PM, Peter Maydell wrote:
> On 29 June 2017 at 10:28, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
>> This fixes an odd bug when a ROM is present somewhere and an alias @0x00000000
>> is pointing to the ROM. The "if (rom)" test fails and we don't get a valid reset
>> state. QEMU later crashes with an exception because the ARMv7-M starts with the
>> ARM instruction set. (eg: PC & 0x01 is 0).
>>
>> This patch uses memory_region_get_offset_within_address_space introduced before
>> to check if an alias doesn't point to a flash somewhere.
>>
>> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> 
> This is awkward, because in the "we have a ROM but it's not been
> copied into memory yet" case, the only thing we have is the
> rom->addr, which is the address which the user's ROM blob said
> it ought to be loaded in at. If the user didn't actually provide
> a ROM blob that loads at 0 that seems a bit like a user error,
> and I don't think this patch will catch all the cases of that
> sort of mistake.

I don't think it's really a user mistake because on the real HW
the alias is configurable.. at least in my case.

There is a "jumper" setting to mirror either the Flash or the
SRAM, etc. So the binaries isn't located at 0 but at the flash
address 0x8000000 or some such. That's the case with u-boot and
the precompiled examples I found for this stm32fxxxx board.

  For instance if address 0 is real flash and the
> high address alias is modelled by having the high address be the
> alias, then if the user passes us an ELF file saying "load to
> the high address" then this change won't catch that I think
> (because doing the memory_region_find/get_offset_within_address_space
> will return 0, which has already been tried). You'd need to
> somehow have a way to say "find all the addresses within this
> AS where this MR is mapped" and try them all...

This is more likely to be a user error :). Maybe we can load
the ROM before the reset but that seems a lot more invasive..

BTW isn't there a trick with the ELF entry somewhere? Or is that
for the Cortex-A?

Thanks,
Fred

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH v1 2/3] arm: fix the armv7m reset state

[Peter Maydell]

On 29 June 2017 at 17:41, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
> On 06/29/2017 05:14 PM, Peter Maydell wrote:
>> This is awkward, because in the "we have a ROM but it's not been
>> copied into memory yet" case, the only thing we have is the
>> rom->addr, which is the address which the user's ROM blob said
>> it ought to be loaded in at. If the user didn't actually provide
>> a ROM blob that loads at 0 that seems a bit like a user error,
>> and I don't think this patch will catch all the cases of that
>> sort of mistake.
>
>
> I don't think it's really a user mistake because on the real HW
> the alias is configurable.. at least in my case.
>
> There is a "jumper" setting to mirror either the Flash or the
> SRAM, etc. So the binaries isn't located at 0 but at the flash
> address 0x8000000 or some such. That's the case with u-boot and
> the precompiled examples I found for this stm32fxxxx board.
>
>>  For instance if address 0 is real flash and the
>> high address alias is modelled by having the high address be the
>> alias, then if the user passes us an ELF file saying "load to
>> the high address" then this change won't catch that I think
>> (because doing the memory_region_find/get_offset_within_address_space
>> will return 0, which has already been tried). You'd need to
>> somehow have a way to say "find all the addresses within this
>> AS where this MR is mapped" and try them all...
>
>
> This is more likely to be a user error :). Maybe we can load
> the ROM before the reset but that seems a lot more invasive..

It's the same thing, though, right? If the user's ELF file
says "vector table is at 0x8000000" then we should either
(a) say that's a user error, or
(b) handle it right, whether we implemented the QEMU model with
the flash at 0 and the alias at 0x8000000, or with the flash
at 0x8000000 and the alias at 0.

> BTW isn't there a trick with the ELF entry somewhere? Or is that
> for the Cortex-A?

We don't honour the ELF entry point on M profile (arguably
a bug) -- armv7m_load_kernel() ignores the entry point
returned by load_elf() in the 'entry' variable.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 2/3] arm: fix the armv7m reset state

[KONRAD Frederic]

On 06/29/2017 06:45 PM, Peter Maydell wrote:
> On 29 June 2017 at 17:41, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
>> On 06/29/2017 05:14 PM, Peter Maydell wrote:
>>> This is awkward, because in the "we have a ROM but it's not been
>>> copied into memory yet" case, the only thing we have is the
>>> rom->addr, which is the address which the user's ROM blob said
>>> it ought to be loaded in at. If the user didn't actually provide
>>> a ROM blob that loads at 0 that seems a bit like a user error,
>>> and I don't think this patch will catch all the cases of that
>>> sort of mistake.
>>
>>
>> I don't think it's really a user mistake because on the real HW
>> the alias is configurable.. at least in my case.
>>
>> There is a "jumper" setting to mirror either the Flash or the
>> SRAM, etc. So the binaries isn't located at 0 but at the flash
>> address 0x8000000 or some such. That's the case with u-boot and
>> the precompiled examples I found for this stm32fxxxx board.
>>
>>>   For instance if address 0 is real flash and the
>>> high address alias is modelled by having the high address be the
>>> alias, then if the user passes us an ELF file saying "load to
>>> the high address" then this change won't catch that I think
>>> (because doing the memory_region_find/get_offset_within_address_space
>>> will return 0, which has already been tried). You'd need to
>>> somehow have a way to say "find all the addresses within this
>>> AS where this MR is mapped" and try them all...
>>
>>
>> This is more likely to be a user error :). Maybe we can load
>> the ROM before the reset but that seems a lot more invasive..
> 
> It's the same thing, though, right? If the user's ELF file
> says "vector table is at 0x8000000" then we should either
> (a) say that's a user error, or
> (b) handle it right, whether we implemented the QEMU model with
> the flash at 0 and the alias at 0x8000000, or with the flash
> at 0x8000000 and the alias at 0.

Hi Peter,

Fondamentaly yes, it is the same.. but it seems really strange to
me to load the elf in the alias.

If I choose (a) I'll need to objcpy all the elf to 0 or modify
the build script which should work on the real board.. This seems
not really a good option to me.

If I choose (b) I won't be able to load it to SRAM and it is
basically the same result I'll need to move or modify the config.

Thanks,
Fred

> 
>> BTW isn't there a trick with the ELF entry somewhere? Or is that
>> for the Cortex-A?
> 
> We don't honour the ELF entry point on M profile (arguably
> a bug) -- armv7m_load_kernel() ignores the entry point
> returned by load_elf() in the 'entry' variable.
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH v1 2/3] arm: fix the armv7m reset state

[Peter Maydell]

On 30 June 2017 at 09:24, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
> On 06/29/2017 06:45 PM, Peter Maydell wrote:
>> It's the same thing, though, right? If the user's ELF file
>> says "vector table is at 0x8000000" then we should either
>> (a) say that's a user error, or
>> (b) handle it right, whether we implemented the QEMU model with
>> the flash at 0 and the alias at 0x8000000, or with the flash
>> at 0x8000000 and the alias at 0.

> Fondamentaly yes, it is the same.. but it seems really strange to
> me to load the elf in the alias.

The user creating the ELF file has no idea whether we
modelled the flash at 0 and the alias at highmem or
the other way around -- that is an implementation detail
that should be completely invisible to the user.
My two suggestions are based on that point -- either we
should treat "ELF loaded at highmem" as always wrong, or
we should always support it.

> If I choose (a) I'll need to objcpy all the elf to 0 or modify
> the build script which should work on the real board.. This seems
> not really a good option to me.

I agree that I don't like this.

> If I choose (b) I won't be able to load it to SRAM and it is
> basically the same result I'll need to move or modify the config.

I don't understand this, though. Option (b) is probably painful
to implement (I don't have a good idea of how to do it) but
it ought to mean that the ELF files that work on the board
also work for QEMU (regardless of how the board model
implemented the aliased flash).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 2/3] arm: fix the armv7m reset state

[KONRAD Frederic]

On 06/30/2017 11:06 AM, Peter Maydell wrote:
> On 30 June 2017 at 09:24, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
>> On 06/29/2017 06:45 PM, Peter Maydell wrote:
>>> It's the same thing, though, right? If the user's ELF file
>>> says "vector table is at 0x8000000" then we should either
>>> (a) say that's a user error, or
>>> (b) handle it right, whether we implemented the QEMU model with
>>> the flash at 0 and the alias at 0x8000000, or with the flash
>>> at 0x8000000 and the alias at 0.
> 
>> Fondamentaly yes, it is the same.. but it seems really strange to
>> me to load the elf in the alias.
> 
> The user creating the ELF file has no idea whether we
> modelled the flash at 0 and the alias at highmem or
> the other way around -- that is an implementation detail
> that should be completely invisible to the user.
> My two suggestions are based on that point -- either we
> should treat "ELF loaded at highmem" as always wrong, or
> we should always support it.
> 
>> If I choose (a) I'll need to objcpy all the elf to 0 or modify
>> the build script which should work on the real board.. This seems
>> not really a good option to me.
> 
> I agree that I don't like this.
> 
>> If I choose (b) I won't be able to load it to SRAM and it is
>> basically the same result I'll need to move or modify the config.
> 
> I don't understand this, though. Option (b) is probably painful
> to implement (I don't have a good idea of how to do it) but
> it ought to mean that the ELF files that work on the board
> also work for QEMU (regardless of how the board model
> implemented the aliased flash).
> 

Yes that's exactly what I want.

Basically the 0x00000000 alias can point to the SRAM or the ROM
during the reset depending on some boot config. The ELF is
directly loaded in the ROM or in the SRAM and my patch allows to
fetch the two first words in the reset handler to make it work
for any boot config.

For example this devices p69:
www.st.com/resource/en/reference_manual/DM00031020.pdf

Thanks,
Fred

> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH v1 2/3] arm: fix the armv7m reset state

[Peter Maydell]

On 3 July 2017 at 08:31, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
> On 06/30/2017 11:06 AM, Peter Maydell wrote:
>> On 30 June 2017 at 09:24, KONRAD Frederic <frederic.konrad@adacore.com>
>> wrote:
>>> If I choose (b) I won't be able to load it to SRAM and it is
>>> basically the same result I'll need to move or modify the config.
>>
>>
>> I don't understand this, though. Option (b) is probably painful
>> to implement (I don't have a good idea of how to do it) but
>> it ought to mean that the ELF files that work on the board
>> also work for QEMU (regardless of how the board model
>> implemented the aliased flash).
>>
>
> Yes that's exactly what I want.
>
> Basically the 0x00000000 alias can point to the SRAM or the ROM
> during the reset depending on some boot config. The ELF is
> directly loaded in the ROM or in the SRAM and my patch allows to
> fetch the two first words in the reset handler to make it work
> for any boot config.

Yes, but it only works if you implemented it that way
round, and not for board implementations which put the
real device at 0 and the alias at high memory. I'd like a fix
which deals with all of this, not just with the particular
arrangement your board implementation has.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 2/3] arm: fix the armv7m reset state

[KONRAD Frederic]

On 07/03/2017 10:51 AM, Peter Maydell wrote:
> On 3 July 2017 at 08:31, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
>> On 06/30/2017 11:06 AM, Peter Maydell wrote:
>>> On 30 June 2017 at 09:24, KONRAD Frederic <frederic.konrad@adacore.com>
>>> wrote:
>>>> If I choose (b) I won't be able to load it to SRAM and it is
>>>> basically the same result I'll need to move or modify the config.
>>>
>>>
>>> I don't understand this, though. Option (b) is probably painful
>>> to implement (I don't have a good idea of how to do it) but
>>> it ought to mean that the ELF files that work on the board
>>> also work for QEMU (regardless of how the board model
>>> implemented the aliased flash).
>>>
>>
>> Yes that's exactly what I want.
>>
>> Basically the 0x00000000 alias can point to the SRAM or the ROM
>> during the reset depending on some boot config. The ELF is
>> directly loaded in the ROM or in the SRAM and my patch allows to
>> fetch the two first words in the reset handler to make it work
>> for any boot config.
> 
> Yes, but it only works if you implemented it that way
> round, and not for board implementations which put the
> real device at 0 and the alias at high memory. I'd like a fix
> which deals with all of this, not just with the particular
> arrangement your board implementation has.

Ok got it, I'll check if I can do something clean which can
handle both ways.

Fred

> 
> thanks
> -- PMM
> 

[Qemu-devel] [PATCH v1 3/3] armv7m_systick: abort instead of locking on a bad rate

[KONRAD Frederic]

This helps the board developer by asserting that system_clock_rate is not
null. Using systick with a zero rate will lead to a deadlock so better showing
the error.

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
---
 hw/timer/armv7m_systick.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/timer/armv7m_systick.c b/hw/timer/armv7m_systick.c
index df8d280..745efb7 100644
--- a/hw/timer/armv7m_systick.c
+++ b/hw/timer/armv7m_systick.c
@@ -54,6 +54,9 @@ static void systick_reload(SysTickState *s, int reset)
         s->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     }
     s->tick += (s->reload + 1) * systick_scale(s);
+
+    /* system_clock_scale = 0 leads to a nasty deadlock, better aborting */
+    assert(systick_scale(s));
     timer_mod(s->timer, s->tick);
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v1 3/3] armv7m_systick: abort instead of locking on a bad rate

[Philippe Mathieu-Daudé]

Hi Frederic,

On 06/29/2017 06:28 AM, KONRAD Frederic wrote:
> This helps the board developer by asserting that system_clock_rate is not
> null. Using systick with a zero rate will lead to a deadlock so better showing
> the error.
> 
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> ---
>   hw/timer/armv7m_systick.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/hw/timer/armv7m_systick.c b/hw/timer/armv7m_systick.c
> index df8d280..745efb7 100644
> --- a/hw/timer/armv7m_systick.c
> +++ b/hw/timer/armv7m_systick.c
> @@ -54,6 +54,9 @@ static void systick_reload(SysTickState *s, int reset)
>           s->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>       }
>       s->tick += (s->reload + 1) * systick_scale(s);
> +
> +    /* system_clock_scale = 0 leads to a nasty deadlock, better aborting */
> +    assert(systick_scale(s));
>       timer_mod(s->timer, s->tick);
>   }

This is true it is better to abort here than risking a deadlock.

However it seems to me they are 3 issues here:
- the deadlock pattern is caused by using a global variable,
- stellaris:ssys_calculate_system_clock() no checking RCC.SYSDIV and 
RCC2.SYSDIV2 values <= 2 are reserved (GUEST_ERROR)
- stellaris:ssys_write(RCC2) not overrides correctly RCC

I'd rather take this opportunity to fix the deadlock pattern using a 
getter/setter on system_clock_scale, doing the zero check in the setter 
and eventually aborting in the getter, what do you think?

Regards,

Phil.

Re: [Qemu-devel] [PATCH v1 3/3] armv7m_systick: abort instead of locking on a bad rate

[Peter Maydell]

On 29 June 2017 at 13:35, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> This is true it is better to abort here than risking a deadlock.
>
> However it seems to me they are 3 issues here:
> - the deadlock pattern is caused by using a global variable,
> - stellaris:ssys_calculate_system_clock() no checking RCC.SYSDIV and
> RCC2.SYSDIV2 values <= 2 are reserved (GUEST_ERROR)
> - stellaris:ssys_write(RCC2) not overrides correctly RCC

Stellaris works fine. It's other ARMv7M boards (which might forget
to set system_clock_scale) which don't work.

> I'd rather take this opportunity to fix the deadlock pattern using a
> getter/setter on system_clock_scale, doing the zero check in the setter and
> eventually aborting in the getter, what do you think?

We should be using a clock property on the CPU instead of system_clock_scale.
Unfortunately Konrad's series attempting to add that infrastructure
is still in the "trying to sort out the right API in code review"
stage. I don't think it's worth trying to fiddle with the API
for it before we have the right eventual infrastructure in place.

(What system_clock_scale is actually doing is setting the
emulated frequency of the CPU, since that affects the
frequency of the timer.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 3/3] armv7m_systick: abort instead of locking on a bad rate

[KONRAD Frederic]

On 06/29/2017 02:43 PM, Peter Maydell wrote:
> On 29 June 2017 at 13:35, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> This is true it is better to abort here than risking a deadlock.
>>
>> However it seems to me they are 3 issues here:
>> - the deadlock pattern is caused by using a global variable,
>> - stellaris:ssys_calculate_system_clock() no checking RCC.SYSDIV and
>> RCC2.SYSDIV2 values <= 2 are reserved (GUEST_ERROR)
>> - stellaris:ssys_write(RCC2) not overrides correctly RCC
> 
> Stellaris works fine. It's other ARMv7M boards (which might forget
> to set system_clock_scale) which don't work.

Yes actually the issue is with new boards when you forgot to set
system_clock_scale (as it happened to me :)).

Peter you suggest we fix that later when the clock is in place?

Fred

> 
>> I'd rather take this opportunity to fix the deadlock pattern using a
>> getter/setter on system_clock_scale, doing the zero check in the setter and
>> eventually aborting in the getter, what do you think?
> 
> We should be using a clock property on the CPU instead of system_clock_scale.
> Unfortunately Konrad's series attempting to add that infrastructure
> is still in the "trying to sort out the right API in code review"
> stage. I don't think it's worth trying to fiddle with the API
> for it before we have the right eventual infrastructure in place.
> 
> (What system_clock_scale is actually doing is setting the
> emulated frequency of the CPU, since that affects the
> frequency of the timer.)
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH v1 3/3] armv7m_systick: abort instead of locking on a bad rate

[Philippe Mathieu-Daudé]

On 06/29/2017 09:43 AM, Peter Maydell wrote:
> On 29 June 2017 at 13:35, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> This is true it is better to abort here than risking a deadlock.
>>
>> However it seems to me they are 3 issues here:
>> - the deadlock pattern is caused by using a global variable,
>> - stellaris:ssys_calculate_system_clock() no checking RCC.SYSDIV and
>> RCC2.SYSDIV2 values <= 2 are reserved (GUEST_ERROR)
>> - stellaris:ssys_write(RCC2) not overrides correctly RCC
> 
> Stellaris works fine. It's other ARMv7M boards (which might forget
> to set system_clock_scale) which don't work.

Oh I misread ssys_calculate_system_clock(), indeed system_clock_scale 
can not get below 5 so no deadlock on Stellaris.

>> I'd rather take this opportunity to fix the deadlock pattern using a
>> getter/setter on system_clock_scale, doing the zero check in the setter and
>> eventually aborting in the getter, what do you think?
> 
> We should be using a clock property on the CPU instead of system_clock_scale.
> Unfortunately Konrad's series attempting to add that infrastructure
> is still in the "trying to sort out the right API in code review"
> stage. I don't think it's worth trying to fiddle with the API
> for it before we have the right eventual infrastructure in place.

I see. I'd rather move the comment and assert() in systick_scale().

> (What system_clock_scale is actually doing is setting the
> emulated frequency of the CPU, since that affects the
> frequency of the timer.)

Re: [Qemu-devel] [PATCH v1 3/3] armv7m_systick: abort instead of locking on a bad rate

[KONRAD Frederic]

On 06/29/2017 03:02 PM, Philippe Mathieu-Daudé wrote:
> On 06/29/2017 09:43 AM, Peter Maydell wrote:
>> On 29 June 2017 at 13:35, Philippe Mathieu-Daudé 
>> <f4bug@amsat.org> wrote:
>>> This is true it is better to abort here than risking a deadlock.
>>>
>>> However it seems to me they are 3 issues here:
>>> - the deadlock pattern is caused by using a global variable,
>>> - stellaris:ssys_calculate_system_clock() no checking 
>>> RCC.SYSDIV and
>>> RCC2.SYSDIV2 values <= 2 are reserved (GUEST_ERROR)
>>> - stellaris:ssys_write(RCC2) not overrides correctly RCC
>>
>> Stellaris works fine. It's other ARMv7M boards (which might forget
>> to set system_clock_scale) which don't work.
> 
> Oh I misread ssys_calculate_system_clock(), indeed 
> system_clock_scale can not get below 5 so no deadlock on Stellaris.
> 
>>> I'd rather take this opportunity to fix the deadlock pattern 
>>> using a
>>> getter/setter on system_clock_scale, doing the zero check in 
>>> the setter and
>>> eventually aborting in the getter, what do you think?
>>
>> We should be using a clock property on the CPU instead of 
>> system_clock_scale.
>> Unfortunately Konrad's series attempting to add that 
>> infrastructure
>> is still in the "trying to sort out the right API in code review"
>> stage. I don't think it's worth trying to fiddle with the API
>> for it before we have the right eventual infrastructure in place.
> 
> I see. I'd rather move the comment and assert() in systick_scale().

Makes sense, I missed the potential divide-by-zero exception
which might happen in SysTick Current Value:

val = ((s->tick - (t + 1)) / systick_scale(s)) + 1;

I'll do the change and re-post a V2 when there will be some input
on the two first patches.

Thanks,
Fred

> 
>> (What system_clock_scale is actually doing is setting the
>> emulated frequency of the CPU, since that affects the
>> frequency of the timer.)
> 

Re: [Qemu-devel] [PATCH v1 0/3] Some armv7m fixes

[no-reply]

Hi,

This series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.

Subject: [Qemu-devel] [PATCH v1 0/3] Some armv7m fixes
Message-id: 1498728533-23160-1-git-send-email-frederic.konrad@adacore.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
time make docker-test-quick@centos6
time make docker-test-build@min-glib
time make docker-test-mingw@fedora
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
fatal: Cannot update paths and switch to branch 'test' at the same time.
Did you intend to checkout 'origin/patchew/1498728533-23160-1-git-send-email-frederic.konrad@adacore.com' which can not be resolved as commit?
Traceback (most recent call last):
  File "/home/fam/bin/patchew", line 440, in test_one
    git_clone_repo(clone, r["repo"], r["head"], logf)
  File "/home/fam/bin/patchew", line 53, in git_clone_repo
    cwd=clone)
  File "/usr/lib64/python3.5/subprocess.py", line 271, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'checkout', 'origin/patchew/1498728533-23160-1-git-send-email-frederic.konrad@adacore.com', '-b', 'test']' returned non-zero exit status 128



---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v1 0/3] Some armv7m fixes

[Fam Zheng]

On Thu, 07/06 16:49, no-reply@patchew.org wrote:
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> fatal: Cannot update paths and switch to branch 'test' at the same time.
> Did you intend to checkout 'origin/patchew/1498728533-23160-1-git-send-email-frederic.konrad@adacore.com' which can not be resolved as commit?
> Traceback (most recent call last):
>   File "/home/fam/bin/patchew", line 440, in test_one
>     git_clone_repo(clone, r["repo"], r["head"], logf)
>   File "/home/fam/bin/patchew", line 53, in git_clone_repo
>     cwd=clone)
>   File "/usr/lib64/python3.5/subprocess.py", line 271, in check_call
>     raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['git', 'checkout', 'origin/patchew/1498728533-23160-1-git-send-email-frederic.konrad@adacore.com', '-b', 'test']' returned non-zero exit status 128

Ignore this please, patchew is recovering from a bad state.