Re: [PATCH v3 1/5] KVM: arm64: Refactor PMU attribute error handling

From: Marc Zyngier <maz@kernel.org>
To: Andrew Jones <drjones@redhat.com>
Cc: kvm@vger.kernel.org, kernel-team@android.com, graf@amazon.com,
	Robin Murphy <robin.murphy@arm.com>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 1/5] KVM: arm64: Refactor PMU attribute error handling
Date: Tue, 08 Sep 2020 11:09:31 +0100	[thread overview]
Message-ID: <79c1a29f8596c4b0c8af7dcfdc600f36@kernel.org> (raw)
In-Reply-To: <20200908095318.nzbnadvgcmxvt3xs@kamzik.brq.redhat.com>

Hi Andrew,

On 2020-09-08 10:53, Andrew Jones wrote:
> Hi Marc,
> 
> On Tue, Sep 08, 2020 at 08:58:26AM +0100, Marc Zyngier wrote:
>> The PMU emulation error handling is pretty messy when dealing with
>> attributes. Let's refactor it so that we have less duplication,
>> and that it is easy to extend later on.
>> 
>> A functional change is that kvm_arm_pmu_v3_init() used to return
>> -ENXIO when the PMU feature wasn't set. The error is now reported
>> as -ENODEV, matching the documentation.
> 
> Hmm, I didn't think we could make changes like that, since some 
> userspace
> somewhere may now depend on the buggy interface.

Well, this is the whole point of this patch: discussing whether
this change is acceptable or whether existing VMMs are relying
on such behaviour. We *could* leave it as is, but I thought I'd
bring it up!

> That said, I'm not really
> against the change, but maybe it should go as a separate patch.

Sure, why not.

>> -ENXIO is still returned
>> when the interrupt isn't properly configured.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/kvm/pmu-emul.c | 21 +++++++++------------
>>  1 file changed, 9 insertions(+), 12 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>> index f0d0312c0a55..93d797df42c6 100644
>> --- a/arch/arm64/kvm/pmu-emul.c
>> +++ b/arch/arm64/kvm/pmu-emul.c
>> @@ -735,15 +735,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>> 
>>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>  {
>> -	if (!kvm_arm_support_pmu_v3())
>> -		return -ENODEV;
>> -
>> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>> -		return -ENXIO;
>> -
>> -	if (vcpu->arch.pmu.created)
>> -		return -EBUSY;
>> -
>>  	if (irqchip_in_kernel(vcpu->kvm)) {
>>  		int ret;
>> 
>> @@ -796,6 +787,15 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int 
>> irq)
>> 
>>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct 
>> kvm_device_attr *attr)
>>  {
>> +	if (!kvm_arm_support_pmu_v3())
>> +		return -ENODEV;
>> +
>> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>> +		return -ENODEV;
> 
> nit: could combine these two if's w/ an ||

This was made to make the userspace visible change obvious to the
reviewer. Now that you have noticed it, I'm happy to merge these
two! ;-)

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm