On Wed, Jun 17, 2020 at 07:37:42PM +0200, Paolo Bonzini wrote:
> 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.
I can live with that justification as long as we don't support
heterogeneous VCPU configurations. And, if that ever happens, then I
guess we'll be reworking a lot more than just the interface of these
cpu feature probes.
Thanks,
drew
> 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@xxxxxxxxxx>
>
> 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.
>
>
