Re: [PATCH v8 02/13] KVM: arm64: PMU: Set the default PMU for the guest before vCPU reset

[Marc Zyngier]

On Fri, 20 Oct 2023 22:40:42 +0100,
Raghavendra Rao Ananta <rananta@xxxxxxxxxx> 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, assign a
> default PMU to the guest just before doing the reset.
> 
> Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
> Signed-off-by: Raghavendra Rao Ananta <rananta@xxxxxxxxxx>
> ---
>  arch/arm64/kvm/arm.c      | 19 +++++++++++++++++++
>  arch/arm64/kvm/pmu-emul.c | 16 ++++------------
>  include/kvm/arm_pmu.h     |  6 ++++++
>  3 files changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index c6cad400490f9..08c2f76983b9d 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1319,6 +1319,21 @@ static bool kvm_vcpu_init_changed(struct kvm_vcpu *vcpu,
>  			     KVM_VCPU_MAX_FEATURES);
>  }
>  
> +static int kvm_setup_vcpu(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +
> +	/*
> +	 * 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) && !kvm->arch.arm_pmu &&
> +	    kvm_arm_set_default_pmu(kvm))
> +		return -EINVAL;

nit: I'm not keen on re-interpreting the error code. If
kvm_arm_set_default_pmu() returns an error, we should return *that*
particular error, and not any other. Something like:

static int kvm_setup_vcpu(struct kvm_vcpu *vcpu)
{
	struct kvm *kvm = vcpu->kvm;
	int err = 0;

	/*
	 * 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) && !kvm->arch.arm_pmu)
		err = kvm_arm_set_default_pmu(kvm);

	return err;
}

> +
> +	return 0;
> +}
> +
>  static int __kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
>  				 const struct kvm_vcpu_init *init)
>  {
> @@ -1334,6 +1349,10 @@ static int __kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
>  
>  	bitmap_copy(kvm->arch.vcpu_features, &features, KVM_VCPU_MAX_FEATURES);
>  
> +	ret = kvm_setup_vcpu(vcpu);
> +	if (ret)
> +		goto out_unlock;
> +

Hmmm. Contrary to what the commit message says, the default PMU is not
picked at reset time, but at the point where the target is set (the
very first vcpu init). Which is pretty different from reset, which
happens more than once.

I also can't say I'm over the moon with yet another function that does
a very tiny bit of initialisation outside of the rest of the code that
performs the vcpu init. Following things is an absolute maze...

>  	/* Now we know what it is, we can reset it. */
>  	kvm_reset_vcpu(vcpu);
>  
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index eb5dcb12dafe9..66c244021ff08 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -717,10 +717,9 @@ static struct arm_pmu *kvm_pmu_probe_armpmu(void)
>  	 * It is still necessary to get a valid cpu, though, to probe for the
>  	 * default PMU instance as userspace is not required to specify a PMU
>  	 * type. In order to uphold the preexisting behavior KVM selects the
> -	 * PMU instance for the core where the first call to the
> -	 * KVM_ARM_VCPU_PMU_V3_CTRL attribute group occurs. A dependent use case
> -	 * would be a user with disdain of all things big.LITTLE that affines
> -	 * the VMM to a particular cluster of cores.
> +	 * PMU instance for the core just before the vcpu reset. A dependent use
> +	 * case would be a user with disdain of all things big.LITTLE that
> +	 * affines the VMM to a particular cluster of cores.

Same problem, see above.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.