Re: [PATCH v3 3/4] KVM: arm64: Add KVM_ARM_VCPU_PMU_V3_SET_PMU attribute

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Marc,

On Thu, Jan 06, 2022 at 06:16:04PM +0000, Marc Zyngier wrote:
> On Thu, 06 Jan 2022 11:54:11 +0000,
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> > 
> > Hi Marc,
> > 
> > On Tue, Dec 14, 2021 at 12:28:15PM +0000, Marc Zyngier wrote:
> > > On Mon, 13 Dec 2021 15:23:08 +0000,
> > > Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> > > >
> > > > [..]
> > > >
> > > > @@ -910,7 +922,16 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
> > > >  	init_irq_work(&vcpu->arch.pmu.overflow_work,
> > > >  		      kvm_pmu_perf_overflow_notify_vcpu);
> > > >  
> > > > -	vcpu->arch.pmu.created = true;
> > > > +	atomic_set(&vcpu->arch.pmu.created, 1);
> > > > +
> > > > +	kvm_for_each_vcpu(i, v, kvm) {
> > > > +		if (!atomic_read(&v->arch.pmu.created))
> > > > +			continue;
> > > > +
> > > > +		if (v->arch.pmu.arm_pmu != arm_pmu)
> > > > +			return -ENXIO;
> > > > +	}
> > > 
> > > If you did store the arm_pmu at the VM level, you wouldn't need this.
> > > You could detect the discrepancy in the set_pmu ioctl.
> > 
> > I chose to set at the VCPU level to be consistent with how KVM treats the
> > PMU interrupt ID when the interrupt is a PPI, where the interrupt ID must
> > be the same for all VCPUs and it is stored at the VCPU. However, looking at
> > the code again, it occurs to me that it is stored at the VCPU when it's a
> > PPI because it's simpler to do it that way, as the code remains the same
> > when the interrupt ID is a SPI, which must be *different* between VCPUs. So
> > in the end, having the PMU stored at the VM level does match how KVM uses
> > it, which looks to be better than my approach.
> > 
> > This is the change you proposed in your branch [1]:
> > 
> > +static int kvm_arm_pmu_v3_set_pmu(struct kvm_vcpu *vcpu, int pmu_id)
> > +{
> > +       struct kvm *kvm = vcpu->kvm;
> > +       struct arm_pmu_entry *entry;
> > +       struct arm_pmu *arm_pmu;
> > +       int ret = -ENXIO;
> > +
> > +       mutex_lock(&kvm->lock);
> > +       mutex_lock(&arm_pmus_lock);
> > +
> > +       list_for_each_entry(entry, &arm_pmus, entry) {
> > +               arm_pmu = entry->arm_pmu;
> > +               if (arm_pmu->pmu.type == pmu_id) {
> > +                       /* Can't change PMU if filters are already in place */
> > +                       if (kvm->arch.arm_pmu != arm_pmu &&
> > +                           kvm->arch.pmu_filter) {
> > +                               ret = -EBUSY;
> > +                               break;
> > +                       }
> > +
> > +                       kvm->arch.arm_pmu = arm_pmu;
> > +                       ret = 0;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       mutex_unlock(&arm_pmus_lock);
> > +       mutex_unlock(&kvm->lock);
> > +       return ret;
> > +}
> > 
> > As I understand the code, userspace only needs to call
> > KVM_ARM_VCPU_PMU_V3_CTRL(KVM_ARM_VCPU_PMU_V3_SET_PMU) *once* (on one VCPU
> > fd) to set the PMU for all the VCPUs; subsequent calls (on the same VCPU or
> > on another VCPU) with a different PMU id will change the PMU for all VCPUs.
> > 
> > Two remarks:
> > 
> > 1. The documentation for the VCPU ioctls states this (from
> > Documentation/virt/kvm/devices/vcpu.rst):
> > 
> > "
> > ======================
> > Generic vcpu interface
> > ======================
> > 
> > The virtual cpu "device" also accepts the ioctls KVM_SET_DEVICE_ATTR,
> > KVM_GET_DEVICE_ATTR, and KVM_HAS_DEVICE_ATTR. The interface uses the same struct
> > kvm_device_attr as other devices, but **targets VCPU-wide settings and
> > controls**" (emphasis added).
> > 
> > But I guess having VCPU ioctls affect *only* the VCPU hasn't really been
> > true ever since PMU event filtering has been added. I'll send a patch to
> > change that part of the documentation for arm64.
> > 
> > I was thinking maybe a VM capability would be better suited for changing a
> > VM-wide setting, what do you think? I don't have a strong preference either
> > way.
> 
> I'm not sure it is worth the hassle of changing the API, as we'll have
> to keep the current one forever.

I was suggesting to use a capability for setting the PMU, it's too late to
change how the events filter is set.

> 
> > 
> > 2. What's to stop userspace to change the PMU after at least one VCPU has
> > run? That can be easily observed by the guest when reading PMCEIDx_EL0.
> 
> That's a good point. We need something here. It is a bit odd as to do
> that, you need to fully enable a PMU on one CPU, but not on the other,
> then run the first while changing stuff on the other. Something along
> those lines (untested):
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 4bf28905d438..4f53520e84fd 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -139,6 +139,7 @@ struct kvm_arch {
>  
>  	/* Memory Tagging Extension enabled for the guest */
>  	bool mte_enabled;
> +	bool ran_once;
>  };
>  
>  struct kvm_vcpu_fault_info {
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 83297fa97243..3045d7f609df 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -606,6 +606,10 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>  
>  	vcpu->arch.has_run_once = true;
>  
> +	mutex_lock(&kvm->lock);
> +	kvm->arch.ran_once = true;
> +	mutex_unlock(&kvm->lock);
> +
>  	kvm_arm_vcpu_init_debug(vcpu);
>  
>  	if (likely(irqchip_in_kernel(kvm))) {
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index dfc0430d6418..95100c541244 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -959,8 +959,9 @@ static int kvm_arm_pmu_v3_set_pmu(struct kvm_vcpu *vcpu, int pmu_id)
>  		arm_pmu = entry->arm_pmu;
>  		if (arm_pmu->pmu.type == pmu_id) {
>  			/* Can't change PMU if filters are already in place */
> -			if (kvm->arch.arm_pmu != arm_pmu &&
> -			    kvm->arch.pmu_filter) {
> +			if ((kvm->arch.arm_pmu != arm_pmu &&
> +			     kvm->arch.pmu_filter) ||
> +			    kvm->arch.ran_once) {
>  				ret = -EBUSY;
>  				break;
>  			}
> @@ -1040,6 +1041,11 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  
>  		mutex_lock(&vcpu->kvm->lock);
>  
> +		if (vcpu->kvm->arch.ran_once) {
> +			mutex_unlock(&vcpu->kvm->lock);
> +			return -EBUSY;
> +		}
> +
>  		if (!vcpu->kvm->arch.pmu_filter) {
>  			vcpu->kvm->arch.pmu_filter = bitmap_alloc(nr_events, GFP_KERNEL_ACCOUNT);
>  			if (!vcpu->kvm->arch.pmu_filter) {
> 
> which should prevent both PMU or filters to be changed once a single
> vcpu as run.
> 
> Thoughts?

Haven't tested it either, but it looks good to me. If you agree, I can pick
the diff, turn it into a patch and send it for the next iteration of this
series as a fix for the PMU events filter, while keeping your authorship.

Thanks,
Alex

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel