Re: [PATCH] target/arm/kvm: Check supported feature per accelerator (not per vCPU)

[Paolo Bonzini]

On 17/06/20 17:23, Andrew Jones wrote:
>>
>> Fix by kvm_arm_<FEATURE>_supported() functions take a AccelState
>> argument (already realized/valid at this point) instead of a
>> CPUState argument.
> I'd rather not do that. IMO, a CPU feature test should operate on CPU,
> not an "accelerator".

If it's a test that the feature is enabled (e.g. via -cpu) then I agree.  
For something that ends up as a KVM_CHECK_EXTENSION or KVM_ENABLE_CAP on 
the KVM fd, however, I think passing an AccelState is better.
kvm_arm_pmu_supported case is clearly the latter, even the error message
hints at that:

+        if (kvm_enabled() && !kvm_arm_pmu_supported(current_accel())) {
             error_setg(errp, "'pmu' feature not supported by KVM on this 
host");
             return;
         }

but the same is true of kvm_arm_aarch32_supported and kvm_arm_sve_supported.

Applying the change to kvm_arm_pmu_supported as you suggest below would be
a bit of a bandaid because it would not have consistent prototypes.  Sp
for Philippe's patch

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks,

Paolo

> How that test is implemented is another story.
> If the CPUState isn't interesting, but it points to something that is,
> or there's another function that uses globals to get the job done, then
> fine, but the callers of a CPU feature test shouldn't need to know that.
> 
> I think we should just revert d70c996df23f and then apply the same
> change to kvm_arm_pmu_supported() that other similar functions got
> with 4f7f589381d5.