| Message ID | 20230728181907.1759513-3-reijiw@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: PMU: Fix PMUver related handling for vPMU support | expand |
On Fri, Jul 28, 2023 at 11:19:04AM -0700, Reiji Watanabe wrote: > Disallow userspace from configuring vPMU for guests on systems > where the PMUVer is not uniform across all PEs. > KVM has not been advertising PMUv3 to the guests with vPMU on > such systems anyway, and such systems would be extremely > uncommon and unlikely to even use KVM. This doesn't actually disallow userspace from configuring a vPMU, it only hides the KVM cap. kvm_host_pmu_init() will still insert the host PMU instance in the list of valid PMUs, and there doesn't appear to be any check against the static key anywhere on that path. FWIW, this static key is actually responsible for indicating whether KVM supports context switching the PMU between host/guest. While vPMU obviously depends on that, the perf subsystem also allows the host to program events to count while the guest is running. I actually prefer where we flip the static key, as PMU context switching depends on both KVM support as well as the PMU driver coming up successfully. Instead, you could hoist the check against the sanitised PMU version into kvm_host_pmu_init(), maybe something like: diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c index 560650972478..f6a0e558472f 100644 --- a/arch/arm64/kvm/pmu-emul.c +++ b/arch/arm64/kvm/pmu-emul.c @@ -672,8 +672,11 @@ void kvm_host_pmu_init(struct arm_pmu *pmu) { struct arm_pmu_entry *entry; - if (pmu->pmuver == ID_AA64DFR0_EL1_PMUVer_NI || - pmu->pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF) + /* + * Check the sanitised PMU version for the system, as KVM does not + * support implementations where PMUv3 exists on a subset of CPUs. + */ + if (!pmuv3_implemented(kvm_arm_pmu_get_pmuver_limit())) return; mutex_lock(&arm_pmus_lock);
Hi Oliver, > This doesn't actually disallow userspace from configuring a vPMU, it > only hides the KVM cap. kvm_host_pmu_init() will still insert the host > PMU instance in the list of valid PMUs, and there doesn't appear to be > any check against the static key anywhere on that path. In v6.5-rc3, which I used as the base, or even in v6.5-rc4, it appears kvm_reset_vcpu() checks against the static key. So, the initial KVM_ARM_VCPU_INIT with vPMU configured will fail on the systems. Or am I missing something ? (Or is that going to be removed by other patches that are already queued?) But, right, it still insert the host PMU instance in the list, which is unnecessary. > I actually prefer where we flip the static key, as PMU context switching > depends on both KVM support as well as the PMU driver coming up successfully. > Instead, you could hoist the check against the sanitised PMU version into > kvm_host_pmu_init(), maybe something like: Thank you, it looks better. I will fix this in v3. Thank you, Reiji > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > index 560650972478..f6a0e558472f 100644 > --- a/arch/arm64/kvm/pmu-emul.c > +++ b/arch/arm64/kvm/pmu-emul.c > @@ -672,8 +672,11 @@ void kvm_host_pmu_init(struct arm_pmu *pmu) > { > struct arm_pmu_entry *entry; > > - if (pmu->pmuver == ID_AA64DFR0_EL1_PMUVer_NI || > - pmu->pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF) > + /* > + * Check the sanitised PMU version for the system, as KVM does not > + * support implementations where PMUv3 exists on a subset of CPUs. > + */ > + if (!pmuv3_implemented(kvm_arm_pmu_get_pmuver_limit())) > return; > > mutex_lock(&arm_pmus_lock); > > -- > Thanks, > Oliver
On Mon, Jul 31, 2023 at 04:54:04AM -0700, Reiji Watanabe wrote: > Hi Oliver, > > > This doesn't actually disallow userspace from configuring a vPMU, it > > only hides the KVM cap. kvm_host_pmu_init() will still insert the host > > PMU instance in the list of valid PMUs, and there doesn't appear to be > > any check against the static key anywhere on that path. > > In v6.5-rc3, which I used as the base, or even in v6.5-rc4, > it appears kvm_reset_vcpu() checks against the static key. > So, the initial KVM_ARM_VCPU_INIT with vPMU configured will > fail on the systems. Or am I missing something ? (Or is that > going to be removed by other patches that are already queued?) No, I definitely missed something, sorry for the noise! I had only checked the PMU attributes, forgot about feature flags for a moment. > > I actually prefer where we flip the static key, as PMU context switching > > depends on both KVM support as well as the PMU driver coming up successfully. > > Instead, you could hoist the check against the sanitised PMU version into > > kvm_host_pmu_init(), maybe something like: > > Thank you, it looks better. I will fix this in v3. Sounds good, thanks! -- Best, Oliver
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 72dc53a75d1c..1d410dea21ac 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -2053,6 +2053,7 @@ static int __init init_subsystems(void) goto out; kvm_register_perf_callbacks(NULL); + kvm_pmu_init(); out: if (err) diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c index dee83119e112..6fb5c59948a8 100644 --- a/arch/arm64/kvm/pmu-emul.c +++ b/arch/arm64/kvm/pmu-emul.c @@ -685,9 +685,6 @@ void kvm_host_pmu_init(struct arm_pmu *pmu) entry->arm_pmu = pmu; list_add_tail(&entry->entry, &arm_pmus); - if (list_is_singular(&arm_pmus)) - static_branch_enable(&kvm_arm_pmu_available); - out_unlock: mutex_unlock(&arm_pmus_lock); } @@ -1057,3 +1054,11 @@ u8 kvm_arm_pmu_get_pmuver_limit(void) ID_AA64DFR0_EL1_PMUVer_V3P5); return FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), tmp); } + +void kvm_pmu_init(void) +{ + u8 pmuver = kvm_arm_pmu_get_pmuver_limit(); + + if (pmuv3_implemented(pmuver)) + static_branch_enable(&kvm_arm_pmu_available); +} diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h index 847da6fc2713..9cf50e16305a 100644 --- a/include/kvm/arm_pmu.h +++ b/include/kvm/arm_pmu.h @@ -74,6 +74,7 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu); struct kvm_pmu_events *kvm_get_pmu_events(void); void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu); void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu); +void kvm_pmu_init(void); #define kvm_vcpu_has_pmu(vcpu) \ (test_bit(KVM_ARM_VCPU_PMU_V3, (vcpu)->arch.features)) @@ -110,6 +111,8 @@ static inline bool kvm_arm_support_pmu_v3(void) return false; } +static inline void kvm_pmu_init(void) {}; + #define kvm_arm_pmu_irq_initialized(v) (false) static inline u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx)
Disallow userspace from configuring vPMU for guests on systems where the PMUVer is not uniform across all PEs. KVM has not been advertising PMUv3 to the guests with vPMU on such systems anyway, and such systems would be extremely uncommon and unlikely to even use KVM. Signed-off-by: Reiji Watanabe <reijiw@google.com> --- arch/arm64/kvm/arm.c | 1 + arch/arm64/kvm/pmu-emul.c | 11 ++++++++--- include/kvm/arm_pmu.h | 3 +++ 3 files changed, 12 insertions(+), 3 deletions(-)