| Message ID | 20231020214053.2144305-7-rananta@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: PMU: Allow userspace to limit the number of PMCs on vCPU | expand |
On Fri, 20 Oct 2023 22:40:46 +0100, Raghavendra Rao Ananta <rananta@google.com> wrote: > > For unimplemented counters, the registers PM{C,I}NTEN{SET,CLR} > and PMOVS{SET,CLR} are expected to have the corresponding bits RAZ. > Hence to ensure correct KVM's PMU emulation, mask out the bits in > these registers for these unimplemented counters before the first > vCPU run. > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > --- > arch/arm64/kvm/arm.c | 2 +- > arch/arm64/kvm/pmu-emul.c | 11 +++++++++++ > include/kvm/arm_pmu.h | 2 ++ > 3 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index e3074a9e23a8b..3c0bb80483fb1 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -857,7 +857,7 @@ static int check_vcpu_requests(struct kvm_vcpu *vcpu) > } > > if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu)) > - kvm_pmu_handle_pmcr(vcpu, kvm_vcpu_read_pmcr(vcpu)); > + kvm_vcpu_handle_request_reload_pmu(vcpu); Please rename this to kvm_vcpu_reload_pmu(). That's long enough. But see below. > > if (kvm_check_request(KVM_REQ_RESYNC_PMU_EL0, vcpu)) > kvm_vcpu_pmu_restore_guest(vcpu); > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > index 9e24581206c24..31e4933293b76 100644 > --- a/arch/arm64/kvm/pmu-emul.c > +++ b/arch/arm64/kvm/pmu-emul.c > @@ -788,6 +788,17 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) > return val & mask; > } > > +void kvm_vcpu_handle_request_reload_pmu(struct kvm_vcpu *vcpu) > +{ > + u64 mask = kvm_pmu_valid_counter_mask(vcpu); > + > + kvm_pmu_handle_pmcr(vcpu, kvm_vcpu_read_pmcr(vcpu)); > + > + __vcpu_sys_reg(vcpu, PMOVSSET_EL0) &= mask; > + __vcpu_sys_reg(vcpu, PMINTENSET_EL1) &= mask; > + __vcpu_sys_reg(vcpu, PMCNTENSET_EL0) &= mask; > +} Why is this done on a vcpu request? Why can't it be done upfront, when we're requesting the reload? Or when assigning the PMU? Or when setting PMCR_EL0? M.
On Mon, Oct 23, 2023 at 5:42 AM Marc Zyngier <maz@kernel.org> wrote: > > On Fri, 20 Oct 2023 22:40:46 +0100, > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > For unimplemented counters, the registers PM{C,I}NTEN{SET,CLR} > > and PMOVS{SET,CLR} are expected to have the corresponding bits RAZ. > > Hence to ensure correct KVM's PMU emulation, mask out the bits in > > these registers for these unimplemented counters before the first > > vCPU run. > > > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > > --- > > arch/arm64/kvm/arm.c | 2 +- > > arch/arm64/kvm/pmu-emul.c | 11 +++++++++++ > > include/kvm/arm_pmu.h | 2 ++ > > 3 files changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > > index e3074a9e23a8b..3c0bb80483fb1 100644 > > --- a/arch/arm64/kvm/arm.c > > +++ b/arch/arm64/kvm/arm.c > > @@ -857,7 +857,7 @@ static int check_vcpu_requests(struct kvm_vcpu *vcpu) > > } > > > > if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu)) > > - kvm_pmu_handle_pmcr(vcpu, kvm_vcpu_read_pmcr(vcpu)); > > + kvm_vcpu_handle_request_reload_pmu(vcpu); > > Please rename this to kvm_vcpu_reload_pmu(). That's long enough. But > see below. > Sounds good. > > > > if (kvm_check_request(KVM_REQ_RESYNC_PMU_EL0, vcpu)) > > kvm_vcpu_pmu_restore_guest(vcpu); > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > > index 9e24581206c24..31e4933293b76 100644 > > --- a/arch/arm64/kvm/pmu-emul.c > > +++ b/arch/arm64/kvm/pmu-emul.c > > @@ -788,6 +788,17 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) > > return val & mask; > > } > > > > +void kvm_vcpu_handle_request_reload_pmu(struct kvm_vcpu *vcpu) > > +{ > > + u64 mask = kvm_pmu_valid_counter_mask(vcpu); > > + > > + kvm_pmu_handle_pmcr(vcpu, kvm_vcpu_read_pmcr(vcpu)); > > + > > + __vcpu_sys_reg(vcpu, PMOVSSET_EL0) &= mask; > > + __vcpu_sys_reg(vcpu, PMINTENSET_EL1) &= mask; > > + __vcpu_sys_reg(vcpu, PMCNTENSET_EL0) &= mask; > > +} > > Why is this done on a vcpu request? Why can't it be done upfront, when > we're requesting the reload? Or when assigning the PMU? Or when > setting PMCR_EL0? > The idea was to do this only once, after userspace has configured the PMCR.N (and has no option to change it), but before we run the guest for the first time. So, I guess this can be done when we are requesting the reload, if you prefer. When assigning the PMU, it could be too early to sanitize as the userspace would not have configured the PMCR.N yet. It can probably be done when userspace configures PMCR.N, but since this field is per-guest, we may have to apply the setting for all the vCPUs during the ioctl, which may get a little ugly. Thank you. Raghavendra
On Mon, 23 Oct 2023 18:42:43 +0100, Raghavendra Rao Ananta <rananta@google.com> wrote: > > On Mon, Oct 23, 2023 at 5:42 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Fri, 20 Oct 2023 22:40:46 +0100, > > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > > > For unimplemented counters, the registers PM{C,I}NTEN{SET,CLR} > > > and PMOVS{SET,CLR} are expected to have the corresponding bits RAZ. > > > Hence to ensure correct KVM's PMU emulation, mask out the bits in > > > these registers for these unimplemented counters before the first > > > vCPU run. > > > > > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > > > --- > > > arch/arm64/kvm/arm.c | 2 +- > > > arch/arm64/kvm/pmu-emul.c | 11 +++++++++++ > > > include/kvm/arm_pmu.h | 2 ++ > > > 3 files changed, 14 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > > > index e3074a9e23a8b..3c0bb80483fb1 100644 > > > --- a/arch/arm64/kvm/arm.c > > > +++ b/arch/arm64/kvm/arm.c > > > @@ -857,7 +857,7 @@ static int check_vcpu_requests(struct kvm_vcpu *vcpu) > > > } > > > > > > if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu)) > > > - kvm_pmu_handle_pmcr(vcpu, kvm_vcpu_read_pmcr(vcpu)); > > > + kvm_vcpu_handle_request_reload_pmu(vcpu); > > > > Please rename this to kvm_vcpu_reload_pmu(). That's long enough. But > > see below. > > > Sounds good. > > > > > > > if (kvm_check_request(KVM_REQ_RESYNC_PMU_EL0, vcpu)) > > > kvm_vcpu_pmu_restore_guest(vcpu); > > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > > > index 9e24581206c24..31e4933293b76 100644 > > > --- a/arch/arm64/kvm/pmu-emul.c > > > +++ b/arch/arm64/kvm/pmu-emul.c > > > @@ -788,6 +788,17 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) > > > return val & mask; > > > } > > > > > > +void kvm_vcpu_handle_request_reload_pmu(struct kvm_vcpu *vcpu) > > > +{ > > > + u64 mask = kvm_pmu_valid_counter_mask(vcpu); > > > + > > > + kvm_pmu_handle_pmcr(vcpu, kvm_vcpu_read_pmcr(vcpu)); > > > + > > > + __vcpu_sys_reg(vcpu, PMOVSSET_EL0) &= mask; > > > + __vcpu_sys_reg(vcpu, PMINTENSET_EL1) &= mask; > > > + __vcpu_sys_reg(vcpu, PMCNTENSET_EL0) &= mask; > > > +} > > > > Why is this done on a vcpu request? Why can't it be done upfront, when > > we're requesting the reload? Or when assigning the PMU? Or when > > setting PMCR_EL0? > > > The idea was to do this only once, after userspace has configured the > PMCR.N (and has no option to change it), but before we run the guest > for the first time. So, I guess this can be done when we are > requesting the reload, if you prefer. Well, I'm trying to limit the proliferation of these one-off "helpers" that make the code hard to follow. So it isn't "what I prefer", but what makes the code easier to understand without having to follow a maze of pointless abstraction. > When assigning the PMU, it could be too early to sanitize as the > userspace would not have configured the PMCR.N yet. > It can probably be done when userspace configures PMCR.N, but since > this field is per-guest, we may have to apply the setting for all the > vCPUs during the ioctl, which may get a little ugly. Right. So it has to happen at the point where userspace cannot write to PMCR_EL0 anymore, for which any of the options I mentioned is too early. What you have is thus correct. But it would have helped if that rationale was captured in the commit message. M.
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index e3074a9e23a8b..3c0bb80483fb1 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -857,7 +857,7 @@ static int check_vcpu_requests(struct kvm_vcpu *vcpu) } if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu)) - kvm_pmu_handle_pmcr(vcpu, kvm_vcpu_read_pmcr(vcpu)); + kvm_vcpu_handle_request_reload_pmu(vcpu); if (kvm_check_request(KVM_REQ_RESYNC_PMU_EL0, vcpu)) kvm_vcpu_pmu_restore_guest(vcpu); diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c index 9e24581206c24..31e4933293b76 100644 --- a/arch/arm64/kvm/pmu-emul.c +++ b/arch/arm64/kvm/pmu-emul.c @@ -788,6 +788,17 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) return val & mask; } +void kvm_vcpu_handle_request_reload_pmu(struct kvm_vcpu *vcpu) +{ + u64 mask = kvm_pmu_valid_counter_mask(vcpu); + + kvm_pmu_handle_pmcr(vcpu, kvm_vcpu_read_pmcr(vcpu)); + + __vcpu_sys_reg(vcpu, PMOVSSET_EL0) &= mask; + __vcpu_sys_reg(vcpu, PMINTENSET_EL1) &= mask; + __vcpu_sys_reg(vcpu, PMCNTENSET_EL0) &= mask; +} + int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu) { if (!kvm_vcpu_has_pmu(vcpu)) diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h index 2e90f38090e6d..567dc288a5ddb 100644 --- a/include/kvm/arm_pmu.h +++ b/include/kvm/arm_pmu.h @@ -63,6 +63,7 @@ void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val); void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val); void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data, u64 select_idx); +void kvm_vcpu_handle_request_reload_pmu(struct kvm_vcpu *vcpu); int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr); int kvm_arm_pmu_v3_get_attr(struct kvm_vcpu *vcpu, @@ -142,6 +143,7 @@ static inline void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val) {} static inline void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val) {} static inline void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data, u64 select_idx) {} +static inline void vm_vcpu_handle_request_reload_pmu(struct kvm_vcpu *vcpu) {} static inline int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr) {
For unimplemented counters, the registers PM{C,I}NTEN{SET,CLR} and PMOVS{SET,CLR} are expected to have the corresponding bits RAZ. Hence to ensure correct KVM's PMU emulation, mask out the bits in these registers for these unimplemented counters before the first vCPU run. Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> --- arch/arm64/kvm/arm.c | 2 +- arch/arm64/kvm/pmu-emul.c | 11 +++++++++++ include/kvm/arm_pmu.h | 2 ++ 3 files changed, 14 insertions(+), 1 deletion(-)