Re: [PATCH v2] x86/vpmu: Fix race-condition in vpmu_load

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.09.2022 22:48, Tamas K Lengyel wrote:
> --- a/xen/arch/x86/cpu/vpmu.c
> +++ b/xen/arch/x86/cpu/vpmu.c
> @@ -376,57 +376,24 @@ void vpmu_save(struct vcpu *v)
>      vpmu->last_pcpu = pcpu;
>      per_cpu(last_vcpu, pcpu) = v;
>  
> +    vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
> +
>      if ( alternative_call(vpmu_ops.arch_vpmu_save, v, 0) )
>          vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
>  
> +    vpmu_reset(vpmu, VPMU_CONTEXT_SAVE);
> +
>      apic_write(APIC_LVTPC, PMU_APIC_VECTOR | APIC_LVT_MASKED);
>  }
>  
>  int vpmu_load(struct vcpu *v, bool_t from_guest)
>  {
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    int pcpu = smp_processor_id(), ret;
> -    struct vcpu *prev = NULL;
> +    int ret;
>  
>      if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
>          return 0;
>  
> -    /* First time this VCPU is running here */
> -    if ( vpmu->last_pcpu != pcpu )
> -    {
> -        /*
> -         * Get the context from last pcpu that we ran on. Note that if 
> another
> -         * VCPU is running there it must have saved this VPCU's context 
> before
> -         * startig to run (see below).
> -         * There should be no race since remote pcpu will disable interrupts
> -         * before saving the context.
> -         */
> -        if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
> -        {
> -            on_selected_cpus(cpumask_of(vpmu->last_pcpu),
> -                             vpmu_save_force, (void *)v, 1);
> -            vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
> -        }
> -    } 
> -
> -    /* Prevent forced context save from remote CPU */
> -    local_irq_disable();
> -
> -    prev = per_cpu(last_vcpu, pcpu);
> -
> -    if ( prev != v && prev )
> -    {
> -        vpmu = vcpu_vpmu(prev);
> -
> -        /* Someone ran here before us */
> -        vpmu_save_force(prev);
> -        vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
> -
> -        vpmu = vcpu_vpmu(v);
> -    }
> -
> -    local_irq_enable();
> -
>      /* Only when PMU is counting, we load PMU context immediately. */
>      if ( !vpmu_is_set(vpmu, VPMU_RUNNING) ||
>           (!has_vlapic(vpmu_vcpu(vpmu)->domain) &&

What about the other two uses of vpmu_save_force() in this file? I looks
to me as if only the use in mem_sharing.c needs to be retained.

Also, going forward, please Cc Boris right on new iterations of this fix
(if any; I'm not going to exclude I'm wrong with the above and all is
fine with this version).

Jan