On Fri, Feb 22, 2019 at 09:10:46AM +0000, Julien Thierry wrote:

Hi Dave,

On 18/02/2019 19:52, Dave Martin wrote:

quoted

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 <Dave.Martin@arm.com>
---
 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 <asm/kvm_asm.h>
 #include <asm/kvm_coproc.h>
 #include <asm/kvm_mmu.h>
+#include <asm/virt.h>
 
 /* 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 <redacted>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel