linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-05 18:14 Julien Grall
  2017-04-05 19:49 ` Boris Ostrovsky
  2017-04-05 21:26 ` Stefano Stabellini
  0 siblings, 2 replies; 26+ messages in thread
From: Julien Grall @ 2017-04-05 18:14 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, linux-arm-kernel, linux-kernel, Julien Grall,
	Boris Ostrovsky, Juergen Gross

When rebooting DOM0 with ACPI, the kernel is crashing with the stack trace [1].

This is happening because when EFI runtimes are enabled, the reset code
(see machin_restart) will first try to use EFI restart method.

However, the EFI restart code is expecting the reset_system callback to
be always set. This is not the case for Xen and will lead to crash.

Looking at the reboot path, it is expected to fallback on an alternative
reboot method if one does not work. So implement reset_system callback
as a NOP for Xen.

[   36.999270] reboot: Restarting system
[   37.002921] Internal error: Attempting to execute userspace memory: 86000004 [#1] PREEMPT SMP
[   37.011460] Modules linked in:
[   37.014598] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 4.11.0-rc1-00003-g1e248b60a39b-dirty #506
[   37.023903] Hardware name: (null) (DT)
[   37.027734] task: ffff800902068000 task.stack: ffff800902064000
[   37.033739] PC is at 0x0
[   37.036359] LR is at efi_reboot+0x94/0xd0
[   37.040438] pc : [<0000000000000000>] lr : [<ffff00000880f2c4>] pstate: 404001c5
[   37.047920] sp : ffff800902067cf0
[   37.051314] x29: ffff800902067cf0 x28: ffff800902068000
[   37.056709] x27: ffff000008992000 x26: 000000000000008e
[   37.062104] x25: 0000000000000123 x24: 0000000000000015
[   37.067499] x23: 0000000000000000 x22: ffff000008e6e250
[   37.072894] x21: ffff000008e6e000 x20: 0000000000000000
[   37.078289] x19: ffff000008e5d4c8 x18: 0000000000000010
[   37.083684] x17: 0000ffffa7c27470 x16: 00000000deadbeef
[   37.089079] x15: 0000000000000006 x14: ffff000088f42bef
[   37.094474] x13: ffff000008f42bfd x12: ffff000008e706c0
[   37.099870] x11: ffff000008e70000 x10: 0000000005f5e0ff
[   37.105265] x9 : ffff800902067a50 x8 : 6974726174736552
[   37.110660] x7 : ffff000008cc6fb8 x6 : ffff000008cc6fb0
[   37.116055] x5 : ffff000008c97dd8 x4 : 0000000000000000
[   37.121453] x3 : 0000000000000000 x2 : 0000000000000000
[   37.126845] x1 : 0000000000000000 x0 : 0000000000000000
[   37.132239]
[   37.133808] Process systemd-shutdow (pid: 1, stack limit = 0xffff800902064000)
[   37.141118] Stack: (0xffff800902067cf0 to 0xffff800902068000)
[   37.146949] 7ce0:                                   ffff800902067d40 ffff000008085334
[   37.154869] 7d00: 0000000000000000 ffff000008f3b000 ffff800902067d40 ffff0000080852e0
[   37.162787] 7d20: ffff000008cc6fb0 ffff000008cc6fb8 ffff000008c7f580 ffff000008c97dd8
[   37.170706] 7d40: ffff800902067d60 ffff0000080e2c2c 0000000000000000 0000000001234567
[   37.178624] 7d60: ffff800902067d80 ffff0000080e2ee8 0000000000000000 ffff0000080e2df4
[   37.186544] 7d80: 0000000000000000 ffff0000080830f0 0000000000000000 00008008ff1c1000
[   37.194462] 7da0: ffffffffffffffff 0000ffffa7c4b1cc 0000000000000000 0000000000000024
[   37.202380] 7dc0: ffff800902067dd0 0000000000000005 0000fffff24743c8 0000000000000004
[   37.210299] 7de0: 0000fffff2475f03 0000000000000010 0000fffff2474418 0000000000000005
[   37.218218] 7e00: 0000fffff2474578 000000000000000a 0000aaaad6b722c0 0000000000000001
[   37.226136] 7e20: 0000000000000123 0000000000000038 ffff800902067e50 ffff0000081e7294
[   37.234055] 7e40: ffff800902067e60 ffff0000081e935c ffff800902067e60 ffff0000081e9388
[   37.241973] 7e60: ffff800902067eb0 ffff0000081ea388 0000000000000000 00008008ff1c1000
[   37.249892] 7e80: ffffffffffffffff 0000ffffa7c4a79c 0000000000000000 ffff000000020000
[   37.257810] 7ea0: 0000010000000004 0000000000000000 0000000000000000 ffff0000080830f0
[   37.265729] 7ec0: fffffffffee1dead 0000000028121969 0000000001234567 0000000000000000
[   37.273651] 7ee0: ffffffffffffffff 8080000000800000 0000800000008080 feffa9a9d4ff2d66
[   37.281567] 7f00: 000000000000008e feffa9a9d5b60e0f 7f7fffffffff7f7f 0101010101010101
[   37.289485] 7f20: 0000000000000010 0000000000000008 000000000000003a 0000ffffa7ccf588
[   37.297404] 7f40: 0000aaaad6b87d00 0000ffffa7c4b1b0 0000fffff2474be0 0000aaaad6b88000
[   37.305326] 7f60: 0000fffff2474fb0 0000000001234567 0000000000000000 0000000000000000
[   37.313240] 7f80: 0000000000000000 0000000000000001 0000aaaad6b70d4d 0000000000000000
[   37.321159] 7fa0: 0000000000000001 0000fffff2474ea0 0000aaaad6b5e2e0 0000fffff2474e80
[   37.329078] 7fc0: 0000ffffa7c4b1cc 0000000000000000 fffffffffee1dead 000000000000008e
[   37.336997] 7fe0: 0000000000000000 0000000000000000 9ce839cffee77eab fafdbf9f7ed57f2f
[   37.344911] Call trace:
[   37.347437] Exception stack(0xffff800902067b20 to 0xffff800902067c50)
[   37.353970] 7b20: ffff000008e5d4c8 0001000000000000 0000000080f82000 0000000000000000
[   37.361883] 7b40: ffff800902067b60 ffff000008e17000 ffff000008f44c68 00000001081081b4
[   37.369802] 7b60: ffff800902067bf0 ffff000008108478 0000000000000000 ffff000008c235b0
[   37.377721] 7b80: ffff800902067ce0 0000000000000000 0000000000000000 0000000000000015
[   37.385643] 7ba0: 0000000000000123 000000000000008e ffff000008992000 ffff800902068000
[   37.393557] 7bc0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[   37.401477] 7be0: 0000000000000000 ffff000008c97dd8 ffff000008cc6fb0 ffff000008cc6fb8
[   37.409396] 7c00: 6974726174736552 ffff800902067a50 0000000005f5e0ff ffff000008e70000
[   37.417318] 7c20: ffff000008e706c0 ffff000008f42bfd ffff000088f42bef 0000000000000006
[   37.425234] 7c40: 00000000deadbeef 0000ffffa7c27470
[   37.430190] [<          (null)>]           (null)
[   37.434982] [<ffff000008085334>] machine_restart+0x6c/0x70
[   37.440550] [<ffff0000080e2c2c>] kernel_restart+0x6c/0x78
[   37.446030] [<ffff0000080e2ee8>] SyS_reboot+0x130/0x228
[   37.451337] [<ffff0000080830f0>] el0_svc_naked+0x24/0x28
[   37.456737] Code: bad PC value
[   37.459891] ---[ end trace 76e2fc17e050aecd ]---

Signed-off-by: Julien Grall <julien.grall@arm.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>

--

The x86 code has theoritically a similar issue, altought EFI does not
seem to be the preferred method. I have left it unimplemented on x86 and
CCed Linux Xen x86 maintainers to know their view here.

This should also probably be fixed in stable tree.
---
 arch/arm/xen/efi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/arm/xen/efi.c b/arch/arm/xen/efi.c
index 16db419f9e90..3b29788c29e9 100644
--- a/arch/arm/xen/efi.c
+++ b/arch/arm/xen/efi.c
@@ -19,6 +19,14 @@
 #include <xen/xen-ops.h>
 #include <asm/xen/xen-ops.h>
 
+static void xen_efi_reset_system(int reset_type,
+				 efi_status_t status,
+				 unsigned long data_size,
+				 efi_char16_t *data)
+{
+	/* NOP implementation, reset will fallback on an alternative method */
+}
+
 /* Set XEN EFI runtime services function pointers. Other fields of struct efi,
  * e.g. efi.systab, will be set like normal EFI.
  */
@@ -35,6 +43,6 @@ void __init xen_efi_runtime_setup(void)
 	efi.update_capsule           = xen_efi_update_capsule;
 	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
 	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
-	efi.reset_system             = NULL; /* Functionality provided by Xen. */
+	efi.reset_system             = xen_efi_reset_system;
 }
 EXPORT_SYMBOL_GPL(xen_efi_runtime_setup);
-- 
2.11.0

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-05 18:14 [PATCH] arm64: xen: Implement EFI reset_system callback Julien Grall
@ 2017-04-05 19:49 ` Boris Ostrovsky
  2017-04-06  6:23   ` Juergen Gross
  2017-04-05 21:26 ` Stefano Stabellini
  1 sibling, 1 reply; 26+ messages in thread
From: Boris Ostrovsky @ 2017-04-05 19:49 UTC (permalink / raw)
  To: Julien Grall, xen-devel
  Cc: sstabellini, linux-arm-kernel, linux-kernel, Juergen Gross, Daniel Kiper

On 04/05/2017 02:14 PM, Julien Grall wrote:
> When rebooting DOM0 with ACPI, the kernel is crashing with the stack trace [1].
>
> This is happening because when EFI runtimes are enabled, the reset code
> (see machin_restart) will first try to use EFI restart method.
>
> However, the EFI restart code is expecting the reset_system callback to
> be always set. This is not the case for Xen and will lead to crash.
>
> Looking at the reboot path, it is expected to fallback on an alternative
> reboot method if one does not work. So implement reset_system callback
> as a NOP for Xen.
>
> [   36.999270] reboot: Restarting system
> [   37.002921] Internal error: Attempting to execute userspace memory: 86000004 [#1] PREEMPT SMP
> [   37.011460] Modules linked in:
> [   37.014598] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 4.11.0-rc1-00003-g1e248b60a39b-dirty #506
> [   37.023903] Hardware name: (null) (DT)
> [   37.027734] task: ffff800902068000 task.stack: ffff800902064000
> [   37.033739] PC is at 0x0
> [   37.036359] LR is at efi_reboot+0x94/0xd0
> [   37.040438] pc : [<0000000000000000>] lr : [<ffff00000880f2c4>] pstate: 404001c5
> [   37.047920] sp : ffff800902067cf0
> [   37.051314] x29: ffff800902067cf0 x28: ffff800902068000
> [   37.056709] x27: ffff000008992000 x26: 000000000000008e
> [   37.062104] x25: 0000000000000123 x24: 0000000000000015
> [   37.067499] x23: 0000000000000000 x22: ffff000008e6e250
> [   37.072894] x21: ffff000008e6e000 x20: 0000000000000000
> [   37.078289] x19: ffff000008e5d4c8 x18: 0000000000000010
> [   37.083684] x17: 0000ffffa7c27470 x16: 00000000deadbeef
> [   37.089079] x15: 0000000000000006 x14: ffff000088f42bef
> [   37.094474] x13: ffff000008f42bfd x12: ffff000008e706c0
> [   37.099870] x11: ffff000008e70000 x10: 0000000005f5e0ff
> [   37.105265] x9 : ffff800902067a50 x8 : 6974726174736552
> [   37.110660] x7 : ffff000008cc6fb8 x6 : ffff000008cc6fb0
> [   37.116055] x5 : ffff000008c97dd8 x4 : 0000000000000000
> [   37.121453] x3 : 0000000000000000 x2 : 0000000000000000
> [   37.126845] x1 : 0000000000000000 x0 : 0000000000000000
> [   37.132239]
> [   37.133808] Process systemd-shutdow (pid: 1, stack limit = 0xffff800902064000)
> [   37.141118] Stack: (0xffff800902067cf0 to 0xffff800902068000)
> [   37.146949] 7ce0:                                   ffff800902067d40 ffff000008085334
> [   37.154869] 7d00: 0000000000000000 ffff000008f3b000 ffff800902067d40 ffff0000080852e0
> [   37.162787] 7d20: ffff000008cc6fb0 ffff000008cc6fb8 ffff000008c7f580 ffff000008c97dd8
> [   37.170706] 7d40: ffff800902067d60 ffff0000080e2c2c 0000000000000000 0000000001234567
> [   37.178624] 7d60: ffff800902067d80 ffff0000080e2ee8 0000000000000000 ffff0000080e2df4
> [   37.186544] 7d80: 0000000000000000 ffff0000080830f0 0000000000000000 00008008ff1c1000
> [   37.194462] 7da0: ffffffffffffffff 0000ffffa7c4b1cc 0000000000000000 0000000000000024
> [   37.202380] 7dc0: ffff800902067dd0 0000000000000005 0000fffff24743c8 0000000000000004
> [   37.210299] 7de0: 0000fffff2475f03 0000000000000010 0000fffff2474418 0000000000000005
> [   37.218218] 7e00: 0000fffff2474578 000000000000000a 0000aaaad6b722c0 0000000000000001
> [   37.226136] 7e20: 0000000000000123 0000000000000038 ffff800902067e50 ffff0000081e7294
> [   37.234055] 7e40: ffff800902067e60 ffff0000081e935c ffff800902067e60 ffff0000081e9388
> [   37.241973] 7e60: ffff800902067eb0 ffff0000081ea388 0000000000000000 00008008ff1c1000
> [   37.249892] 7e80: ffffffffffffffff 0000ffffa7c4a79c 0000000000000000 ffff000000020000
> [   37.257810] 7ea0: 0000010000000004 0000000000000000 0000000000000000 ffff0000080830f0
> [   37.265729] 7ec0: fffffffffee1dead 0000000028121969 0000000001234567 0000000000000000
> [   37.273651] 7ee0: ffffffffffffffff 8080000000800000 0000800000008080 feffa9a9d4ff2d66
> [   37.281567] 7f00: 000000000000008e feffa9a9d5b60e0f 7f7fffffffff7f7f 0101010101010101
> [   37.289485] 7f20: 0000000000000010 0000000000000008 000000000000003a 0000ffffa7ccf588
> [   37.297404] 7f40: 0000aaaad6b87d00 0000ffffa7c4b1b0 0000fffff2474be0 0000aaaad6b88000
> [   37.305326] 7f60: 0000fffff2474fb0 0000000001234567 0000000000000000 0000000000000000
> [   37.313240] 7f80: 0000000000000000 0000000000000001 0000aaaad6b70d4d 0000000000000000
> [   37.321159] 7fa0: 0000000000000001 0000fffff2474ea0 0000aaaad6b5e2e0 0000fffff2474e80
> [   37.329078] 7fc0: 0000ffffa7c4b1cc 0000000000000000 fffffffffee1dead 000000000000008e
> [   37.336997] 7fe0: 0000000000000000 0000000000000000 9ce839cffee77eab fafdbf9f7ed57f2f
> [   37.344911] Call trace:
> [   37.347437] Exception stack(0xffff800902067b20 to 0xffff800902067c50)
> [   37.353970] 7b20: ffff000008e5d4c8 0001000000000000 0000000080f82000 0000000000000000
> [   37.361883] 7b40: ffff800902067b60 ffff000008e17000 ffff000008f44c68 00000001081081b4
> [   37.369802] 7b60: ffff800902067bf0 ffff000008108478 0000000000000000 ffff000008c235b0
> [   37.377721] 7b80: ffff800902067ce0 0000000000000000 0000000000000000 0000000000000015
> [   37.385643] 7ba0: 0000000000000123 000000000000008e ffff000008992000 ffff800902068000
> [   37.393557] 7bc0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [   37.401477] 7be0: 0000000000000000 ffff000008c97dd8 ffff000008cc6fb0 ffff000008cc6fb8
> [   37.409396] 7c00: 6974726174736552 ffff800902067a50 0000000005f5e0ff ffff000008e70000
> [   37.417318] 7c20: ffff000008e706c0 ffff000008f42bfd ffff000088f42bef 0000000000000006
> [   37.425234] 7c40: 00000000deadbeef 0000ffffa7c27470
> [   37.430190] [<          (null)>]           (null)
> [   37.434982] [<ffff000008085334>] machine_restart+0x6c/0x70
> [   37.440550] [<ffff0000080e2c2c>] kernel_restart+0x6c/0x78
> [   37.446030] [<ffff0000080e2ee8>] SyS_reboot+0x130/0x228
> [   37.451337] [<ffff0000080830f0>] el0_svc_naked+0x24/0x28
> [   37.456737] Code: bad PC value
> [   37.459891] ---[ end trace 76e2fc17e050aecd ]---
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
>
> --
>
> The x86 code has theoritically a similar issue, altought EFI does not
> seem to be the preferred method. I have left it unimplemented on x86 and
> CCed Linux Xen x86 maintainers to know their view here.

(+Daniel)

This could be a problem for x86 as well, at least theoretically.
xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.

So I think we should have a similar routine there.

-boris


>
> This should also probably be fixed in stable tree.
> ---
>  arch/arm/xen/efi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/xen/efi.c b/arch/arm/xen/efi.c
> index 16db419f9e90..3b29788c29e9 100644
> --- a/arch/arm/xen/efi.c
> +++ b/arch/arm/xen/efi.c
> @@ -19,6 +19,14 @@
>  #include <xen/xen-ops.h>
>  #include <asm/xen/xen-ops.h>
>  
> +static void xen_efi_reset_system(int reset_type,
> +				 efi_status_t status,
> +				 unsigned long data_size,
> +				 efi_char16_t *data)
> +{
> +	/* NOP implementation, reset will fallback on an alternative method */
> +}
> +
>  /* Set XEN EFI runtime services function pointers. Other fields of struct efi,
>   * e.g. efi.systab, will be set like normal EFI.
>   */
> @@ -35,6 +43,6 @@ void __init xen_efi_runtime_setup(void)
>  	efi.update_capsule           = xen_efi_update_capsule;
>  	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
>  	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
> -	efi.reset_system             = NULL; /* Functionality provided by Xen. */
> +	efi.reset_system             = xen_efi_reset_system;
>  }
>  EXPORT_SYMBOL_GPL(xen_efi_runtime_setup);

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-05 18:14 [PATCH] arm64: xen: Implement EFI reset_system callback Julien Grall
  2017-04-05 19:49 ` Boris Ostrovsky
@ 2017-04-05 21:26 ` Stefano Stabellini
  1 sibling, 0 replies; 26+ messages in thread
From: Stefano Stabellini @ 2017-04-05 21:26 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, sstabellini, linux-arm-kernel, linux-kernel,
	Boris Ostrovsky, Juergen Gross

On Wed, 5 Apr 2017, Julien Grall wrote:
> When rebooting DOM0 with ACPI, the kernel is crashing with the stack trace [1].
> 
> This is happening because when EFI runtimes are enabled, the reset code
> (see machin_restart) will first try to use EFI restart method.
> 
> However, the EFI restart code is expecting the reset_system callback to
> be always set. This is not the case for Xen and will lead to crash.
> 
> Looking at the reboot path, it is expected to fallback on an alternative
> reboot method if one does not work. So implement reset_system callback
> as a NOP for Xen.
> 
> [   36.999270] reboot: Restarting system
> [   37.002921] Internal error: Attempting to execute userspace memory: 86000004 [#1] PREEMPT SMP
> [   37.011460] Modules linked in:
> [   37.014598] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 4.11.0-rc1-00003-g1e248b60a39b-dirty #506
> [   37.023903] Hardware name: (null) (DT)
> [   37.027734] task: ffff800902068000 task.stack: ffff800902064000
> [   37.033739] PC is at 0x0
> [   37.036359] LR is at efi_reboot+0x94/0xd0
> [   37.040438] pc : [<0000000000000000>] lr : [<ffff00000880f2c4>] pstate: 404001c5
> [   37.047920] sp : ffff800902067cf0
> [   37.051314] x29: ffff800902067cf0 x28: ffff800902068000
> [   37.056709] x27: ffff000008992000 x26: 000000000000008e
> [   37.062104] x25: 0000000000000123 x24: 0000000000000015
> [   37.067499] x23: 0000000000000000 x22: ffff000008e6e250
> [   37.072894] x21: ffff000008e6e000 x20: 0000000000000000
> [   37.078289] x19: ffff000008e5d4c8 x18: 0000000000000010
> [   37.083684] x17: 0000ffffa7c27470 x16: 00000000deadbeef
> [   37.089079] x15: 0000000000000006 x14: ffff000088f42bef
> [   37.094474] x13: ffff000008f42bfd x12: ffff000008e706c0
> [   37.099870] x11: ffff000008e70000 x10: 0000000005f5e0ff
> [   37.105265] x9 : ffff800902067a50 x8 : 6974726174736552
> [   37.110660] x7 : ffff000008cc6fb8 x6 : ffff000008cc6fb0
> [   37.116055] x5 : ffff000008c97dd8 x4 : 0000000000000000
> [   37.121453] x3 : 0000000000000000 x2 : 0000000000000000
> [   37.126845] x1 : 0000000000000000 x0 : 0000000000000000
> [   37.132239]
> [   37.133808] Process systemd-shutdow (pid: 1, stack limit = 0xffff800902064000)
> [   37.141118] Stack: (0xffff800902067cf0 to 0xffff800902068000)
> [   37.146949] 7ce0:                                   ffff800902067d40 ffff000008085334
> [   37.154869] 7d00: 0000000000000000 ffff000008f3b000 ffff800902067d40 ffff0000080852e0
> [   37.162787] 7d20: ffff000008cc6fb0 ffff000008cc6fb8 ffff000008c7f580 ffff000008c97dd8
> [   37.170706] 7d40: ffff800902067d60 ffff0000080e2c2c 0000000000000000 0000000001234567
> [   37.178624] 7d60: ffff800902067d80 ffff0000080e2ee8 0000000000000000 ffff0000080e2df4
> [   37.186544] 7d80: 0000000000000000 ffff0000080830f0 0000000000000000 00008008ff1c1000
> [   37.194462] 7da0: ffffffffffffffff 0000ffffa7c4b1cc 0000000000000000 0000000000000024
> [   37.202380] 7dc0: ffff800902067dd0 0000000000000005 0000fffff24743c8 0000000000000004
> [   37.210299] 7de0: 0000fffff2475f03 0000000000000010 0000fffff2474418 0000000000000005
> [   37.218218] 7e00: 0000fffff2474578 000000000000000a 0000aaaad6b722c0 0000000000000001
> [   37.226136] 7e20: 0000000000000123 0000000000000038 ffff800902067e50 ffff0000081e7294
> [   37.234055] 7e40: ffff800902067e60 ffff0000081e935c ffff800902067e60 ffff0000081e9388
> [   37.241973] 7e60: ffff800902067eb0 ffff0000081ea388 0000000000000000 00008008ff1c1000
> [   37.249892] 7e80: ffffffffffffffff 0000ffffa7c4a79c 0000000000000000 ffff000000020000
> [   37.257810] 7ea0: 0000010000000004 0000000000000000 0000000000000000 ffff0000080830f0
> [   37.265729] 7ec0: fffffffffee1dead 0000000028121969 0000000001234567 0000000000000000
> [   37.273651] 7ee0: ffffffffffffffff 8080000000800000 0000800000008080 feffa9a9d4ff2d66
> [   37.281567] 7f00: 000000000000008e feffa9a9d5b60e0f 7f7fffffffff7f7f 0101010101010101
> [   37.289485] 7f20: 0000000000000010 0000000000000008 000000000000003a 0000ffffa7ccf588
> [   37.297404] 7f40: 0000aaaad6b87d00 0000ffffa7c4b1b0 0000fffff2474be0 0000aaaad6b88000
> [   37.305326] 7f60: 0000fffff2474fb0 0000000001234567 0000000000000000 0000000000000000
> [   37.313240] 7f80: 0000000000000000 0000000000000001 0000aaaad6b70d4d 0000000000000000
> [   37.321159] 7fa0: 0000000000000001 0000fffff2474ea0 0000aaaad6b5e2e0 0000fffff2474e80
> [   37.329078] 7fc0: 0000ffffa7c4b1cc 0000000000000000 fffffffffee1dead 000000000000008e
> [   37.336997] 7fe0: 0000000000000000 0000000000000000 9ce839cffee77eab fafdbf9f7ed57f2f
> [   37.344911] Call trace:
> [   37.347437] Exception stack(0xffff800902067b20 to 0xffff800902067c50)
> [   37.353970] 7b20: ffff000008e5d4c8 0001000000000000 0000000080f82000 0000000000000000
> [   37.361883] 7b40: ffff800902067b60 ffff000008e17000 ffff000008f44c68 00000001081081b4
> [   37.369802] 7b60: ffff800902067bf0 ffff000008108478 0000000000000000 ffff000008c235b0
> [   37.377721] 7b80: ffff800902067ce0 0000000000000000 0000000000000000 0000000000000015
> [   37.385643] 7ba0: 0000000000000123 000000000000008e ffff000008992000 ffff800902068000
> [   37.393557] 7bc0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [   37.401477] 7be0: 0000000000000000 ffff000008c97dd8 ffff000008cc6fb0 ffff000008cc6fb8
> [   37.409396] 7c00: 6974726174736552 ffff800902067a50 0000000005f5e0ff ffff000008e70000
> [   37.417318] 7c20: ffff000008e706c0 ffff000008f42bfd ffff000088f42bef 0000000000000006
> [   37.425234] 7c40: 00000000deadbeef 0000ffffa7c27470
> [   37.430190] [<          (null)>]           (null)
> [   37.434982] [<ffff000008085334>] machine_restart+0x6c/0x70
> [   37.440550] [<ffff0000080e2c2c>] kernel_restart+0x6c/0x78
> [   37.446030] [<ffff0000080e2ee8>] SyS_reboot+0x130/0x228
> [   37.451337] [<ffff0000080830f0>] el0_svc_naked+0x24/0x28
> [   37.456737] Code: bad PC value
> [   37.459891] ---[ end trace 76e2fc17e050aecd ]---
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> --
> 
> The x86 code has theoritically a similar issue, altought EFI does not
> seem to be the preferred method. I have left it unimplemented on x86 and
> CCed Linux Xen x86 maintainers to know their view here.
> 
> This should also probably be fixed in stable tree.
> ---
>  arch/arm/xen/efi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/xen/efi.c b/arch/arm/xen/efi.c
> index 16db419f9e90..3b29788c29e9 100644
> --- a/arch/arm/xen/efi.c
> +++ b/arch/arm/xen/efi.c
> @@ -19,6 +19,14 @@
>  #include <xen/xen-ops.h>
>  #include <asm/xen/xen-ops.h>
>  
> +static void xen_efi_reset_system(int reset_type,
> +				 efi_status_t status,
> +				 unsigned long data_size,
> +				 efi_char16_t *data)
> +{
> +	/* NOP implementation, reset will fallback on an alternative method */
> +}
> +
>  /* Set XEN EFI runtime services function pointers. Other fields of struct efi,
>   * e.g. efi.systab, will be set like normal EFI.
>   */
> @@ -35,6 +43,6 @@ void __init xen_efi_runtime_setup(void)
>  	efi.update_capsule           = xen_efi_update_capsule;
>  	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
>  	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
> -	efi.reset_system             = NULL; /* Functionality provided by Xen. */
> +	efi.reset_system             = xen_efi_reset_system;
>  }
>  EXPORT_SYMBOL_GPL(xen_efi_runtime_setup);
> -- 
> 2.11.0
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-05 19:49 ` Boris Ostrovsky
@ 2017-04-06  6:23   ` Juergen Gross
  2017-04-06  8:32     ` Julien Grall
  0 siblings, 1 reply; 26+ messages in thread
From: Juergen Gross @ 2017-04-06  6:23 UTC (permalink / raw)
  To: Boris Ostrovsky, Julien Grall, xen-devel
  Cc: sstabellini, linux-arm-kernel, linux-kernel, Daniel Kiper

On 05/04/17 21:49, Boris Ostrovsky wrote:
> On 04/05/2017 02:14 PM, Julien Grall wrote:
>> When rebooting DOM0 with ACPI, the kernel is crashing with the stack trace [1].
>>
>> This is happening because when EFI runtimes are enabled, the reset code
>> (see machin_restart) will first try to use EFI restart method.
>>
>> However, the EFI restart code is expecting the reset_system callback to
>> be always set. This is not the case for Xen and will lead to crash.
>>
>> Looking at the reboot path, it is expected to fallback on an alternative
>> reboot method if one does not work. So implement reset_system callback
>> as a NOP for Xen.
>>
>> [   36.999270] reboot: Restarting system
>> [   37.002921] Internal error: Attempting to execute userspace memory: 86000004 [#1] PREEMPT SMP
>> [   37.011460] Modules linked in:
>> [   37.014598] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 4.11.0-rc1-00003-g1e248b60a39b-dirty #506
>> [   37.023903] Hardware name: (null) (DT)
>> [   37.027734] task: ffff800902068000 task.stack: ffff800902064000
>> [   37.033739] PC is at 0x0
>> [   37.036359] LR is at efi_reboot+0x94/0xd0
>> [   37.040438] pc : [<0000000000000000>] lr : [<ffff00000880f2c4>] pstate: 404001c5
>> [   37.047920] sp : ffff800902067cf0
>> [   37.051314] x29: ffff800902067cf0 x28: ffff800902068000
>> [   37.056709] x27: ffff000008992000 x26: 000000000000008e
>> [   37.062104] x25: 0000000000000123 x24: 0000000000000015
>> [   37.067499] x23: 0000000000000000 x22: ffff000008e6e250
>> [   37.072894] x21: ffff000008e6e000 x20: 0000000000000000
>> [   37.078289] x19: ffff000008e5d4c8 x18: 0000000000000010
>> [   37.083684] x17: 0000ffffa7c27470 x16: 00000000deadbeef
>> [   37.089079] x15: 0000000000000006 x14: ffff000088f42bef
>> [   37.094474] x13: ffff000008f42bfd x12: ffff000008e706c0
>> [   37.099870] x11: ffff000008e70000 x10: 0000000005f5e0ff
>> [   37.105265] x9 : ffff800902067a50 x8 : 6974726174736552
>> [   37.110660] x7 : ffff000008cc6fb8 x6 : ffff000008cc6fb0
>> [   37.116055] x5 : ffff000008c97dd8 x4 : 0000000000000000
>> [   37.121453] x3 : 0000000000000000 x2 : 0000000000000000
>> [   37.126845] x1 : 0000000000000000 x0 : 0000000000000000
>> [   37.132239]
>> [   37.133808] Process systemd-shutdow (pid: 1, stack limit = 0xffff800902064000)
>> [   37.141118] Stack: (0xffff800902067cf0 to 0xffff800902068000)
>> [   37.146949] 7ce0:                                   ffff800902067d40 ffff000008085334
>> [   37.154869] 7d00: 0000000000000000 ffff000008f3b000 ffff800902067d40 ffff0000080852e0
>> [   37.162787] 7d20: ffff000008cc6fb0 ffff000008cc6fb8 ffff000008c7f580 ffff000008c97dd8
>> [   37.170706] 7d40: ffff800902067d60 ffff0000080e2c2c 0000000000000000 0000000001234567
>> [   37.178624] 7d60: ffff800902067d80 ffff0000080e2ee8 0000000000000000 ffff0000080e2df4
>> [   37.186544] 7d80: 0000000000000000 ffff0000080830f0 0000000000000000 00008008ff1c1000
>> [   37.194462] 7da0: ffffffffffffffff 0000ffffa7c4b1cc 0000000000000000 0000000000000024
>> [   37.202380] 7dc0: ffff800902067dd0 0000000000000005 0000fffff24743c8 0000000000000004
>> [   37.210299] 7de0: 0000fffff2475f03 0000000000000010 0000fffff2474418 0000000000000005
>> [   37.218218] 7e00: 0000fffff2474578 000000000000000a 0000aaaad6b722c0 0000000000000001
>> [   37.226136] 7e20: 0000000000000123 0000000000000038 ffff800902067e50 ffff0000081e7294
>> [   37.234055] 7e40: ffff800902067e60 ffff0000081e935c ffff800902067e60 ffff0000081e9388
>> [   37.241973] 7e60: ffff800902067eb0 ffff0000081ea388 0000000000000000 00008008ff1c1000
>> [   37.249892] 7e80: ffffffffffffffff 0000ffffa7c4a79c 0000000000000000 ffff000000020000
>> [   37.257810] 7ea0: 0000010000000004 0000000000000000 0000000000000000 ffff0000080830f0
>> [   37.265729] 7ec0: fffffffffee1dead 0000000028121969 0000000001234567 0000000000000000
>> [   37.273651] 7ee0: ffffffffffffffff 8080000000800000 0000800000008080 feffa9a9d4ff2d66
>> [   37.281567] 7f00: 000000000000008e feffa9a9d5b60e0f 7f7fffffffff7f7f 0101010101010101
>> [   37.289485] 7f20: 0000000000000010 0000000000000008 000000000000003a 0000ffffa7ccf588
>> [   37.297404] 7f40: 0000aaaad6b87d00 0000ffffa7c4b1b0 0000fffff2474be0 0000aaaad6b88000
>> [   37.305326] 7f60: 0000fffff2474fb0 0000000001234567 0000000000000000 0000000000000000
>> [   37.313240] 7f80: 0000000000000000 0000000000000001 0000aaaad6b70d4d 0000000000000000
>> [   37.321159] 7fa0: 0000000000000001 0000fffff2474ea0 0000aaaad6b5e2e0 0000fffff2474e80
>> [   37.329078] 7fc0: 0000ffffa7c4b1cc 0000000000000000 fffffffffee1dead 000000000000008e
>> [   37.336997] 7fe0: 0000000000000000 0000000000000000 9ce839cffee77eab fafdbf9f7ed57f2f
>> [   37.344911] Call trace:
>> [   37.347437] Exception stack(0xffff800902067b20 to 0xffff800902067c50)
>> [   37.353970] 7b20: ffff000008e5d4c8 0001000000000000 0000000080f82000 0000000000000000
>> [   37.361883] 7b40: ffff800902067b60 ffff000008e17000 ffff000008f44c68 00000001081081b4
>> [   37.369802] 7b60: ffff800902067bf0 ffff000008108478 0000000000000000 ffff000008c235b0
>> [   37.377721] 7b80: ffff800902067ce0 0000000000000000 0000000000000000 0000000000000015
>> [   37.385643] 7ba0: 0000000000000123 000000000000008e ffff000008992000 ffff800902068000
>> [   37.393557] 7bc0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> [   37.401477] 7be0: 0000000000000000 ffff000008c97dd8 ffff000008cc6fb0 ffff000008cc6fb8
>> [   37.409396] 7c00: 6974726174736552 ffff800902067a50 0000000005f5e0ff ffff000008e70000
>> [   37.417318] 7c20: ffff000008e706c0 ffff000008f42bfd ffff000088f42bef 0000000000000006
>> [   37.425234] 7c40: 00000000deadbeef 0000ffffa7c27470
>> [   37.430190] [<          (null)>]           (null)
>> [   37.434982] [<ffff000008085334>] machine_restart+0x6c/0x70
>> [   37.440550] [<ffff0000080e2c2c>] kernel_restart+0x6c/0x78
>> [   37.446030] [<ffff0000080e2ee8>] SyS_reboot+0x130/0x228
>> [   37.451337] [<ffff0000080830f0>] el0_svc_naked+0x24/0x28
>> [   37.456737] Code: bad PC value
>> [   37.459891] ---[ end trace 76e2fc17e050aecd ]---
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: Juergen Gross <jgross@suse.com>
>>
>> --
>>
>> The x86 code has theoritically a similar issue, altought EFI does not
>> seem to be the preferred method. I have left it unimplemented on x86 and
>> CCed Linux Xen x86 maintainers to know their view here.
> 
> (+Daniel)
> 
> This could be a problem for x86 as well, at least theoretically.
> xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
> 
> So I think we should have a similar routine there.

+1

I don't see any problem with such a routine added, in contrast to
potential "reboots" instead of power off without it.

So I think this dummy xen_efi_reset_system() should be added to
drivers/xen/efi.c instead.

>> This should also probably be fixed in stable tree.

Yes.


Juergen

>> ---
>>  arch/arm/xen/efi.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/xen/efi.c b/arch/arm/xen/efi.c
>> index 16db419f9e90..3b29788c29e9 100644
>> --- a/arch/arm/xen/efi.c
>> +++ b/arch/arm/xen/efi.c
>> @@ -19,6 +19,14 @@
>>  #include <xen/xen-ops.h>
>>  #include <asm/xen/xen-ops.h>
>>  
>> +static void xen_efi_reset_system(int reset_type,
>> +				 efi_status_t status,
>> +				 unsigned long data_size,
>> +				 efi_char16_t *data)
>> +{
>> +	/* NOP implementation, reset will fallback on an alternative method */
>> +}
>> +
>>  /* Set XEN EFI runtime services function pointers. Other fields of struct efi,
>>   * e.g. efi.systab, will be set like normal EFI.
>>   */
>> @@ -35,6 +43,6 @@ void __init xen_efi_runtime_setup(void)
>>  	efi.update_capsule           = xen_efi_update_capsule;
>>  	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
>>  	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
>> -	efi.reset_system             = NULL; /* Functionality provided by Xen. */
>> +	efi.reset_system             = xen_efi_reset_system;
>>  }
>>  EXPORT_SYMBOL_GPL(xen_efi_runtime_setup);
> 
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-06  6:23   ` Juergen Gross
@ 2017-04-06  8:32     ` Julien Grall
  2017-04-06  8:37       ` Juergen Gross
  2017-04-06 14:27       ` Daniel Kiper
  0 siblings, 2 replies; 26+ messages in thread
From: Julien Grall @ 2017-04-06  8:32 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky, xen-devel
  Cc: sstabellini, linux-arm-kernel, linux-kernel, Daniel Kiper

Hi Juergen,

On 06/04/17 07:23, Juergen Gross wrote:
> On 05/04/17 21:49, Boris Ostrovsky wrote:
>> On 04/05/2017 02:14 PM, Julien Grall wrote:
>>> The x86 code has theoritically a similar issue, altought EFI does not
>>> seem to be the preferred method. I have left it unimplemented on x86 and
>>> CCed Linux Xen x86 maintainers to know their view here.
>>
>> (+Daniel)
>>
>> This could be a problem for x86 as well, at least theoretically.
>> xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
>>
>> So I think we should have a similar routine there.
>
> +1
>
> I don't see any problem with such a routine added, in contrast to
> potential "reboots" instead of power off without it.
>
> So I think this dummy xen_efi_reset_system() should be added to
> drivers/xen/efi.c instead.

I will resend the patch during day with xen_efi_reset_system moved to 
common code and implement the x86 counterpart (thought, I will not be 
able to test it).

>
>>> This should also probably be fixed in stable tree.
>
> Yes.

I will CC stable.

Thank you,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-06  8:32     ` Julien Grall
@ 2017-04-06  8:37       ` Juergen Gross
  2017-04-06 14:27       ` Daniel Kiper
  1 sibling, 0 replies; 26+ messages in thread
From: Juergen Gross @ 2017-04-06  8:37 UTC (permalink / raw)
  To: Julien Grall, Boris Ostrovsky, xen-devel
  Cc: sstabellini, linux-arm-kernel, linux-kernel, Daniel Kiper

On 06/04/17 10:32, Julien Grall wrote:
> Hi Juergen,
> 
> On 06/04/17 07:23, Juergen Gross wrote:
>> On 05/04/17 21:49, Boris Ostrovsky wrote:
>>> On 04/05/2017 02:14 PM, Julien Grall wrote:
>>>> The x86 code has theoritically a similar issue, altought EFI does not
>>>> seem to be the preferred method. I have left it unimplemented on x86
>>>> and
>>>> CCed Linux Xen x86 maintainers to know their view here.
>>>
>>> (+Daniel)
>>>
>>> This could be a problem for x86 as well, at least theoretically.
>>> xen_machine_power_off() may call pm_power_off(), which is
>>> efi.reset_system.
>>>
>>> So I think we should have a similar routine there.
>>
>> +1
>>
>> I don't see any problem with such a routine added, in contrast to
>> potential "reboots" instead of power off without it.
>>
>> So I think this dummy xen_efi_reset_system() should be added to
>> drivers/xen/efi.c instead.
> 
> I will resend the patch during day with xen_efi_reset_system moved to
> common code and implement the x86 counterpart (thought, I will not be
> able to test it).

I'm rather sure it isn't hit very often. Otherwise there would be more
complaints about crashes during power off (in fact I do remember several
occasions where somebody claimed power off seemed to do a reboot only).


Juergen

>>
>>>> This should also probably be fixed in stable tree.
>>
>> Yes.
> 
> I will CC stable.
> 
> Thank you,
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-06  8:32     ` Julien Grall
  2017-04-06  8:37       ` Juergen Gross
@ 2017-04-06 14:27       ` Daniel Kiper
  2017-04-06 14:32         ` Julien Grall
  2017-04-06 14:38         ` Juergen Gross
  1 sibling, 2 replies; 26+ messages in thread
From: Daniel Kiper @ 2017-04-06 14:27 UTC (permalink / raw)
  To: Julien Grall
  Cc: Juergen Gross, Boris Ostrovsky, xen-devel, sstabellini,
	linux-arm-kernel, linux-kernel

On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
> Hi Juergen,
>
> On 06/04/17 07:23, Juergen Gross wrote:
> >On 05/04/17 21:49, Boris Ostrovsky wrote:
> >>On 04/05/2017 02:14 PM, Julien Grall wrote:
> >>>The x86 code has theoritically a similar issue, altought EFI does not
> >>>seem to be the preferred method. I have left it unimplemented on x86 and
> >>>CCed Linux Xen x86 maintainers to know their view here.
> >>
> >>(+Daniel)
> >>
> >>This could be a problem for x86 as well, at least theoretically.
> >>xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
> >>
> >>So I think we should have a similar routine there.
> >
> >+1
> >
> >I don't see any problem with such a routine added, in contrast to
> >potential "reboots" instead of power off without it.
> >
> >So I think this dummy xen_efi_reset_system() should be added to
> >drivers/xen/efi.c instead.
>
> I will resend the patch during day with xen_efi_reset_system moved
> to common code and implement the x86 counterpart (thought, I will
> not be able to test it).

I think that this is ARM specific issue. On x86 machine_restart() calls
xen_restart(). Hence, everything works. So, I think that it should be
fixed only for ARM. Anyway, please CC me when you send a patch.

Daniel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-06 14:27       ` Daniel Kiper
@ 2017-04-06 14:32         ` Julien Grall
  2017-04-06 14:37           ` Boris Ostrovsky
  2017-04-06 14:38         ` Juergen Gross
  1 sibling, 1 reply; 26+ messages in thread
From: Julien Grall @ 2017-04-06 14:32 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: Juergen Gross, Boris Ostrovsky, xen-devel, sstabellini,
	linux-arm-kernel, linux-kernel

Hi Daniel,

On 06/04/17 15:27, Daniel Kiper wrote:
> On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 06/04/17 07:23, Juergen Gross wrote:
>>> On 05/04/17 21:49, Boris Ostrovsky wrote:
>>>> On 04/05/2017 02:14 PM, Julien Grall wrote:
>>>>> The x86 code has theoritically a similar issue, altought EFI does not
>>>>> seem to be the preferred method. I have left it unimplemented on x86 and
>>>>> CCed Linux Xen x86 maintainers to know their view here.
>>>>
>>>> (+Daniel)
>>>>
>>>> This could be a problem for x86 as well, at least theoretically.
>>>> xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
>>>>
>>>> So I think we should have a similar routine there.
>>>
>>> +1
>>>
>>> I don't see any problem with such a routine added, in contrast to
>>> potential "reboots" instead of power off without it.
>>>
>>> So I think this dummy xen_efi_reset_system() should be added to
>>> drivers/xen/efi.c instead.
>>
>> I will resend the patch during day with xen_efi_reset_system moved
>> to common code and implement the x86 counterpart (thought, I will
>> not be able to test it).
>
> I think that this is ARM specific issue. On x86 machine_restart() calls
> xen_restart(). Hence, everything works. So, I think that it should be
> fixed only for ARM. Anyway, please CC me when you send a patch.

This thread already a fix for ARM64. So do I need to resend a patch with 
x86 fixed or not?

Cheers,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-06 14:32         ` Julien Grall
@ 2017-04-06 14:37           ` Boris Ostrovsky
  0 siblings, 0 replies; 26+ messages in thread
From: Boris Ostrovsky @ 2017-04-06 14:37 UTC (permalink / raw)
  To: Julien Grall, Daniel Kiper
  Cc: Juergen Gross, xen-devel, sstabellini, linux-arm-kernel, linux-kernel

On 04/06/2017 10:32 AM, Julien Grall wrote:
> Hi Daniel,
>
> On 06/04/17 15:27, Daniel Kiper wrote:
>> On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 06/04/17 07:23, Juergen Gross wrote:
>>>> On 05/04/17 21:49, Boris Ostrovsky wrote:
>>>>> On 04/05/2017 02:14 PM, Julien Grall wrote:
>>>>>> The x86 code has theoritically a similar issue, altought EFI does
>>>>>> not
>>>>>> seem to be the preferred method. I have left it unimplemented on
>>>>>> x86 and
>>>>>> CCed Linux Xen x86 maintainers to know their view here.
>>>>>
>>>>> (+Daniel)
>>>>>
>>>>> This could be a problem for x86 as well, at least theoretically.
>>>>> xen_machine_power_off() may call pm_power_off(), which is
>>>>> efi.reset_system.
>>>>>
>>>>> So I think we should have a similar routine there.
>>>>
>>>> +1
>>>>
>>>> I don't see any problem with such a routine added, in contrast to
>>>> potential "reboots" instead of power off without it.
>>>>
>>>> So I think this dummy xen_efi_reset_system() should be added to
>>>> drivers/xen/efi.c instead.
>>>
>>> I will resend the patch during day with xen_efi_reset_system moved
>>> to common code and implement the x86 counterpart (thought, I will
>>> not be able to test it).
>>
>> I think that this is ARM specific issue. On x86 machine_restart() calls
>> xen_restart(). Hence, everything works. So, I think that it should be
>> fixed only for ARM. Anyway, please CC me when you send a patch.
>
> This thread already a fix for ARM64. So do I need to resend a patch
> with x86 fixed or not?

Yes please. Daniel is correct that we are safe with xen_restart().
However, we are not safe when machine_ops.power_off is called.

Thanks.
-boris

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-06 14:27       ` Daniel Kiper
  2017-04-06 14:32         ` Julien Grall
@ 2017-04-06 14:38         ` Juergen Gross
  2017-04-06 15:20           ` Daniel Kiper
  1 sibling, 1 reply; 26+ messages in thread
From: Juergen Gross @ 2017-04-06 14:38 UTC (permalink / raw)
  To: Daniel Kiper, Julien Grall
  Cc: Boris Ostrovsky, xen-devel, sstabellini, linux-arm-kernel, linux-kernel

On 06/04/17 16:27, Daniel Kiper wrote:
> On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 06/04/17 07:23, Juergen Gross wrote:
>>> On 05/04/17 21:49, Boris Ostrovsky wrote:
>>>> On 04/05/2017 02:14 PM, Julien Grall wrote:
>>>>> The x86 code has theoritically a similar issue, altought EFI does not
>>>>> seem to be the preferred method. I have left it unimplemented on x86 and
>>>>> CCed Linux Xen x86 maintainers to know their view here.
>>>>
>>>> (+Daniel)
>>>>
>>>> This could be a problem for x86 as well, at least theoretically.
>>>> xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
>>>>
>>>> So I think we should have a similar routine there.
>>>
>>> +1
>>>
>>> I don't see any problem with such a routine added, in contrast to
>>> potential "reboots" instead of power off without it.
>>>
>>> So I think this dummy xen_efi_reset_system() should be added to
>>> drivers/xen/efi.c instead.
>>
>> I will resend the patch during day with xen_efi_reset_system moved
>> to common code and implement the x86 counterpart (thought, I will
>> not be able to test it).
> 
> I think that this is ARM specific issue. On x86 machine_restart() calls
> xen_restart(). Hence, everything works. So, I think that it should be
> fixed only for ARM. Anyway, please CC me when you send a patch.

What about xen_machine_power_off() (as stated in Boris' mail)?


Juergen

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-06 14:38         ` Juergen Gross
@ 2017-04-06 15:20           ` Daniel Kiper
  2017-04-06 15:39             ` Julien Grall
  0 siblings, 1 reply; 26+ messages in thread
From: Daniel Kiper @ 2017-04-06 15:20 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Julien Grall, Boris Ostrovsky, xen-devel, sstabellini,
	linux-arm-kernel, linux-kernel

On Thu, Apr 06, 2017 at 04:38:24PM +0200, Juergen Gross wrote:
> On 06/04/17 16:27, Daniel Kiper wrote:
> > On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
> >> Hi Juergen,
> >>
> >> On 06/04/17 07:23, Juergen Gross wrote:
> >>> On 05/04/17 21:49, Boris Ostrovsky wrote:
> >>>> On 04/05/2017 02:14 PM, Julien Grall wrote:
> >>>>> The x86 code has theoritically a similar issue, altought EFI does not
> >>>>> seem to be the preferred method. I have left it unimplemented on x86 and
> >>>>> CCed Linux Xen x86 maintainers to know their view here.
> >>>>
> >>>> (+Daniel)
> >>>>
> >>>> This could be a problem for x86 as well, at least theoretically.
> >>>> xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
> >>>>
> >>>> So I think we should have a similar routine there.
> >>>
> >>> +1
> >>>
> >>> I don't see any problem with such a routine added, in contrast to
> >>> potential "reboots" instead of power off without it.
> >>>
> >>> So I think this dummy xen_efi_reset_system() should be added to
> >>> drivers/xen/efi.c instead.
> >>
> >> I will resend the patch during day with xen_efi_reset_system moved
> >> to common code and implement the x86 counterpart (thought, I will
> >> not be able to test it).
> >
> > I think that this is ARM specific issue. On x86 machine_restart() calls
> > xen_restart(). Hence, everything works. So, I think that it should be
> > fixed only for ARM. Anyway, please CC me when you send a patch.
>
> What about xen_machine_power_off() (as stated in Boris' mail)?

Guys what do you think about that:

--- a/drivers/firmware/efi/reboot.c
+++ b/drivers/firmware/efi/reboot.c
@@ -55,7 +55,7 @@ static void efi_power_off(void)

 static int __init efi_shutdown_init(void)
 {
-       if (!efi_enabled(EFI_RUNTIME_SERVICES))
+       if (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_PARAVIRT))
                return -ENODEV;

        if (efi_poweroff_required())


Julien, for ARM64 please take a look at arch/arm64/kernel/efi.c:efi_poweroff_required(void).

I hope that tweaks for both files should solve our problem.

Daniel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-06 15:20           ` Daniel Kiper
@ 2017-04-06 15:39             ` Julien Grall
  2017-04-06 15:55               ` Mark Rutland
  2017-04-06 16:06               ` Daniel Kiper
  0 siblings, 2 replies; 26+ messages in thread
From: Julien Grall @ 2017-04-06 15:39 UTC (permalink / raw)
  To: Daniel Kiper, Juergen Gross
  Cc: Boris Ostrovsky, xen-devel, sstabellini, linux-arm-kernel, linux-kernel

Hi Daniel,

On 06/04/17 16:20, Daniel Kiper wrote:
> On Thu, Apr 06, 2017 at 04:38:24PM +0200, Juergen Gross wrote:
>> On 06/04/17 16:27, Daniel Kiper wrote:
>>> On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
>>>> Hi Juergen,
>>>>
>>>> On 06/04/17 07:23, Juergen Gross wrote:
>>>>> On 05/04/17 21:49, Boris Ostrovsky wrote:
>>>>>> On 04/05/2017 02:14 PM, Julien Grall wrote:
>>>>>>> The x86 code has theoritically a similar issue, altought EFI does not
>>>>>>> seem to be the preferred method. I have left it unimplemented on x86 and
>>>>>>> CCed Linux Xen x86 maintainers to know their view here.
>>>>>>
>>>>>> (+Daniel)
>>>>>>
>>>>>> This could be a problem for x86 as well, at least theoretically.
>>>>>> xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
>>>>>>
>>>>>> So I think we should have a similar routine there.
>>>>>
>>>>> +1
>>>>>
>>>>> I don't see any problem with such a routine added, in contrast to
>>>>> potential "reboots" instead of power off without it.
>>>>>
>>>>> So I think this dummy xen_efi_reset_system() should be added to
>>>>> drivers/xen/efi.c instead.
>>>>
>>>> I will resend the patch during day with xen_efi_reset_system moved
>>>> to common code and implement the x86 counterpart (thought, I will
>>>> not be able to test it).
>>>
>>> I think that this is ARM specific issue. On x86 machine_restart() calls
>>> xen_restart(). Hence, everything works. So, I think that it should be
>>> fixed only for ARM. Anyway, please CC me when you send a patch.
>>
>> What about xen_machine_power_off() (as stated in Boris' mail)?
>
> Guys what do you think about that:
>
> --- a/drivers/firmware/efi/reboot.c
> +++ b/drivers/firmware/efi/reboot.c
> @@ -55,7 +55,7 @@ static void efi_power_off(void)
>
>  static int __init efi_shutdown_init(void)
>  {
> -       if (!efi_enabled(EFI_RUNTIME_SERVICES))
> +       if (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_PARAVIRT))
>                 return -ENODEV;
>
>         if (efi_poweroff_required())
>
>
> Julien, for ARM64 please take a look at arch/arm64/kernel/efi.c:efi_poweroff_required(void).
>
> I hope that tweaks for both files should solve our problem.

This sounds good for power off (I haven't tried to power off DOM0 yet). 
But this will not solve the restart problem (see machine_restart in 
arch/arm64/kernel/process.c) which call directly efi_reboot.

Cheers,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-06 15:39             ` Julien Grall
@ 2017-04-06 15:55               ` Mark Rutland
  2017-04-18 13:46                 ` Matt Fleming
  2017-04-06 16:06               ` Daniel Kiper
  1 sibling, 1 reply; 26+ messages in thread
From: Mark Rutland @ 2017-04-06 15:55 UTC (permalink / raw)
  To: Julien Grall
  Cc: Daniel Kiper, Juergen Gross, Boris Ostrovsky, sstabellini,
	linux-kernel, linux-arm-kernel, xen-devel, Matt Fleming,
	Ard Biesheuvel, linux-efi

[Adding the EFI maintainers]

Tl;DR: Xen's EFI wrappery doesn't implement reset_system, so when
invoked on arm64 we get a NULL dereference.

On Thu, Apr 06, 2017 at 04:39:13PM +0100, Julien Grall wrote:
> On 06/04/17 16:20, Daniel Kiper wrote:
> >On Thu, Apr 06, 2017 at 04:38:24PM +0200, Juergen Gross wrote:
> >>On 06/04/17 16:27, Daniel Kiper wrote:
> >>>On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
> >>>>Hi Juergen,
> >>>>
> >>>>On 06/04/17 07:23, Juergen Gross wrote:
> >>>>>On 05/04/17 21:49, Boris Ostrovsky wrote:
> >>>>>>On 04/05/2017 02:14 PM, Julien Grall wrote:
> >>>>>>>The x86 code has theoritically a similar issue, altought EFI does not
> >>>>>>>seem to be the preferred method. I have left it unimplemented on x86 and
> >>>>>>>CCed Linux Xen x86 maintainers to know their view here.
> >>>>>>
> >>>>>>(+Daniel)
> >>>>>>
> >>>>>>This could be a problem for x86 as well, at least theoretically.
> >>>>>>xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
> >>>>>>
> >>>>>>So I think we should have a similar routine there.
> >>>>>
> >>>>>+1
> >>>>>
> >>>>>I don't see any problem with such a routine added, in contrast to
> >>>>>potential "reboots" instead of power off without it.
> >>>>>
> >>>>>So I think this dummy xen_efi_reset_system() should be added to
> >>>>>drivers/xen/efi.c instead.
> >>>>
> >>>>I will resend the patch during day with xen_efi_reset_system moved
> >>>>to common code and implement the x86 counterpart (thought, I will
> >>>>not be able to test it).
> >>>
> >>>I think that this is ARM specific issue. On x86 machine_restart() calls
> >>>xen_restart(). Hence, everything works. So, I think that it should be
> >>>fixed only for ARM. Anyway, please CC me when you send a patch.
> >>
> >>What about xen_machine_power_off() (as stated in Boris' mail)?
> >
> >Guys what do you think about that:
> >
> >--- a/drivers/firmware/efi/reboot.c
> >+++ b/drivers/firmware/efi/reboot.c
> >@@ -55,7 +55,7 @@ static void efi_power_off(void)
> >
> > static int __init efi_shutdown_init(void)
> > {
> >-       if (!efi_enabled(EFI_RUNTIME_SERVICES))
> >+       if (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_PARAVIRT))
> >                return -ENODEV;
> >
> >        if (efi_poweroff_required())
> >
> >
> >Julien, for ARM64 please take a look at arch/arm64/kernel/efi.c:efi_poweroff_required(void).
> >
> >I hope that tweaks for both files should solve our problem.
> 
> This sounds good for power off (I haven't tried to power off DOM0
> yet).

Please, let's keep the Xen knowledge constrained to the Xen EFI wrapper,
rather than spreading it further.

IMO, given reset_system is a *mandatory* function, the Xen wrapper
should provide an implementation.

I don't see why you can't implement a wrapper that calls the usual Xen
poweroff/reset functions.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-06 15:39             ` Julien Grall
  2017-04-06 15:55               ` Mark Rutland
@ 2017-04-06 16:06               ` Daniel Kiper
  2017-04-06 16:22                 ` Juergen Gross
  1 sibling, 1 reply; 26+ messages in thread
From: Daniel Kiper @ 2017-04-06 16:06 UTC (permalink / raw)
  To: Julien Grall
  Cc: Juergen Gross, Boris Ostrovsky, xen-devel, sstabellini,
	linux-arm-kernel, linux-kernel

Hi Julien,

On Thu, Apr 06, 2017 at 04:39:13PM +0100, Julien Grall wrote:
> Hi Daniel,
>
> On 06/04/17 16:20, Daniel Kiper wrote:
> >On Thu, Apr 06, 2017 at 04:38:24PM +0200, Juergen Gross wrote:
> >>On 06/04/17 16:27, Daniel Kiper wrote:
> >>>On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
> >>>>Hi Juergen,
> >>>>
> >>>>On 06/04/17 07:23, Juergen Gross wrote:
> >>>>>On 05/04/17 21:49, Boris Ostrovsky wrote:
> >>>>>>On 04/05/2017 02:14 PM, Julien Grall wrote:
> >>>>>>>The x86 code has theoritically a similar issue, altought EFI does not
> >>>>>>>seem to be the preferred method. I have left it unimplemented on x86 and
> >>>>>>>CCed Linux Xen x86 maintainers to know their view here.
> >>>>>>
> >>>>>>(+Daniel)
> >>>>>>
> >>>>>>This could be a problem for x86 as well, at least theoretically.
> >>>>>>xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
> >>>>>>
> >>>>>>So I think we should have a similar routine there.
> >>>>>
> >>>>>+1
> >>>>>
> >>>>>I don't see any problem with such a routine added, in contrast to
> >>>>>potential "reboots" instead of power off without it.
> >>>>>
> >>>>>So I think this dummy xen_efi_reset_system() should be added to
> >>>>>drivers/xen/efi.c instead.
> >>>>
> >>>>I will resend the patch during day with xen_efi_reset_system moved
> >>>>to common code and implement the x86 counterpart (thought, I will
> >>>>not be able to test it).
> >>>
> >>>I think that this is ARM specific issue. On x86 machine_restart() calls
> >>>xen_restart(). Hence, everything works. So, I think that it should be
> >>>fixed only for ARM. Anyway, please CC me when you send a patch.
> >>
> >>What about xen_machine_power_off() (as stated in Boris' mail)?
> >
> >Guys what do you think about that:
> >
> >--- a/drivers/firmware/efi/reboot.c
> >+++ b/drivers/firmware/efi/reboot.c
> >@@ -55,7 +55,7 @@ static void efi_power_off(void)
> >
> > static int __init efi_shutdown_init(void)
> > {
> >-       if (!efi_enabled(EFI_RUNTIME_SERVICES))
> >+       if (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_PARAVIRT))
> >                return -ENODEV;
> >
> >        if (efi_poweroff_required())
> >
> >
> >Julien, for ARM64 please take a look at arch/arm64/kernel/efi.c:efi_poweroff_required(void).
> >
> >I hope that tweaks for both files should solve our problem.
>
> This sounds good for power off (I haven't tried to power off DOM0
> yet). But this will not solve the restart problem (see
> machine_restart in arch/arm64/kernel/process.c) which call directly
> efi_reboot.

Hmmm... It seems to me that efi.reset_system override with empty function
in arch/arm/xen/efi.c is the best solution. So, I see three patches here.
One for drivers/firmware/efi/reboot.c, one for arch/arm/xen/efi.c and one
for arch/arm64/kernel/efi.c. Does it make sense?

Daniel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-06 16:06               ` Daniel Kiper
@ 2017-04-06 16:22                 ` Juergen Gross
  2017-04-06 16:43                   ` Daniel Kiper
  0 siblings, 1 reply; 26+ messages in thread
From: Juergen Gross @ 2017-04-06 16:22 UTC (permalink / raw)
  To: Daniel Kiper, Julien Grall
  Cc: Boris Ostrovsky, xen-devel, sstabellini, linux-arm-kernel, linux-kernel

On 06/04/17 18:06, Daniel Kiper wrote:
> Hi Julien,
> 
> On Thu, Apr 06, 2017 at 04:39:13PM +0100, Julien Grall wrote:
>> Hi Daniel,
>>
>> On 06/04/17 16:20, Daniel Kiper wrote:
>>> On Thu, Apr 06, 2017 at 04:38:24PM +0200, Juergen Gross wrote:
>>>> On 06/04/17 16:27, Daniel Kiper wrote:
>>>>> On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
>>>>>> Hi Juergen,
>>>>>>
>>>>>> On 06/04/17 07:23, Juergen Gross wrote:
>>>>>>> On 05/04/17 21:49, Boris Ostrovsky wrote:
>>>>>>>> On 04/05/2017 02:14 PM, Julien Grall wrote:
>>>>>>>>> The x86 code has theoritically a similar issue, altought EFI does not
>>>>>>>>> seem to be the preferred method. I have left it unimplemented on x86 and
>>>>>>>>> CCed Linux Xen x86 maintainers to know their view here.
>>>>>>>>
>>>>>>>> (+Daniel)
>>>>>>>>
>>>>>>>> This could be a problem for x86 as well, at least theoretically.
>>>>>>>> xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
>>>>>>>>
>>>>>>>> So I think we should have a similar routine there.
>>>>>>>
>>>>>>> +1
>>>>>>>
>>>>>>> I don't see any problem with such a routine added, in contrast to
>>>>>>> potential "reboots" instead of power off without it.
>>>>>>>
>>>>>>> So I think this dummy xen_efi_reset_system() should be added to
>>>>>>> drivers/xen/efi.c instead.
>>>>>>
>>>>>> I will resend the patch during day with xen_efi_reset_system moved
>>>>>> to common code and implement the x86 counterpart (thought, I will
>>>>>> not be able to test it).
>>>>>
>>>>> I think that this is ARM specific issue. On x86 machine_restart() calls
>>>>> xen_restart(). Hence, everything works. So, I think that it should be
>>>>> fixed only for ARM. Anyway, please CC me when you send a patch.
>>>>
>>>> What about xen_machine_power_off() (as stated in Boris' mail)?
>>>
>>> Guys what do you think about that:
>>>
>>> --- a/drivers/firmware/efi/reboot.c
>>> +++ b/drivers/firmware/efi/reboot.c
>>> @@ -55,7 +55,7 @@ static void efi_power_off(void)
>>>
>>> static int __init efi_shutdown_init(void)
>>> {
>>> -       if (!efi_enabled(EFI_RUNTIME_SERVICES))
>>> +       if (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_PARAVIRT))
>>>                return -ENODEV;
>>>
>>>        if (efi_poweroff_required())
>>>
>>>
>>> Julien, for ARM64 please take a look at arch/arm64/kernel/efi.c:efi_poweroff_required(void).
>>>
>>> I hope that tweaks for both files should solve our problem.
>>
>> This sounds good for power off (I haven't tried to power off DOM0
>> yet). But this will not solve the restart problem (see
>> machine_restart in arch/arm64/kernel/process.c) which call directly
>> efi_reboot.
> 
> Hmmm... It seems to me that efi.reset_system override with empty function
> in arch/arm/xen/efi.c is the best solution. So, I see three patches here.
> One for drivers/firmware/efi/reboot.c, one for arch/arm/xen/efi.c and one
> for arch/arm64/kernel/efi.c. Does it make sense?

I still think the empty function should be in drivers/xen/efi.c and we
should use it in arch/x86/xen/efi.c, too.


Juergen

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-06 16:22                 ` Juergen Gross
@ 2017-04-06 16:43                   ` Daniel Kiper
  2017-04-06 17:39                     ` Juergen Gross
  0 siblings, 1 reply; 26+ messages in thread
From: Daniel Kiper @ 2017-04-06 16:43 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Julien Grall, Boris Ostrovsky, xen-devel, sstabellini,
	linux-arm-kernel, linux-kernel

On Thu, Apr 06, 2017 at 06:22:44PM +0200, Juergen Gross wrote:
> On 06/04/17 18:06, Daniel Kiper wrote:
> > Hi Julien,
> >
> > On Thu, Apr 06, 2017 at 04:39:13PM +0100, Julien Grall wrote:
> >> Hi Daniel,
> >>
> >> On 06/04/17 16:20, Daniel Kiper wrote:
> >>> On Thu, Apr 06, 2017 at 04:38:24PM +0200, Juergen Gross wrote:
> >>>> On 06/04/17 16:27, Daniel Kiper wrote:
> >>>>> On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
> >>>>>> Hi Juergen,
> >>>>>>
> >>>>>> On 06/04/17 07:23, Juergen Gross wrote:
> >>>>>>> On 05/04/17 21:49, Boris Ostrovsky wrote:
> >>>>>>>> On 04/05/2017 02:14 PM, Julien Grall wrote:
> >>>>>>>>> The x86 code has theoritically a similar issue, altought EFI does not
> >>>>>>>>> seem to be the preferred method. I have left it unimplemented on x86 and
> >>>>>>>>> CCed Linux Xen x86 maintainers to know their view here.
> >>>>>>>>
> >>>>>>>> (+Daniel)
> >>>>>>>>
> >>>>>>>> This could be a problem for x86 as well, at least theoretically.
> >>>>>>>> xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
> >>>>>>>>
> >>>>>>>> So I think we should have a similar routine there.
> >>>>>>>
> >>>>>>> +1
> >>>>>>>
> >>>>>>> I don't see any problem with such a routine added, in contrast to
> >>>>>>> potential "reboots" instead of power off without it.
> >>>>>>>
> >>>>>>> So I think this dummy xen_efi_reset_system() should be added to
> >>>>>>> drivers/xen/efi.c instead.
> >>>>>>
> >>>>>> I will resend the patch during day with xen_efi_reset_system moved
> >>>>>> to common code and implement the x86 counterpart (thought, I will
> >>>>>> not be able to test it).
> >>>>>
> >>>>> I think that this is ARM specific issue. On x86 machine_restart() calls
> >>>>> xen_restart(). Hence, everything works. So, I think that it should be
> >>>>> fixed only for ARM. Anyway, please CC me when you send a patch.
> >>>>
> >>>> What about xen_machine_power_off() (as stated in Boris' mail)?
> >>>
> >>> Guys what do you think about that:
> >>>
> >>> --- a/drivers/firmware/efi/reboot.c
> >>> +++ b/drivers/firmware/efi/reboot.c
> >>> @@ -55,7 +55,7 @@ static void efi_power_off(void)
> >>>
> >>> static int __init efi_shutdown_init(void)
> >>> {
> >>> -       if (!efi_enabled(EFI_RUNTIME_SERVICES))
> >>> +       if (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_PARAVIRT))
> >>>                return -ENODEV;
> >>>
> >>>        if (efi_poweroff_required())
> >>>
> >>>
> >>> Julien, for ARM64 please take a look at arch/arm64/kernel/efi.c:efi_poweroff_required(void).
> >>>
> >>> I hope that tweaks for both files should solve our problem.
> >>
> >> This sounds good for power off (I haven't tried to power off DOM0
> >> yet). But this will not solve the restart problem (see
> >> machine_restart in arch/arm64/kernel/process.c) which call directly
> >> efi_reboot.
> >
> > Hmmm... It seems to me that efi.reset_system override with empty function
> > in arch/arm/xen/efi.c is the best solution. So, I see three patches here.
> > One for drivers/firmware/efi/reboot.c, one for arch/arm/xen/efi.c and one
> > for arch/arm64/kernel/efi.c. Does it make sense?
>
> I still think the empty function should be in drivers/xen/efi.c and we
> should use it in arch/x86/xen/efi.c, too.

If you wish we can go that way too. Though I thing that we should fix
drivers/firmware/efi/reboot.c:efi_shutdown_init() too. Just in case.

Daniel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-06 16:43                   ` Daniel Kiper
@ 2017-04-06 17:39                     ` Juergen Gross
  2017-04-18 18:37                       ` Stefano Stabellini
  0 siblings, 1 reply; 26+ messages in thread
From: Juergen Gross @ 2017-04-06 17:39 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: Julien Grall, Boris Ostrovsky, xen-devel, sstabellini,
	linux-arm-kernel, linux-kernel

On 06/04/17 18:43, Daniel Kiper wrote:
> On Thu, Apr 06, 2017 at 06:22:44PM +0200, Juergen Gross wrote:
>> On 06/04/17 18:06, Daniel Kiper wrote:
>>> Hi Julien,
>>>
>>> On Thu, Apr 06, 2017 at 04:39:13PM +0100, Julien Grall wrote:
>>>> Hi Daniel,
>>>>
>>>> On 06/04/17 16:20, Daniel Kiper wrote:
>>>>> On Thu, Apr 06, 2017 at 04:38:24PM +0200, Juergen Gross wrote:
>>>>>> On 06/04/17 16:27, Daniel Kiper wrote:
>>>>>>> On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
>>>>>>>> Hi Juergen,
>>>>>>>>
>>>>>>>> On 06/04/17 07:23, Juergen Gross wrote:
>>>>>>>>> On 05/04/17 21:49, Boris Ostrovsky wrote:
>>>>>>>>>> On 04/05/2017 02:14 PM, Julien Grall wrote:
>>>>>>>>>>> The x86 code has theoritically a similar issue, altought EFI does not
>>>>>>>>>>> seem to be the preferred method. I have left it unimplemented on x86 and
>>>>>>>>>>> CCed Linux Xen x86 maintainers to know their view here.
>>>>>>>>>>
>>>>>>>>>> (+Daniel)
>>>>>>>>>>
>>>>>>>>>> This could be a problem for x86 as well, at least theoretically.
>>>>>>>>>> xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
>>>>>>>>>>
>>>>>>>>>> So I think we should have a similar routine there.
>>>>>>>>>
>>>>>>>>> +1
>>>>>>>>>
>>>>>>>>> I don't see any problem with such a routine added, in contrast to
>>>>>>>>> potential "reboots" instead of power off without it.
>>>>>>>>>
>>>>>>>>> So I think this dummy xen_efi_reset_system() should be added to
>>>>>>>>> drivers/xen/efi.c instead.
>>>>>>>>
>>>>>>>> I will resend the patch during day with xen_efi_reset_system moved
>>>>>>>> to common code and implement the x86 counterpart (thought, I will
>>>>>>>> not be able to test it).
>>>>>>>
>>>>>>> I think that this is ARM specific issue. On x86 machine_restart() calls
>>>>>>> xen_restart(). Hence, everything works. So, I think that it should be
>>>>>>> fixed only for ARM. Anyway, please CC me when you send a patch.
>>>>>>
>>>>>> What about xen_machine_power_off() (as stated in Boris' mail)?
>>>>>
>>>>> Guys what do you think about that:
>>>>>
>>>>> --- a/drivers/firmware/efi/reboot.c
>>>>> +++ b/drivers/firmware/efi/reboot.c
>>>>> @@ -55,7 +55,7 @@ static void efi_power_off(void)
>>>>>
>>>>> static int __init efi_shutdown_init(void)
>>>>> {
>>>>> -       if (!efi_enabled(EFI_RUNTIME_SERVICES))
>>>>> +       if (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_PARAVIRT))
>>>>>                return -ENODEV;
>>>>>
>>>>>        if (efi_poweroff_required())
>>>>>
>>>>>
>>>>> Julien, for ARM64 please take a look at arch/arm64/kernel/efi.c:efi_poweroff_required(void).
>>>>>
>>>>> I hope that tweaks for both files should solve our problem.
>>>>
>>>> This sounds good for power off (I haven't tried to power off DOM0
>>>> yet). But this will not solve the restart problem (see
>>>> machine_restart in arch/arm64/kernel/process.c) which call directly
>>>> efi_reboot.
>>>
>>> Hmmm... It seems to me that efi.reset_system override with empty function
>>> in arch/arm/xen/efi.c is the best solution. So, I see three patches here.
>>> One for drivers/firmware/efi/reboot.c, one for arch/arm/xen/efi.c and one
>>> for arch/arm64/kernel/efi.c. Does it make sense?
>>
>> I still think the empty function should be in drivers/xen/efi.c and we
>> should use it in arch/x86/xen/efi.c, too.
> 
> If you wish we can go that way too. Though I thing that we should fix
> drivers/firmware/efi/reboot.c:efi_shutdown_init() too. Just in case.

Sure, go ahead. I won't object.


Juergen

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-06 15:55               ` Mark Rutland
@ 2017-04-18 13:46                 ` Matt Fleming
  2017-04-19 19:29                   ` Daniel Kiper
  0 siblings, 1 reply; 26+ messages in thread
From: Matt Fleming @ 2017-04-18 13:46 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Julien Grall, Daniel Kiper, Juergen Gross, Boris Ostrovsky,
	sstabellini, linux-kernel, linux-arm-kernel, xen-devel,
	Ard Biesheuvel, linux-efi

On Thu, 06 Apr, at 04:55:11PM, Mark Rutland wrote:
> 
> Please, let's keep the Xen knowledge constrained to the Xen EFI wrapper,
> rather than spreading it further.
> 
> IMO, given reset_system is a *mandatory* function, the Xen wrapper
> should provide an implementation.
> 
> I don't see why you can't implement a wrapper that calls the usual Xen
> poweroff/reset functions.

I realise I'm making a sweeping generalisation, but adding
EFI_PARAVIRT is almost always the wrong thing to do.

I'd much prefer to see Mark's idea implemented.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-06 17:39                     ` Juergen Gross
@ 2017-04-18 18:37                       ` Stefano Stabellini
  2017-04-18 18:43                         ` Juergen Gross
  0 siblings, 1 reply; 26+ messages in thread
From: Stefano Stabellini @ 2017-04-18 18:37 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Daniel Kiper, Julien Grall, Boris Ostrovsky, xen-devel,
	sstabellini, linux-arm-kernel, linux-kernel

On Thu, 6 Apr 2017, Juergen Gross wrote:
> On 06/04/17 18:43, Daniel Kiper wrote:
> > On Thu, Apr 06, 2017 at 06:22:44PM +0200, Juergen Gross wrote:
> >> On 06/04/17 18:06, Daniel Kiper wrote:
> >>> Hi Julien,
> >>>
> >>> On Thu, Apr 06, 2017 at 04:39:13PM +0100, Julien Grall wrote:
> >>>> Hi Daniel,
> >>>>
> >>>> On 06/04/17 16:20, Daniel Kiper wrote:
> >>>>> On Thu, Apr 06, 2017 at 04:38:24PM +0200, Juergen Gross wrote:
> >>>>>> On 06/04/17 16:27, Daniel Kiper wrote:
> >>>>>>> On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
> >>>>>>>> Hi Juergen,
> >>>>>>>>
> >>>>>>>> On 06/04/17 07:23, Juergen Gross wrote:
> >>>>>>>>> On 05/04/17 21:49, Boris Ostrovsky wrote:
> >>>>>>>>>> On 04/05/2017 02:14 PM, Julien Grall wrote:
> >>>>>>>>>>> The x86 code has theoritically a similar issue, altought EFI does not
> >>>>>>>>>>> seem to be the preferred method. I have left it unimplemented on x86 and
> >>>>>>>>>>> CCed Linux Xen x86 maintainers to know their view here.
> >>>>>>>>>>
> >>>>>>>>>> (+Daniel)
> >>>>>>>>>>
> >>>>>>>>>> This could be a problem for x86 as well, at least theoretically.
> >>>>>>>>>> xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
> >>>>>>>>>>
> >>>>>>>>>> So I think we should have a similar routine there.
> >>>>>>>>>
> >>>>>>>>> +1
> >>>>>>>>>
> >>>>>>>>> I don't see any problem with such a routine added, in contrast to
> >>>>>>>>> potential "reboots" instead of power off without it.
> >>>>>>>>>
> >>>>>>>>> So I think this dummy xen_efi_reset_system() should be added to
> >>>>>>>>> drivers/xen/efi.c instead.
> >>>>>>>>
> >>>>>>>> I will resend the patch during day with xen_efi_reset_system moved
> >>>>>>>> to common code and implement the x86 counterpart (thought, I will
> >>>>>>>> not be able to test it).
> >>>>>>>
> >>>>>>> I think that this is ARM specific issue. On x86 machine_restart() calls
> >>>>>>> xen_restart(). Hence, everything works. So, I think that it should be
> >>>>>>> fixed only for ARM. Anyway, please CC me when you send a patch.
> >>>>>>
> >>>>>> What about xen_machine_power_off() (as stated in Boris' mail)?
> >>>>>
> >>>>> Guys what do you think about that:
> >>>>>
> >>>>> --- a/drivers/firmware/efi/reboot.c
> >>>>> +++ b/drivers/firmware/efi/reboot.c
> >>>>> @@ -55,7 +55,7 @@ static void efi_power_off(void)
> >>>>>
> >>>>> static int __init efi_shutdown_init(void)
> >>>>> {
> >>>>> -       if (!efi_enabled(EFI_RUNTIME_SERVICES))
> >>>>> +       if (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_PARAVIRT))
> >>>>>                return -ENODEV;
> >>>>>
> >>>>>        if (efi_poweroff_required())
> >>>>>
> >>>>>
> >>>>> Julien, for ARM64 please take a look at arch/arm64/kernel/efi.c:efi_poweroff_required(void).
> >>>>>
> >>>>> I hope that tweaks for both files should solve our problem.
> >>>>
> >>>> This sounds good for power off (I haven't tried to power off DOM0
> >>>> yet). But this will not solve the restart problem (see
> >>>> machine_restart in arch/arm64/kernel/process.c) which call directly
> >>>> efi_reboot.
> >>>
> >>> Hmmm... It seems to me that efi.reset_system override with empty function
> >>> in arch/arm/xen/efi.c is the best solution. So, I see three patches here.
> >>> One for drivers/firmware/efi/reboot.c, one for arch/arm/xen/efi.c and one
> >>> for arch/arm64/kernel/efi.c. Does it make sense?
> >>
> >> I still think the empty function should be in drivers/xen/efi.c and we
> >> should use it in arch/x86/xen/efi.c, too.
> > 
> > If you wish we can go that way too. Though I thing that we should fix
> > drivers/firmware/efi/reboot.c:efi_shutdown_init() too. Just in case.
> 
> Sure, go ahead. I won't object.

For the Xen on ARM side, the original patch that started this thread
(20170405181417.15985-1-julien.grall@arm.com) is good to go, right?

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-18 18:37                       ` Stefano Stabellini
@ 2017-04-18 18:43                         ` Juergen Gross
  2017-04-18 18:46                           ` Stefano Stabellini
  0 siblings, 1 reply; 26+ messages in thread
From: Juergen Gross @ 2017-04-18 18:43 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Daniel Kiper, Julien Grall, Boris Ostrovsky, xen-devel,
	linux-arm-kernel, linux-kernel

On 18/04/17 20:37, Stefano Stabellini wrote:
> On Thu, 6 Apr 2017, Juergen Gross wrote:
>> On 06/04/17 18:43, Daniel Kiper wrote:
>>> On Thu, Apr 06, 2017 at 06:22:44PM +0200, Juergen Gross wrote:
>>>> On 06/04/17 18:06, Daniel Kiper wrote:
>>>>> Hi Julien,
>>>>>
>>>>> On Thu, Apr 06, 2017 at 04:39:13PM +0100, Julien Grall wrote:
>>>>>> Hi Daniel,
>>>>>>
>>>>>> On 06/04/17 16:20, Daniel Kiper wrote:
>>>>>>> On Thu, Apr 06, 2017 at 04:38:24PM +0200, Juergen Gross wrote:
>>>>>>>> On 06/04/17 16:27, Daniel Kiper wrote:
>>>>>>>>> On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
>>>>>>>>>> Hi Juergen,
>>>>>>>>>>
>>>>>>>>>> On 06/04/17 07:23, Juergen Gross wrote:
>>>>>>>>>>> On 05/04/17 21:49, Boris Ostrovsky wrote:
>>>>>>>>>>>> On 04/05/2017 02:14 PM, Julien Grall wrote:
>>>>>>>>>>>>> The x86 code has theoritically a similar issue, altought EFI does not
>>>>>>>>>>>>> seem to be the preferred method. I have left it unimplemented on x86 and
>>>>>>>>>>>>> CCed Linux Xen x86 maintainers to know their view here.
>>>>>>>>>>>>
>>>>>>>>>>>> (+Daniel)
>>>>>>>>>>>>
>>>>>>>>>>>> This could be a problem for x86 as well, at least theoretically.
>>>>>>>>>>>> xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
>>>>>>>>>>>>
>>>>>>>>>>>> So I think we should have a similar routine there.
>>>>>>>>>>>
>>>>>>>>>>> +1
>>>>>>>>>>>
>>>>>>>>>>> I don't see any problem with such a routine added, in contrast to
>>>>>>>>>>> potential "reboots" instead of power off without it.
>>>>>>>>>>>
>>>>>>>>>>> So I think this dummy xen_efi_reset_system() should be added to
>>>>>>>>>>> drivers/xen/efi.c instead.
>>>>>>>>>>
>>>>>>>>>> I will resend the patch during day with xen_efi_reset_system moved
>>>>>>>>>> to common code and implement the x86 counterpart (thought, I will
>>>>>>>>>> not be able to test it).
>>>>>>>>>
>>>>>>>>> I think that this is ARM specific issue. On x86 machine_restart() calls
>>>>>>>>> xen_restart(). Hence, everything works. So, I think that it should be
>>>>>>>>> fixed only for ARM. Anyway, please CC me when you send a patch.
>>>>>>>>
>>>>>>>> What about xen_machine_power_off() (as stated in Boris' mail)?
>>>>>>>
>>>>>>> Guys what do you think about that:
>>>>>>>
>>>>>>> --- a/drivers/firmware/efi/reboot.c
>>>>>>> +++ b/drivers/firmware/efi/reboot.c
>>>>>>> @@ -55,7 +55,7 @@ static void efi_power_off(void)
>>>>>>>
>>>>>>> static int __init efi_shutdown_init(void)
>>>>>>> {
>>>>>>> -       if (!efi_enabled(EFI_RUNTIME_SERVICES))
>>>>>>> +       if (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_PARAVIRT))
>>>>>>>                return -ENODEV;
>>>>>>>
>>>>>>>        if (efi_poweroff_required())
>>>>>>>
>>>>>>>
>>>>>>> Julien, for ARM64 please take a look at arch/arm64/kernel/efi.c:efi_poweroff_required(void).
>>>>>>>
>>>>>>> I hope that tweaks for both files should solve our problem.
>>>>>>
>>>>>> This sounds good for power off (I haven't tried to power off DOM0
>>>>>> yet). But this will not solve the restart problem (see
>>>>>> machine_restart in arch/arm64/kernel/process.c) which call directly
>>>>>> efi_reboot.
>>>>>
>>>>> Hmmm... It seems to me that efi.reset_system override with empty function
>>>>> in arch/arm/xen/efi.c is the best solution. So, I see three patches here.
>>>>> One for drivers/firmware/efi/reboot.c, one for arch/arm/xen/efi.c and one
>>>>> for arch/arm64/kernel/efi.c. Does it make sense?
>>>>
>>>> I still think the empty function should be in drivers/xen/efi.c and we
>>>> should use it in arch/x86/xen/efi.c, too.
>>>
>>> If you wish we can go that way too. Though I thing that we should fix
>>> drivers/firmware/efi/reboot.c:efi_shutdown_init() too. Just in case.
>>
>> Sure, go ahead. I won't object.
> 
> For the Xen on ARM side, the original patch that started this thread
> (20170405181417.15985-1-julien.grall@arm.com) is good to go, right?
> 

As I said: the dummy xen_efi_reset_system() should be in
drivers/xen/efi.c


Juergen

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-18 18:43                         ` Juergen Gross
@ 2017-04-18 18:46                           ` Stefano Stabellini
  2017-04-18 18:51                             ` Juergen Gross
  0 siblings, 1 reply; 26+ messages in thread
From: Stefano Stabellini @ 2017-04-18 18:46 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Stefano Stabellini, Daniel Kiper, Julien Grall, Boris Ostrovsky,
	xen-devel, linux-arm-kernel, linux-kernel

On Tue, 18 Apr 2017, Juergen Gross wrote:
> On 18/04/17 20:37, Stefano Stabellini wrote:
> > On Thu, 6 Apr 2017, Juergen Gross wrote:
> >> On 06/04/17 18:43, Daniel Kiper wrote:
> >>> On Thu, Apr 06, 2017 at 06:22:44PM +0200, Juergen Gross wrote:
> >>>> On 06/04/17 18:06, Daniel Kiper wrote:
> >>>>> Hi Julien,
> >>>>>
> >>>>> On Thu, Apr 06, 2017 at 04:39:13PM +0100, Julien Grall wrote:
> >>>>>> Hi Daniel,
> >>>>>>
> >>>>>> On 06/04/17 16:20, Daniel Kiper wrote:
> >>>>>>> On Thu, Apr 06, 2017 at 04:38:24PM +0200, Juergen Gross wrote:
> >>>>>>>> On 06/04/17 16:27, Daniel Kiper wrote:
> >>>>>>>>> On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
> >>>>>>>>>> Hi Juergen,
> >>>>>>>>>>
> >>>>>>>>>> On 06/04/17 07:23, Juergen Gross wrote:
> >>>>>>>>>>> On 05/04/17 21:49, Boris Ostrovsky wrote:
> >>>>>>>>>>>> On 04/05/2017 02:14 PM, Julien Grall wrote:
> >>>>>>>>>>>>> The x86 code has theoritically a similar issue, altought EFI does not
> >>>>>>>>>>>>> seem to be the preferred method. I have left it unimplemented on x86 and
> >>>>>>>>>>>>> CCed Linux Xen x86 maintainers to know their view here.
> >>>>>>>>>>>>
> >>>>>>>>>>>> (+Daniel)
> >>>>>>>>>>>>
> >>>>>>>>>>>> This could be a problem for x86 as well, at least theoretically.
> >>>>>>>>>>>> xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
> >>>>>>>>>>>>
> >>>>>>>>>>>> So I think we should have a similar routine there.
> >>>>>>>>>>>
> >>>>>>>>>>> +1
> >>>>>>>>>>>
> >>>>>>>>>>> I don't see any problem with such a routine added, in contrast to
> >>>>>>>>>>> potential "reboots" instead of power off without it.
> >>>>>>>>>>>
> >>>>>>>>>>> So I think this dummy xen_efi_reset_system() should be added to
> >>>>>>>>>>> drivers/xen/efi.c instead.
> >>>>>>>>>>
> >>>>>>>>>> I will resend the patch during day with xen_efi_reset_system moved
> >>>>>>>>>> to common code and implement the x86 counterpart (thought, I will
> >>>>>>>>>> not be able to test it).
> >>>>>>>>>
> >>>>>>>>> I think that this is ARM specific issue. On x86 machine_restart() calls
> >>>>>>>>> xen_restart(). Hence, everything works. So, I think that it should be
> >>>>>>>>> fixed only for ARM. Anyway, please CC me when you send a patch.
> >>>>>>>>
> >>>>>>>> What about xen_machine_power_off() (as stated in Boris' mail)?
> >>>>>>>
> >>>>>>> Guys what do you think about that:
> >>>>>>>
> >>>>>>> --- a/drivers/firmware/efi/reboot.c
> >>>>>>> +++ b/drivers/firmware/efi/reboot.c
> >>>>>>> @@ -55,7 +55,7 @@ static void efi_power_off(void)
> >>>>>>>
> >>>>>>> static int __init efi_shutdown_init(void)
> >>>>>>> {
> >>>>>>> -       if (!efi_enabled(EFI_RUNTIME_SERVICES))
> >>>>>>> +       if (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_PARAVIRT))
> >>>>>>>                return -ENODEV;
> >>>>>>>
> >>>>>>>        if (efi_poweroff_required())
> >>>>>>>
> >>>>>>>
> >>>>>>> Julien, for ARM64 please take a look at arch/arm64/kernel/efi.c:efi_poweroff_required(void).
> >>>>>>>
> >>>>>>> I hope that tweaks for both files should solve our problem.
> >>>>>>
> >>>>>> This sounds good for power off (I haven't tried to power off DOM0
> >>>>>> yet). But this will not solve the restart problem (see
> >>>>>> machine_restart in arch/arm64/kernel/process.c) which call directly
> >>>>>> efi_reboot.
> >>>>>
> >>>>> Hmmm... It seems to me that efi.reset_system override with empty function
> >>>>> in arch/arm/xen/efi.c is the best solution. So, I see three patches here.
> >>>>> One for drivers/firmware/efi/reboot.c, one for arch/arm/xen/efi.c and one
> >>>>> for arch/arm64/kernel/efi.c. Does it make sense?
> >>>>
> >>>> I still think the empty function should be in drivers/xen/efi.c and we
> >>>> should use it in arch/x86/xen/efi.c, too.
> >>>
> >>> If you wish we can go that way too. Though I thing that we should fix
> >>> drivers/firmware/efi/reboot.c:efi_shutdown_init() too. Just in case.
> >>
> >> Sure, go ahead. I won't object.
> > 
> > For the Xen on ARM side, the original patch that started this thread
> > (20170405181417.15985-1-julien.grall@arm.com) is good to go, right?
> > 
> 
> As I said: the dummy xen_efi_reset_system() should be in
> drivers/xen/efi.c

OK. Who is working on it?

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-18 18:46                           ` Stefano Stabellini
@ 2017-04-18 18:51                             ` Juergen Gross
  2017-04-20 18:09                               ` Julien Grall
  0 siblings, 1 reply; 26+ messages in thread
From: Juergen Gross @ 2017-04-18 18:51 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Daniel Kiper, Julien Grall, Boris Ostrovsky, xen-devel,
	linux-arm-kernel, linux-kernel

On 18/04/17 20:46, Stefano Stabellini wrote:
> On Tue, 18 Apr 2017, Juergen Gross wrote:
>> On 18/04/17 20:37, Stefano Stabellini wrote:
>>> On Thu, 6 Apr 2017, Juergen Gross wrote:
>>>> On 06/04/17 18:43, Daniel Kiper wrote:
>>>>> On Thu, Apr 06, 2017 at 06:22:44PM +0200, Juergen Gross wrote:
>>>>>> On 06/04/17 18:06, Daniel Kiper wrote:
>>>>>>> Hi Julien,
>>>>>>>
>>>>>>> On Thu, Apr 06, 2017 at 04:39:13PM +0100, Julien Grall wrote:
>>>>>>>> Hi Daniel,
>>>>>>>>
>>>>>>>> On 06/04/17 16:20, Daniel Kiper wrote:
>>>>>>>>> On Thu, Apr 06, 2017 at 04:38:24PM +0200, Juergen Gross wrote:
>>>>>>>>>> On 06/04/17 16:27, Daniel Kiper wrote:
>>>>>>>>>>> On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
>>>>>>>>>>>> Hi Juergen,
>>>>>>>>>>>>
>>>>>>>>>>>> On 06/04/17 07:23, Juergen Gross wrote:
>>>>>>>>>>>>> On 05/04/17 21:49, Boris Ostrovsky wrote:
>>>>>>>>>>>>>> On 04/05/2017 02:14 PM, Julien Grall wrote:
>>>>>>>>>>>>>>> The x86 code has theoritically a similar issue, altought EFI does not
>>>>>>>>>>>>>>> seem to be the preferred method. I have left it unimplemented on x86 and
>>>>>>>>>>>>>>> CCed Linux Xen x86 maintainers to know their view here.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> (+Daniel)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This could be a problem for x86 as well, at least theoretically.
>>>>>>>>>>>>>> xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So I think we should have a similar routine there.
>>>>>>>>>>>>>
>>>>>>>>>>>>> +1
>>>>>>>>>>>>>
>>>>>>>>>>>>> I don't see any problem with such a routine added, in contrast to
>>>>>>>>>>>>> potential "reboots" instead of power off without it.
>>>>>>>>>>>>>
>>>>>>>>>>>>> So I think this dummy xen_efi_reset_system() should be added to
>>>>>>>>>>>>> drivers/xen/efi.c instead.
>>>>>>>>>>>>
>>>>>>>>>>>> I will resend the patch during day with xen_efi_reset_system moved
>>>>>>>>>>>> to common code and implement the x86 counterpart (thought, I will
>>>>>>>>>>>> not be able to test it).
>>>>>>>>>>>
>>>>>>>>>>> I think that this is ARM specific issue. On x86 machine_restart() calls
>>>>>>>>>>> xen_restart(). Hence, everything works. So, I think that it should be
>>>>>>>>>>> fixed only for ARM. Anyway, please CC me when you send a patch.
>>>>>>>>>>
>>>>>>>>>> What about xen_machine_power_off() (as stated in Boris' mail)?
>>>>>>>>>
>>>>>>>>> Guys what do you think about that:
>>>>>>>>>
>>>>>>>>> --- a/drivers/firmware/efi/reboot.c
>>>>>>>>> +++ b/drivers/firmware/efi/reboot.c
>>>>>>>>> @@ -55,7 +55,7 @@ static void efi_power_off(void)
>>>>>>>>>
>>>>>>>>> static int __init efi_shutdown_init(void)
>>>>>>>>> {
>>>>>>>>> -       if (!efi_enabled(EFI_RUNTIME_SERVICES))
>>>>>>>>> +       if (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_PARAVIRT))
>>>>>>>>>                return -ENODEV;
>>>>>>>>>
>>>>>>>>>        if (efi_poweroff_required())
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Julien, for ARM64 please take a look at arch/arm64/kernel/efi.c:efi_poweroff_required(void).
>>>>>>>>>
>>>>>>>>> I hope that tweaks for both files should solve our problem.
>>>>>>>>
>>>>>>>> This sounds good for power off (I haven't tried to power off DOM0
>>>>>>>> yet). But this will not solve the restart problem (see
>>>>>>>> machine_restart in arch/arm64/kernel/process.c) which call directly
>>>>>>>> efi_reboot.
>>>>>>>
>>>>>>> Hmmm... It seems to me that efi.reset_system override with empty function
>>>>>>> in arch/arm/xen/efi.c is the best solution. So, I see three patches here.
>>>>>>> One for drivers/firmware/efi/reboot.c, one for arch/arm/xen/efi.c and one
>>>>>>> for arch/arm64/kernel/efi.c. Does it make sense?
>>>>>>
>>>>>> I still think the empty function should be in drivers/xen/efi.c and we
>>>>>> should use it in arch/x86/xen/efi.c, too.
>>>>>
>>>>> If you wish we can go that way too. Though I thing that we should fix
>>>>> drivers/firmware/efi/reboot.c:efi_shutdown_init() too. Just in case.
>>>>
>>>> Sure, go ahead. I won't object.
>>>
>>> For the Xen on ARM side, the original patch that started this thread
>>> (20170405181417.15985-1-julien.grall@arm.com) is good to go, right?
>>>
>>
>> As I said: the dummy xen_efi_reset_system() should be in
>> drivers/xen/efi.c
> 
> OK. Who is working on it?

Didn't Julien say he would do it?


Juergen

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-18 13:46                 ` Matt Fleming
@ 2017-04-19 19:29                   ` Daniel Kiper
  2017-04-19 19:37                     ` Matt Fleming
  0 siblings, 1 reply; 26+ messages in thread
From: Daniel Kiper @ 2017-04-19 19:29 UTC (permalink / raw)
  To: Matt Fleming
  Cc: Mark Rutland, Julien Grall, Juergen Gross, Boris Ostrovsky,
	sstabellini, linux-kernel, linux-arm-kernel, xen-devel,
	Ard Biesheuvel, linux-efi

On Tue, Apr 18, 2017 at 02:46:50PM +0100, Matt Fleming wrote:
> On Thu, 06 Apr, at 04:55:11PM, Mark Rutland wrote:
> >
> > Please, let's keep the Xen knowledge constrained to the Xen EFI wrapper,
> > rather than spreading it further.
> >
> > IMO, given reset_system is a *mandatory* function, the Xen wrapper
> > should provide an implementation.
> >
> > I don't see why you can't implement a wrapper that calls the usual Xen
> > poweroff/reset functions.
>
> I realise I'm making a sweeping generalisation, but adding
> EFI_PARAVIRT is almost always the wrong thing to do.

Why?

> I'd much prefer to see Mark's idea implemented.

If you wish I do not object.

Daniel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-19 19:29                   ` Daniel Kiper
@ 2017-04-19 19:37                     ` Matt Fleming
  2017-04-19 19:43                       ` Daniel Kiper
  0 siblings, 1 reply; 26+ messages in thread
From: Matt Fleming @ 2017-04-19 19:37 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: Mark Rutland, Julien Grall, Juergen Gross, Boris Ostrovsky,
	sstabellini, linux-kernel, linux-arm-kernel, xen-devel,
	Ard Biesheuvel, linux-efi

On Wed, 19 Apr, at 09:29:06PM, Daniel Kiper wrote:
> On Tue, Apr 18, 2017 at 02:46:50PM +0100, Matt Fleming wrote:
> > On Thu, 06 Apr, at 04:55:11PM, Mark Rutland wrote:
> > >
> > > Please, let's keep the Xen knowledge constrained to the Xen EFI wrapper,
> > > rather than spreading it further.
> > >
> > > IMO, given reset_system is a *mandatory* function, the Xen wrapper
> > > should provide an implementation.
> > >
> > > I don't see why you can't implement a wrapper that calls the usual Xen
> > > poweroff/reset functions.
> >
> > I realise I'm making a sweeping generalisation, but adding
> > EFI_PARAVIRT is almost always the wrong thing to do.
> 
> Why?
 
Because it makes paravirt a special case, and there's usually very
little reason to make it special in the EFI code. Special-casing means
more branches, more code paths, a bigger testing matrix and more
complex code. 

EFI_PARAVIRT does have its uses, like for those scenarios where we
don't have a table of function pointers that can be overidden for
paravirt.

But we do have such a table for ->reset_system().

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-19 19:37                     ` Matt Fleming
@ 2017-04-19 19:43                       ` Daniel Kiper
  0 siblings, 0 replies; 26+ messages in thread
From: Daniel Kiper @ 2017-04-19 19:43 UTC (permalink / raw)
  To: Matt Fleming
  Cc: Mark Rutland, Julien Grall, Juergen Gross, Boris Ostrovsky,
	sstabellini, linux-kernel, linux-arm-kernel, xen-devel,
	Ard Biesheuvel, linux-efi

On Wed, Apr 19, 2017 at 08:37:38PM +0100, Matt Fleming wrote:
> On Wed, 19 Apr, at 09:29:06PM, Daniel Kiper wrote:
> > On Tue, Apr 18, 2017 at 02:46:50PM +0100, Matt Fleming wrote:
> > > On Thu, 06 Apr, at 04:55:11PM, Mark Rutland wrote:
> > > >
> > > > Please, let's keep the Xen knowledge constrained to the Xen EFI wrapper,
> > > > rather than spreading it further.
> > > >
> > > > IMO, given reset_system is a *mandatory* function, the Xen wrapper
> > > > should provide an implementation.
> > > >
> > > > I don't see why you can't implement a wrapper that calls the usual Xen
> > > > poweroff/reset functions.
> > >
> > > I realise I'm making a sweeping generalisation, but adding
> > > EFI_PARAVIRT is almost always the wrong thing to do.
> >
> > Why?
>
> Because it makes paravirt a special case, and there's usually very
> little reason to make it special in the EFI code. Special-casing means
> more branches, more code paths, a bigger testing matrix and more
> complex code.
>
> EFI_PARAVIRT does have its uses, like for those scenarios where we
> don't have a table of function pointers that can be overidden for
> paravirt.
>
> But we do have such a table for ->reset_system().

This is more or less what I expected. Thanks a lot for explanation.

Daniel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
  2017-04-18 18:51                             ` Juergen Gross
@ 2017-04-20 18:09                               ` Julien Grall
  0 siblings, 0 replies; 26+ messages in thread
From: Julien Grall @ 2017-04-20 18:09 UTC (permalink / raw)
  To: Juergen Gross, Stefano Stabellini
  Cc: Daniel Kiper, Boris Ostrovsky, xen-devel, linux-arm-kernel,
	linux-kernel, Mark Rutland, ard.biesheuvel, linux-efi, matt

Hi,

On 18/04/17 19:51, Juergen Gross wrote:
> On 18/04/17 20:46, Stefano Stabellini wrote:
>> On Tue, 18 Apr 2017, Juergen Gross wrote:
>>> On 18/04/17 20:37, Stefano Stabellini wrote:
>>>> On Thu, 6 Apr 2017, Juergen Gross wrote:
>>>>> On 06/04/17 18:43, Daniel Kiper wrote:
>>>>>> On Thu, Apr 06, 2017 at 06:22:44PM +0200, Juergen Gross wrote:
>>>>>>> On 06/04/17 18:06, Daniel Kiper wrote:
>>>>>>>> Hi Julien,
>>>>>>>>
>>>>>>>> On Thu, Apr 06, 2017 at 04:39:13PM +0100, Julien Grall wrote:
>>>>>>>>> Hi Daniel,
>>>>>>>>>
>>>>>>>>> On 06/04/17 16:20, Daniel Kiper wrote:
>>>>>>>>>> On Thu, Apr 06, 2017 at 04:38:24PM +0200, Juergen Gross wrote:
>>>>>>>>>>> On 06/04/17 16:27, Daniel Kiper wrote:
>>>>>>>>>>>> On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
>>>>>>>>>>>>> Hi Juergen,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 06/04/17 07:23, Juergen Gross wrote:
>>>>>>>>>>>>>> On 05/04/17 21:49, Boris Ostrovsky wrote:
>>>>>>>>>>>>>>> On 04/05/2017 02:14 PM, Julien Grall wrote:
>>>>>>>>>>>>>>>> The x86 code has theoritically a similar issue, altought EFI does not
>>>>>>>>>>>>>>>> seem to be the preferred method. I have left it unimplemented on x86 and
>>>>>>>>>>>>>>>> CCed Linux Xen x86 maintainers to know their view here.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> (+Daniel)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This could be a problem for x86 as well, at least theoretically.
>>>>>>>>>>>>>>> xen_machine_power_off() may call pm_power_off(), which is efi.reset_system.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> So I think we should have a similar routine there.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> +1
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I don't see any problem with such a routine added, in contrast to
>>>>>>>>>>>>>> potential "reboots" instead of power off without it.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So I think this dummy xen_efi_reset_system() should be added to
>>>>>>>>>>>>>> drivers/xen/efi.c instead.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I will resend the patch during day with xen_efi_reset_system moved
>>>>>>>>>>>>> to common code and implement the x86 counterpart (thought, I will
>>>>>>>>>>>>> not be able to test it).
>>>>>>>>>>>>
>>>>>>>>>>>> I think that this is ARM specific issue. On x86 machine_restart() calls
>>>>>>>>>>>> xen_restart(). Hence, everything works. So, I think that it should be
>>>>>>>>>>>> fixed only for ARM. Anyway, please CC me when you send a patch.
>>>>>>>>>>>
>>>>>>>>>>> What about xen_machine_power_off() (as stated in Boris' mail)?
>>>>>>>>>>
>>>>>>>>>> Guys what do you think about that:
>>>>>>>>>>
>>>>>>>>>> --- a/drivers/firmware/efi/reboot.c
>>>>>>>>>> +++ b/drivers/firmware/efi/reboot.c
>>>>>>>>>> @@ -55,7 +55,7 @@ static void efi_power_off(void)
>>>>>>>>>>
>>>>>>>>>> static int __init efi_shutdown_init(void)
>>>>>>>>>> {
>>>>>>>>>> -       if (!efi_enabled(EFI_RUNTIME_SERVICES))
>>>>>>>>>> +       if (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_PARAVIRT))
>>>>>>>>>>                return -ENODEV;
>>>>>>>>>>
>>>>>>>>>>        if (efi_poweroff_required())
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Julien, for ARM64 please take a look at arch/arm64/kernel/efi.c:efi_poweroff_required(void).
>>>>>>>>>>
>>>>>>>>>> I hope that tweaks for both files should solve our problem.
>>>>>>>>>
>>>>>>>>> This sounds good for power off (I haven't tried to power off DOM0
>>>>>>>>> yet). But this will not solve the restart problem (see
>>>>>>>>> machine_restart in arch/arm64/kernel/process.c) which call directly
>>>>>>>>> efi_reboot.
>>>>>>>>
>>>>>>>> Hmmm... It seems to me that efi.reset_system override with empty function
>>>>>>>> in arch/arm/xen/efi.c is the best solution. So, I see three patches here.
>>>>>>>> One for drivers/firmware/efi/reboot.c, one for arch/arm/xen/efi.c and one
>>>>>>>> for arch/arm64/kernel/efi.c. Does it make sense?
>>>>>>>
>>>>>>> I still think the empty function should be in drivers/xen/efi.c and we
>>>>>>> should use it in arch/x86/xen/efi.c, too.
>>>>>>
>>>>>> If you wish we can go that way too. Though I thing that we should fix
>>>>>> drivers/firmware/efi/reboot.c:efi_shutdown_init() too. Just in case.
>>>>>
>>>>> Sure, go ahead. I won't object.
>>>>
>>>> For the Xen on ARM side, the original patch that started this thread
>>>> (20170405181417.15985-1-julien.grall@arm.com) is good to go, right?
>>>>
>>>
>>> As I said: the dummy xen_efi_reset_system() should be in
>>> drivers/xen/efi.c
>>
>> OK. Who is working on it?
>
> Didn't Julien say he would do it?

Yes. I looked at bit closer to the problem mention with power off. 
xen_efi_reset_system cannot be a NOP because there may not be fallback 
alternatives (see machine_power_off in arch/arm64/kernel/process.c)

So I think we would have to translate EFI_RESET* to Xen SHUTDOWN_* and 
then call HYPERVISOR_sched_op directly.

I will send a new version soon.

Cheers,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2017-04-20 18:09 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-05 18:14 [PATCH] arm64: xen: Implement EFI reset_system callback Julien Grall
2017-04-05 19:49 ` Boris Ostrovsky
2017-04-06  6:23   ` Juergen Gross
2017-04-06  8:32     ` Julien Grall
2017-04-06  8:37       ` Juergen Gross
2017-04-06 14:27       ` Daniel Kiper
2017-04-06 14:32         ` Julien Grall
2017-04-06 14:37           ` Boris Ostrovsky
2017-04-06 14:38         ` Juergen Gross
2017-04-06 15:20           ` Daniel Kiper
2017-04-06 15:39             ` Julien Grall
2017-04-06 15:55               ` Mark Rutland
2017-04-18 13:46                 ` Matt Fleming
2017-04-19 19:29                   ` Daniel Kiper
2017-04-19 19:37                     ` Matt Fleming
2017-04-19 19:43                       ` Daniel Kiper
2017-04-06 16:06               ` Daniel Kiper
2017-04-06 16:22                 ` Juergen Gross
2017-04-06 16:43                   ` Daniel Kiper
2017-04-06 17:39                     ` Juergen Gross
2017-04-18 18:37                       ` Stefano Stabellini
2017-04-18 18:43                         ` Juergen Gross
2017-04-18 18:46                           ` Stefano Stabellini
2017-04-18 18:51                             ` Juergen Gross
2017-04-20 18:09                               ` Julien Grall
2017-04-05 21:26 ` Stefano Stabellini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).