[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] target/arm/kvm: Check supported feature per accelerator (not
From: |
Paolo Bonzini |
Subject: |
Re: [PATCH] target/arm/kvm: Check supported feature per accelerator (not per vCPU) |
Date: |
Wed, 17 Jun 2020 19:37:42 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 |
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.