Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220518132512.37864-1-likexu@tencent.com> <20220518132512.37864-4-likexu@tencent.com>
 <31f3de9a-a752-e322-ebd0-731c42afd47a@redhat.com>
In-Reply-To: <31f3de9a-a752-e322-ebd0-731c42afd47a@redhat.com>
From:   Jim Mattson <jmattson@google.com>
Date:   Fri, 20 May 2022 06:00:35 -0700
Message-ID: <CALMp9eSp5d3mq2GaT+AhDSXaZ=HN0SUSnW5VGY+o4NHLz=VkSA@mail.gmail.com>
Subject: Re: [PATCH RESEND v3 03/11] KVM: x86/pmu: Protect kvm->arch.pmu_event_filter
 with SRCU
To:     Paolo Bonzini <pbonzini@redhat.com>
Cc:     Like Xu <like.xu.linux@gmail.com>,
        Sean Christopherson <seanjc@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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, May 20, 2022 at 5:51 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> 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

I agree with Paolo that existing usage is covered by
srcu_read_lock/unlock, but (a) it's not easy to confirm this, and (b)
this is very fragile.

Whichever way we decide to go, the userspace MSR filter and the PMU
event filter should adopt the same approach.