Re: [Qemu-arm] [PATCH v10 20/23] target-arm/powerctl: defer cpu reset work to CPU context

[Peter Maydell]

On 7 February 2017 at 16:52, Alex Bennée <address@hidden> wrote:
>
> Peter Maydell <address@hidden> writes:
>> I find this too hard to reason about :-(  We store the current
>> CPU state in not one but two CPU structure fields, and then
>> we check and manipulate it not by doing "take lock; access
>> and update; drop lock" but by doing successive atomic
>> operations on the various different flags. This seems unnecessarily
>> tricky, and the patch doesn't provide any documentation about
>> what's going on and what the constraints are to avoid races.
>
> I was trying to avoid changing the powered_off state to a new variable
> (maybe pcsi_power_state?) because it meant I didn't have to touch the
> kvm code that treated powered_off as a simple bool. However I can unify
> the state into one variable if you wish.

Changing it also makes migration compatibility awkward.
(Speaking of which, your new flag needs a migration subsection
and appropriate needed function to only transfer it if it's set.)

> As for locking do we have a handy per-CPU lock already or would it be
> enough to use the BQL for this? Although that may be problematic as the
> cpu_reset/on can come from two places (the initial realization and the
> runtime switching of power state.)

Nobody's going to put powering CPUs up and down in their critical
path, so I think the BQL would be fine.

>>> -    /* Start the new CPU at the requested address */
>>> -    cpu_set_pc(target_cpu_state, entry);
>>> +    /* To avoid racing with a CPU we are just kicking off we do the
>>> +     * final bit of preparation for the work in the target CPUs
>>> +     * context.
>>> +     */
>>> +    info = g_new(struct cpu_on_info, 1);
>>> +    info->entry = entry;
>>> +    info->context_id = context_id;
>>> +    info->target_el = target_el;
>>> +    info->target_aa64 = target_aa64;
>>>
>>> -    qemu_cpu_kick(target_cpu_state);
>>> +    async_run_on_cpu(target_cpu_state, arm_set_cpu_on_async_work,
>>> +                     RUN_ON_CPU_HOST_PTR(info));
>>>
>>>      /* We are good to go */
>>>      return QEMU_ARM_POWERCTL_RET_SUCCESS;
>>>  }
>>
>>> @@ -221,8 +284,9 @@ int arm_reset_cpu(uint64_t cpuid)
>>>          return QEMU_ARM_POWERCTL_IS_OFF;
>>>      }
>>>
>>> -    /* Reset the cpu */
>>> -    cpu_reset(target_cpu_state);
>>> +    /* Queue work to run under the target vCPUs context */
>>> +    async_run_on_cpu(target_cpu_state, arm_reset_cpu_async_work,
>>> +                     RUN_ON_CPU_NULL);
>>
>> The imx6 datasheet says that the RST bit in the SCR register
>> is set by the guest to trigger a reset, and then the bit
>> remains set until the reset has finished (at which point it
>> self-clears). At the moment the imx6_src.c code does this:
>>
>>         if (EXTRACT(change_mask, CORE0_RST)) {
>>             arm_reset_cpu(0);
>>             clear_bit(CORE0_RST_SHIFT, &current_value);
>>         }
>>
>> ie it assumes that arm_reset_cpu() is synchronous.
>
> Well for this case the imx6 could queue the "clear_bit" function as
> async work and then it will be correctly reported after the reset has
> occurred.

It could, but as of this patchset it doesn't...

thanks
-- PMM