[PATCH 1/2] KVM: arm64: Restrict symbol aliasing to outside nVHE

[PATCH 1/2] KVM: arm64: Restrict symbol aliasing to outside nVHE

[Andrew Scull]

nVHE symbols are prefixed but this is hidden for the host by aliasing
the non-prefixed symbol to the prefixed version with a macro. This runs
into problems if nVHE tries to use the symbol as it becomes doubly
prefixed. Avoid this by omitting the aliasing macro for nVHE.

Cc: David Brazdil <dbrazdil@google.com>
Signed-off-by: Andrew Scull <ascull@google.com>
---
 arch/arm64/include/asm/kvm_asm.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index fb1a922b31ba..413911d6446a 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -99,8 +99,11 @@ struct kvm_s2_mmu;
 
 DECLARE_KVM_NVHE_SYM(__kvm_hyp_init);
 DECLARE_KVM_HYP_SYM(__kvm_hyp_vector);
+
+#ifndef __KVM_NVHE_HYPERVISOR__
 #define __kvm_hyp_init		CHOOSE_NVHE_SYM(__kvm_hyp_init)
 #define __kvm_hyp_vector	CHOOSE_HYP_SYM(__kvm_hyp_vector)
+#endif
 
 #ifdef CONFIG_KVM_INDIRECT_VECTORS
 extern atomic_t arm64_el2_vector_last_slot;
-- 
2.28.0.rc0.142.g3c755180ce-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 2/2] KVM: arm64: nVHE: Don't consume host SErrors with RAS

[Andrew Scull]

The ESB at the start of the vectors causes any SErrors to be consumed to
DISR_EL1. If the exception came from the host and the ESB caught an
SError, it would not be noticed until a guest exits and DISR_EL1 is
checked. Further, the SError would be attributed to the guest and not
the host.

To avoid these problems, use a different exception vector for the host
that does not use an ESB but instead leaves any host SError pending. A
guest will not be entered if an SError is pending so it will always be
the host that will receive and handle it.

Hyp initialization is now passed the vector that is used for the host
and the vector for guests is stored in a percpu variable as
kvm_get_hyp_vector() is not suitable for calling from nVHE hyp.

Fixes: 0e5b9c085dce ("KVM: arm64: Consume pending SError as early as possible")
Cc: James Morse <james.morse@arm.com>
Signed-off-by: Andrew Scull <ascull@google.com>
---
 arch/arm64/include/asm/kvm_asm.h  |  2 ++
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kernel/image-vars.h    |  1 +
 arch/arm64/kvm/arm.c              | 15 ++++++++++-
 arch/arm64/kvm/hyp/hyp-entry.S    | 42 +++++++++++++++++++++++++++++++
 arch/arm64/kvm/hyp/nvhe/switch.c  |  3 +++
 6 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index 413911d6446a..81f29a2c361a 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -98,10 +98,12 @@ struct kvm_vcpu;
 struct kvm_s2_mmu;
 
 DECLARE_KVM_NVHE_SYM(__kvm_hyp_init);
+DECLARE_KVM_NVHE_SYM(__kvm_hyp_host_vector);
 DECLARE_KVM_HYP_SYM(__kvm_hyp_vector);
 
 #ifndef __KVM_NVHE_HYPERVISOR__
 #define __kvm_hyp_init		CHOOSE_NVHE_SYM(__kvm_hyp_init)
+#define __kvm_hyp_host_vector	CHOOSE_NVHE_SYM(__kvm_hyp_host_vector)
 #define __kvm_hyp_vector	CHOOSE_HYP_SYM(__kvm_hyp_vector)
 #endif
 
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e1a32c0707bb..6b21d1c71a83 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -575,6 +575,7 @@ void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome);
 struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
 
 DECLARE_PER_CPU(kvm_host_data_t, kvm_host_data);
+DECLARE_PER_CPU(unsigned long, kvm_hyp_vector);
 
 static inline void kvm_init_host_cpu_context(struct kvm_cpu_context *cpu_ctxt)
 {
diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h
index 9e897c500237..7e93b0c426d4 100644
--- a/arch/arm64/kernel/image-vars.h
+++ b/arch/arm64/kernel/image-vars.h
@@ -71,6 +71,7 @@ KVM_NVHE_ALIAS(kvm_update_va_mask);
 /* Global kernel state accessed by nVHE hyp code. */
 KVM_NVHE_ALIAS(arm64_ssbd_callback_required);
 KVM_NVHE_ALIAS(kvm_host_data);
+KVM_NVHE_ALIAS(kvm_hyp_vector);
 KVM_NVHE_ALIAS(kvm_vgic_global_state);
 
 /* Kernel constant needed to compute idmap addresses. */
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 98f05bdac3c1..bb7c74b05fcd 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -47,6 +47,7 @@ __asm__(".arch_extension	virt");
 #endif
 
 DEFINE_PER_CPU(kvm_host_data_t, kvm_host_data);
+DEFINE_PER_CPU(unsigned long, kvm_hyp_vector);
 static DEFINE_PER_CPU(unsigned long, kvm_arm_hyp_stack_page);
 
 /* The VMID used in the VTTBR */
@@ -1274,7 +1275,10 @@ static void cpu_init_hyp_mode(void)
 
 	pgd_ptr = kvm_mmu_get_httbr();
 	hyp_stack_ptr = __this_cpu_read(kvm_arm_hyp_stack_page) + PAGE_SIZE;
-	vector_ptr = (unsigned long)kvm_get_hyp_vector();
+
+	/* Get the hyp address of the vectors used for the host and guests. */
+	vector_ptr = (unsigned long)kern_hyp_va(kvm_ksym_ref(__kvm_hyp_host_vector));
+	__this_cpu_write(kvm_hyp_vector, (unsigned long)kvm_get_hyp_vector());
 
 	/*
 	 * Call initialization code, and switch to the full blown HYP code.
@@ -1537,6 +1541,7 @@ static int init_hyp_mode(void)
 
 	for_each_possible_cpu(cpu) {
 		kvm_host_data_t *cpu_data;
+		unsigned long *vector;
 
 		cpu_data = per_cpu_ptr(&kvm_host_data, cpu);
 		err = create_hyp_mappings(cpu_data, cpu_data + 1, PAGE_HYP);
@@ -1545,6 +1550,14 @@ static int init_hyp_mode(void)
 			kvm_err("Cannot map host CPU state: %d\n", err);
 			goto out_err;
 		}
+
+		vector = per_cpu_ptr(&kvm_hyp_vector, cpu);
+		err = create_hyp_mappings(vector, vector + 1, PAGE_HYP);
+
+		if (err) {
+			kvm_err("Cannot map hyp guest vector address\n");
+			goto out_err;
+		}
 	}
 
 	err = hyp_map_aux_data();
diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
index 689fccbc9de7..2c5bec3ecb2a 100644
--- a/arch/arm64/kvm/hyp/hyp-entry.S
+++ b/arch/arm64/kvm/hyp/hyp-entry.S
@@ -213,7 +213,10 @@ SYM_CODE_END(\label)
 	invalid_vector	el2h_sync_invalid
 	invalid_vector	el2h_irq_invalid
 	invalid_vector	el2h_fiq_invalid
+	invalid_vector	el1_sync_invalid
+	invalid_vector	el1_irq_invalid
 	invalid_vector	el1_fiq_invalid
+	invalid_vector	el1_error_invalid
 
 	.ltorg
 
@@ -271,6 +274,45 @@ SYM_CODE_START(__kvm_hyp_vector)
 	valid_vect	el1_error		// Error 32-bit EL1
 SYM_CODE_END(__kvm_hyp_vector)
 
+#ifdef __KVM_NVHE_HYPERVISOR__
+.macro valid_host_vect target
+	.align 7
+	stp	x0, x1, [sp, #-16]!
+	b	\target
+.endm
+
+/*
+ * The host vectors do not use an ESB instruction in order to avoid consuming
+ * SErrors that should only be comsumed by the host. The host is also known to
+ * be 64-bit so any 32-bit exceptions can be treated as invalid.
+ *
+ * Indirection is not applied to the host vectors because the host already
+ * knows the address of hyp by virtue of loading it there.
+ */
+	.align 11
+SYM_CODE_START(__kvm_hyp_host_vector)
+	invalid_vect	el2t_sync_invalid	// Synchronous EL2t
+	invalid_vect	el2t_irq_invalid	// IRQ EL2t
+	invalid_vect	el2t_fiq_invalid	// FIQ EL2t
+	invalid_vect	el2t_error_invalid	// Error EL2t
+
+	valid_host_vect	el2_sync		// Synchronous EL2h
+	invalid_vect	el2h_irq_invalid	// IRQ EL2h
+	invalid_vect	el2h_fiq_invalid	// FIQ EL2h
+	valid_host_vect	el2_error		// Error EL2h
+
+	valid_host_vect	el1_sync		// Synchronous 64-bit EL1
+	valid_host_vect	el1_irq			// IRQ 64-bit EL1
+	invalid_vect	el1_fiq_invalid		// FIQ 64-bit EL1
+	valid_host_vect	el1_error		// Error 64-bit EL1
+
+	invalid_vect	el1_sync_invalid	// Synchronous 32-bit EL1
+	invalid_vect	el1_irq_invalid		// IRQ 32-bit EL1
+	invalid_vect	el1_fiq_invalid		// FIQ 32-bit EL1
+	invalid_vect	el1_error_invalid	// Error 32-bit EL1
+SYM_CODE_END(__kvm_hyp_host_vector)
+#endif
+
 #ifdef CONFIG_KVM_INDIRECT_VECTORS
 .macro hyp_ventry
 	.align 7
diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
index 341be2f2f312..3a711449ecd5 100644
--- a/arch/arm64/kvm/hyp/nvhe/switch.c
+++ b/arch/arm64/kvm/hyp/nvhe/switch.c
@@ -42,6 +42,7 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
 	}
 
 	write_sysreg(val, cptr_el2);
+	write_sysreg(__hyp_this_cpu_read(kvm_hyp_vector), vbar_el2);
 
 	if (cpus_have_final_cap(ARM64_WORKAROUND_SPECULATIVE_AT)) {
 		struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt;
@@ -60,6 +61,7 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
 
 static void __deactivate_traps(struct kvm_vcpu *vcpu)
 {
+	extern char __kvm_hyp_host_vector[];
 	u64 mdcr_el2;
 
 	___deactivate_traps(vcpu);
@@ -91,6 +93,7 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu)
 	write_sysreg(mdcr_el2, mdcr_el2);
 	write_sysreg(HCR_HOST_NVHE_FLAGS, hcr_el2);
 	write_sysreg(CPTR_EL2_DEFAULT, cptr_el2);
+	write_sysreg(__kvm_hyp_host_vector, vbar_el2);
 }
 
 static void __deactivate_vm(struct kvm_vcpu *vcpu)
-- 
2.28.0.rc0.142.g3c755180ce-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: nVHE: Don't consume host SErrors with RAS

[Marc Zyngier]

On 2020-07-30 16:18, Andrew Scull wrote:
> The ESB at the start of the vectors causes any SErrors to be consumed 
> to
> DISR_EL1. If the exception came from the host and the ESB caught an
> SError, it would not be noticed until a guest exits and DISR_EL1 is
> checked. Further, the SError would be attributed to the guest and not
> the host.
> 
> To avoid these problems, use a different exception vector for the host
> that does not use an ESB but instead leaves any host SError pending. A
> guest will not be entered if an SError is pending so it will always be
> the host that will receive and handle it.
> 
> Hyp initialization is now passed the vector that is used for the host
> and the vector for guests is stored in a percpu variable as
> kvm_get_hyp_vector() is not suitable for calling from nVHE hyp.
> 
> Fixes: 0e5b9c085dce ("KVM: arm64: Consume pending SError as early as 
> possible")
> Cc: James Morse <james.morse@arm.com>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  arch/arm64/include/asm/kvm_asm.h  |  2 ++
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kernel/image-vars.h    |  1 +
>  arch/arm64/kvm/arm.c              | 15 ++++++++++-
>  arch/arm64/kvm/hyp/hyp-entry.S    | 42 +++++++++++++++++++++++++++++++
>  arch/arm64/kvm/hyp/nvhe/switch.c  |  3 +++
>  6 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h 
> b/arch/arm64/include/asm/kvm_asm.h
> index 413911d6446a..81f29a2c361a 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -98,10 +98,12 @@ struct kvm_vcpu;
>  struct kvm_s2_mmu;
> 
>  DECLARE_KVM_NVHE_SYM(__kvm_hyp_init);
> +DECLARE_KVM_NVHE_SYM(__kvm_hyp_host_vector);
>  DECLARE_KVM_HYP_SYM(__kvm_hyp_vector);
> 
>  #ifndef __KVM_NVHE_HYPERVISOR__
>  #define __kvm_hyp_init		CHOOSE_NVHE_SYM(__kvm_hyp_init)
> +#define __kvm_hyp_host_vector	CHOOSE_NVHE_SYM(__kvm_hyp_host_vector)
>  #define __kvm_hyp_vector	CHOOSE_HYP_SYM(__kvm_hyp_vector)
>  #endif
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index e1a32c0707bb..6b21d1c71a83 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -575,6 +575,7 @@ void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 
> syndrome);
>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long 
> mpidr);
> 
>  DECLARE_PER_CPU(kvm_host_data_t, kvm_host_data);
> +DECLARE_PER_CPU(unsigned long, kvm_hyp_vector);
> 
>  static inline void kvm_init_host_cpu_context(struct kvm_cpu_context 
> *cpu_ctxt)
>  {
> diff --git a/arch/arm64/kernel/image-vars.h 
> b/arch/arm64/kernel/image-vars.h
> index 9e897c500237..7e93b0c426d4 100644
> --- a/arch/arm64/kernel/image-vars.h
> +++ b/arch/arm64/kernel/image-vars.h
> @@ -71,6 +71,7 @@ KVM_NVHE_ALIAS(kvm_update_va_mask);
>  /* Global kernel state accessed by nVHE hyp code. */
>  KVM_NVHE_ALIAS(arm64_ssbd_callback_required);
>  KVM_NVHE_ALIAS(kvm_host_data);
> +KVM_NVHE_ALIAS(kvm_hyp_vector);
>  KVM_NVHE_ALIAS(kvm_vgic_global_state);
> 
>  /* Kernel constant needed to compute idmap addresses. */
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 98f05bdac3c1..bb7c74b05fcd 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -47,6 +47,7 @@ __asm__(".arch_extension	virt");
>  #endif
> 
>  DEFINE_PER_CPU(kvm_host_data_t, kvm_host_data);
> +DEFINE_PER_CPU(unsigned long, kvm_hyp_vector);
>  static DEFINE_PER_CPU(unsigned long, kvm_arm_hyp_stack_page);
> 
>  /* The VMID used in the VTTBR */
> @@ -1274,7 +1275,10 @@ static void cpu_init_hyp_mode(void)
> 
>  	pgd_ptr = kvm_mmu_get_httbr();
>  	hyp_stack_ptr = __this_cpu_read(kvm_arm_hyp_stack_page) + PAGE_SIZE;
> -	vector_ptr = (unsigned long)kvm_get_hyp_vector();
> +
> +	/* Get the hyp address of the vectors used for the host and guests. 
> */
> +	vector_ptr = (unsigned 
> long)kern_hyp_va(kvm_ksym_ref(__kvm_hyp_host_vector));
> +	__this_cpu_write(kvm_hyp_vector, (unsigned 
> long)kvm_get_hyp_vector());
> 
>  	/*
>  	 * Call initialization code, and switch to the full blown HYP code.
> @@ -1537,6 +1541,7 @@ static int init_hyp_mode(void)
> 
>  	for_each_possible_cpu(cpu) {
>  		kvm_host_data_t *cpu_data;
> +		unsigned long *vector;
> 
>  		cpu_data = per_cpu_ptr(&kvm_host_data, cpu);
>  		err = create_hyp_mappings(cpu_data, cpu_data + 1, PAGE_HYP);
> @@ -1545,6 +1550,14 @@ static int init_hyp_mode(void)
>  			kvm_err("Cannot map host CPU state: %d\n", err);
>  			goto out_err;
>  		}
> +
> +		vector = per_cpu_ptr(&kvm_hyp_vector, cpu);
> +		err = create_hyp_mappings(vector, vector + 1, PAGE_HYP);
> +
> +		if (err) {
> +			kvm_err("Cannot map hyp guest vector address\n");
> +			goto out_err;
> +		}
>  	}
> 
>  	err = hyp_map_aux_data();
> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S 
> b/arch/arm64/kvm/hyp/hyp-entry.S
> index 689fccbc9de7..2c5bec3ecb2a 100644
> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> @@ -213,7 +213,10 @@ SYM_CODE_END(\label)
>  	invalid_vector	el2h_sync_invalid
>  	invalid_vector	el2h_irq_invalid
>  	invalid_vector	el2h_fiq_invalid
> +	invalid_vector	el1_sync_invalid
> +	invalid_vector	el1_irq_invalid
>  	invalid_vector	el1_fiq_invalid
> +	invalid_vector	el1_error_invalid
> 
>  	.ltorg
> 
> @@ -271,6 +274,45 @@ SYM_CODE_START(__kvm_hyp_vector)
>  	valid_vect	el1_error		// Error 32-bit EL1
>  SYM_CODE_END(__kvm_hyp_vector)
> 
> +#ifdef __KVM_NVHE_HYPERVISOR__
> +.macro valid_host_vect target
> +	.align 7
> +	stp	x0, x1, [sp, #-16]!
> +	b	\target
> +.endm
> +
> +/*
> + * The host vectors do not use an ESB instruction in order to avoid 
> consuming
> + * SErrors that should only be comsumed by the host. The host is also 
> known to
> + * be 64-bit so any 32-bit exceptions can be treated as invalid.
> + *
> + * Indirection is not applied to the host vectors because the host 
> already
> + * knows the address of hyp by virtue of loading it there.
> + */
> +	.align 11
> +SYM_CODE_START(__kvm_hyp_host_vector)
> +	invalid_vect	el2t_sync_invalid	// Synchronous EL2t
> +	invalid_vect	el2t_irq_invalid	// IRQ EL2t
> +	invalid_vect	el2t_fiq_invalid	// FIQ EL2t
> +	invalid_vect	el2t_error_invalid	// Error EL2t
> +
> +	valid_host_vect	el2_sync		// Synchronous EL2h
> +	invalid_vect	el2h_irq_invalid	// IRQ EL2h
> +	invalid_vect	el2h_fiq_invalid	// FIQ EL2h
> +	valid_host_vect	el2_error		// Error EL2h
> +
> +	valid_host_vect	el1_sync		// Synchronous 64-bit EL1

I quite like the idea, but it feels like you have stopped short
of doing interesting things here.

Since you have started pulling host and guest vectors apart,
you should be able to do a much better job than what we have
today when we try to identify the host by looking at vttbr_el2.

It also means that the macro that pushes things on the stack could
be removed as we end-up having very different requirements.

What do you think?

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: nVHE: Don't consume host SErrors with RAS

[Andrew Scull]

On Thu, Jul 30, 2020 at 05:02:57PM +0100, Marc Zyngier wrote:
> On 2020-07-30 16:18, Andrew Scull wrote:
> > The ESB at the start of the vectors causes any SErrors to be consumed to
> > DISR_EL1. If the exception came from the host and the ESB caught an
> > SError, it would not be noticed until a guest exits and DISR_EL1 is
> > checked. Further, the SError would be attributed to the guest and not
> > the host.
> > 
> > To avoid these problems, use a different exception vector for the host
> > that does not use an ESB but instead leaves any host SError pending. A
> > guest will not be entered if an SError is pending so it will always be
> > the host that will receive and handle it.
> > 
> > Hyp initialization is now passed the vector that is used for the host
> > and the vector for guests is stored in a percpu variable as
> > kvm_get_hyp_vector() is not suitable for calling from nVHE hyp.
> > 
> > Fixes: 0e5b9c085dce ("KVM: arm64: Consume pending SError as early as
> > possible")
> > Cc: James Morse <james.morse@arm.com>
> > Signed-off-by: Andrew Scull <ascull@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_asm.h  |  2 ++
> >  arch/arm64/include/asm/kvm_host.h |  1 +
> >  arch/arm64/kernel/image-vars.h    |  1 +
> >  arch/arm64/kvm/arm.c              | 15 ++++++++++-
> >  arch/arm64/kvm/hyp/hyp-entry.S    | 42 +++++++++++++++++++++++++++++++
> >  arch/arm64/kvm/hyp/nvhe/switch.c  |  3 +++
> >  6 files changed, 63 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_asm.h
> > b/arch/arm64/include/asm/kvm_asm.h
> > index 413911d6446a..81f29a2c361a 100644
> > --- a/arch/arm64/include/asm/kvm_asm.h
> > +++ b/arch/arm64/include/asm/kvm_asm.h
> > @@ -98,10 +98,12 @@ struct kvm_vcpu;
> >  struct kvm_s2_mmu;
> > 
> >  DECLARE_KVM_NVHE_SYM(__kvm_hyp_init);
> > +DECLARE_KVM_NVHE_SYM(__kvm_hyp_host_vector);
> >  DECLARE_KVM_HYP_SYM(__kvm_hyp_vector);
> > 
> >  #ifndef __KVM_NVHE_HYPERVISOR__
> >  #define __kvm_hyp_init		CHOOSE_NVHE_SYM(__kvm_hyp_init)
> > +#define __kvm_hyp_host_vector	CHOOSE_NVHE_SYM(__kvm_hyp_host_vector)
> >  #define __kvm_hyp_vector	CHOOSE_HYP_SYM(__kvm_hyp_vector)
> >  #endif
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h
> > b/arch/arm64/include/asm/kvm_host.h
> > index e1a32c0707bb..6b21d1c71a83 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -575,6 +575,7 @@ void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64
> > syndrome);
> >  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long
> > mpidr);
> > 
> >  DECLARE_PER_CPU(kvm_host_data_t, kvm_host_data);
> > +DECLARE_PER_CPU(unsigned long, kvm_hyp_vector);
> > 
> >  static inline void kvm_init_host_cpu_context(struct kvm_cpu_context
> > *cpu_ctxt)
> >  {
> > diff --git a/arch/arm64/kernel/image-vars.h
> > b/arch/arm64/kernel/image-vars.h
> > index 9e897c500237..7e93b0c426d4 100644
> > --- a/arch/arm64/kernel/image-vars.h
> > +++ b/arch/arm64/kernel/image-vars.h
> > @@ -71,6 +71,7 @@ KVM_NVHE_ALIAS(kvm_update_va_mask);
> >  /* Global kernel state accessed by nVHE hyp code. */
> >  KVM_NVHE_ALIAS(arm64_ssbd_callback_required);
> >  KVM_NVHE_ALIAS(kvm_host_data);
> > +KVM_NVHE_ALIAS(kvm_hyp_vector);
> >  KVM_NVHE_ALIAS(kvm_vgic_global_state);
> > 
> >  /* Kernel constant needed to compute idmap addresses. */
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 98f05bdac3c1..bb7c74b05fcd 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -47,6 +47,7 @@ __asm__(".arch_extension	virt");
> >  #endif
> > 
> >  DEFINE_PER_CPU(kvm_host_data_t, kvm_host_data);
> > +DEFINE_PER_CPU(unsigned long, kvm_hyp_vector);
> >  static DEFINE_PER_CPU(unsigned long, kvm_arm_hyp_stack_page);
> > 
> >  /* The VMID used in the VTTBR */
> > @@ -1274,7 +1275,10 @@ static void cpu_init_hyp_mode(void)
> > 
> >  	pgd_ptr = kvm_mmu_get_httbr();
> >  	hyp_stack_ptr = __this_cpu_read(kvm_arm_hyp_stack_page) + PAGE_SIZE;
> > -	vector_ptr = (unsigned long)kvm_get_hyp_vector();
> > +
> > +	/* Get the hyp address of the vectors used for the host and guests. */
> > +	vector_ptr = (unsigned
> > long)kern_hyp_va(kvm_ksym_ref(__kvm_hyp_host_vector));
> > +	__this_cpu_write(kvm_hyp_vector, (unsigned long)kvm_get_hyp_vector());
> > 
> >  	/*
> >  	 * Call initialization code, and switch to the full blown HYP code.
> > @@ -1537,6 +1541,7 @@ static int init_hyp_mode(void)
> > 
> >  	for_each_possible_cpu(cpu) {
> >  		kvm_host_data_t *cpu_data;
> > +		unsigned long *vector;
> > 
> >  		cpu_data = per_cpu_ptr(&kvm_host_data, cpu);
> >  		err = create_hyp_mappings(cpu_data, cpu_data + 1, PAGE_HYP);
> > @@ -1545,6 +1550,14 @@ static int init_hyp_mode(void)
> >  			kvm_err("Cannot map host CPU state: %d\n", err);
> >  			goto out_err;
> >  		}
> > +
> > +		vector = per_cpu_ptr(&kvm_hyp_vector, cpu);
> > +		err = create_hyp_mappings(vector, vector + 1, PAGE_HYP);
> > +
> > +		if (err) {
> > +			kvm_err("Cannot map hyp guest vector address\n");
> > +			goto out_err;
> > +		}
> >  	}
> > 
> >  	err = hyp_map_aux_data();
> > diff --git a/arch/arm64/kvm/hyp/hyp-entry.S
> > b/arch/arm64/kvm/hyp/hyp-entry.S
> > index 689fccbc9de7..2c5bec3ecb2a 100644
> > --- a/arch/arm64/kvm/hyp/hyp-entry.S
> > +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> > @@ -213,7 +213,10 @@ SYM_CODE_END(\label)
> >  	invalid_vector	el2h_sync_invalid
> >  	invalid_vector	el2h_irq_invalid
> >  	invalid_vector	el2h_fiq_invalid
> > +	invalid_vector	el1_sync_invalid
> > +	invalid_vector	el1_irq_invalid
> >  	invalid_vector	el1_fiq_invalid
> > +	invalid_vector	el1_error_invalid
> > 
> >  	.ltorg
> > 
> > @@ -271,6 +274,45 @@ SYM_CODE_START(__kvm_hyp_vector)
> >  	valid_vect	el1_error		// Error 32-bit EL1
> >  SYM_CODE_END(__kvm_hyp_vector)
> > 
> > +#ifdef __KVM_NVHE_HYPERVISOR__
> > +.macro valid_host_vect target
> > +	.align 7
> > +	stp	x0, x1, [sp, #-16]!
> > +	b	\target
> > +.endm
> > +
> > +/*
> > + * The host vectors do not use an ESB instruction in order to avoid
> > consuming
> > + * SErrors that should only be comsumed by the host. The host is also
> > known to
> > + * be 64-bit so any 32-bit exceptions can be treated as invalid.
> > + *
> > + * Indirection is not applied to the host vectors because the host
> > already
> > + * knows the address of hyp by virtue of loading it there.
> > + */
> > +	.align 11
> > +SYM_CODE_START(__kvm_hyp_host_vector)
> > +	invalid_vect	el2t_sync_invalid	// Synchronous EL2t
> > +	invalid_vect	el2t_irq_invalid	// IRQ EL2t
> > +	invalid_vect	el2t_fiq_invalid	// FIQ EL2t
> > +	invalid_vect	el2t_error_invalid	// Error EL2t
> > +
> > +	valid_host_vect	el2_sync		// Synchronous EL2h
> > +	invalid_vect	el2h_irq_invalid	// IRQ EL2h
> > +	invalid_vect	el2h_fiq_invalid	// FIQ EL2h
> > +	valid_host_vect	el2_error		// Error EL2h
> > +
> > +	valid_host_vect	el1_sync		// Synchronous 64-bit EL1
> 
> I quite like the idea, but it feels like you have stopped short
> of doing interesting things here.
> 
> Since you have started pulling host and guest vectors apart,
> you should be able to do a much better job than what we have
> today when we try to identify the host by looking at vttbr_el2.
> 
> It also means that the macro that pushes things on the stack could
> be removed as we end-up having very different requirements.
> 
> What do you think?

You're right that I've chosen to stop after the fix and haven't gone on
to take some of the obvious refactors that this would allow. The main
reason for this being that the refactoring would, at times, be at odds
with the "host as a vcpu" series [1] that I am also working on.

In that series, the requirements change again so I was planning to look
at how to refactor the series based on this rather than refactor once
now and then again in the series. Let me know if this doesn't seem like
the right approach.

[1] -- https://lore.kernel.org/kvmarm/20200715184438.1390996-1-ascull@google.com/T/#t
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: nVHE: Don't consume host SErrors with RAS

[Andrew Scull]

On Thu, Jul 30, 2020 at 04:18:23PM +0100, Andrew Scull wrote:
> The ESB at the start of the vectors causes any SErrors to be consumed to
> DISR_EL1. If the exception came from the host and the ESB caught an
> SError, it would not be noticed until a guest exits and DISR_EL1 is
> checked. Further, the SError would be attributed to the guest and not
> the host.
> 
> To avoid these problems, use a different exception vector for the host
> that does not use an ESB but instead leaves any host SError pending. A
> guest will not be entered if an SError is pending so it will always be
> the host that will receive and handle it.

Thinking further, I'm not sure this actually solves all of the problem.
It does prevent hyp from causing a host SError to be consumed but, IIUC,
there could be an SError already deferred by the host and logged in
DISR_EL1 that hyp would not preserve if a guest is run.

I think the host's DISR_EL1 would need to be saved and restored in the
vcpu context switch which, from a cursory read of the ARM, is possible
without having to virtualize SErrors for the host.

> Hyp initialization is now passed the vector that is used for the host
> and the vector for guests is stored in a percpu variable as
> kvm_get_hyp_vector() is not suitable for calling from nVHE hyp.
> 
> Fixes: 0e5b9c085dce ("KVM: arm64: Consume pending SError as early as possible")
> Cc: James Morse <james.morse@arm.com>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  arch/arm64/include/asm/kvm_asm.h  |  2 ++
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kernel/image-vars.h    |  1 +
>  arch/arm64/kvm/arm.c              | 15 ++++++++++-
>  arch/arm64/kvm/hyp/hyp-entry.S    | 42 +++++++++++++++++++++++++++++++
>  arch/arm64/kvm/hyp/nvhe/switch.c  |  3 +++
>  6 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 413911d6446a..81f29a2c361a 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -98,10 +98,12 @@ struct kvm_vcpu;
>  struct kvm_s2_mmu;
>  
>  DECLARE_KVM_NVHE_SYM(__kvm_hyp_init);
> +DECLARE_KVM_NVHE_SYM(__kvm_hyp_host_vector);
>  DECLARE_KVM_HYP_SYM(__kvm_hyp_vector);
>  
>  #ifndef __KVM_NVHE_HYPERVISOR__
>  #define __kvm_hyp_init		CHOOSE_NVHE_SYM(__kvm_hyp_init)
> +#define __kvm_hyp_host_vector	CHOOSE_NVHE_SYM(__kvm_hyp_host_vector)
>  #define __kvm_hyp_vector	CHOOSE_HYP_SYM(__kvm_hyp_vector)
>  #endif
>  
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index e1a32c0707bb..6b21d1c71a83 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -575,6 +575,7 @@ void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome);
>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
>  
>  DECLARE_PER_CPU(kvm_host_data_t, kvm_host_data);
> +DECLARE_PER_CPU(unsigned long, kvm_hyp_vector);
>  
>  static inline void kvm_init_host_cpu_context(struct kvm_cpu_context *cpu_ctxt)
>  {
> diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h
> index 9e897c500237..7e93b0c426d4 100644
> --- a/arch/arm64/kernel/image-vars.h
> +++ b/arch/arm64/kernel/image-vars.h
> @@ -71,6 +71,7 @@ KVM_NVHE_ALIAS(kvm_update_va_mask);
>  /* Global kernel state accessed by nVHE hyp code. */
>  KVM_NVHE_ALIAS(arm64_ssbd_callback_required);
>  KVM_NVHE_ALIAS(kvm_host_data);
> +KVM_NVHE_ALIAS(kvm_hyp_vector);
>  KVM_NVHE_ALIAS(kvm_vgic_global_state);
>  
>  /* Kernel constant needed to compute idmap addresses. */
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 98f05bdac3c1..bb7c74b05fcd 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -47,6 +47,7 @@ __asm__(".arch_extension	virt");
>  #endif
>  
>  DEFINE_PER_CPU(kvm_host_data_t, kvm_host_data);
> +DEFINE_PER_CPU(unsigned long, kvm_hyp_vector);
>  static DEFINE_PER_CPU(unsigned long, kvm_arm_hyp_stack_page);
>  
>  /* The VMID used in the VTTBR */
> @@ -1274,7 +1275,10 @@ static void cpu_init_hyp_mode(void)
>  
>  	pgd_ptr = kvm_mmu_get_httbr();
>  	hyp_stack_ptr = __this_cpu_read(kvm_arm_hyp_stack_page) + PAGE_SIZE;
> -	vector_ptr = (unsigned long)kvm_get_hyp_vector();
> +
> +	/* Get the hyp address of the vectors used for the host and guests. */
> +	vector_ptr = (unsigned long)kern_hyp_va(kvm_ksym_ref(__kvm_hyp_host_vector));
> +	__this_cpu_write(kvm_hyp_vector, (unsigned long)kvm_get_hyp_vector());
>  
>  	/*
>  	 * Call initialization code, and switch to the full blown HYP code.
> @@ -1537,6 +1541,7 @@ static int init_hyp_mode(void)
>  
>  	for_each_possible_cpu(cpu) {
>  		kvm_host_data_t *cpu_data;
> +		unsigned long *vector;
>  
>  		cpu_data = per_cpu_ptr(&kvm_host_data, cpu);
>  		err = create_hyp_mappings(cpu_data, cpu_data + 1, PAGE_HYP);
> @@ -1545,6 +1550,14 @@ static int init_hyp_mode(void)
>  			kvm_err("Cannot map host CPU state: %d\n", err);
>  			goto out_err;
>  		}
> +
> +		vector = per_cpu_ptr(&kvm_hyp_vector, cpu);
> +		err = create_hyp_mappings(vector, vector + 1, PAGE_HYP);
> +
> +		if (err) {
> +			kvm_err("Cannot map hyp guest vector address\n");
> +			goto out_err;
> +		}
>  	}
>  
>  	err = hyp_map_aux_data();
> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
> index 689fccbc9de7..2c5bec3ecb2a 100644
> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> @@ -213,7 +213,10 @@ SYM_CODE_END(\label)
>  	invalid_vector	el2h_sync_invalid
>  	invalid_vector	el2h_irq_invalid
>  	invalid_vector	el2h_fiq_invalid
> +	invalid_vector	el1_sync_invalid
> +	invalid_vector	el1_irq_invalid
>  	invalid_vector	el1_fiq_invalid
> +	invalid_vector	el1_error_invalid
>  
>  	.ltorg
>  
> @@ -271,6 +274,45 @@ SYM_CODE_START(__kvm_hyp_vector)
>  	valid_vect	el1_error		// Error 32-bit EL1
>  SYM_CODE_END(__kvm_hyp_vector)
>  
> +#ifdef __KVM_NVHE_HYPERVISOR__
> +.macro valid_host_vect target
> +	.align 7
> +	stp	x0, x1, [sp, #-16]!
> +	b	\target
> +.endm
> +
> +/*
> + * The host vectors do not use an ESB instruction in order to avoid consuming
> + * SErrors that should only be comsumed by the host. The host is also known to
> + * be 64-bit so any 32-bit exceptions can be treated as invalid.
> + *
> + * Indirection is not applied to the host vectors because the host already
> + * knows the address of hyp by virtue of loading it there.
> + */
> +	.align 11
> +SYM_CODE_START(__kvm_hyp_host_vector)
> +	invalid_vect	el2t_sync_invalid	// Synchronous EL2t
> +	invalid_vect	el2t_irq_invalid	// IRQ EL2t
> +	invalid_vect	el2t_fiq_invalid	// FIQ EL2t
> +	invalid_vect	el2t_error_invalid	// Error EL2t
> +
> +	valid_host_vect	el2_sync		// Synchronous EL2h
> +	invalid_vect	el2h_irq_invalid	// IRQ EL2h
> +	invalid_vect	el2h_fiq_invalid	// FIQ EL2h
> +	valid_host_vect	el2_error		// Error EL2h
> +
> +	valid_host_vect	el1_sync		// Synchronous 64-bit EL1
> +	valid_host_vect	el1_irq			// IRQ 64-bit EL1
> +	invalid_vect	el1_fiq_invalid		// FIQ 64-bit EL1
> +	valid_host_vect	el1_error		// Error 64-bit EL1
> +
> +	invalid_vect	el1_sync_invalid	// Synchronous 32-bit EL1
> +	invalid_vect	el1_irq_invalid		// IRQ 32-bit EL1
> +	invalid_vect	el1_fiq_invalid		// FIQ 32-bit EL1
> +	invalid_vect	el1_error_invalid	// Error 32-bit EL1
> +SYM_CODE_END(__kvm_hyp_host_vector)
> +#endif
> +
>  #ifdef CONFIG_KVM_INDIRECT_VECTORS
>  .macro hyp_ventry
>  	.align 7
> diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
> index 341be2f2f312..3a711449ecd5 100644
> --- a/arch/arm64/kvm/hyp/nvhe/switch.c
> +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
> @@ -42,6 +42,7 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
>  	}
>  
>  	write_sysreg(val, cptr_el2);
> +	write_sysreg(__hyp_this_cpu_read(kvm_hyp_vector), vbar_el2);
>  
>  	if (cpus_have_final_cap(ARM64_WORKAROUND_SPECULATIVE_AT)) {
>  		struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt;
> @@ -60,6 +61,7 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
>  
>  static void __deactivate_traps(struct kvm_vcpu *vcpu)
>  {
> +	extern char __kvm_hyp_host_vector[];
>  	u64 mdcr_el2;
>  
>  	___deactivate_traps(vcpu);
> @@ -91,6 +93,7 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu)
>  	write_sysreg(mdcr_el2, mdcr_el2);
>  	write_sysreg(HCR_HOST_NVHE_FLAGS, hcr_el2);
>  	write_sysreg(CPTR_EL2_DEFAULT, cptr_el2);
> +	write_sysreg(__kvm_hyp_host_vector, vbar_el2);
>  }
>  
>  static void __deactivate_vm(struct kvm_vcpu *vcpu)
> -- 
> 2.28.0.rc0.142.g3c755180ce-goog
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: nVHE: Don't consume host SErrors with RAS

[Marc Zyngier]

Hi Andrew,

On 2020-07-30 23:31, Andrew Scull wrote:
> On Thu, Jul 30, 2020 at 04:18:23PM +0100, Andrew Scull wrote:
>> The ESB at the start of the vectors causes any SErrors to be consumed 
>> to
>> DISR_EL1. If the exception came from the host and the ESB caught an
>> SError, it would not be noticed until a guest exits and DISR_EL1 is
>> checked. Further, the SError would be attributed to the guest and not
>> the host.
>> 
>> To avoid these problems, use a different exception vector for the host
>> that does not use an ESB but instead leaves any host SError pending. A
>> guest will not be entered if an SError is pending so it will always be
>> the host that will receive and handle it.
> 
> Thinking further, I'm not sure this actually solves all of the problem.
> It does prevent hyp from causing a host SError to be consumed but, 
> IIUC,
> there could be an SError already deferred by the host and logged in
> DISR_EL1 that hyp would not preserve if a guest is run.
> 
> I think the host's DISR_EL1 would need to be saved and restored in the
> vcpu context switch which, from a cursory read of the ARM, is possible
> without having to virtualize SErrors for the host.

The question is what do you if you have something pending in DISR_EL1
at the point where you enter EL2? Context switching it is not going to
help. One problem is that you'd need to do an ESB, corrupting DISR_EL1,
before any memory access (I'm assuming you can get traps where all
registers are live). I can't see how we square this circle.

Furthermore, assuming you find a way to do it, what do you do with it?

(a) Either there was something pending already and it is still pending,

(b) Or there was nothing pending and you now have an error that you
     don't know how to report (the host EL1 never issued an ESB)

We could just error out on hypercalls if DISR_EL1 is non-zero, but
I don't see how we do that for traps, as it would just confuse the host
EL1.

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: nVHE: Don't consume host SErrors with RAS

[Andrew Scull]

On Fri, Jul 31, 2020 at 09:00:03AM +0100, Marc Zyngier wrote:
> Hi Andrew,
> 
> On 2020-07-30 23:31, Andrew Scull wrote:
> > On Thu, Jul 30, 2020 at 04:18:23PM +0100, Andrew Scull wrote:
> > > The ESB at the start of the vectors causes any SErrors to be
> > > consumed to
> > > DISR_EL1. If the exception came from the host and the ESB caught an
> > > SError, it would not be noticed until a guest exits and DISR_EL1 is
> > > checked. Further, the SError would be attributed to the guest and not
> > > the host.
> > > 
> > > To avoid these problems, use a different exception vector for the host
> > > that does not use an ESB but instead leaves any host SError pending. A
> > > guest will not be entered if an SError is pending so it will always be
> > > the host that will receive and handle it.
> > 
> > Thinking further, I'm not sure this actually solves all of the problem.
> > It does prevent hyp from causing a host SError to be consumed but, IIUC,
> > there could be an SError already deferred by the host and logged in
> > DISR_EL1 that hyp would not preserve if a guest is run.
> > 
> > I think the host's DISR_EL1 would need to be saved and restored in the
> > vcpu context switch which, from a cursory read of the ARM, is possible
> > without having to virtualize SErrors for the host.
> 
> The question is what do you if you have something pending in DISR_EL1
> at the point where you enter EL2? Context switching it is not going to
> help. One problem is that you'd need to do an ESB, corrupting DISR_EL1,
> before any memory access (I'm assuming you can get traps where all
> registers are live). I can't see how we square this circle.

I'll expand on what I understand (or think I do) about RAS at the
moment. It should hopefully highlight anything wrong with my reasoning
for your questions.

DISR_EL1.A being set means a pending physical SError has been
consumed/cleared. The host has already deferred an SError so saving and
restoring (i.e. preserving) DISR_EL1 for the host would mean the
deferred SError is as it was on return to the host.

If there is a pending physical SError, we'd have to keep it pending so
the host can consume it. __guest_enter has the dsb-isb for RAS so
SErrors will become pending, but it doesn't consume them. I can't
remember now whether this was reliable or not; I assumed it was as it is
gated on the RAS config.

The above didn't need an ESB for the host but incorrect assumptions
might change that.

> Furthermore, assuming you find a way to do it, what do you do with it?
> 
> (a) Either there was something pending already and it is still pending,

If a physical SError is pending, you leave it pending and go back to the
host so it can consume it.

> (b) Or there was nothing pending and you now have an error that you
>     don't know how to report (the host EL1 never issued an ESB)

If there isn't a physical SError pending, either there is no SError at
all (easy) or the SError has already been consumed to DISR_EL1 by a host
ESB and we'd preserve DISR_EL1 for the host to handle however it chooses.

> We could just error out on hypercalls if DISR_EL1 is non-zero, but
> I don't see how we do that for traps, as it would just confuse the host
> EL1.

Traps would need to be left pending. Detected but not consumed with the
dsb-isb in __guest_enter.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: nVHE: Don't consume host SErrors with RAS

[James Morse]

Hi Andrew,

On 30/07/2020 16:18, Andrew Scull wrote:
> The ESB at the start of the vectors causes any SErrors to be consumed to
> DISR_EL1. If the exception came from the host and the ESB caught an
> SError, it would not be noticed until a guest exits and DISR_EL1 is
> checked. Further, the SError would be attributed to the guest and not
> the host.

Yup, this happens because the world has moved underneath this code:

Previously any v8.2 RAS capable system would have been crazy to turn off VHE, so v8.0 and
v8.1 systems had a nop here instead, and v8.2 systems had VHE, so there were no 'from the
host' EL2 vectors.


> To avoid these problems, use a different exception vector for the host
> that does not use an ESB but instead leaves any host SError pending. A
> guest will not be entered if an SError is pending so it will always be
> the host that will receive and handle it.
> 
> Hyp initialization is now passed the vector that is used for the host
> and the vector for guests is stored in a percpu variable as
> kvm_get_hyp_vector() is not suitable for calling from nVHE hyp.

> Fixes: 0e5b9c085dce ("KVM: arm64: Consume pending SError as early as possible")

Surely this can only happen if you had turned VHE off?


Thanks,

James
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: nVHE: Don't consume host SErrors with RAS

[James Morse]

Hi Andrew,

On 30/07/2020 23:31, Andrew Scull wrote:
> On Thu, Jul 30, 2020 at 04:18:23PM +0100, Andrew Scull wrote:
>> The ESB at the start of the vectors causes any SErrors to be consumed to
>> DISR_EL1. If the exception came from the host and the ESB caught an
>> SError, it would not be noticed until a guest exits and DISR_EL1 is
>> checked. Further, the SError would be attributed to the guest and not
>> the host.
>>
>> To avoid these problems, use a different exception vector for the host
>> that does not use an ESB but instead leaves any host SError pending. A
>> guest will not be entered if an SError is pending so it will always be
>> the host that will receive and handle it.

> Thinking further, I'm not sure this actually solves all of the problem.
> It does prevent hyp from causing a host SError to be consumed but, IIUC,
> there could be an SError already deferred by the host and logged in
> DISR_EL1 that hyp would not preserve if a guest is run.

I think that would be a host bug.

The ESB-instruction is the only thing that writes to DISR_EL1, and only if PSTATE.A is
set. The host should:
* Read DISR_EL1 after running the ESB-instruction,
* Not call into HYP from SError masked code!

(VHE only does it to match the nVHE behaviour, as KVM isn't prepared to handle these).


'ESB-instruction' is pedantry to avoid the risk of it being confused with IESB, which is
just the barrier bit, not the writes-to-DISR bit.


> I think the host's DISR_EL1 would need to be saved and restored in the
> vcpu context switch which, from a cursory read of the ARM, is possible
> without having to virtualize SErrors for the host.

... I thought this was a redirected register. Reads from EL1 when HCR_EL2.AMO is set get
the value from VDISR_EL2, meaning the guest can't read DISR_EL1 at all.
(see 'Accessing DISR_EL1' in the register description, "D13.7.1
DISR_EL1, Deferred Interrupt Status Register" of DDI0487F.a


>> Hyp initialization is now passed the vector that is used for the host
>> and the vector for guests is stored in a percpu variable as
>> kvm_get_hyp_vector() is not suitable for calling from nVHE hyp.


Thanks,

James
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: nVHE: Don't consume host SErrors with RAS

[James Morse]

Hi Andrew,

On 31/07/2020 11:20, Andrew Scull wrote:
> On Fri, Jul 31, 2020 at 09:00:03AM +0100, Marc Zyngier wrote:
>> On 2020-07-30 23:31, Andrew Scull wrote:
>>> On Thu, Jul 30, 2020 at 04:18:23PM +0100, Andrew Scull wrote:
>>>> The ESB at the start of the vectors causes any SErrors to be
>>>> consumed to
>>>> DISR_EL1. If the exception came from the host and the ESB caught an
>>>> SError, it would not be noticed until a guest exits and DISR_EL1 is
>>>> checked. Further, the SError would be attributed to the guest and not
>>>> the host.
>>>>
>>>> To avoid these problems, use a different exception vector for the host
>>>> that does not use an ESB but instead leaves any host SError pending. A
>>>> guest will not be entered if an SError is pending so it will always be
>>>> the host that will receive and handle it.
>>>
>>> Thinking further, I'm not sure this actually solves all of the problem.
>>> It does prevent hyp from causing a host SError to be consumed but, IIUC,
>>> there could be an SError already deferred by the host and logged in
>>> DISR_EL1 that hyp would not preserve if a guest is run.
>>>
>>> I think the host's DISR_EL1 would need to be saved and restored in the
>>> vcpu context switch which, from a cursory read of the ARM, is possible
>>> without having to virtualize SErrors for the host.
>>
>> The question is what do you if you have something pending in DISR_EL1
>> at the point where you enter EL2? Context switching it is not going to
>> help. One problem is that you'd need to do an ESB, corrupting DISR_EL1,
>> before any memory access (I'm assuming you can get traps where all
>> registers are live). I can't see how we square this circle.

> I'll expand on what I understand (or think I do) about RAS at the
> moment. It should hopefully highlight anything wrong with my reasoning
> for your questions.
> 
> DISR_EL1.A being set means a pending physical SError has been
> consumed/cleared. The host has already deferred an SError so saving and
> restoring (i.e. preserving) DISR_EL1 for the host would mean the
> deferred SError is as it was on return to the host.

[..]

> If there is a pending physical SError, we'd have to keep it pending so
> the host can consume it.

I agree!


> __guest_enter has the dsb-isb for RAS so
> SErrors will become pending, but it doesn't consume them.


> I can't
> remember now whether this was reliable or not; I assumed it was as it is
> gated on the RAS config.

It should be reliable for 'asynchronous external abort', but not 'SError Interrupt', both
of which are signalled via the SError vector.

The DSB completes previous memory accesses, so that the components they land in can return
an abort if they need to. The ISB ensures the abort is seen as pending in ISR_EL1 before
the CPU reads it.

The ESB-instruction is potentially cheaper, (as cheap as a nop if all errors are going to
be reported as uncontained anyway), but it has the nasty side effect of consuming the
error, which we don't want here.


> The above didn't need an ESB for the host but incorrect assumptions
> might change that.
> 
>> Furthermore, assuming you find a way to do it, what do you do with it?
>>
>> (a) Either there was something pending already and it is still pending,

> If a physical SError is pending, you leave it pending and go back to the
> host so it can consume it.

Great idea!
If the CPU has IESB, you can just check ISR_EL1 on entry from the host.
If not, you'll need some extra barriers.


>> (b) Or there was nothing pending and you now have an error that you
>>     don't know how to report (the host EL1 never issued an ESB)
> 
> If there isn't a physical SError pending, either there is no SError at
> all (easy) or the SError has already been consumed to DISR_EL1 by a host
> ESB and we'd preserve DISR_EL1 for the host to handle however it chooses.

(I think this is a host bug)


>> We could just error out on hypercalls if DISR_EL1 is non-zero, but
>> I don't see how we do that for traps, as it would just confuse the host
>> EL1.
> 
> Traps would need to be left pending. Detected but not consumed with the
> dsb-isb in __guest_enter.

You're trapping stuff from the host?

If you might trap something between the ESB-instruction and the host reading DISR_EL1 then
you can't use the ESB-instruction in your trap handling path, as it would over-write it.
This sucks as presumably you want to make it common with the really-a-guest trap handling
path.

The best thing to do would be to go the whole-hog and route SError to EL2 with
HCR_EL2.AMO. SError taken out of the host can be re-injected with HCR_EL2.VSE and
VSESR_EL2, which will be consumed to VDISR_EL2 if the 'host' executes an ESB-instruction.

DISR_EL1 then belongs to EL2, meaning its free to use the ESB-instruction as it likes.


Thanks,

James
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: nVHE: Don't consume host SErrors with RAS

[Andrew Scull]

On Wed, Aug 05, 2020 at 03:33:22PM +0100, James Morse wrote:
> Hi Andrew,
> 
> On 30/07/2020 16:18, Andrew Scull wrote:
> > The ESB at the start of the vectors causes any SErrors to be consumed to
> > DISR_EL1. If the exception came from the host and the ESB caught an
> > SError, it would not be noticed until a guest exits and DISR_EL1 is
> > checked. Further, the SError would be attributed to the guest and not
> > the host.
> 
> Yup, this happens because the world has moved underneath this code:

That's understandable!

> Previously any v8.2 RAS capable system would have been crazy to turn off VHE, so v8.0 and
> v8.1 systems had a nop here instead, and v8.2 systems had VHE, so there were no 'from the
> host' EL2 vectors.

RAS with nVHE is a buildable config and we're being foolish enough to
think about it so hopefully we can work something out :)

> > To avoid these problems, use a different exception vector for the host
> > that does not use an ESB but instead leaves any host SError pending. A
> > guest will not be entered if an SError is pending so it will always be
> > the host that will receive and handle it.
> > 
> > Hyp initialization is now passed the vector that is used for the host
> > and the vector for guests is stored in a percpu variable as
> > kvm_get_hyp_vector() is not suitable for calling from nVHE hyp.
> 
> > Fixes: 0e5b9c085dce ("KVM: arm64: Consume pending SError as early as possible")
> 
> Surely this can only happen if you had turned VHE off?

Correct, I'll make that clearer in the description.

> Thanks,
> 
> James
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: nVHE: Don't consume host SErrors with RAS

[Andrew Scull]

On Wed, Aug 05, 2020 at 03:34:11PM +0100, James Morse wrote:
> Hi Andrew,
> 
> On 30/07/2020 23:31, Andrew Scull wrote:
> > On Thu, Jul 30, 2020 at 04:18:23PM +0100, Andrew Scull wrote:
> >> The ESB at the start of the vectors causes any SErrors to be consumed to
> >> DISR_EL1. If the exception came from the host and the ESB caught an
> >> SError, it would not be noticed until a guest exits and DISR_EL1 is
> >> checked. Further, the SError would be attributed to the guest and not
> >> the host.
> >>
> >> To avoid these problems, use a different exception vector for the host
> >> that does not use an ESB but instead leaves any host SError pending. A
> >> guest will not be entered if an SError is pending so it will always be
> >> the host that will receive and handle it.
> 
> > Thinking further, I'm not sure this actually solves all of the problem.
> > It does prevent hyp from causing a host SError to be consumed but, IIUC,
> > there could be an SError already deferred by the host and logged in
> > DISR_EL1 that hyp would not preserve if a guest is run.
> 
> I think that would be a host bug.
> 
> The ESB-instruction is the only thing that writes to DISR_EL1, and only if PSTATE.A is
> set. The host should:
> * Read DISR_EL1 after running the ESB-instruction,
> * Not call into HYP from SError masked code!

Good to know that this is the intent and not just what appears to happen
today.

> (VHE only does it to match the nVHE behaviour, as KVM isn't prepared to handle these).
> 
> 
> 'ESB-instruction' is pedantry to avoid the risk of it being confused with IESB, which is
> just the barrier bit, not the writes-to-DISR bit.
> 
> 
> > I think the host's DISR_EL1 would need to be saved and restored in the
> > vcpu context switch which, from a cursory read of the ARM, is possible
> > without having to virtualize SErrors for the host.
> 
> ... I thought this was a redirected register. Reads from EL1 when HCR_EL2.AMO is set get
> the value from VDISR_EL2, meaning the guest can't read DISR_EL1 at all.
> (see 'Accessing DISR_EL1' in the register description, "D13.7.1
> DISR_EL1, Deferred Interrupt Status Register" of DDI0487F.a

The host doesn't run with HCR_EL2.AMO set so it uses DISR_EL1 directly,
but hyp also uses DISR_EL1 directly during __guest_exit. That is the
clobbering I was concerned about. It may not be a problem most of the
time given what you said above, but I think something like the diff
below should be enough to be sure it is preserved:

 arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
index 28f349288f3a..a34210c1c877 100644
--- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
+++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
@@ -56,8 +56,12 @@ static inline void __sysreg_save_el2_return_state(struct kvm_cpu_context *ctxt)
 	ctxt->regs.pc			= read_sysreg_el2(SYS_ELR);
 	ctxt->regs.pstate		= read_sysreg_el2(SYS_SPSR);
 
-	if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN))
-		ctxt_sys_reg(ctxt, DISR_EL1) = read_sysreg_s(SYS_VDISR_EL2);
+	if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN)) {
+		if (kvm_is_host_cpu_context(ctxt))
+			ctxt_sys_reg(ctxt, DISR_EL1) = read_sysreg_s(SYS_DISR_EL1);
+		else
+			ctxt_sys_reg(ctxt, DISR_EL1) = read_sysreg_s(SYS_VDISR_EL2);
+	}
 }
 
 static inline void __sysreg_restore_common_state(struct kvm_cpu_context *ctxt)
@@ -152,8 +156,12 @@ static inline void __sysreg_restore_el2_return_state(struct kvm_cpu_context *ctx
 	write_sysreg_el2(ctxt->regs.pc,			SYS_ELR);
 	write_sysreg_el2(pstate,			SYS_SPSR);
 
-	if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN))
-		write_sysreg_s(ctxt_sys_reg(ctxt, DISR_EL1), SYS_VDISR_EL2);
+	if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN)) {
+		if (kvm_is_host_cpu_context(ctxt))
+			write_sysreg_s(ctxt_sys_reg(ctxt, DISR_EL1), SYS_DISR_EL1);
+		else
+			write_sysreg_s(ctxt_sys_reg(ctxt, DISR_EL1), SYS_VDISR_EL2);
+	}
 }
 
 static inline void __sysreg32_save_state(struct kvm_vcpu *vcpu)


> >> Hyp initialization is now passed the vector that is used for the host
> >> and the vector for guests is stored in a percpu variable as
> >> kvm_get_hyp_vector() is not suitable for calling from nVHE hyp.
> 
> 
> Thanks,
> 
> James
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: nVHE: Don't consume host SErrors with RAS

[Andrew Scull]

On Wed, Aug 05, 2020 at 03:37:27PM +0100, James Morse wrote:
> Hi Andrew,
> 
> On 31/07/2020 11:20, Andrew Scull wrote:
> > On Fri, Jul 31, 2020 at 09:00:03AM +0100, Marc Zyngier wrote:
> >> On 2020-07-30 23:31, Andrew Scull wrote:
> >>> On Thu, Jul 30, 2020 at 04:18:23PM +0100, Andrew Scull wrote:
> >>>> The ESB at the start of the vectors causes any SErrors to be
> >>>> consumed to
> >>>> DISR_EL1. If the exception came from the host and the ESB caught an
> >>>> SError, it would not be noticed until a guest exits and DISR_EL1 is
> >>>> checked. Further, the SError would be attributed to the guest and not
> >>>> the host.
> >>>>
> >>>> To avoid these problems, use a different exception vector for the host
> >>>> that does not use an ESB but instead leaves any host SError pending. A
> >>>> guest will not be entered if an SError is pending so it will always be
> >>>> the host that will receive and handle it.
> >>>
> >>> Thinking further, I'm not sure this actually solves all of the problem.
> >>> It does prevent hyp from causing a host SError to be consumed but, IIUC,
> >>> there could be an SError already deferred by the host and logged in
> >>> DISR_EL1 that hyp would not preserve if a guest is run.
> >>>
> >>> I think the host's DISR_EL1 would need to be saved and restored in the
> >>> vcpu context switch which, from a cursory read of the ARM, is possible
> >>> without having to virtualize SErrors for the host.
> >>
> >> The question is what do you if you have something pending in DISR_EL1
> >> at the point where you enter EL2? Context switching it is not going to
> >> help. One problem is that you'd need to do an ESB, corrupting DISR_EL1,
> >> before any memory access (I'm assuming you can get traps where all
> >> registers are live). I can't see how we square this circle.
> 
> > I'll expand on what I understand (or think I do) about RAS at the
> > moment. It should hopefully highlight anything wrong with my reasoning
> > for your questions.
> > 
> > DISR_EL1.A being set means a pending physical SError has been
> > consumed/cleared. The host has already deferred an SError so saving and
> > restoring (i.e. preserving) DISR_EL1 for the host would mean the
> > deferred SError is as it was on return to the host.
> 
> [..]
> 
> > If there is a pending physical SError, we'd have to keep it pending so
> > the host can consume it.
> 
> I agree!
> 
> 
> > __guest_enter has the dsb-isb for RAS so
> > SErrors will become pending, but it doesn't consume them.
> 
> 
> > I can't
> > remember now whether this was reliable or not; I assumed it was as it is
> > gated on the RAS config.
> 
> It should be reliable for 'asynchronous external abort', but not 'SError Interrupt', both
> of which are signalled via the SError vector.

Uh oh, more terms with differences that I don't understand yet.

I assume that there aren't SError Interrupts that we would be concerned
about being deliered to the guest that should have been delivered to the
host after checing for asynchronous external aborts? At least, that seem
to be the current behaviour if I'm reading things right.

> The DSB completes previous memory accesses, so that the components they land in can return
> an abort if they need to. The ISB ensures the abort is seen as pending in ISR_EL1 before
> the CPU reads it.
> 
> The ESB-instruction is potentially cheaper, (as cheap as a nop if all errors are going to
> be reported as uncontained anyway), but it has the nasty side effect of consuming the
> error, which we don't want here.
>
>
> > The above didn't need an ESB for the host but incorrect assumptions
> > might change that.
> > 
> >> Furthermore, assuming you find a way to do it, what do you do with it?
> >>
> >> (a) Either there was something pending already and it is still pending,
> 
> > If a physical SError is pending, you leave it pending and go back to the
> > host so it can consume it.
> 
> Great idea!
> If the CPU has IESB, you can just check ISR_EL1 on entry from the host.
> If not, you'll need some extra barriers.

I'm not sure it would be easy to check ISR_EL1 and bail to the host in
the general case as it would require a meaningful return code for each
context, something like EAGAIN that doesn't exist in the hyp interfaces.

I was instead thinking that EL2 would continue as usual except it won't
consume any SErrors with the ESB. Care is needed when a vCPU is being
run and the DSB-ISB + ISR_EL1 check in __guest_enter is be there to
prevent a guest being entered if the host needs to handle an SError.

> >> (b) Or there was nothing pending and you now have an error that you
> >>     don't know how to report (the host EL1 never issued an ESB)
> > 
> > If there isn't a physical SError pending, either there is no SError at
> > all (easy) or the SError has already been consumed to DISR_EL1 by a host
> > ESB and we'd preserve DISR_EL1 for the host to handle however it chooses.
> 
> (I think this is a host bug)
> 
> 
> >> We could just error out on hypercalls if DISR_EL1 is non-zero, but
> >> I don't see how we do that for traps, as it would just confuse the host
> >> EL1.
> > 
> > Traps would need to be left pending. Detected but not consumed with the
> > dsb-isb in __guest_enter.
> 
> You're trapping stuff from the host?

Not yet, but it might happen e.g. to context switch FPSIMD under
protected KVM.

> If you might trap something between the ESB-instruction and the host reading DISR_EL1 then
> you can't use the ESB-instruction in your trap handling path, as it would over-write it.
> This sucks as presumably you want to make it common with the really-a-guest trap handling
> path.
> 
> The best thing to do would be to go the whole-hog and route SError to EL2 with
> HCR_EL2.AMO. SError taken out of the host can be re-injected with HCR_EL2.VSE and
> VSESR_EL2, which will be consumed to VDISR_EL2 if the 'host' executes an ESB-instruction.
> 
> DISR_EL1 then belongs to EL2, meaning its free to use the ESB-instruction as it likes.

Does a save and restore of DISR_EL1 (see other thread on this series)
address the concerns here or would you still think it better to start
virtualizing for the host?

> Thanks,
> 
> James
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: nVHE: Don't consume host SErrors with RAS

[James Morse]

Hi Andrew,

On 11/08/2020 16:12, Andrew Scull wrote:
> On Wed, Aug 05, 2020 at 03:37:27PM +0100, James Morse wrote:
>> On 31/07/2020 11:20, Andrew Scull wrote:
>>> If there is a pending physical SError, we'd have to keep it pending so
>>> the host can consume it.

>>> __guest_enter has the dsb-isb for RAS so
>>> SErrors will become pending, but it doesn't consume them.

>>> I can't
>>> remember now whether this was reliable or not; I assumed it was as it is
>>> gated on the RAS config.
>>
>> It should be reliable for 'asynchronous external abort', but not 'SError Interrupt', both
>> of which are signalled via the SError vector.
> 
> Uh oh, more terms with differences that I don't understand yet.

> I assume that there aren't SError Interrupts that we would be concerned
> about being deliered to the guest that should have been delivered to the
> host after checing for asynchronous external aborts? At least, that seem
> to be the current behaviour if I'm reading things right.

Currently KVM assume everything is an asynchronous-external-abort as that was the pre-RAS
behaviour. SError interrupt means something along the lines of 'part of the SoC fell out',
and I expect we're not going to get very far before we trip over the problem again.
If we don't, its probably something related to the guest.

With v8.2 we can tell these things apart, so we could try harder, but I don't think
anything uses SError-Interrupt. Its much more likely that catastrophic errors are taken to
EL3 firmware, or handled by a dedicated microcontroller.


>> The DSB completes previous memory accesses, so that the components they land in can return
>> an abort if they need to. The ISB ensures the abort is seen as pending in ISR_EL1 before
>> the CPU reads it.
>>
>> The ESB-instruction is potentially cheaper, (as cheap as a nop if all errors are going to
>> be reported as uncontained anyway), but it has the nasty side effect of consuming the
>> error, which we don't want here.
>>
>>
>>> The above didn't need an ESB for the host but incorrect assumptions
>>> might change that.
>>>
>>>> Furthermore, assuming you find a way to do it, what do you do with it?
>>>>
>>>> (a) Either there was something pending already and it is still pending,
>>
>>> If a physical SError is pending, you leave it pending and go back to the
>>> host so it can consume it.
>>
>> Great idea!
>> If the CPU has IESB, you can just check ISR_EL1 on entry from the host.
>> If not, you'll need some extra barriers.

> I'm not sure it would be easy to check ISR_EL1 and bail to the host in
> the general case as it would require a meaningful return code for each
> context, something like EAGAIN that doesn't exist in the hyp interfaces.

I guess whether its a problem depends on how much work we're going to do. It doesn't feel
right to carry on once we know something is burning.

If its a bug to make an HVC from SError-masked code, this would only happen if IESB
synchronised the error.

For __kvm_vcpu_run_*(), we could return immediately as if we'd taken an IRQ out of the
guest, this is pretty much the trick __guest_enter pulls. We should probably do this today.

For others, we could rewind ELR_EL2 and forcibly emulate the exception back into the host.
If it returns, it runs the HVC again. Its probably not worth the effort, unless we do the
same for the guest.... which won't promise not to do this from masked


> I was instead thinking that EL2 would continue as usual except it won't
> consume any SErrors with the ESB.
What we don't know can't hurt us! I think this is fine provided we don't end up making the
blackout window huge. A whole set of extra vectors is I guess the cost of this.


Will KVM ever want to handle its own errors? (it doesn't today).

If its okay to merge KVM's asynchronous external abort and the hosts, then I agree this
works, we'd need to be careful not to unmask SError or run ESB.

If KVM wants to know if its an asynchronous external abort that it caused, you'd need to
virtualise the errors for the host.


>>>> (b) Or there was nothing pending and you now have an error that you
>>>>     don't know how to report (the host EL1 never issued an ESB)
>>>
>>> If there isn't a physical SError pending, either there is no SError at
>>> all (easy) or the SError has already been consumed to DISR_EL1 by a host
>>> ESB and we'd preserve DISR_EL1 for the host to handle however it chooses.
>>
>> (I think this is a host bug)
>>
>>
>>>> We could just error out on hypercalls if DISR_EL1 is non-zero, but
>>>> I don't see how we do that for traps, as it would just confuse the host
>>>> EL1.
>>>
>>> Traps would need to be left pending. Detected but not consumed with the
>>> dsb-isb in __guest_enter.
>>
>> You're trapping stuff from the host?
> 
> Not yet, but it might happen e.g. to context switch FPSIMD under
> protected KVM.

Sounds fun.


>> If you might trap something between the ESB-instruction and the host reading DISR_EL1 then
>> you can't use the ESB-instruction in your trap handling path, as it would over-write it.
>> This sucks as presumably you want to make it common with the really-a-guest trap handling
>> path.
>>
>> The best thing to do would be to go the whole-hog and route SError to EL2 with
>> HCR_EL2.AMO. SError taken out of the host can be re-injected with HCR_EL2.VSE and
>> VSESR_EL2, which will be consumed to VDISR_EL2 if the 'host' executes an ESB-instruction.
>>
>> DISR_EL1 then belongs to EL2, meaning its free to use the ESB-instruction as it likes.
> 
> Does a save and restore of DISR_EL1 (see other thread on this series)
> address the concerns here or would you still think it better to start
> virtualizing for the host?

Other than trapping things from the host, I'm confused as to the circumstances that
DISR_EL1 would get clobbered. (is there an example?)

Isn't that unnecessary in the "EL2 would continue as usual except it won't
consume any SErrors with the ESB." world?


DISR_EL1 belongs to the thing that is consuming SError for the platform. The hardware
takes care of ensuring the guest can never corrupt this. KVMs EL2 code is part of the
host, so it magically knows its not overwriting anything when uses the ESB-instruction.
I can see how trapping stuff from the host would change that.

Save restoring it looks really weird, having two things consuming errors leads to problems
around who consumes what. Not handling the DISR_EL1 value as quickly as possible looks
problematic.

(I don't think I'm answering the question here).


For trapping, or KVM detecting its own asynchronous-external-abort, I think you need to
turn the virtualisation on. Otherwise its avoiding ESB-instructions and unmasking SError,
which comes with the cost of special copies of things like the vectors.


Thanks,

James
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: nVHE: Don't consume host SErrors with RAS

[James Morse]

Hi Andrew,

On 11/08/2020 15:53, Andrew Scull wrote:
> On Wed, Aug 05, 2020 at 03:34:11PM +0100, James Morse wrote:
>> On 30/07/2020 23:31, Andrew Scull wrote:
>>> On Thu, Jul 30, 2020 at 04:18:23PM +0100, Andrew Scull wrote:
>>>> The ESB at the start of the vectors causes any SErrors to be consumed to
>>>> DISR_EL1. If the exception came from the host and the ESB caught an

>>> I think the host's DISR_EL1 would need to be saved and restored in the
>>> vcpu context switch which, from a cursory read of the ARM, is possible
>>> without having to virtualize SErrors for the host.
>>
>> ... I thought this was a redirected register. Reads from EL1 when HCR_EL2.AMO is set get
>> the value from VDISR_EL2, meaning the guest can't read DISR_EL1 at all.
>> (see 'Accessing DISR_EL1' in the register description, "D13.7.1
>> DISR_EL1, Deferred Interrupt Status Register" of DDI0487F.a

> The host doesn't run with HCR_EL2.AMO set so it uses DISR_EL1 directly,
> but hyp also uses DISR_EL1 directly during __guest_exit. That is the
> clobbering I was concerned about. It may not be a problem most of the
> time given what you said above, but I think something like the diff
> below should be enough to be sure it is preserved:

On guest-exit, can't we just clobber the register, and make it the hosts problem to ensure
it always cleared it after use? (as it does today).

Save restore here is extra work to do all the time, to preserve a value the the host
should never need anyway.

I think this is just another weird case where the host really isn't like a vcpu.


Thanks,

James
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm