Re: [PATCH v3 2/8] KVM: arm64: Rework detection of SVE, !VHE systems

[Christoffer Dall <christoffer.dall@arm.com>]

On Thu, Dec 06, 2018 at 05:31:20PM +0000, Marc Zyngier wrote:
> An SVE system is so far the only case where we mandate VHE. As we're
> starting to grow this requirements, let's slightly rework the way we
> deal with that situation, allowing for easy extension of this check.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/include/asm/kvm_host.h   | 2 +-
>  arch/arm64/include/asm/kvm_host.h | 6 +++---
>  virt/kvm/arm/arm.c                | 8 ++++----
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 5ca5d9af0c26..2184d9ddb418 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -285,7 +285,7 @@ void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
>  
>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
>  
> -static inline bool kvm_arch_check_sve_has_vhe(void) { return true; }
> +static inline bool kvm_arch_requires_vhe(void) { return false; }
>  static inline void kvm_arch_hardware_unsetup(void) {}
>  static inline void kvm_arch_sync_events(struct kvm *kvm) {}
>  static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 52fbc823ff8c..d6d9aa76a943 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -422,7 +422,7 @@ static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
>  	}
>  }
>  
> -static inline bool kvm_arch_check_sve_has_vhe(void)
> +static inline bool kvm_arch_requires_vhe(void)
>  {
>  	/*
>  	 * The Arm architecture specifies that implementation of SVE
> @@ -430,9 +430,9 @@ static inline bool kvm_arch_check_sve_has_vhe(void)
>  	 * relies on this when SVE is present:
>  	 */
>  	if (system_supports_sve())
> -		return has_vhe();
> -	else
>  		return true;
> +
> +	return false;
>  }
>  
>  static inline void kvm_arch_hardware_unsetup(void) {}
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 23774970c9df..1db4c15edcdd 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -1640,8 +1640,10 @@ int kvm_arch_init(void *opaque)
>  		return -ENODEV;
>  	}
>  
> -	if (!kvm_arch_check_sve_has_vhe()) {
> -		kvm_pr_unimpl("SVE system without VHE unsupported.  Broken cpu?");
> +	in_hyp_mode = is_kernel_in_hyp_mode();
> +
> +	if (!in_hyp_mode && kvm_arch_requires_vhe()) {
> +		kvm_pr_unimpl("CPU requiring VHE was booted in non-VHE mode");

nit: The error message feels weird to me (are we reporting CPU bugs?)
and I'm not sure about the unimpl and I think there's a linse space
missing.

How about:

	kvm_err("Cannot support this CPU in non-VHE mode, not initializing\n");

If we wanted to be super helpful, we could expand
kvm_arch_requires_vhe() with a print statement saying:
		
	kvm_err("SVE detected, requiring VHE mode\n");

But thay may be overkill.


>  		return -ENODEV;
>  	}
>  
> @@ -1657,8 +1659,6 @@ int kvm_arch_init(void *opaque)
>  	if (err)
>  		return err;
>  
> -	in_hyp_mode = is_kernel_in_hyp_mode();
> -
>  	if (!in_hyp_mode) {
>  		err = init_hyp_mode();
>  		if (err)
> -- 
> 2.19.2
> 

Otherwise:

Acked-by: Christoffer Dall <christoffer.dall@arm.com>

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