Re: [PATCH v4 06/11] hvf: Simplify post reset/init/loadvm hooks

From: Roman Bolshakov <r.bolshakov@yadro.com>
To: Alexander Graf <agraf@csgraf.de>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Eduardo Habkost <ehabkost@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	qemu-devel@nongnu.org, Cameron Esfahani <dirty@apple.com>,
	qemu-arm@nongnu.org, Frank Yang <lfy@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Peter Collingbourne <pcc@google.com>
Subject: Re: [PATCH v4 06/11] hvf: Simplify post reset/init/loadvm hooks
Date: Fri, 4 Dec 2020 19:07:56 +0300	[thread overview]
Message-ID: <20201204160756.GE86904@SPB-NB-133.local> (raw)
In-Reply-To: <20201203234857.21051-7-agraf@csgraf.de>

On Fri, Dec 04, 2020 at 12:48:52AM +0100, Alexander Graf wrote:
> The hooks we have that call us after reset, init and loadvm really all
> just want to say "The reference of all register state is in the QEMU
> vcpu struct, please push it".
> 
> We already have a working pushing mechanism though called cpu->vcpu_dirty,
> so we can just reuse that for all of the above, syncing state properly the
> next time we actually execute a vCPU.
> 
> This fixes PSCI resets on ARM, as they modify CPU state even after the
> post init call has completed, but before we execute the vCPU again.
> 
> To also make the scheme work for x86, we have to make sure we don't
> move stale eflags into our env when the vcpu state is dirty.
> 
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> ---
>  accel/hvf/hvf-cpus.c     | 27 +++++++--------------------
>  target/i386/hvf/x86hvf.c |  5 ++++-
>  2 files changed, 11 insertions(+), 21 deletions(-)
> 
> diff --git a/accel/hvf/hvf-cpus.c b/accel/hvf/hvf-cpus.c
> index 1b0c868944..71721e17de 100644
> --- a/accel/hvf/hvf-cpus.c
> +++ b/accel/hvf/hvf-cpus.c
> @@ -275,39 +275,26 @@ static void hvf_cpu_synchronize_state(CPUState *cpu)
>      }
>  }
>  
> -static void do_hvf_cpu_synchronize_post_reset(CPUState *cpu,
> -                                              run_on_cpu_data arg)
> +static void do_hvf_cpu_synchronize_set_dirty(CPUState *cpu,
> +                                             run_on_cpu_data arg)
>  {
> -    hvf_put_registers(cpu);
> -    cpu->vcpu_dirty = false;
> +    /* QEMU state is the reference, push it to HVF now and on next entry */

It's only signalling now. The actual push is delayed until the next
entry.

It'd be good if Paolo or Eduardo would also peek at this change because
it makes HVF a bit different from other accels.

HVF's post_reset, post_init and pre_loadvm no longer result into QEMU
state being pushed to HVF. I'm not sure I can fully grasp if there're
undesired side-effects of this so it's something worth broader review.

If nobody raises objections:

Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Tested-by: Roman Bolshakov <r.bolshakov@yadro.com>

Thanks,
Roman

> +    cpu->vcpu_dirty = true;
>  }
>  
>  static void hvf_cpu_synchronize_post_reset(CPUState *cpu)
>  {
> -    run_on_cpu(cpu, do_hvf_cpu_synchronize_post_reset, RUN_ON_CPU_NULL);
> -}
> -
> -static void do_hvf_cpu_synchronize_post_init(CPUState *cpu,
> -                                             run_on_cpu_data arg)
> -{
> -    hvf_put_registers(cpu);
> -    cpu->vcpu_dirty = false;
> +    run_on_cpu(cpu, do_hvf_cpu_synchronize_set_dirty, RUN_ON_CPU_NULL);
>  }
>  
>  static void hvf_cpu_synchronize_post_init(CPUState *cpu)
>  {
> -    run_on_cpu(cpu, do_hvf_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
> -}
> -
> -static void do_hvf_cpu_synchronize_pre_loadvm(CPUState *cpu,
> -                                              run_on_cpu_data arg)
> -{
> -    cpu->vcpu_dirty = true;
> +    run_on_cpu(cpu, do_hvf_cpu_synchronize_set_dirty, RUN_ON_CPU_NULL);
>  }
>  
>  static void hvf_cpu_synchronize_pre_loadvm(CPUState *cpu)
>  {
> -    run_on_cpu(cpu, do_hvf_cpu_synchronize_pre_loadvm, RUN_ON_CPU_NULL);
> +    run_on_cpu(cpu, do_hvf_cpu_synchronize_set_dirty, RUN_ON_CPU_NULL);
>  }
>  
>  static void hvf_vcpu_destroy(CPUState *cpu)
> diff --git a/target/i386/hvf/x86hvf.c b/target/i386/hvf/x86hvf.c
> index 0f2aeb1cf8..3111c0be4c 100644
> --- a/target/i386/hvf/x86hvf.c
> +++ b/target/i386/hvf/x86hvf.c
> @@ -435,7 +435,10 @@ int hvf_process_events(CPUState *cpu_state)
>      X86CPU *cpu = X86_CPU(cpu_state);
>      CPUX86State *env = &cpu->env;
>  
> -    env->eflags = rreg(cpu_state->hvf->fd, HV_X86_RFLAGS);
> +    if (!cpu_state->vcpu_dirty) {
> +        /* light weight sync for CPU_INTERRUPT_HARD and IF_MASK */
> +        env->eflags = rreg(cpu_state->hvf->fd, HV_X86_RFLAGS);
> +    }
>  
>      if (cpu_state->interrupt_request & CPU_INTERRUPT_INIT) {
>          cpu_synchronize_state(cpu_state);
> -- 
> 2.24.3 (Apple Git-128)
> 