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

[Andrew Jones]

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@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.
> 
>