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

[Peter Maydell]

On 9 February 2017 at 17:09, Alex Bennée <address@hidden> wrote:
> When switching a new vCPU on we want to complete a bunch of the setup
> work before we start scheduling the vCPU thread. To do this cleanly we
> defer vCPU setup to async work which will run the vCPUs execution
> context as the thread is woken up. The scheduling of the work will kick
> the vCPU awake.
>
> This avoids potential races in MTTCG system emulation.
>
> Signed-off-by: Alex Bennée <address@hidden>
> Reviewed-by: Richard Henderson <address@hidden>
>
> ---
> v7
>   - add const to static mode_for_el[] array
>   - fix checkpatch long lines
> v10
>   - use async work for arm_set_cpu_off, arm_reset_cpu as well
>   - model ON_PENDING to deal with racing arm_set_cpu_on
> v11
>   - move to single cpu->power_state protected by BQL
>   - bump migration format
> ---
>  target/arm/arm-powerctl.c | 202 
> +++++++++++++++++++++++++++++++---------------
>  target/arm/arm-powerctl.h |   2 +
>  target/arm/cpu.c          |   4 +-
>  target/arm/cpu.h          |  13 ++-
>  target/arm/kvm.c          |   7 +-
>  target/arm/machine.c      |   6 +-
>  target/arm/psci.c         |  16 +++-
>  7 files changed, 174 insertions(+), 76 deletions(-)
>
> diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c
> index fbb7a15daa..3b4c51978c 100644
> --- a/target/arm/arm-powerctl.c
> +++ b/target/arm/arm-powerctl.c
> @@ -14,6 +14,7 @@
>  #include "internals.h"
>  #include "arm-powerctl.h"
>  #include "qemu/log.h"
> +#include "qemu/main-loop.h"
>  #include "exec/exec-all.h"
>
>  #ifndef DEBUG_ARM_POWERCTL
> @@ -48,11 +49,93 @@ CPUState *arm_get_cpu_by_id(uint64_t id)
>      return NULL;
>  }
>
> +struct cpu_on_info {

Since you'll need to respin this anyway, our coding
style likes CamelCase for struct names.

> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index fa5ec76090..15619a0430 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -286,8 +286,8 @@ static int cpu_post_load(void *opaque, int version_id)
>
>  const VMStateDescription vmstate_arm_cpu = {
>      .name = "cpu",
> -    .version_id = 22,
> -    .minimum_version_id = 22,
> +    .version_id = 23,
> +    .minimum_version_id = 23,

Sorry, you can't do this, it will break migration. You need
to do something cleverer with a subsection or other trick.

David Gilbert's recent patch on list to docs/migration.txt
should have some examples of how to do this.

>      .pre_save = cpu_pre_save,
>      .post_load = cpu_post_load,
>      .fields = (VMStateField[]) {
> @@ -329,7 +329,7 @@ const VMStateDescription vmstate_arm_cpu = {
>          VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
>          VMSTATE_TIMER_PTR(gt_timer[GTIMER_PHYS], ARMCPU),
>          VMSTATE_TIMER_PTR(gt_timer[GTIMER_VIRT], ARMCPU),
> -        VMSTATE_BOOL(powered_off, ARMCPU),
> +        VMSTATE_UINT32(power_state, ARMCPU),
>          VMSTATE_END_OF_LIST()
>      },
>      .subsections = (const VMStateDescription*[]) {

I think the rest is OK.

thanks
-- PMM