On Thu, Aug 17, 2023 at 12:30:19AM +0000, Raghavendra Rao Ananta wrote:
> From: Reiji Watanabe <reijiw@xxxxxxxxxx>
>
> The following patches will use the number of counters information
> from the arm_pmu and use this to set the PMCR.N for the guest
> during vCPU reset. However, since the guest is not associated
> with any arm_pmu until userspace configures the vPMU device
> attributes, and a reset can happen before this event, call
> kvm_arm_support_pmu_v3() just before doing the reset.
>
> No functional change intended.
But there absolutely is a functional change here, and user visible at
that. KVM_ARM_VCPU_INIT ioctls can now fail with -ENODEV, which is not
part of the documented errors for the interface.
> Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
> Signed-off-by: Raghavendra Rao Ananta <rananta@xxxxxxxxxx>
> ---
> arch/arm64/kvm/pmu-emul.c | 9 +--------
> arch/arm64/kvm/reset.c | 18 +++++++++++++-----
> include/kvm/arm_pmu.h | 6 ++++++
> 3 files changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 0ffd1efa90c07..b87822024828a 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -865,7 +865,7 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
> return true;
> }
>
> -static int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
> +int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
> {
> lockdep_assert_held(&kvm->arch.config_lock);
>
> @@ -937,13 +937,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> if (vcpu->arch.pmu.created)
> return -EBUSY;
>
> - if (!kvm->arch.arm_pmu) {
> - int ret = kvm_arm_set_vm_pmu(kvm, NULL);
> -
> - if (ret)
> - return ret;
> - }
> -
> switch (attr->attr) {
> case KVM_ARM_VCPU_PMU_V3_IRQ: {
> int __user *uaddr = (int __user *)(long)attr->addr;
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index bc8556b6f4590..4c20f1ccd0789 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -206,6 +206,7 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
> */
> int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> {
> + struct kvm *kvm = vcpu->kvm;
> struct vcpu_reset_state reset_state;
> int ret;
> bool loaded;
> @@ -216,6 +217,18 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> vcpu->arch.reset_state.reset = false;
> spin_unlock(&vcpu->arch.mp_state_lock);
>
> + /*
> + * When the vCPU has a PMU, but no PMU is set for the guest
> + * yet, set the default one.
> + */
> + if (kvm_vcpu_has_pmu(vcpu) && unlikely(!kvm->arch.arm_pmu)) {
> + ret = -EINVAL;
> + if (kvm_arm_support_pmu_v3())
> + ret = kvm_arm_set_vm_pmu(kvm, NULL);
> + if (ret)
> + return ret;
> + }
> +
On top of my prior suggestion w.r.t. the default PMU helper, I'd rather
see this block look like:
if (kvm_vcpu_has_pmu(vcpu)) {
if (!kvm_arm_support_pmu_v3())
return -EINVAL;
/*
* When the vCPU has a PMU but no PMU is set for the
* guest yet, set the default one.
*/
if (unlikely(!kvm->arch.arm_pmu) && kvm_set_default_pmu(kvm))
return -EINVAL;
}
This would eliminate the possibility of returning ENODEV to userspace
where we shouldn't.
--
Thanks,
Oliver
