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

From: KONRAD Frederic <frederic.konrad@adacore.com>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
Cc: "Peter Maydell" <peter.maydell@linaro.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	alistair@alistair23.me, "Alex Bennée" <alex.bennee@linaro.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH v1 3/3] armv7m_systick: abort instead of locking on a bad rate
Date: Thu, 29 Jun 2017 15:17:37 +0200	[thread overview]
Message-ID: <d4df2047-0058-d0ae-6d4c-48bf8c408f2c@adacore.com> (raw)
In-Reply-To: <95e817f5-904e-83fd-eadb-2ab00ebddd9a@amsat.org>

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.)
> 