Re: [PATCH v1 3/3] RISC-V: KVM: Fix perf-backed counter accounting across stop and read

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Starting an already-started counter overwrites its internal value before failing, causing subsequent reads to return corrupt data.
- [High] Hardware counters configured with `SBI_PMU_CFG_FLAG_AUTO_START` are not marked as started, preventing them from ever being successfully stopped.
- [High] Reading high 32-bit PMU CSRs (like CSR_CYCLEH) in a 32-bit RISC-V KVM guest results in an illegal instruction trap, breaking hardware performance monitoring on RV32.
- [Medium] The variable `sbiret` accumulates the error state across iterations of the counter mask, causing subsequent valid counters to fail snapshotting if an earlier counter was already stopped.
--

--- Patch [3]: [PATCH v1 3/3] RISC-V: KVM: Fix perf-backed counter accounting across stop and read ---
commit 4a25612420d054353a1af5f2be56663ddf614797
Author: SeungJu Cheon <[email protected]>

RISC-V: KVM: Fix perf-backed counter accounting across stop and read

pmu_ctr_read() adds the event count returned by perf_event_read_value()
to counter_val, which can accumulate the same count repeatedly across
reads. kvm_riscv_vcpu_pmu_ctr_stop() also leaves counter_val stale by
not folding the current event count into it.

Make reads of perf-backed counters side-effect free, and use
perf_event_pause() when stopping a counter to fold the current event
count into counter_val while resetting it. This preserves the counter
value across stop/start and lets the snapshot path use counter_val
directly.

> diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> index 45dd5233340dc..7d50490cc6222 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c
> @@ -268,12 +268,13 @@ static int pmu_ctr_read(struct kvm_vcpu *vcpu, unsigned long cidx,
>  			return -EINVAL;
>  
>  		pmc->counter_val = kvpmu->fw_event[fevent_code].value;
> +		*out_val = pmc->counter_val;
>  	} else if (pmc->perf_event) {
> -		pmc->counter_val += perf_event_read_value(pmc->perf_event, &enabled, &running);
> +		*out_val = pmc->counter_val +
> +			   perf_event_read_value(pmc->perf_event, &enabled, &running);
>  	} else {

[Severity: High]
This isn't a bug introduced by this patch, but does reading high 32-bit
PMU CSRs in a 32-bit RISC-V KVM guest result in an illegal instruction trap?

If an RV32 guest executes a CSR read instruction on a high PMU CSR (like
CSR_CYCLEH), kvm_riscv_vcpu_pmu_read_hpm() calculates the index:

arch/riscv/kvm/vcpu_pmu.c:kvm_riscv_vcpu_pmu_read_hpm() {
    ...
    cidx = csr_num - CSR_CYCLE;

    if (pmu_ctr_read(vcpu, cidx, val) < 0)
        return KVM_INSN_ILLEGAL_TRAP;
    ...
}

For high CSRs, which are offset by 0x80 from the base CSRs on RV32, cidx
would be out of bounds. Will this cause the bounds check at the beginning
of pmu_ctr_read() to fail and reject the high CSR read, breaking hardware
performance monitoring entirely on RV32?

>  		return -EINVAL;
>  	}
> -	*out_val = pmc->counter_val;
>  
>  	return 0;
>  }

[ ... ]

> @@ -679,8 +679,11 @@ int kvm_riscv_vcpu_pmu_ctr_stop(struct kvm_vcpu *vcpu, unsigned long ctr_base,
>  			}
>  		} else if (pmc->perf_event) {
>  			if (pmc->started) {
> -				/* Stop counting the counter */
> -				perf_event_disable(pmc->perf_event);
> +				/*
> +				 * Stop the counter and fold the live count into counter_val.
> +				 * Reset the event value to avoid redundant accumulation.
> +				 */
> +				pmc->counter_val += perf_event_pause(pmc->perf_event, true);
>  				pmc->started = false;
>  			} else {
>  				sbiret = SBI_ERR_ALREADY_STOPPED;
>  			}

[Severity: High]
This is a pre-existing issue, but does pmc->started accurately track the
running state for counters configured with auto-start?

In kvm_pmu_create_perf_event(), the perf event is enabled for AUTO_START
without updating the tracking state pmc->started:

arch/riscv/kvm/vcpu_pmu.c:kvm_pmu_create_perf_event() {
    ...
    if (flags & SBI_PMU_CFG_FLAG_AUTO_START)
        perf_event_enable(pmc->perf_event);
    ...
}

When the guest later tries to stop the counter here in
kvm_riscv_vcpu_pmu_ctr_stop(), will it hit this !pmc->started branch,
skip calling perf_event_pause(), and leak the running state of the counter
on the host?

> @@ -694,9 +697,6 @@ int kvm_riscv_vcpu_pmu_ctr_stop(struct kvm_vcpu *vcpu, unsigned long ctr_base,
>  		}
>  
>  		if (snap_flag_set && !sbiret) {
> -			if (pmc->perf_event)
> -				pmc->counter_val += perf_event_read_value(pmc->perf_event,
> -									  &enabled, &running);

[Severity: Medium]
This is a pre-existing issue, but does sbiret accumulate the error state
across iterations of the counter mask?

If a guest VM calls SBI PMU stop with a counter mask containing multiple
counters, and an earlier counter is already stopped, sbiret is set to
SBI_ERR_ALREADY_STOPPED and never reset.

Will subsequent running counters be successfully stopped, but have their
snapshot update bypassed in shared memory because sbiret remains non-zero?

>  			/*
>  			 * The counter and overflow indices in the snapshot region are w.r.to
>  			 * cbase. Modify the set bit in the counter mask instead of the pmc_index


While looking at this area, I also noticed another pre-existing issue related
to starting counters:

[Severity: High]
This is a pre-existing issue, but does starting an already-started counter
overwrite its internal value before failing?

arch/riscv/kvm/vcpu_pmu.c:kvm_riscv_vcpu_pmu_ctr_start() {
    ...
    if (flags & SBI_PMU_START_FLAG_SET_INIT_VALUE) {
        pmc->counter_val = ival;
    }
    ...
    if (unlikely(pmc->started)) {
        sbiret = SBI_ERR_ALREADY_STARTED;
        continue;
    }
    ...
}

If a guest VM calls SBI PMU start on an already-started counter with the
initialization flag set, pmc->counter_val is overwritten unconditionally
before the pmc->started check aborts the operation.

Could this cause subsequent reads to return corrupt data, allowing a guest
OS to corrupt the internal state machine of its own PMU counters?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3