On Fri, Feb 22, 2019 at 09:10:46AM +, Julien Thierry wrote:
> Hi Dave,
>
> On 18/02/2019 19:52, Dave Martin wrote:
> > To provide a uniform way to check for KVM SVE support amongst other
> > features, this patch adds a suitable capability KVM_CAP_ARM_SVE,
> > and reports it as present when SVE is available.
> >
> > Signed-off-by: Dave Martin
> > ---
> > arch/arm64/kvm/reset.c | 8
> > include/uapi/linux/kvm.h | 1 +
> > 2 files changed, 9 insertions(+)
> >
> > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > index e67cd2e..f636b34 100644
> > --- a/arch/arm64/kvm/reset.c
> > +++ b/arch/arm64/kvm/reset.c
> > @@ -35,6 +35,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > /* Maximum phys_shift supported for any VM on this host */
> > static u32 kvm_ipa_limit;
> > @@ -93,6 +94,9 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm,
> > long ext)
> > case KVM_CAP_ARM_VM_IPA_SIZE:
> > r = kvm_ipa_limit;
> > break;
> > + case KVM_CAP_ARM_SVE:
> > + r = system_supports_sve();
> > + break;
> > default:
> > r = 0;
> > }
> > @@ -105,6 +109,10 @@ static int kvm_reset_sve(struct kvm_vcpu *vcpu)
> > if (!system_supports_sve())
> > return -EINVAL;
> >
> > + /* Verify that KVM startup enforced this when SVE was detected: */
> > + if (WARN_ON(!has_vhe()))
> > + return -EINVAL;
>
> I'm wondering, wouldn't it make more sense to check for this when
> userland tries to set KVM_ARM_VCPU_SVE?
>
> Reviewed-by: Julien Thierry
I have a vague memory of these being some sort of reason for this, but
looking at the code now I can't see why the check can't be moved to
kvm_reset_vcpu().
How does the following look? The effective is no different, but the
code arrangement may be a bit less surprising this way:
int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
{
[...]
if (test_bit(KVM_ARM_VCPU_SVE, vcpu->arch.features)) {
if (!system_supports_sve())
return -EINVAL;
if (WARN_ON(!has_vhe()))
return -EINVAL;
/* KVM statup should enforce this on SVE hardware: */
ret = kvm_reset_sve(vcpu);
if (ret)
return ret;
}
This function is bascially the arch backend for KVM_ARM_VCPU_INIT, so I
don't see a better place to do this right now. Adding an extra hook
just for this kind of thing feels like overkill...
Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
