Re: [PATCH] arm64: xen: Implement EFI reset_system callback
From: Julien Grall
Date: Thu Apr 20 2017 - 14:09:39 EST
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@xxxxxxx) 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