Acquire SRCU in the VM-Exit fastpath if and only if KVM needs to check the
PMU event filter, to further trim the amount of code that is executed with
SRCU protection in the fastpath. Counter-intuitively, holding SRCU can do
more harm than good due to masking potential bugs, and introducing a new
SRCU-protected asset to code reachable via kvm_skip_emulated_instruction()
would be quite notable, i.e. definitely worth auditing.
E.g. the primary user of kvm->srcu is KVM's memslots, accessing memslots
all but guarantees guest memory may be accessed, accessing guest memory
can fault, and page faults might sleep, which isn't allowed while IRQs are
disabled. Not acquiring SRCU means the (hypothetical) illegal sleep would
be flagged when running with PROVE_RCU=y, even if DEBUG_ATOMIC_SLEEP=n.
Note, performance is NOT a motivating factor, as SRCU lock/unlock only
adds ~15 cycles of latency to fastpath VM-Exits. I.e. overhead isn't a
concern _if_ SRCU protection needs to be extended beyond PMU events, e.g.
to honor userspace MSR filters.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/pmu.c | 4 +++-
arch/x86/kvm/x86.c | 18 +++++-------------
2 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index e75671b6e88c..3206412a35a1 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -955,7 +955,7 @@ static void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu,
DECLARE_BITMAP(bitmap, X86_PMC_IDX_MAX);
struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
struct kvm_pmc *pmc;
- int i;
+ int i, idx;
BUILD_BUG_ON(sizeof(pmu->global_ctrl) * BITS_PER_BYTE != X86_PMC_IDX_MAX);
@@ -968,12 +968,14 @@ static void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu,
(unsigned long *)&pmu->global_ctrl, X86_PMC_IDX_MAX))
return;
+ idx = srcu_read_lock(&vcpu->kvm->srcu);
kvm_for_each_pmc(pmu, pmc, i, bitmap) {
if (!pmc_is_event_allowed(pmc) || !cpl_is_matched(pmc))
continue;
kvm_pmu_incr_counter(pmc);
}
+ srcu_read_unlock(&vcpu->kvm->srcu, idx);
}
void kvm_pmu_instruction_retired(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f2b2eaaec6f8..a56f83b40a55 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2137,7 +2137,6 @@ fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
{
u64 data = kvm_read_edx_eax(vcpu);
u32 msr = kvm_rcx_read(vcpu);
- int r;
switch (msr) {
case APIC_BASE_MSR + (APIC_ICR >> 4):
@@ -2152,13 +2151,12 @@ fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
return EXIT_FASTPATH_NONE;
}
- kvm_vcpu_srcu_read_lock(vcpu);
- r = kvm_skip_emulated_instruction(vcpu);
- kvm_vcpu_srcu_read_unlock(vcpu);
-
trace_kvm_msr_write(msr, data);
- return r ? EXIT_FASTPATH_REENTER_GUEST : EXIT_FASTPATH_EXIT_USERSPACE;
+ if (!kvm_skip_emulated_instruction(vcpu))
+ return EXIT_FASTPATH_EXIT_USERSPACE;
+
+ return EXIT_FASTPATH_REENTER_GUEST;
}
EXPORT_SYMBOL_GPL(handle_fastpath_set_msr_irqoff);
@@ -11251,13 +11249,7 @@ EXPORT_SYMBOL_GPL(kvm_emulate_halt);
fastpath_t handle_fastpath_hlt(struct kvm_vcpu *vcpu)
{
- int ret;
-
- kvm_vcpu_srcu_read_lock(vcpu);
- ret = kvm_emulate_halt(vcpu);
- kvm_vcpu_srcu_read_unlock(vcpu);
-
- if (!ret)
+ if (!kvm_emulate_halt(vcpu))
return EXIT_FASTPATH_EXIT_USERSPACE;
if (kvm_vcpu_running(vcpu))
--
2.50.1.565.gc32cd1483b-goog
On 8/6/2025 3:05 AM, Sean Christopherson wrote:
> Acquire SRCU in the VM-Exit fastpath if and only if KVM needs to check the
> PMU event filter, to further trim the amount of code that is executed with
> SRCU protection in the fastpath. Counter-intuitively, holding SRCU can do
> more harm than good due to masking potential bugs, and introducing a new
> SRCU-protected asset to code reachable via kvm_skip_emulated_instruction()
> would be quite notable, i.e. definitely worth auditing.
>
> E.g. the primary user of kvm->srcu is KVM's memslots, accessing memslots
> all but guarantees guest memory may be accessed, accessing guest memory
> can fault, and page faults might sleep, which isn't allowed while IRQs are
> disabled. Not acquiring SRCU means the (hypothetical) illegal sleep would
> be flagged when running with PROVE_RCU=y, even if DEBUG_ATOMIC_SLEEP=n.
>
> Note, performance is NOT a motivating factor, as SRCU lock/unlock only
> adds ~15 cycles of latency to fastpath VM-Exits. I.e. overhead isn't a
> concern _if_ SRCU protection needs to be extended beyond PMU events, e.g.
> to honor userspace MSR filters.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> arch/x86/kvm/pmu.c | 4 +++-
> arch/x86/kvm/x86.c | 18 +++++-------------
> 2 files changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index e75671b6e88c..3206412a35a1 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -955,7 +955,7 @@ static void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu,
> DECLARE_BITMAP(bitmap, X86_PMC_IDX_MAX);
> struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> struct kvm_pmc *pmc;
> - int i;
> + int i, idx;
>
> BUILD_BUG_ON(sizeof(pmu->global_ctrl) * BITS_PER_BYTE != X86_PMC_IDX_MAX);
>
> @@ -968,12 +968,14 @@ static void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu,
> (unsigned long *)&pmu->global_ctrl, X86_PMC_IDX_MAX))
> return;
>
> + idx = srcu_read_lock(&vcpu->kvm->srcu);
It looks the asset what "kvm->srcu" protects here is
kvm->arch.pmu_event_filter which is only read by pmc_is_event_allowed().
Besides here, pmc_is_event_allowed() is called by reprogram_counter() but
without srcu_read_lock()/srcu_read_unlock() protection.
So should we shrink the protection range further and move the
srcu_read_lock()/srcu_read_unlock() pair into pmc_is_event_allowed()
helper? The side effect is it would bring some extra overhead since
srcu_read_lock()/srcu_read_unlock() could be called multiple times. An
alternative could be to add srcu_read_lock()/srcu_read_unlock() around
pmc_is_event_allowed() in reprogram_counter() helper as well.
The other part looks good to me.
> kvm_for_each_pmc(pmu, pmc, i, bitmap) {
> if (!pmc_is_event_allowed(pmc) || !cpl_is_matched(pmc))
> continue;
>
> kvm_pmu_incr_counter(pmc);
> }
> + srcu_read_unlock(&vcpu->kvm->srcu, idx);
> }
>
> void kvm_pmu_instruction_retired(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index f2b2eaaec6f8..a56f83b40a55 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2137,7 +2137,6 @@ fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
> {
> u64 data = kvm_read_edx_eax(vcpu);
> u32 msr = kvm_rcx_read(vcpu);
> - int r;
>
> switch (msr) {
> case APIC_BASE_MSR + (APIC_ICR >> 4):
> @@ -2152,13 +2151,12 @@ fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
> return EXIT_FASTPATH_NONE;
> }
>
> - kvm_vcpu_srcu_read_lock(vcpu);
> - r = kvm_skip_emulated_instruction(vcpu);
> - kvm_vcpu_srcu_read_unlock(vcpu);
> -
> trace_kvm_msr_write(msr, data);
>
> - return r ? EXIT_FASTPATH_REENTER_GUEST : EXIT_FASTPATH_EXIT_USERSPACE;
> + if (!kvm_skip_emulated_instruction(vcpu))
> + return EXIT_FASTPATH_EXIT_USERSPACE;
> +
> + return EXIT_FASTPATH_REENTER_GUEST;
> }
> EXPORT_SYMBOL_GPL(handle_fastpath_set_msr_irqoff);
>
> @@ -11251,13 +11249,7 @@ EXPORT_SYMBOL_GPL(kvm_emulate_halt);
>
> fastpath_t handle_fastpath_hlt(struct kvm_vcpu *vcpu)
> {
> - int ret;
> -
> - kvm_vcpu_srcu_read_lock(vcpu);
> - ret = kvm_emulate_halt(vcpu);
> - kvm_vcpu_srcu_read_unlock(vcpu);
> -
> - if (!ret)
> + if (!kvm_emulate_halt(vcpu))
> return EXIT_FASTPATH_EXIT_USERSPACE;
>
> if (kvm_vcpu_running(vcpu))
On Wed, Aug 06, 2025, Dapeng Mi wrote: > > On 8/6/2025 3:05 AM, Sean Christopherson wrote: > > Acquire SRCU in the VM-Exit fastpath if and only if KVM needs to check the > > PMU event filter, to further trim the amount of code that is executed with > > SRCU protection in the fastpath. Counter-intuitively, holding SRCU can do > > more harm than good due to masking potential bugs, and introducing a new > > SRCU-protected asset to code reachable via kvm_skip_emulated_instruction() > > would be quite notable, i.e. definitely worth auditing. > > > > E.g. the primary user of kvm->srcu is KVM's memslots, accessing memslots > > all but guarantees guest memory may be accessed, accessing guest memory > > can fault, and page faults might sleep, which isn't allowed while IRQs are > > disabled. Not acquiring SRCU means the (hypothetical) illegal sleep would > > be flagged when running with PROVE_RCU=y, even if DEBUG_ATOMIC_SLEEP=n. > > > > Note, performance is NOT a motivating factor, as SRCU lock/unlock only > > adds ~15 cycles of latency to fastpath VM-Exits. I.e. overhead isn't a > > concern _if_ SRCU protection needs to be extended beyond PMU events, e.g. > > to honor userspace MSR filters. > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- ... > > @@ -968,12 +968,14 @@ static void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu, > > (unsigned long *)&pmu->global_ctrl, X86_PMC_IDX_MAX)) > > return; > > > > + idx = srcu_read_lock(&vcpu->kvm->srcu); > > It looks the asset what "kvm->srcu" protects here is > kvm->arch.pmu_event_filter which is only read by pmc_is_event_allowed(). > Besides here, pmc_is_event_allowed() is called by reprogram_counter() but > without srcu_read_lock()/srcu_read_unlock() protection. No, reprogram_counter() is only called called in the context of KVM_RUN, i.e. with the vCPU loaded and thus with kvm->srcu already head for read (acquired by kvm_arch_vcpu_ioctl_run()). > So should we shrink the protection range further and move the > srcu_read_lock()/srcu_read_unlock() pair into pmc_is_event_allowed() > helper? The side effect is it would bring some extra overhead since > srcu_read_lock()/srcu_read_unlock() could be called multiple times. No, I don't think it's worth getting that precise. As you note, there will be extra overhead, and it could actually become non-trivial amount of overhead, albeit in a somewhat pathological scenario. And cpl_is_matched() is easy to audit, i.e. is very low risk with respect to having "bad" behavior that's hidden by virtue of holding SRCU. E.g. if the guest is using all general purpose PMCs to count instructions retired, then KVM would acquire/release SRCU 8+ times. On Intel, the fastpath can run in <800 cycles. Adding 8 * 2 full memory barriers (difficult to measure, but somewhere in the neighborhood of ~10 cycles per barrier) would increase the latency by 10-20%. Again, that's an extreme scenario, but since there's almost nothing to gain from pushing SRCU acquisition into the filter checks, I don't see any reason to go with an approach that we *know* is sub-optimal. > An alternative could be to add srcu_read_lock()/srcu_read_unlock() around > pmc_is_event_allowed() in reprogram_counter() helper as well. As above, there's no need to modify reprogram_counter(). I don't see any future where reprogram_counter() would be safe to call in the fastpath, there's simply too much going on, i.e. I think reprogram_counter() will always be a non-issue.
On 8/7/2025 1:33 AM, Sean Christopherson wrote: > On Wed, Aug 06, 2025, Dapeng Mi wrote: >> On 8/6/2025 3:05 AM, Sean Christopherson wrote: >>> Acquire SRCU in the VM-Exit fastpath if and only if KVM needs to check the >>> PMU event filter, to further trim the amount of code that is executed with >>> SRCU protection in the fastpath. Counter-intuitively, holding SRCU can do >>> more harm than good due to masking potential bugs, and introducing a new >>> SRCU-protected asset to code reachable via kvm_skip_emulated_instruction() >>> would be quite notable, i.e. definitely worth auditing. >>> >>> E.g. the primary user of kvm->srcu is KVM's memslots, accessing memslots >>> all but guarantees guest memory may be accessed, accessing guest memory >>> can fault, and page faults might sleep, which isn't allowed while IRQs are >>> disabled. Not acquiring SRCU means the (hypothetical) illegal sleep would >>> be flagged when running with PROVE_RCU=y, even if DEBUG_ATOMIC_SLEEP=n. >>> >>> Note, performance is NOT a motivating factor, as SRCU lock/unlock only >>> adds ~15 cycles of latency to fastpath VM-Exits. I.e. overhead isn't a >>> concern _if_ SRCU protection needs to be extended beyond PMU events, e.g. >>> to honor userspace MSR filters. >>> >>> Signed-off-by: Sean Christopherson <seanjc@google.com> >>> --- > ... > >>> @@ -968,12 +968,14 @@ static void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu, >>> (unsigned long *)&pmu->global_ctrl, X86_PMC_IDX_MAX)) >>> return; >>> >>> + idx = srcu_read_lock(&vcpu->kvm->srcu); >> It looks the asset what "kvm->srcu" protects here is >> kvm->arch.pmu_event_filter which is only read by pmc_is_event_allowed(). >> Besides here, pmc_is_event_allowed() is called by reprogram_counter() but >> without srcu_read_lock()/srcu_read_unlock() protection. > No, reprogram_counter() is only called called in the context of KVM_RUN, i.e. with > the vCPU loaded and thus with kvm->srcu already head for read (acquired by > kvm_arch_vcpu_ioctl_run()). Not sure if I understand correctly, but KVM_SET_PMU_EVENT_FILTER ioctl is a VM-level ioctl and it can be set when vCPUs are running. So assume KVM_SET_PMU_EVENT_FILTER ioctl is called at vCPU0 and vCPU1 is running reprogram_counter(). Is it safe without srcu_read_lock()/srcu_read_unlock() protection? > >> So should we shrink the protection range further and move the >> srcu_read_lock()/srcu_read_unlock() pair into pmc_is_event_allowed() >> helper? The side effect is it would bring some extra overhead since >> srcu_read_lock()/srcu_read_unlock() could be called multiple times. > No, I don't think it's worth getting that precise. As you note, there will be > extra overhead, and it could actually become non-trivial amount of overhead, > albeit in a somewhat pathological scenario. And cpl_is_matched() is easy to > audit, i.e. is very low risk with respect to having "bad" behavior that's hidden > by virtue of holding SRCU. > > E.g. if the guest is using all general purpose PMCs to count instructions > retired, then KVM would acquire/release SRCU 8+ times. On Intel, the fastpath > can run in <800 cycles. Adding 8 * 2 full memory barriers (difficult to measure, > but somewhere in the neighborhood of ~10 cycles per barrier) would increase the > latency by 10-20%. > > Again, that's an extreme scenario, but since there's almost nothing to gain from > pushing SRCU acquisition into the filter checks, I don't see any reason to go > with an approach that we *know* is sub-optimal. Yeah, indeed. If there is no need to add srcu_read_lock()/srcu_read_unlock() protection in reprogram_counter(), I'm good with this. Thanks. > >> An alternative could be to add srcu_read_lock()/srcu_read_unlock() around >> pmc_is_event_allowed() in reprogram_counter() helper as well. > As above, there's no need to modify reprogram_counter(). I don't see any future > where reprogram_counter() would be safe to call in the fastpath, there's simply > too much going on, i.e. I think reprogram_counter() will always be a non-issue.
On Thu, Aug 07, 2025, Dapeng Mi wrote:
>
> On 8/7/2025 1:33 AM, Sean Christopherson wrote:
> > On Wed, Aug 06, 2025, Dapeng Mi wrote:
> >> On 8/6/2025 3:05 AM, Sean Christopherson wrote:
> >>> Acquire SRCU in the VM-Exit fastpath if and only if KVM needs to check the
> >>> PMU event filter, to further trim the amount of code that is executed with
> >>> SRCU protection in the fastpath. Counter-intuitively, holding SRCU can do
> >>> more harm than good due to masking potential bugs, and introducing a new
> >>> SRCU-protected asset to code reachable via kvm_skip_emulated_instruction()
> >>> would be quite notable, i.e. definitely worth auditing.
> >>>
> >>> E.g. the primary user of kvm->srcu is KVM's memslots, accessing memslots
> >>> all but guarantees guest memory may be accessed, accessing guest memory
> >>> can fault, and page faults might sleep, which isn't allowed while IRQs are
> >>> disabled. Not acquiring SRCU means the (hypothetical) illegal sleep would
> >>> be flagged when running with PROVE_RCU=y, even if DEBUG_ATOMIC_SLEEP=n.
> >>>
> >>> Note, performance is NOT a motivating factor, as SRCU lock/unlock only
> >>> adds ~15 cycles of latency to fastpath VM-Exits. I.e. overhead isn't a
> >>> concern _if_ SRCU protection needs to be extended beyond PMU events, e.g.
> >>> to honor userspace MSR filters.
> >>>
> >>> Signed-off-by: Sean Christopherson <seanjc@google.com>
> >>> ---
> > ...
> >
> >>> @@ -968,12 +968,14 @@ static void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu,
> >>> (unsigned long *)&pmu->global_ctrl, X86_PMC_IDX_MAX))
> >>> return;
> >>>
> >>> + idx = srcu_read_lock(&vcpu->kvm->srcu);
> >> It looks the asset what "kvm->srcu" protects here is
> >> kvm->arch.pmu_event_filter which is only read by pmc_is_event_allowed().
> >> Besides here, pmc_is_event_allowed() is called by reprogram_counter() but
> >> without srcu_read_lock()/srcu_read_unlock() protection.
> > No, reprogram_counter() is only called called in the context of KVM_RUN, i.e. with
> > the vCPU loaded and thus with kvm->srcu already head for read (acquired by
> > kvm_arch_vcpu_ioctl_run()).
>
> Not sure if I understand correctly, but KVM_SET_PMU_EVENT_FILTER ioctl is a
> VM-level ioctl and it can be set when vCPUs are running. So assume
> KVM_SET_PMU_EVENT_FILTER ioctl is called at vCPU0 and vCPU1 is running
> reprogram_counter(). Is it safe without srcu_read_lock()/srcu_read_unlock()
> protection?
No, but reprogram_counter() can be reached if and only if the CPU holds SRCU.
kvm_arch_vcpu_ioctl_run() => kvm_vcpu_srcu_read_lock(vcpu);
|
-> vcpu_run()
|
-> vcpu_enter_guest()
|
-> kvm_pmu_handle_event()
|
-> reprogram_counter()
On 8/7/2025 9:31 PM, Sean Christopherson wrote: > On Thu, Aug 07, 2025, Dapeng Mi wrote: >> On 8/7/2025 1:33 AM, Sean Christopherson wrote: >>> On Wed, Aug 06, 2025, Dapeng Mi wrote: >>>> On 8/6/2025 3:05 AM, Sean Christopherson wrote: >>>>> Acquire SRCU in the VM-Exit fastpath if and only if KVM needs to check the >>>>> PMU event filter, to further trim the amount of code that is executed with >>>>> SRCU protection in the fastpath. Counter-intuitively, holding SRCU can do >>>>> more harm than good due to masking potential bugs, and introducing a new >>>>> SRCU-protected asset to code reachable via kvm_skip_emulated_instruction() >>>>> would be quite notable, i.e. definitely worth auditing. >>>>> >>>>> E.g. the primary user of kvm->srcu is KVM's memslots, accessing memslots >>>>> all but guarantees guest memory may be accessed, accessing guest memory >>>>> can fault, and page faults might sleep, which isn't allowed while IRQs are >>>>> disabled. Not acquiring SRCU means the (hypothetical) illegal sleep would >>>>> be flagged when running with PROVE_RCU=y, even if DEBUG_ATOMIC_SLEEP=n. >>>>> >>>>> Note, performance is NOT a motivating factor, as SRCU lock/unlock only >>>>> adds ~15 cycles of latency to fastpath VM-Exits. I.e. overhead isn't a >>>>> concern _if_ SRCU protection needs to be extended beyond PMU events, e.g. >>>>> to honor userspace MSR filters. >>>>> >>>>> Signed-off-by: Sean Christopherson <seanjc@google.com> >>>>> --- >>> ... >>> >>>>> @@ -968,12 +968,14 @@ static void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu, >>>>> (unsigned long *)&pmu->global_ctrl, X86_PMC_IDX_MAX)) >>>>> return; >>>>> >>>>> + idx = srcu_read_lock(&vcpu->kvm->srcu); >>>> It looks the asset what "kvm->srcu" protects here is >>>> kvm->arch.pmu_event_filter which is only read by pmc_is_event_allowed(). >>>> Besides here, pmc_is_event_allowed() is called by reprogram_counter() but >>>> without srcu_read_lock()/srcu_read_unlock() protection. >>> No, reprogram_counter() is only called called in the context of KVM_RUN, i.e. with >>> the vCPU loaded and thus with kvm->srcu already head for read (acquired by >>> kvm_arch_vcpu_ioctl_run()). >> Not sure if I understand correctly, but KVM_SET_PMU_EVENT_FILTER ioctl is a >> VM-level ioctl and it can be set when vCPUs are running. So assume >> KVM_SET_PMU_EVENT_FILTER ioctl is called at vCPU0 and vCPU1 is running >> reprogram_counter(). Is it safe without srcu_read_lock()/srcu_read_unlock() >> protection? > No, but reprogram_counter() can be reached if and only if the CPU holds SRCU. > > kvm_arch_vcpu_ioctl_run() => kvm_vcpu_srcu_read_lock(vcpu); > | > -> vcpu_run() > | > -> vcpu_enter_guest() > | > -> kvm_pmu_handle_event() > | > -> reprogram_counter() oh, yes. I missed it. Thanks for explaining. Reviewed-by: Dapeng Mi <dapeng1.mi@linux.intel.com> >
© 2016 - 2025 Red Hat, Inc.