On 7 February 2017 at 16:52, Alex Bennée <alex.ben...@linaro.org> wrote:
>
> Peter Maydell <peter.mayd...@linaro.org> 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, ¤t_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
