Re: [PATCH v4 10/27] accel/mshv: Add vCPU creation and execution loop

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Sep 16, 2025 at 06:48:30PM +0200, Magnus Kulke wrote:
> Create MSHV vCPUs using MSHV_CREATE_VP and initialize their state.
> Register the MSHV CPU execution loop loop with the QEMU accelerator
> framework to enable guest code execution.
> 
> The target/i386 functionality is still mostly stubbed out and will be
> populated in a later commit in this series.
> 
> Signed-off-by: Magnus Kulke <magnuskulke@linux.microsoft.com>
> ---
>  accel/mshv/mshv-all.c       | 188 +++++++++++++++++++++++++++++++++---
>  accel/mshv/trace-events     |   2 +
>  include/system/mshv.h       |  17 ++++
>  target/i386/mshv/mshv-cpu.c |  64 ++++++++++++
>  4 files changed, 259 insertions(+), 12 deletions(-)
> 
> diff --git a/accel/mshv/mshv-all.c b/accel/mshv/mshv-all.c
> index 63f2ed5fa1..b49988d294 100644
> --- a/accel/mshv/mshv-all.c
> +++ b/accel/mshv/mshv-all.c
> @@ -392,6 +392,24 @@ int mshv_hvcall(int fd, const struct mshv_root_hvcall *args)
>      return ret;
>  }
>  
> +static int mshv_init_vcpu(CPUState *cpu)
> +{
> +    int vm_fd = mshv_state->vm;
> +    uint8_t vp_index = cpu->cpu_index;
> +    int ret;
> +
> +    mshv_arch_init_vcpu(cpu);
> +    cpu->accel = g_new0(AccelCPUState, 1);
> +
> +    ret = mshv_create_vcpu(vm_fd, vp_index, &cpu->accel->cpufd);
> +    if (ret < 0) {
> +        return -1;
> +    }
> +
> +    cpu->accel->dirty = true;
> +
> +    return 0;
> +}
>  
>  static int mshv_init(AccelState *as, MachineState *ms)
>  {
> @@ -414,6 +432,8 @@ static int mshv_init(AccelState *as, MachineState *ms)
>          return -1;
>      }
>  
> +    mshv_init_mmio_emu();
> +
>      mshv_init_msicontrol();
>  
>      ret = create_vm(mshv_fd, &vm_fd);
> @@ -443,40 +463,184 @@ static int mshv_init(AccelState *as, MachineState *ms)
>      return 0;
>  }
>  
> +static int mshv_destroy_vcpu(CPUState *cpu)
> +{
> +    int cpu_fd = mshv_vcpufd(cpu);
> +    int vm_fd = mshv_state->vm;
> +
> +    mshv_remove_vcpu(vm_fd, cpu_fd);
> +    mshv_vcpufd(cpu) = 0;
> +
> +    mshv_arch_destroy_vcpu(cpu);
> +    g_free(cpu->accel);
> +    g_clear_pointer(&cpu->accel, g_free);

This is a double-free.

   g_clear_pointer(&cpu->accel, g_free);

is equivalent to

   g_free(cpu->accel)
   cpu->accel = NULL;

So you don't need the earlier 'g_free(cpu->accel)'
call


> +void mshv_arch_destroy_vcpu(CPUState *cpu)
> +{
> +    X86CPU *x86_cpu = X86_CPU(cpu);
> +    CPUX86State *env = &x86_cpu->env;
> +
> +    g_free(env->emu_mmio_buf);
> +    env->emu_mmio_buf = NULL;

These two lines could be replaced with

  g_clear_pointer(&env->emu_mmio_buf, g_free);


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|