[PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Marc Zyngier]

The kernel has an awfully complicated boot sequence in order to cope
with the various EL2 configurations, including those that "enhanced"
the architecture. We go from EL2 to EL1, then back to EL2, staying
at EL2 if VHE capable and otherwise go back to EL1.

Here's a paracetamol tablet for you.

The cpu_resume path follows the same logic, because coming up with
two versions of a square wheel is hard.

However, things aren't this straightforward with pKVM, as the host
resume path is always proxied by the hypervisor, which means that
the kernel is always entered at EL1. Which contradicts what the
__boot_cpu_mode[] array contains (it obviously says EL2).

This thus triggers a HVC call from EL1 to EL2 in a vain attempt
to upgrade from EL1 to EL2 VHE, which we are, funnily enough,
reluctant to grant to the host kernel. This is also completely
unexpected, and puzzles your average EL2 hacker.

Address it by fixing up the boot mode at the point the host gets
deprivileged. is_hyp_mode_available() and co already have a static
branch to deal with this, making it pretty safe.

Reported-by: Vincent Donnefort <vdonnefort@google.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/arm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index b6c9bfa8492f..cf075c9b9ab1 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -2107,6 +2107,17 @@ static int pkvm_drop_host_privileges(void)
 	 * once the host stage 2 is installed.
 	 */
 	static_branch_enable(&kvm_protected_mode_initialized);
+
+	/*
+	 * Fixup the boot mode so that we don't take spurious round
+	 * trips via EL2 on cpu_resume. Flush to the PoC for a good
+	 * measure, so that it can be observed by a CPU coming out of
+	 * suspend with the MMU off.
+	 */
+	__boot_cpu_mode[0] = __boot_cpu_mode[1] = BOOT_CPU_MODE_EL1;
+	dcache_clean_poc((unsigned long)__boot_cpu_mode,
+			 (unsigned long)(__boot_cpu_mode + 2));
+
 	on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
 	return ret;
 }
-- 
2.34.1

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

[PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Marc Zyngier]

The kernel has an awfully complicated boot sequence in order to cope
with the various EL2 configurations, including those that "enhanced"
the architecture. We go from EL2 to EL1, then back to EL2, staying
at EL2 if VHE capable and otherwise go back to EL1.

Here's a paracetamol tablet for you.

The cpu_resume path follows the same logic, because coming up with
two versions of a square wheel is hard.

However, things aren't this straightforward with pKVM, as the host
resume path is always proxied by the hypervisor, which means that
the kernel is always entered at EL1. Which contradicts what the
__boot_cpu_mode[] array contains (it obviously says EL2).

This thus triggers a HVC call from EL1 to EL2 in a vain attempt
to upgrade from EL1 to EL2 VHE, which we are, funnily enough,
reluctant to grant to the host kernel. This is also completely
unexpected, and puzzles your average EL2 hacker.

Address it by fixing up the boot mode at the point the host gets
deprivileged. is_hyp_mode_available() and co already have a static
branch to deal with this, making it pretty safe.

Reported-by: Vincent Donnefort <vdonnefort@google.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/arm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index b6c9bfa8492f..cf075c9b9ab1 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -2107,6 +2107,17 @@ static int pkvm_drop_host_privileges(void)
 	 * once the host stage 2 is installed.
 	 */
 	static_branch_enable(&kvm_protected_mode_initialized);
+
+	/*
+	 * Fixup the boot mode so that we don't take spurious round
+	 * trips via EL2 on cpu_resume. Flush to the PoC for a good
+	 * measure, so that it can be observed by a CPU coming out of
+	 * suspend with the MMU off.
+	 */
+	__boot_cpu_mode[0] = __boot_cpu_mode[1] = BOOT_CPU_MODE_EL1;
+	dcache_clean_poc((unsigned long)__boot_cpu_mode,
+			 (unsigned long)(__boot_cpu_mode + 2));
+
 	on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
 	return ret;
 }
-- 
2.34.1

[PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Marc Zyngier]

The kernel has an awfully complicated boot sequence in order to cope
with the various EL2 configurations, including those that "enhanced"
the architecture. We go from EL2 to EL1, then back to EL2, staying
at EL2 if VHE capable and otherwise go back to EL1.

Here's a paracetamol tablet for you.

The cpu_resume path follows the same logic, because coming up with
two versions of a square wheel is hard.

However, things aren't this straightforward with pKVM, as the host
resume path is always proxied by the hypervisor, which means that
the kernel is always entered at EL1. Which contradicts what the
__boot_cpu_mode[] array contains (it obviously says EL2).

This thus triggers a HVC call from EL1 to EL2 in a vain attempt
to upgrade from EL1 to EL2 VHE, which we are, funnily enough,
reluctant to grant to the host kernel. This is also completely
unexpected, and puzzles your average EL2 hacker.

Address it by fixing up the boot mode at the point the host gets
deprivileged. is_hyp_mode_available() and co already have a static
branch to deal with this, making it pretty safe.

Reported-by: Vincent Donnefort <vdonnefort@google.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/arm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index b6c9bfa8492f..cf075c9b9ab1 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -2107,6 +2107,17 @@ static int pkvm_drop_host_privileges(void)
 	 * once the host stage 2 is installed.
 	 */
 	static_branch_enable(&kvm_protected_mode_initialized);
+
+	/*
+	 * Fixup the boot mode so that we don't take spurious round
+	 * trips via EL2 on cpu_resume. Flush to the PoC for a good
+	 * measure, so that it can be observed by a CPU coming out of
+	 * suspend with the MMU off.
+	 */
+	__boot_cpu_mode[0] = __boot_cpu_mode[1] = BOOT_CPU_MODE_EL1;
+	dcache_clean_poc((unsigned long)__boot_cpu_mode,
+			 (unsigned long)(__boot_cpu_mode + 2));
+
 	on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
 	return ret;
 }
-- 
2.34.1


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

Re: [PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Oliver Upton]

On Tue, Oct 11, 2022 at 05:54:00PM +0100, Marc Zyngier wrote:
> The kernel has an awfully complicated boot sequence in order to cope
> with the various EL2 configurations, including those that "enhanced"
> the architecture. We go from EL2 to EL1, then back to EL2, staying
> at EL2 if VHE capable and otherwise go back to EL1.
> 
> Here's a paracetamol tablet for you.

Heh, still have a bit of a headache from this :)

I'm having a hard time following where we skip the EL2 promotion based
on __boot_cpu_mode.

On the cpu_resume() path it looks like we take the return of
init_kernel_el() and pass that along to finalise_el2(). As we are in EL1
at this point, it seems like we'd go init_kernel_el() -> init_el1().

What am I missing?

--
Thanks,
Oliver

> The cpu_resume path follows the same logic, because coming up with
> two versions of a square wheel is hard.
> 
> However, things aren't this straightforward with pKVM, as the host
> resume path is always proxied by the hypervisor, which means that
> the kernel is always entered at EL1. Which contradicts what the
> __boot_cpu_mode[] array contains (it obviously says EL2).
> 
> This thus triggers a HVC call from EL1 to EL2 in a vain attempt
> to upgrade from EL1 to EL2 VHE, which we are, funnily enough,
> reluctant to grant to the host kernel. This is also completely
> unexpected, and puzzles your average EL2 hacker.
> 
> Address it by fixing up the boot mode at the point the host gets
> deprivileged. is_hyp_mode_available() and co already have a static
> branch to deal with this, making it pretty safe.
> 
> Reported-by: Vincent Donnefort <vdonnefort@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/arm.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index b6c9bfa8492f..cf075c9b9ab1 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -2107,6 +2107,17 @@ static int pkvm_drop_host_privileges(void)
>  	 * once the host stage 2 is installed.
>  	 */
>  	static_branch_enable(&kvm_protected_mode_initialized);
> +
> +	/*
> +	 * Fixup the boot mode so that we don't take spurious round
> +	 * trips via EL2 on cpu_resume. Flush to the PoC for a good
> +	 * measure, so that it can be observed by a CPU coming out of
> +	 * suspend with the MMU off.
> +	 */
> +	__boot_cpu_mode[0] = __boot_cpu_mode[1] = BOOT_CPU_MODE_EL1;
> +	dcache_clean_poc((unsigned long)__boot_cpu_mode,
> +			 (unsigned long)(__boot_cpu_mode + 2));
> +
>  	on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
>  	return ret;
>  }
> -- 
> 2.34.1
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Oliver Upton]

On Tue, Oct 11, 2022 at 05:54:00PM +0100, Marc Zyngier wrote:
> The kernel has an awfully complicated boot sequence in order to cope
> with the various EL2 configurations, including those that "enhanced"
> the architecture. We go from EL2 to EL1, then back to EL2, staying
> at EL2 if VHE capable and otherwise go back to EL1.
> 
> Here's a paracetamol tablet for you.

Heh, still have a bit of a headache from this :)

I'm having a hard time following where we skip the EL2 promotion based
on __boot_cpu_mode.

On the cpu_resume() path it looks like we take the return of
init_kernel_el() and pass that along to finalise_el2(). As we are in EL1
at this point, it seems like we'd go init_kernel_el() -> init_el1().

What am I missing?

--
Thanks,
Oliver

> The cpu_resume path follows the same logic, because coming up with
> two versions of a square wheel is hard.
> 
> However, things aren't this straightforward with pKVM, as the host
> resume path is always proxied by the hypervisor, which means that
> the kernel is always entered at EL1. Which contradicts what the
> __boot_cpu_mode[] array contains (it obviously says EL2).
> 
> This thus triggers a HVC call from EL1 to EL2 in a vain attempt
> to upgrade from EL1 to EL2 VHE, which we are, funnily enough,
> reluctant to grant to the host kernel. This is also completely
> unexpected, and puzzles your average EL2 hacker.
> 
> Address it by fixing up the boot mode at the point the host gets
> deprivileged. is_hyp_mode_available() and co already have a static
> branch to deal with this, making it pretty safe.
> 
> Reported-by: Vincent Donnefort <vdonnefort@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/arm.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index b6c9bfa8492f..cf075c9b9ab1 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -2107,6 +2107,17 @@ static int pkvm_drop_host_privileges(void)
>  	 * once the host stage 2 is installed.
>  	 */
>  	static_branch_enable(&kvm_protected_mode_initialized);
> +
> +	/*
> +	 * Fixup the boot mode so that we don't take spurious round
> +	 * trips via EL2 on cpu_resume. Flush to the PoC for a good
> +	 * measure, so that it can be observed by a CPU coming out of
> +	 * suspend with the MMU off.
> +	 */
> +	__boot_cpu_mode[0] = __boot_cpu_mode[1] = BOOT_CPU_MODE_EL1;
> +	dcache_clean_poc((unsigned long)__boot_cpu_mode,
> +			 (unsigned long)(__boot_cpu_mode + 2));
> +
>  	on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
>  	return ret;
>  }
> -- 
> 2.34.1
> 

Re: [PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Oliver Upton]

On Tue, Oct 11, 2022 at 05:54:00PM +0100, Marc Zyngier wrote:
> The kernel has an awfully complicated boot sequence in order to cope
> with the various EL2 configurations, including those that "enhanced"
> the architecture. We go from EL2 to EL1, then back to EL2, staying
> at EL2 if VHE capable and otherwise go back to EL1.
> 
> Here's a paracetamol tablet for you.

Heh, still have a bit of a headache from this :)

I'm having a hard time following where we skip the EL2 promotion based
on __boot_cpu_mode.

On the cpu_resume() path it looks like we take the return of
init_kernel_el() and pass that along to finalise_el2(). As we are in EL1
at this point, it seems like we'd go init_kernel_el() -> init_el1().

What am I missing?

--
Thanks,
Oliver

> The cpu_resume path follows the same logic, because coming up with
> two versions of a square wheel is hard.
> 
> However, things aren't this straightforward with pKVM, as the host
> resume path is always proxied by the hypervisor, which means that
> the kernel is always entered at EL1. Which contradicts what the
> __boot_cpu_mode[] array contains (it obviously says EL2).
> 
> This thus triggers a HVC call from EL1 to EL2 in a vain attempt
> to upgrade from EL1 to EL2 VHE, which we are, funnily enough,
> reluctant to grant to the host kernel. This is also completely
> unexpected, and puzzles your average EL2 hacker.
> 
> Address it by fixing up the boot mode at the point the host gets
> deprivileged. is_hyp_mode_available() and co already have a static
> branch to deal with this, making it pretty safe.
> 
> Reported-by: Vincent Donnefort <vdonnefort@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/arm.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index b6c9bfa8492f..cf075c9b9ab1 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -2107,6 +2107,17 @@ static int pkvm_drop_host_privileges(void)
>  	 * once the host stage 2 is installed.
>  	 */
>  	static_branch_enable(&kvm_protected_mode_initialized);
> +
> +	/*
> +	 * Fixup the boot mode so that we don't take spurious round
> +	 * trips via EL2 on cpu_resume. Flush to the PoC for a good
> +	 * measure, so that it can be observed by a CPU coming out of
> +	 * suspend with the MMU off.
> +	 */
> +	__boot_cpu_mode[0] = __boot_cpu_mode[1] = BOOT_CPU_MODE_EL1;
> +	dcache_clean_poc((unsigned long)__boot_cpu_mode,
> +			 (unsigned long)(__boot_cpu_mode + 2));
> +
>  	on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
>  	return ret;
>  }
> -- 
> 2.34.1
> 

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

Re: [PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Marc Zyngier]

On Tue, 11 Oct 2022 19:48:39 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Tue, Oct 11, 2022 at 05:54:00PM +0100, Marc Zyngier wrote:
> > The kernel has an awfully complicated boot sequence in order to cope
> > with the various EL2 configurations, including those that "enhanced"
> > the architecture. We go from EL2 to EL1, then back to EL2, staying
> > at EL2 if VHE capable and otherwise go back to EL1.
> > 
> > Here's a paracetamol tablet for you.
> 
> Heh, still have a bit of a headache from this :)
> 
> I'm having a hard time following where we skip the EL2 promotion based
> on __boot_cpu_mode.
> 
> On the cpu_resume() path it looks like we take the return of
> init_kernel_el() and pass that along to finalise_el2(). As we are in EL1
> at this point, it seems like we'd go init_kernel_el() -> init_el1().
> 
> What am I missing?

That I'm an idiot.

This is only necessary on pre-6.0, before 005e12676af0 ("arm64: head:
record CPU boot mode after enabling the MMU"), as this code-path
*used* to reload the boot mode from memory. Now, this is directly
passed as a parameter, making this patch useless.

The joys of looking at too many code bases at the same time... I'll
see how we can add it to 5.19.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Marc Zyngier]

On Tue, 11 Oct 2022 19:48:39 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Tue, Oct 11, 2022 at 05:54:00PM +0100, Marc Zyngier wrote:
> > The kernel has an awfully complicated boot sequence in order to cope
> > with the various EL2 configurations, including those that "enhanced"
> > the architecture. We go from EL2 to EL1, then back to EL2, staying
> > at EL2 if VHE capable and otherwise go back to EL1.
> > 
> > Here's a paracetamol tablet for you.
> 
> Heh, still have a bit of a headache from this :)
> 
> I'm having a hard time following where we skip the EL2 promotion based
> on __boot_cpu_mode.
> 
> On the cpu_resume() path it looks like we take the return of
> init_kernel_el() and pass that along to finalise_el2(). As we are in EL1
> at this point, it seems like we'd go init_kernel_el() -> init_el1().
> 
> What am I missing?

That I'm an idiot.

This is only necessary on pre-6.0, before 005e12676af0 ("arm64: head:
record CPU boot mode after enabling the MMU"), as this code-path
*used* to reload the boot mode from memory. Now, this is directly
passed as a parameter, making this patch useless.

The joys of looking at too many code bases at the same time... I'll
see how we can add it to 5.19.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Marc Zyngier]

On Tue, 11 Oct 2022 19:48:39 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Tue, Oct 11, 2022 at 05:54:00PM +0100, Marc Zyngier wrote:
> > The kernel has an awfully complicated boot sequence in order to cope
> > with the various EL2 configurations, including those that "enhanced"
> > the architecture. We go from EL2 to EL1, then back to EL2, staying
> > at EL2 if VHE capable and otherwise go back to EL1.
> > 
> > Here's a paracetamol tablet for you.
> 
> Heh, still have a bit of a headache from this :)
> 
> I'm having a hard time following where we skip the EL2 promotion based
> on __boot_cpu_mode.
> 
> On the cpu_resume() path it looks like we take the return of
> init_kernel_el() and pass that along to finalise_el2(). As we are in EL1
> at this point, it seems like we'd go init_kernel_el() -> init_el1().
> 
> What am I missing?

That I'm an idiot.

This is only necessary on pre-6.0, before 005e12676af0 ("arm64: head:
record CPU boot mode after enabling the MMU"), as this code-path
*used* to reload the boot mode from memory. Now, this is directly
passed as a parameter, making this patch useless.

The joys of looking at too many code bases at the same time... I'll
see how we can add it to 5.19.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

Re: [PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Vincent Donnefort]

On Tue, Oct 11, 2022 at 09:58:22PM +0100, Marc Zyngier wrote:
> On Tue, 11 Oct 2022 19:48:39 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > On Tue, Oct 11, 2022 at 05:54:00PM +0100, Marc Zyngier wrote:
> > > The kernel has an awfully complicated boot sequence in order to cope
> > > with the various EL2 configurations, including those that "enhanced"
> > > the architecture. We go from EL2 to EL1, then back to EL2, staying
> > > at EL2 if VHE capable and otherwise go back to EL1.
> > > 
> > > Here's a paracetamol tablet for you.
> > 
> > Heh, still have a bit of a headache from this :)
> > 
> > I'm having a hard time following where we skip the EL2 promotion based
> > on __boot_cpu_mode.
> > 
> > On the cpu_resume() path it looks like we take the return of
> > init_kernel_el() and pass that along to finalise_el2(). As we are in EL1
> > at this point, it seems like we'd go init_kernel_el() -> init_el1().
> > 
> > What am I missing?
> 
> That I'm an idiot.
> 
> This is only necessary on pre-6.0, before 005e12676af0 ("arm64: head:
> record CPU boot mode after enabling the MMU"), as this code-path
> *used* to reload the boot mode from memory. Now, this is directly
> passed as a parameter, making this patch useless.

On a 5.10 though, the suprious HVCs are gone and I have not observed any
regression.

Thanks!

For a stable fix:

Tested-by: Vincent Donnefort <vdonnefort@google.com>

> 
> The joys of looking at too many code bases at the same time... I'll
> see how we can add it to 5.19.
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Vincent Donnefort]

On Tue, Oct 11, 2022 at 09:58:22PM +0100, Marc Zyngier wrote:
> On Tue, 11 Oct 2022 19:48:39 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > On Tue, Oct 11, 2022 at 05:54:00PM +0100, Marc Zyngier wrote:
> > > The kernel has an awfully complicated boot sequence in order to cope
> > > with the various EL2 configurations, including those that "enhanced"
> > > the architecture. We go from EL2 to EL1, then back to EL2, staying
> > > at EL2 if VHE capable and otherwise go back to EL1.
> > > 
> > > Here's a paracetamol tablet for you.
> > 
> > Heh, still have a bit of a headache from this :)
> > 
> > I'm having a hard time following where we skip the EL2 promotion based
> > on __boot_cpu_mode.
> > 
> > On the cpu_resume() path it looks like we take the return of
> > init_kernel_el() and pass that along to finalise_el2(). As we are in EL1
> > at this point, it seems like we'd go init_kernel_el() -> init_el1().
> > 
> > What am I missing?
> 
> That I'm an idiot.
> 
> This is only necessary on pre-6.0, before 005e12676af0 ("arm64: head:
> record CPU boot mode after enabling the MMU"), as this code-path
> *used* to reload the boot mode from memory. Now, this is directly
> passed as a parameter, making this patch useless.

On a 5.10 though, the suprious HVCs are gone and I have not observed any
regression.

Thanks!

For a stable fix:

Tested-by: Vincent Donnefort <vdonnefort@google.com>

> 
> The joys of looking at too many code bases at the same time... I'll
> see how we can add it to 5.19.
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.

Re: [PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Vincent Donnefort]

On Tue, Oct 11, 2022 at 09:58:22PM +0100, Marc Zyngier wrote:
> On Tue, 11 Oct 2022 19:48:39 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > On Tue, Oct 11, 2022 at 05:54:00PM +0100, Marc Zyngier wrote:
> > > The kernel has an awfully complicated boot sequence in order to cope
> > > with the various EL2 configurations, including those that "enhanced"
> > > the architecture. We go from EL2 to EL1, then back to EL2, staying
> > > at EL2 if VHE capable and otherwise go back to EL1.
> > > 
> > > Here's a paracetamol tablet for you.
> > 
> > Heh, still have a bit of a headache from this :)
> > 
> > I'm having a hard time following where we skip the EL2 promotion based
> > on __boot_cpu_mode.
> > 
> > On the cpu_resume() path it looks like we take the return of
> > init_kernel_el() and pass that along to finalise_el2(). As we are in EL1
> > at this point, it seems like we'd go init_kernel_el() -> init_el1().
> > 
> > What am I missing?
> 
> That I'm an idiot.
> 
> This is only necessary on pre-6.0, before 005e12676af0 ("arm64: head:
> record CPU boot mode after enabling the MMU"), as this code-path
> *used* to reload the boot mode from memory. Now, this is directly
> passed as a parameter, making this patch useless.

On a 5.10 though, the suprious HVCs are gone and I have not observed any
regression.

Thanks!

For a stable fix:

Tested-by: Vincent Donnefort <vdonnefort@google.com>

> 
> The joys of looking at too many code bases at the same time... I'll
> see how we can add it to 5.19.
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.

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

[PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Vincent Donnefort]

From: Marc Zyngier <maz@kernel.org>

The kernel has an awfully complicated boot sequence in order to cope
with the various EL2 configurations, including those that "enhanced"
the architecture. We go from EL2 to EL1, then back to EL2, staying
at EL2 if VHE capable and otherwise go back to EL1.

Here's a paracetamol tablet for you.

The cpu_resume path follows the same logic, because coming up with
two versions of a square wheel is hard.

However, things aren't this straightforward with pKVM, as the host
resume path is always proxied by the hypervisor, which means that
the kernel is always entered at EL1. Which contradicts what the
__boot_cpu_mode[] array contains (it obviously says EL2).

This thus triggers a HVC call from EL1 to EL2 in a vain attempt
to upgrade from EL1 to EL2 VHE, which we are, funnily enough,
reluctant to grant to the host kernel. This is also completely
unexpected, and puzzles your average EL2 hacker.

Address it by fixing up the boot mode at the point the host gets
deprivileged. is_hyp_mode_available() and co already have a static
branch to deal with this, making it pretty safe.

Cc: <stable@vger.kernel.org> # 5.15+
Reported-by: Vincent Donnefort <vdonnefort@google.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Tested-by: Vincent Donnefort <vdonnefort@google.com>

--- 

This patch doesn't have an upstream version. It's been fixed by the side
effect of another upstream patch. see conversation [1]

[1] https://lore.kernel.org/all/20221011165400.1241729-1-maz@kernel.org/

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 4cb265e15361..3fe816c244ce 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -2000,6 +2000,17 @@ static int pkvm_drop_host_privileges(void)
 	 * once the host stage 2 is installed.
 	 */
 	static_branch_enable(&kvm_protected_mode_initialized);
+
+	/*
+	 * Fixup the boot mode so that we don't take spurious round
+	 * trips via EL2 on cpu_resume. Flush to the PoC for a good
+	 * measure, so that it can be observed by a CPU coming out of
+	 * suspend with the MMU off.
+	 */
+	__boot_cpu_mode[0] = __boot_cpu_mode[1] = BOOT_CPU_MODE_EL1;
+	dcache_clean_poc((unsigned long)__boot_cpu_mode,
+			 (unsigned long)(__boot_cpu_mode + 2));
+
 	on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
 	return ret;
 }
-- 
2.38.1.431.g37b22c650d-goog

Re: [PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Vincent Donnefort]

On Tue, Nov 08, 2022 at 10:01:38AM +0000, Vincent Donnefort wrote:
> From: Marc Zyngier <maz@kernel.org>
> 
> The kernel has an awfully complicated boot sequence in order to cope
> with the various EL2 configurations, including those that "enhanced"
> the architecture. We go from EL2 to EL1, then back to EL2, staying
> at EL2 if VHE capable and otherwise go back to EL1.
> 
> Here's a paracetamol tablet for you.
> 
> The cpu_resume path follows the same logic, because coming up with
> two versions of a square wheel is hard.
> 
> However, things aren't this straightforward with pKVM, as the host
> resume path is always proxied by the hypervisor, which means that
> the kernel is always entered at EL1. Which contradicts what the
> __boot_cpu_mode[] array contains (it obviously says EL2).
> 
> This thus triggers a HVC call from EL1 to EL2 in a vain attempt
> to upgrade from EL1 to EL2 VHE, which we are, funnily enough,
> reluctant to grant to the host kernel. This is also completely
> unexpected, and puzzles your average EL2 hacker.
> 
> Address it by fixing up the boot mode at the point the host gets
> deprivileged. is_hyp_mode_available() and co already have a static
> branch to deal with this, making it pretty safe.
> 
> Cc: <stable@vger.kernel.org> # 5.15+
> Reported-by: Vincent Donnefort <vdonnefort@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Tested-by: Vincent Donnefort <vdonnefort@google.com>
> 
> --- 
> 
> This patch doesn't have an upstream version. It's been fixed by the side
> effect of another upstream patch. see conversation [1]
> 
> [1] https://lore.kernel.org/all/20221011165400.1241729-1-maz@kernel.org/
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 4cb265e15361..3fe816c244ce 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -2000,6 +2000,17 @@ static int pkvm_drop_host_privileges(void)
>  	 * once the host stage 2 is installed.
>  	 */
>  	static_branch_enable(&kvm_protected_mode_initialized);
> +
> +	/*
> +	 * Fixup the boot mode so that we don't take spurious round
> +	 * trips via EL2 on cpu_resume. Flush to the PoC for a good
> +	 * measure, so that it can be observed by a CPU coming out of
> +	 * suspend with the MMU off.
> +	 */
> +	__boot_cpu_mode[0] = __boot_cpu_mode[1] = BOOT_CPU_MODE_EL1;
> +	dcache_clean_poc((unsigned long)__boot_cpu_mode,
> +			 (unsigned long)(__boot_cpu_mode + 2));
> +
>  	on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
>  	return ret;
>  }
> -- 
> 2.38.1.431.g37b22c650d-goog
>

Hi Greg,

Any chance to pick this fix for 5.15?

Thanks!

-- 
Vincent.

Re: [PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Greg KH]

On Mon, Nov 28, 2022 at 04:21:28PM +0000, Vincent Donnefort wrote:
> On Tue, Nov 08, 2022 at 10:01:38AM +0000, Vincent Donnefort wrote:
> > From: Marc Zyngier <maz@kernel.org>
> > 
> > The kernel has an awfully complicated boot sequence in order to cope
> > with the various EL2 configurations, including those that "enhanced"
> > the architecture. We go from EL2 to EL1, then back to EL2, staying
> > at EL2 if VHE capable and otherwise go back to EL1.
> > 
> > Here's a paracetamol tablet for you.
> > 
> > The cpu_resume path follows the same logic, because coming up with
> > two versions of a square wheel is hard.
> > 
> > However, things aren't this straightforward with pKVM, as the host
> > resume path is always proxied by the hypervisor, which means that
> > the kernel is always entered at EL1. Which contradicts what the
> > __boot_cpu_mode[] array contains (it obviously says EL2).
> > 
> > This thus triggers a HVC call from EL1 to EL2 in a vain attempt
> > to upgrade from EL1 to EL2 VHE, which we are, funnily enough,
> > reluctant to grant to the host kernel. This is also completely
> > unexpected, and puzzles your average EL2 hacker.
> > 
> > Address it by fixing up the boot mode at the point the host gets
> > deprivileged. is_hyp_mode_available() and co already have a static
> > branch to deal with this, making it pretty safe.
> > 
> > Cc: <stable@vger.kernel.org> # 5.15+
> > Reported-by: Vincent Donnefort <vdonnefort@google.com>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > Tested-by: Vincent Donnefort <vdonnefort@google.com>
> > 
> > --- 
> > 
> > This patch doesn't have an upstream version. It's been fixed by the side
> > effect of another upstream patch. see conversation [1]
> > 
> > [1] https://lore.kernel.org/all/20221011165400.1241729-1-maz@kernel.org/
> > 
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 4cb265e15361..3fe816c244ce 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -2000,6 +2000,17 @@ static int pkvm_drop_host_privileges(void)
> >  	 * once the host stage 2 is installed.
> >  	 */
> >  	static_branch_enable(&kvm_protected_mode_initialized);
> > +
> > +	/*
> > +	 * Fixup the boot mode so that we don't take spurious round
> > +	 * trips via EL2 on cpu_resume. Flush to the PoC for a good
> > +	 * measure, so that it can be observed by a CPU coming out of
> > +	 * suspend with the MMU off.
> > +	 */
> > +	__boot_cpu_mode[0] = __boot_cpu_mode[1] = BOOT_CPU_MODE_EL1;
> > +	dcache_clean_poc((unsigned long)__boot_cpu_mode,
> > +			 (unsigned long)(__boot_cpu_mode + 2));
> > +
> >  	on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
> >  	return ret;
> >  }
> > -- 
> > 2.38.1.431.g37b22c650d-goog
> >
> 
> Hi Greg,
> 
> Any chance to pick this fix for 5.15?

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Re: [PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Vincent Donnefort]

On Mon, Nov 28, 2022 at 06:19:15PM +0100, Greg KH wrote:
> On Mon, Nov 28, 2022 at 04:21:28PM +0000, Vincent Donnefort wrote:
> > On Tue, Nov 08, 2022 at 10:01:38AM +0000, Vincent Donnefort wrote:
> > > From: Marc Zyngier <maz@kernel.org>
> > > 
> > > The kernel has an awfully complicated boot sequence in order to cope
> > > with the various EL2 configurations, including those that "enhanced"
> > > the architecture. We go from EL2 to EL1, then back to EL2, staying
> > > at EL2 if VHE capable and otherwise go back to EL1.
> > > 
> > > Here's a paracetamol tablet for you.
> > > 
> > > The cpu_resume path follows the same logic, because coming up with
> > > two versions of a square wheel is hard.
> > > 
> > > However, things aren't this straightforward with pKVM, as the host
> > > resume path is always proxied by the hypervisor, which means that
> > > the kernel is always entered at EL1. Which contradicts what the
> > > __boot_cpu_mode[] array contains (it obviously says EL2).
> > > 
> > > This thus triggers a HVC call from EL1 to EL2 in a vain attempt
> > > to upgrade from EL1 to EL2 VHE, which we are, funnily enough,
> > > reluctant to grant to the host kernel. This is also completely
> > > unexpected, and puzzles your average EL2 hacker.
> > > 
> > > Address it by fixing up the boot mode at the point the host gets
> > > deprivileged. is_hyp_mode_available() and co already have a static
> > > branch to deal with this, making it pretty safe.
> > > 
> > > Cc: <stable@vger.kernel.org> # 5.15+
> > > Reported-by: Vincent Donnefort <vdonnefort@google.com>
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > Tested-by: Vincent Donnefort <vdonnefort@google.com>
> > > 
> > > --- 
> > > 
> > > This patch doesn't have an upstream version. It's been fixed by the side
> > > effect of another upstream patch. see conversation [1]
> > > 
> > > [1] https://lore.kernel.org/all/20221011165400.1241729-1-maz@kernel.org/
> > > 
> > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > > index 4cb265e15361..3fe816c244ce 100644
> > > --- a/arch/arm64/kvm/arm.c
> > > +++ b/arch/arm64/kvm/arm.c
> > > @@ -2000,6 +2000,17 @@ static int pkvm_drop_host_privileges(void)
> > >  	 * once the host stage 2 is installed.
> > >  	 */
> > >  	static_branch_enable(&kvm_protected_mode_initialized);
> > > +
> > > +	/*
> > > +	 * Fixup the boot mode so that we don't take spurious round
> > > +	 * trips via EL2 on cpu_resume. Flush to the PoC for a good
> > > +	 * measure, so that it can be observed by a CPU coming out of
> > > +	 * suspend with the MMU off.
> > > +	 */
> > > +	__boot_cpu_mode[0] = __boot_cpu_mode[1] = BOOT_CPU_MODE_EL1;
> > > +	dcache_clean_poc((unsigned long)__boot_cpu_mode,
> > > +			 (unsigned long)(__boot_cpu_mode + 2));
> > > +
> > >  	on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
> > >  	return ret;
> > >  }
> > > -- 
> > > 2.38.1.431.g37b22c650d-goog
> > >
> > 
> > Hi Greg,
> > 
> > Any chance to pick this fix for 5.15?
> 
> <formletter>
> 
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read:
>     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
> 
> </formletter>

Sadly this patch doesn't have an upstream version equivalent. The reason is it's
been fixed as a side effect of another feature introduction, hence the
stable-only fix made by Marc. [1]

Not sure how to handle that case.

Re: [PATCH] KVM: arm64: pkvm: Fixup boot mode to reflect that the kernel resumes from EL1

[Greg KH]

On Mon, Nov 28, 2022 at 05:24:42PM +0000, Vincent Donnefort wrote:
> On Mon, Nov 28, 2022 at 06:19:15PM +0100, Greg KH wrote:
> > On Mon, Nov 28, 2022 at 04:21:28PM +0000, Vincent Donnefort wrote:
> > > On Tue, Nov 08, 2022 at 10:01:38AM +0000, Vincent Donnefort wrote:
> > > > From: Marc Zyngier <maz@kernel.org>
> > > > 
> > > > The kernel has an awfully complicated boot sequence in order to cope
> > > > with the various EL2 configurations, including those that "enhanced"
> > > > the architecture. We go from EL2 to EL1, then back to EL2, staying
> > > > at EL2 if VHE capable and otherwise go back to EL1.
> > > > 
> > > > Here's a paracetamol tablet for you.
> > > > 
> > > > The cpu_resume path follows the same logic, because coming up with
> > > > two versions of a square wheel is hard.
> > > > 
> > > > However, things aren't this straightforward with pKVM, as the host
> > > > resume path is always proxied by the hypervisor, which means that
> > > > the kernel is always entered at EL1. Which contradicts what the
> > > > __boot_cpu_mode[] array contains (it obviously says EL2).
> > > > 
> > > > This thus triggers a HVC call from EL1 to EL2 in a vain attempt
> > > > to upgrade from EL1 to EL2 VHE, which we are, funnily enough,
> > > > reluctant to grant to the host kernel. This is also completely
> > > > unexpected, and puzzles your average EL2 hacker.
> > > > 
> > > > Address it by fixing up the boot mode at the point the host gets
> > > > deprivileged. is_hyp_mode_available() and co already have a static
> > > > branch to deal with this, making it pretty safe.
> > > > 
> > > > Cc: <stable@vger.kernel.org> # 5.15+
> > > > Reported-by: Vincent Donnefort <vdonnefort@google.com>
> > > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > > Tested-by: Vincent Donnefort <vdonnefort@google.com>
> > > > 
> > > > --- 
> > > > 
> > > > This patch doesn't have an upstream version. It's been fixed by the side
> > > > effect of another upstream patch. see conversation [1]
> > > > 
> > > > [1] https://lore.kernel.org/all/20221011165400.1241729-1-maz@kernel.org/
> > > > 
> > > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > > > index 4cb265e15361..3fe816c244ce 100644
> > > > --- a/arch/arm64/kvm/arm.c
> > > > +++ b/arch/arm64/kvm/arm.c
> > > > @@ -2000,6 +2000,17 @@ static int pkvm_drop_host_privileges(void)
> > > >  	 * once the host stage 2 is installed.
> > > >  	 */
> > > >  	static_branch_enable(&kvm_protected_mode_initialized);
> > > > +
> > > > +	/*
> > > > +	 * Fixup the boot mode so that we don't take spurious round
> > > > +	 * trips via EL2 on cpu_resume. Flush to the PoC for a good
> > > > +	 * measure, so that it can be observed by a CPU coming out of
> > > > +	 * suspend with the MMU off.
> > > > +	 */
> > > > +	__boot_cpu_mode[0] = __boot_cpu_mode[1] = BOOT_CPU_MODE_EL1;
> > > > +	dcache_clean_poc((unsigned long)__boot_cpu_mode,
> > > > +			 (unsigned long)(__boot_cpu_mode + 2));
> > > > +
> > > >  	on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
> > > >  	return ret;
> > > >  }
> > > > -- 
> > > > 2.38.1.431.g37b22c650d-goog
> > > >
> > > 
> > > Hi Greg,
> > > 
> > > Any chance to pick this fix for 5.15?
> > 
> > <formletter>
> > 
> > This is not the correct way to submit patches for inclusion in the
> > stable kernel tree.  Please read:
> >     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > for how to do this properly.
> > 
> > </formletter>
> 
> Sadly this patch doesn't have an upstream version equivalent. The reason is it's
> been fixed as a side effect of another feature introduction, hence the
> stable-only fix made by Marc. [1]
> 
> Not sure how to handle that case.

It needs to really really really document why this is not relevant for
newer kernels and what commit fixed this instead and why this is all
happening in the changelog text, AND give us a clue in maybe the subject
line [PATCH 5.15] to show that this is only for a specific tree.

it was not obvious here at all, sorry, I missed the --- comments in the
normal flood of other patches.

thanks,

gre gk-h