Hi Peter,
On 9/21/21 11:45, Philippe Mathieu-Daudé wrote:
> On 9/21/21 11:34, Peter Maydell wrote:
>> On Mon, 20 Sept 2021 at 22:44, Philippe Mathieu-Daudé
>> <f4bug@amsat.org> wrote:
>>>
>>> Implement SysemuCPUOps::has_work() handler for the ARM v7M CPU.
>>>
>>> See the comments added in commit 7ecdaa4a963 ("armv7m: Fix
>>> condition check for taking exceptions") which eventually
>>> forgot to implement this has_work() handler:
>>
>> Huh? M-profile and A-profile share the same arm_cpu_has_work()
>> function. Some of the checks the code there does are perhaps
>> unnecessary for M-profile, but they're harmless.
A v7M core is registered as (example for Cortex-M0):
static const ARMCPUInfo arm_tcg_cpus[] = {
...
{ .name = "cortex-m0", .initfn = cortex_m0_initfn,
.class_init = arm_v7m_class_init },
static void arm_tcg_cpu_register_types(void)
{
...
for (i = 0; i < ARRAY_SIZE(arm_tcg_cpus); ++i) {
arm_cpu_register(&arm_tcg_cpus[i]);
void arm_cpu_register(const ARMCPUInfo *info)
{
TypeInfo type_info = {
.parent = TYPE_ARM_CPU,
.instance_size = sizeof(ARMCPU),
.instance_align = __alignof__(ARMCPU),
.instance_init = arm_cpu_instance_init,
.class_size = sizeof(ARMCPUClass),
.class_init = info->class_init ?: cpu_register_class_init,
.class_data = (void *)info,
};
Since we provide info->class_init as arm_v7m_class_init(), only
tcg_ops and gdb_core_xml_file from CPUClass are set:
static void arm_v7m_class_init(ObjectClass *oc, void *data)
{
ARMCPUClass *acc = ARM_CPU_CLASS(oc);
CPUClass *cc = CPU_CLASS(oc);
acc->info = data;
#ifdef CONFIG_TCG
cc->tcg_ops = &arm_v7m_tcg_ops;
#endif /* CONFIG_TCG */
cc->gdb_core_xml_file = "arm-m-profile.xml";
}
Thus v7M cores end calling cpu_common_has_work() registered by
cpu_class_init(), which is:
static bool cpu_common_has_work(CPUState *cs)
{
return false;
}
What am I missing?
>>
>>> * ARMv7-M interrupt masking works differently than -A or -R.
>>> * There is no FIQ/IRQ distinction.
>>>
>>> The NVIC signal any pending interrupt by raising ARM_CPU_IRQ
>>> (see commit 56b7c66f498: "armv7m: QOMify the armv7m container")
>>> which ends setting the CPU_INTERRUPT_HARD bit in interrupt_request.
>>>
>>> Thus arm_v7m_cpu_has_work() implementation is thus quite trivial,
>>> we simply need to check for this bit.
>>>
>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>> Cc: Michael Davidsaver <mdavidsaver@gmail.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> target/arm/cpu_tcg.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
>>> index 0d5adccf1a7..da348938407 100644
>>> --- a/target/arm/cpu_tcg.c
>>> +++ b/target/arm/cpu_tcg.c
>>> @@ -23,6 +23,11 @@
>>> #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
>>>
>>> #if !defined(CONFIG_USER_ONLY) && defined(CONFIG_TCG)
>>> +static bool arm_v7m_cpu_has_work(CPUState *cs)
>>> +{
>>> + return cs->interrupt_request & CPU_INTERRUPT_HARD;
>>> +}
>>
>> This seems to be missing at least the check on
>> cpu->power_state and the CPU_INTERRUPT_EXITTB test.
>>
>> Is there any reason why we shouldn't just continue to
>> share the same function between A and M profile, and avoid
>> the extra function and the ifdefs ?
>
> The only reason I can think of is I should have been resting
> instead of posting this patch :/ I'll re-use arm_cpu_has_work()
> which is, as you said, harmless and safer.
>