Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <31f3de9a-a752-e322-ebd0-731c42afd47a@redhat.com>
Date:   Fri, 20 May 2022 14:51:06 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH RESEND v3 03/11] KVM: x86/pmu: Protect
 kvm->arch.pmu_event_filter with SRCU
Content-Language: en-US
To:     Like Xu <like.xu.linux@gmail.com>
Cc:     Sean Christopherson <seanjc@google.com>,
        Jim Mattson <jmattson@google.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Joerg Roedel <joro@8bytes.org>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
References: <20220518132512.37864-1-likexu@tencent.com>
 <20220518132512.37864-4-likexu@tencent.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
In-Reply-To: <20220518132512.37864-4-likexu@tencent.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/18/22 15:25, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> Similar to "kvm->arch.msr_filter", KVM should guarantee that vCPUs will
> see either the previous filter or the new filter when user space calls
> KVM_SET_PMU_EVENT_FILTER ioctl with the vCPU running so that guest
> pmu events with identical settings in both the old and new filter have
> deterministic behavior.
> 
> Fixes: 66bb8a065f5a ("KVM: x86: PMU Event Filter")
> Signed-off-by: Like Xu <likexu@tencent.com>
> Reviewed-by: Wanpeng Li <wanpengli@tencent.com>

Please always include the call trace where SRCU is not taken.  The ones 
I reconstructed always end up at a place inside srcu_read_lock/unlock:

reprogram_gp_counter/reprogram_fixed_counter
   amd_pmu_set_msr
    kvm_set_msr_common
     svm_set_msr
      __kvm_set_msr
      kvm_set_msr_ignored_check
       kvm_set_msr_with_filter
        kvm_emulate_wrmsr**
        emulator_set_msr_with_filter**
       kvm_set_msr
        emulator_set_msr**
       do_set_msr
        __msr_io
         msr_io
          ioctl(KVM_SET_MSRS)**
   intel_pmu_set_msr
    kvm_set_msr_common
     vmx_set_msr (see svm_set_msr)
   reprogram_counter
    global_ctrl_changed
     intel_pmu_set_msr (see above)
    kvm_pmu_handle_event
     vcpu_enter_guest**
    kvm_pmu_incr_counter
     kvm_pmu_trigger_event
      nested_vmx_run**
      kvm_skip_emulated_instruction**
      x86_emulate_instruction**
   reprogram_fixed_counters
    intel_pmu_set_msr (see above)

Paolo

>   arch/x86/kvm/pmu.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index f189512207db..24624654e476 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -246,8 +246,9 @@ static bool check_pmu_event_filter(struct kvm_pmc *pmc)
>   	struct kvm *kvm = pmc->vcpu->kvm;
>   	bool allow_event = true;
>   	__u64 key;
> -	int idx;
> +	int idx, srcu_idx;
>   
> +	srcu_idx = srcu_read_lock(&kvm->srcu);
>   	filter = srcu_dereference(kvm->arch.pmu_event_filter, &kvm->srcu);
>   	if (!filter)
>   		goto out;
> @@ -270,6 +271,7 @@ static bool check_pmu_event_filter(struct kvm_pmc *pmc)
>   	}
>   
>   out:
> +	srcu_read_unlock(&kvm->srcu, srcu_idx);
>   	return allow_event;
>   }
>