[PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[David Brazdil]

The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET should
not return, as dictated by the PSCI spec. However, there is firmware out
there which breaks this assumption, leading to a hyp panic. Make KVM
more robust to broken firmware by allowing these to return.

Signed-off-by: David Brazdil <dbrazdil@google.com>
---
 arch/arm64/kvm/hyp/nvhe/psci-relay.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
index e3947846ffcb..8e7128cb7667 100644
--- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
+++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
@@ -77,12 +77,6 @@ static unsigned long psci_forward(struct kvm_cpu_context *host_ctxt)
 			 cpu_reg(host_ctxt, 2), cpu_reg(host_ctxt, 3));
 }
 
-static __noreturn unsigned long psci_forward_noreturn(struct kvm_cpu_context *host_ctxt)
-{
-	psci_forward(host_ctxt);
-	hyp_panic(); /* unreachable */
-}
-
 static unsigned int find_cpu_id(u64 mpidr)
 {
 	unsigned int i;
@@ -251,10 +245,13 @@ static unsigned long psci_0_2_handler(u64 func_id, struct kvm_cpu_context *host_
 	case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
 	case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
 		return psci_forward(host_ctxt);
+	/*
+	 * SYSTEM_OFF/RESET should not return according to the spec.
+	 * Allow it so as to stay robust to broken firmware.
+	 */
 	case PSCI_0_2_FN_SYSTEM_OFF:
 	case PSCI_0_2_FN_SYSTEM_RESET:
-		psci_forward_noreturn(host_ctxt);
-		unreachable();
+		return psci_forward(host_ctxt);
 	case PSCI_0_2_FN64_CPU_SUSPEND:
 		return psci_cpu_suspend(func_id, host_ctxt);
 	case PSCI_0_2_FN64_CPU_ON:
-- 
2.29.2.729.g45daf8777d-goog

[PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[David Brazdil]

The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET should
not return, as dictated by the PSCI spec. However, there is firmware out
there which breaks this assumption, leading to a hyp panic. Make KVM
more robust to broken firmware by allowing these to return.

Signed-off-by: David Brazdil <dbrazdil@google.com>
---
 arch/arm64/kvm/hyp/nvhe/psci-relay.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
index e3947846ffcb..8e7128cb7667 100644
--- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
+++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
@@ -77,12 +77,6 @@ static unsigned long psci_forward(struct kvm_cpu_context *host_ctxt)
 			 cpu_reg(host_ctxt, 2), cpu_reg(host_ctxt, 3));
 }
 
-static __noreturn unsigned long psci_forward_noreturn(struct kvm_cpu_context *host_ctxt)
-{
-	psci_forward(host_ctxt);
-	hyp_panic(); /* unreachable */
-}
-
 static unsigned int find_cpu_id(u64 mpidr)
 {
 	unsigned int i;
@@ -251,10 +245,13 @@ static unsigned long psci_0_2_handler(u64 func_id, struct kvm_cpu_context *host_
 	case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
 	case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
 		return psci_forward(host_ctxt);
+	/*
+	 * SYSTEM_OFF/RESET should not return according to the spec.
+	 * Allow it so as to stay robust to broken firmware.
+	 */
 	case PSCI_0_2_FN_SYSTEM_OFF:
 	case PSCI_0_2_FN_SYSTEM_RESET:
-		psci_forward_noreturn(host_ctxt);
-		unreachable();
+		return psci_forward(host_ctxt);
 	case PSCI_0_2_FN64_CPU_SUSPEND:
 		return psci_cpu_suspend(func_id, host_ctxt);
 	case PSCI_0_2_FN64_CPU_ON:
-- 
2.29.2.729.g45daf8777d-goog

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

[PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[David Brazdil]

The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET should
not return, as dictated by the PSCI spec. However, there is firmware out
there which breaks this assumption, leading to a hyp panic. Make KVM
more robust to broken firmware by allowing these to return.

Signed-off-by: David Brazdil <dbrazdil@google.com>
---
 arch/arm64/kvm/hyp/nvhe/psci-relay.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
index e3947846ffcb..8e7128cb7667 100644
--- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
+++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
@@ -77,12 +77,6 @@ static unsigned long psci_forward(struct kvm_cpu_context *host_ctxt)
 			 cpu_reg(host_ctxt, 2), cpu_reg(host_ctxt, 3));
 }
 
-static __noreturn unsigned long psci_forward_noreturn(struct kvm_cpu_context *host_ctxt)
-{
-	psci_forward(host_ctxt);
-	hyp_panic(); /* unreachable */
-}
-
 static unsigned int find_cpu_id(u64 mpidr)
 {
 	unsigned int i;
@@ -251,10 +245,13 @@ static unsigned long psci_0_2_handler(u64 func_id, struct kvm_cpu_context *host_
 	case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
 	case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
 		return psci_forward(host_ctxt);
+	/*
+	 * SYSTEM_OFF/RESET should not return according to the spec.
+	 * Allow it so as to stay robust to broken firmware.
+	 */
 	case PSCI_0_2_FN_SYSTEM_OFF:
 	case PSCI_0_2_FN_SYSTEM_RESET:
-		psci_forward_noreturn(host_ctxt);
-		unreachable();
+		return psci_forward(host_ctxt);
 	case PSCI_0_2_FN64_CPU_SUSPEND:
 		return psci_cpu_suspend(func_id, host_ctxt);
 	case PSCI_0_2_FN64_CPU_ON:
-- 
2.29.2.729.g45daf8777d-goog


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

Re: [PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[Russell King - ARM Linux admin]

On Tue, Dec 29, 2020 at 04:00:59PM +0000, David Brazdil wrote:
> The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET should
> not return, as dictated by the PSCI spec. However, there is firmware out
> there which breaks this assumption, leading to a hyp panic. Make KVM
> more robust to broken firmware by allowing these to return.

Are you sure you should just return?

We've had issues in the past with Linux reboot(2) that returns
to userspace, allowing on 32-bit ARM for example watchdogs to
unexpectedly continue being serviced.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[Russell King - ARM Linux admin]

On Tue, Dec 29, 2020 at 04:00:59PM +0000, David Brazdil wrote:
> The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET should
> not return, as dictated by the PSCI spec. However, there is firmware out
> there which breaks this assumption, leading to a hyp panic. Make KVM
> more robust to broken firmware by allowing these to return.

Are you sure you should just return?

We've had issues in the past with Linux reboot(2) that returns
to userspace, allowing on 32-bit ARM for example watchdogs to
unexpectedly continue being serviced.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[Russell King - ARM Linux admin]

On Tue, Dec 29, 2020 at 04:00:59PM +0000, David Brazdil wrote:
> The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET should
> not return, as dictated by the PSCI spec. However, there is firmware out
> there which breaks this assumption, leading to a hyp panic. Make KVM
> more robust to broken firmware by allowing these to return.

Are you sure you should just return?

We've had issues in the past with Linux reboot(2) that returns
to userspace, allowing on 32-bit ARM for example watchdogs to
unexpectedly continue being serviced.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

Re: [PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[Marc Zyngier]

Hi David,

On 2020-12-29 16:00, David Brazdil wrote:
> The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET 
> should
> not return, as dictated by the PSCI spec. However, there is firmware 
> out
> there which breaks this assumption, leading to a hyp panic. Make KVM
> more robust to broken firmware by allowing these to return.
> 
> Signed-off-by: David Brazdil <dbrazdil@google.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/psci-relay.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> index e3947846ffcb..8e7128cb7667 100644
> --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> @@ -77,12 +77,6 @@ static unsigned long psci_forward(struct
> kvm_cpu_context *host_ctxt)
>  			 cpu_reg(host_ctxt, 2), cpu_reg(host_ctxt, 3));
>  }
> 
> -static __noreturn unsigned long psci_forward_noreturn(struct
> kvm_cpu_context *host_ctxt)
> -{
> -	psci_forward(host_ctxt);
> -	hyp_panic(); /* unreachable */
> -}
> -
>  static unsigned int find_cpu_id(u64 mpidr)
>  {
>  	unsigned int i;
> @@ -251,10 +245,13 @@ static unsigned long psci_0_2_handler(u64
> func_id, struct kvm_cpu_context *host_
>  	case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
>  	case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
>  		return psci_forward(host_ctxt);
> +	/*
> +	 * SYSTEM_OFF/RESET should not return according to the spec.
> +	 * Allow it so as to stay robust to broken firmware.
> +	 */
>  	case PSCI_0_2_FN_SYSTEM_OFF:
>  	case PSCI_0_2_FN_SYSTEM_RESET:
> -		psci_forward_noreturn(host_ctxt);
> -		unreachable();
> +		return psci_forward(host_ctxt);
>  	case PSCI_0_2_FN64_CPU_SUSPEND:
>  		return psci_cpu_suspend(func_id, host_ctxt);
>  	case PSCI_0_2_FN64_CPU_ON:

Thanks for having tracked this.

I wonder whether we should also taint the kernel in this case,
because this is completely unexpected, and a major spec violation.

Ideally, we'd be able to detect this case and prevent pKVM from
getting initialised at all, but I guess there is no way to detect
the sucker without ... calling SYSTEM_RESET?

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[Marc Zyngier]

Hi David,

On 2020-12-29 16:00, David Brazdil wrote:
> The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET 
> should
> not return, as dictated by the PSCI spec. However, there is firmware 
> out
> there which breaks this assumption, leading to a hyp panic. Make KVM
> more robust to broken firmware by allowing these to return.
> 
> Signed-off-by: David Brazdil <dbrazdil@google.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/psci-relay.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> index e3947846ffcb..8e7128cb7667 100644
> --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> @@ -77,12 +77,6 @@ static unsigned long psci_forward(struct
> kvm_cpu_context *host_ctxt)
>  			 cpu_reg(host_ctxt, 2), cpu_reg(host_ctxt, 3));
>  }
> 
> -static __noreturn unsigned long psci_forward_noreturn(struct
> kvm_cpu_context *host_ctxt)
> -{
> -	psci_forward(host_ctxt);
> -	hyp_panic(); /* unreachable */
> -}
> -
>  static unsigned int find_cpu_id(u64 mpidr)
>  {
>  	unsigned int i;
> @@ -251,10 +245,13 @@ static unsigned long psci_0_2_handler(u64
> func_id, struct kvm_cpu_context *host_
>  	case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
>  	case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
>  		return psci_forward(host_ctxt);
> +	/*
> +	 * SYSTEM_OFF/RESET should not return according to the spec.
> +	 * Allow it so as to stay robust to broken firmware.
> +	 */
>  	case PSCI_0_2_FN_SYSTEM_OFF:
>  	case PSCI_0_2_FN_SYSTEM_RESET:
> -		psci_forward_noreturn(host_ctxt);
> -		unreachable();
> +		return psci_forward(host_ctxt);
>  	case PSCI_0_2_FN64_CPU_SUSPEND:
>  		return psci_cpu_suspend(func_id, host_ctxt);
>  	case PSCI_0_2_FN64_CPU_ON:

Thanks for having tracked this.

I wonder whether we should also taint the kernel in this case,
because this is completely unexpected, and a major spec violation.

Ideally, we'd be able to detect this case and prevent pKVM from
getting initialised at all, but I guess there is no way to detect
the sucker without ... calling SYSTEM_RESET?

         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] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[Marc Zyngier]

Hi David,

On 2020-12-29 16:00, David Brazdil wrote:
> The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET 
> should
> not return, as dictated by the PSCI spec. However, there is firmware 
> out
> there which breaks this assumption, leading to a hyp panic. Make KVM
> more robust to broken firmware by allowing these to return.
> 
> Signed-off-by: David Brazdil <dbrazdil@google.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/psci-relay.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> index e3947846ffcb..8e7128cb7667 100644
> --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> @@ -77,12 +77,6 @@ static unsigned long psci_forward(struct
> kvm_cpu_context *host_ctxt)
>  			 cpu_reg(host_ctxt, 2), cpu_reg(host_ctxt, 3));
>  }
> 
> -static __noreturn unsigned long psci_forward_noreturn(struct
> kvm_cpu_context *host_ctxt)
> -{
> -	psci_forward(host_ctxt);
> -	hyp_panic(); /* unreachable */
> -}
> -
>  static unsigned int find_cpu_id(u64 mpidr)
>  {
>  	unsigned int i;
> @@ -251,10 +245,13 @@ static unsigned long psci_0_2_handler(u64
> func_id, struct kvm_cpu_context *host_
>  	case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
>  	case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
>  		return psci_forward(host_ctxt);
> +	/*
> +	 * SYSTEM_OFF/RESET should not return according to the spec.
> +	 * Allow it so as to stay robust to broken firmware.
> +	 */
>  	case PSCI_0_2_FN_SYSTEM_OFF:
>  	case PSCI_0_2_FN_SYSTEM_RESET:
> -		psci_forward_noreturn(host_ctxt);
> -		unreachable();
> +		return psci_forward(host_ctxt);
>  	case PSCI_0_2_FN64_CPU_SUSPEND:
>  		return psci_cpu_suspend(func_id, host_ctxt);
>  	case PSCI_0_2_FN64_CPU_ON:

Thanks for having tracked this.

I wonder whether we should also taint the kernel in this case,
because this is completely unexpected, and a major spec violation.

Ideally, we'd be able to detect this case and prevent pKVM from
getting initialised at all, but I guess there is no way to detect
the sucker without ... calling SYSTEM_RESET?

         M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[Marc Zyngier]

On 2020-12-29 17:04, Russell King - ARM Linux admin wrote:
> On Tue, Dec 29, 2020 at 04:00:59PM +0000, David Brazdil wrote:
>> The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET 
>> should
>> not return, as dictated by the PSCI spec. However, there is firmware 
>> out
>> there which breaks this assumption, leading to a hyp panic. Make KVM
>> more robust to broken firmware by allowing these to return.
> 
> Are you sure you should just return?
> 
> We've had issues in the past with Linux reboot(2) that returns
> to userspace, allowing on 32-bit ARM for example watchdogs to
> unexpectedly continue being serviced.

I don't think this changes anything compared to the case where
the PSCI relay isn't enabled. The EL1 part of the kernel would
see the SYSTEM_RESET call return, and handle it accordingly
(stay in a while(1) loop).

This is consistent with the PSCI relay design goal of being
invisible to the EL1 kernel.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[Marc Zyngier]

On 2020-12-29 17:04, Russell King - ARM Linux admin wrote:
> On Tue, Dec 29, 2020 at 04:00:59PM +0000, David Brazdil wrote:
>> The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET 
>> should
>> not return, as dictated by the PSCI spec. However, there is firmware 
>> out
>> there which breaks this assumption, leading to a hyp panic. Make KVM
>> more robust to broken firmware by allowing these to return.
> 
> Are you sure you should just return?
> 
> We've had issues in the past with Linux reboot(2) that returns
> to userspace, allowing on 32-bit ARM for example watchdogs to
> unexpectedly continue being serviced.

I don't think this changes anything compared to the case where
the PSCI relay isn't enabled. The EL1 part of the kernel would
see the SYSTEM_RESET call return, and handle it accordingly
(stay in a while(1) loop).

This is consistent with the PSCI relay design goal of being
invisible to the EL1 kernel.

Thanks,

         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] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[Marc Zyngier]

On 2020-12-29 17:04, Russell King - ARM Linux admin wrote:
> On Tue, Dec 29, 2020 at 04:00:59PM +0000, David Brazdil wrote:
>> The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET 
>> should
>> not return, as dictated by the PSCI spec. However, there is firmware 
>> out
>> there which breaks this assumption, leading to a hyp panic. Make KVM
>> more robust to broken firmware by allowing these to return.
> 
> Are you sure you should just return?
> 
> We've had issues in the past with Linux reboot(2) that returns
> to userspace, allowing on 32-bit ARM for example watchdogs to
> unexpectedly continue being serviced.

I don't think this changes anything compared to the case where
the PSCI relay isn't enabled. The EL1 part of the kernel would
see the SYSTEM_RESET call return, and handle it accordingly
(stay in a while(1) loop).

This is consistent with the PSCI relay design goal of being
invisible to the EL1 kernel.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[David Brazdil]

On Tue, Dec 29, 2020 at 05:16:41PM +0000, Marc Zyngier wrote:
> Hi David,
> 
> On 2020-12-29 16:00, David Brazdil wrote:
> > The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET should
> > not return, as dictated by the PSCI spec. However, there is firmware out
> > there which breaks this assumption, leading to a hyp panic. Make KVM
> > more robust to broken firmware by allowing these to return.
> > 
> > Signed-off-by: David Brazdil <dbrazdil@google.com>
> > ---
> >  arch/arm64/kvm/hyp/nvhe/psci-relay.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > index e3947846ffcb..8e7128cb7667 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > @@ -77,12 +77,6 @@ static unsigned long psci_forward(struct
> > kvm_cpu_context *host_ctxt)
> >  			 cpu_reg(host_ctxt, 2), cpu_reg(host_ctxt, 3));
> >  }
> > 
> > -static __noreturn unsigned long psci_forward_noreturn(struct
> > kvm_cpu_context *host_ctxt)
> > -{
> > -	psci_forward(host_ctxt);
> > -	hyp_panic(); /* unreachable */
> > -}
> > -
> >  static unsigned int find_cpu_id(u64 mpidr)
> >  {
> >  	unsigned int i;
> > @@ -251,10 +245,13 @@ static unsigned long psci_0_2_handler(u64
> > func_id, struct kvm_cpu_context *host_
> >  	case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
> >  	case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
> >  		return psci_forward(host_ctxt);
> > +	/*
> > +	 * SYSTEM_OFF/RESET should not return according to the spec.
> > +	 * Allow it so as to stay robust to broken firmware.
> > +	 */
> >  	case PSCI_0_2_FN_SYSTEM_OFF:
> >  	case PSCI_0_2_FN_SYSTEM_RESET:
> > -		psci_forward_noreturn(host_ctxt);
> > -		unreachable();
> > +		return psci_forward(host_ctxt);
> >  	case PSCI_0_2_FN64_CPU_SUSPEND:
> >  		return psci_cpu_suspend(func_id, host_ctxt);
> >  	case PSCI_0_2_FN64_CPU_ON:
> 
> Thanks for having tracked this.
> 
> I wonder whether we should also taint the kernel in this case,
> because this is completely unexpected, and a major spec violation.
> 
> Ideally, we'd be able to detect this case and prevent pKVM from
> getting initialised at all, but I guess there is no way to detect
> the sucker without ... calling SYSTEM_RESET?

Yeah, there are no bits to check, unfortunately. :(

David

Re: [PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[David Brazdil]

On Tue, Dec 29, 2020 at 05:16:41PM +0000, Marc Zyngier wrote:
> Hi David,
> 
> On 2020-12-29 16:00, David Brazdil wrote:
> > The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET should
> > not return, as dictated by the PSCI spec. However, there is firmware out
> > there which breaks this assumption, leading to a hyp panic. Make KVM
> > more robust to broken firmware by allowing these to return.
> > 
> > Signed-off-by: David Brazdil <dbrazdil@google.com>
> > ---
> >  arch/arm64/kvm/hyp/nvhe/psci-relay.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > index e3947846ffcb..8e7128cb7667 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > @@ -77,12 +77,6 @@ static unsigned long psci_forward(struct
> > kvm_cpu_context *host_ctxt)
> >  			 cpu_reg(host_ctxt, 2), cpu_reg(host_ctxt, 3));
> >  }
> > 
> > -static __noreturn unsigned long psci_forward_noreturn(struct
> > kvm_cpu_context *host_ctxt)
> > -{
> > -	psci_forward(host_ctxt);
> > -	hyp_panic(); /* unreachable */
> > -}
> > -
> >  static unsigned int find_cpu_id(u64 mpidr)
> >  {
> >  	unsigned int i;
> > @@ -251,10 +245,13 @@ static unsigned long psci_0_2_handler(u64
> > func_id, struct kvm_cpu_context *host_
> >  	case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
> >  	case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
> >  		return psci_forward(host_ctxt);
> > +	/*
> > +	 * SYSTEM_OFF/RESET should not return according to the spec.
> > +	 * Allow it so as to stay robust to broken firmware.
> > +	 */
> >  	case PSCI_0_2_FN_SYSTEM_OFF:
> >  	case PSCI_0_2_FN_SYSTEM_RESET:
> > -		psci_forward_noreturn(host_ctxt);
> > -		unreachable();
> > +		return psci_forward(host_ctxt);
> >  	case PSCI_0_2_FN64_CPU_SUSPEND:
> >  		return psci_cpu_suspend(func_id, host_ctxt);
> >  	case PSCI_0_2_FN64_CPU_ON:
> 
> Thanks for having tracked this.
> 
> I wonder whether we should also taint the kernel in this case,
> because this is completely unexpected, and a major spec violation.
> 
> Ideally, we'd be able to detect this case and prevent pKVM from
> getting initialised at all, but I guess there is no way to detect
> the sucker without ... calling SYSTEM_RESET?

Yeah, there are no bits to check, unfortunately. :(

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

Re: [PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[David Brazdil]

On Tue, Dec 29, 2020 at 05:16:41PM +0000, Marc Zyngier wrote:
> Hi David,
> 
> On 2020-12-29 16:00, David Brazdil wrote:
> > The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET should
> > not return, as dictated by the PSCI spec. However, there is firmware out
> > there which breaks this assumption, leading to a hyp panic. Make KVM
> > more robust to broken firmware by allowing these to return.
> > 
> > Signed-off-by: David Brazdil <dbrazdil@google.com>
> > ---
> >  arch/arm64/kvm/hyp/nvhe/psci-relay.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > index e3947846ffcb..8e7128cb7667 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > @@ -77,12 +77,6 @@ static unsigned long psci_forward(struct
> > kvm_cpu_context *host_ctxt)
> >  			 cpu_reg(host_ctxt, 2), cpu_reg(host_ctxt, 3));
> >  }
> > 
> > -static __noreturn unsigned long psci_forward_noreturn(struct
> > kvm_cpu_context *host_ctxt)
> > -{
> > -	psci_forward(host_ctxt);
> > -	hyp_panic(); /* unreachable */
> > -}
> > -
> >  static unsigned int find_cpu_id(u64 mpidr)
> >  {
> >  	unsigned int i;
> > @@ -251,10 +245,13 @@ static unsigned long psci_0_2_handler(u64
> > func_id, struct kvm_cpu_context *host_
> >  	case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
> >  	case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
> >  		return psci_forward(host_ctxt);
> > +	/*
> > +	 * SYSTEM_OFF/RESET should not return according to the spec.
> > +	 * Allow it so as to stay robust to broken firmware.
> > +	 */
> >  	case PSCI_0_2_FN_SYSTEM_OFF:
> >  	case PSCI_0_2_FN_SYSTEM_RESET:
> > -		psci_forward_noreturn(host_ctxt);
> > -		unreachable();
> > +		return psci_forward(host_ctxt);
> >  	case PSCI_0_2_FN64_CPU_SUSPEND:
> >  		return psci_cpu_suspend(func_id, host_ctxt);
> >  	case PSCI_0_2_FN64_CPU_ON:
> 
> Thanks for having tracked this.
> 
> I wonder whether we should also taint the kernel in this case,
> because this is completely unexpected, and a major spec violation.
> 
> Ideally, we'd be able to detect this case and prevent pKVM from
> getting initialised at all, but I guess there is no way to detect
> the sucker without ... calling SYSTEM_RESET?

Yeah, there are no bits to check, unfortunately. :(

David

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

Re: [PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[Marc Zyngier]

On Tue, 29 Dec 2020 16:00:59 +0000, David Brazdil wrote:
> The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET should
> not return, as dictated by the PSCI spec. However, there is firmware out
> there which breaks this assumption, leading to a hyp panic. Make KVM
> more robust to broken firmware by allowing these to return.

Applied to next, thanks!

[1/1] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return
      commit: 2c91ef39216149df6703c3fa6a47dd9a1e6091c1

Cheers,

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

Re: [PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

[Marc Zyngier]

On Tue, 29 Dec 2020 16:00:59 +0000, David Brazdil wrote:
> The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET should
> not return, as dictated by the PSCI spec. However, there is firmware out
> there which breaks this assumption, leading to a hyp panic. Make KVM
> more robust to broken firmware by allowing these to return.

Applied to next, thanks!

[1/1] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return
      commit: 2c91ef39216149df6703c3fa6a47dd9a1e6091c1

Cheers,

	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: Allow PSCI SYSTEM_OFF/RESET to return

[Marc Zyngier]

On Tue, 29 Dec 2020 16:00:59 +0000, David Brazdil wrote:
> The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET should
> not return, as dictated by the PSCI spec. However, there is firmware out
> there which breaks this assumption, leading to a hyp panic. Make KVM
> more robust to broken firmware by allowing these to return.

Applied to next, thanks!

[1/1] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return
      commit: 2c91ef39216149df6703c3fa6a47dd9a1e6091c1

Cheers,

	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