* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-06 15:39 ` Julien Grall
0 siblings, 0 replies; 84+ messages in thread
From: Julien Grall @ 2017-04-06 15:39 UTC (permalink / raw)
To: linux-arm-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] 84+ 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
-1 siblings, 0 replies; 84+ messages in thread
From: Mark Rutland @ 2017-04-06 15:55 UTC (permalink / raw)
To: Julien Grall
Cc: Juergen Gross, sstabellini, Ard Biesheuvel, Matt Fleming,
Daniel Kiper, linux-kernel, xen-devel, linux-efi,
Boris Ostrovsky, linux-arm-kernel
[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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 84+ messages in thread
* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-06 15:55 ` Mark Rutland
0 siblings, 0 replies; 84+ 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] 84+ messages in thread
* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-06 15:55 ` Mark Rutland
0 siblings, 0 replies; 84+ messages in thread
From: Mark Rutland @ 2017-04-06 15:55 UTC (permalink / raw)
To: linux-arm-kernel
[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] 84+ messages in thread
* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-06 15:55 ` Mark Rutland
0 siblings, 0 replies; 84+ 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-DgEjT+Ai2ygdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
xen-devel-GuqFBffKawuEi8DpZVb4nw, Matt Fleming, Ard Biesheuvel,
linux-efi-u79uwXL29TY76Z2rM5mHXA
[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] 84+ 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
-1 siblings, 0 replies; 84+ messages in thread
From: Matt Fleming @ 2017-04-18 13:46 UTC (permalink / raw)
To: Mark Rutland
Cc: Juergen Gross, sstabellini, Ard Biesheuvel, Daniel Kiper,
linux-kernel, xen-devel, Julien Grall, linux-efi,
Boris Ostrovsky, linux-arm-kernel
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 84+ 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
-1 siblings, 0 replies; 84+ 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] 84+ messages in thread
* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-18 13:46 ` Matt Fleming
0 siblings, 0 replies; 84+ messages in thread
From: Matt Fleming @ 2017-04-18 13:46 UTC (permalink / raw)
To: linux-arm-kernel
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] 84+ messages in thread
* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-18 13:46 ` Matt Fleming
0 siblings, 0 replies; 84+ 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-DgEjT+Ai2ygdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
xen-devel-GuqFBffKawuEi8DpZVb4nw, Ard Biesheuvel,
linux-efi-u79uwXL29TY76Z2rM5mHXA
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] 84+ 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
-1 siblings, 0 replies; 84+ messages in thread
From: Daniel Kiper @ 2017-04-19 19:29 UTC (permalink / raw)
To: Matt Fleming
Cc: Mark Rutland, Juergen Gross, sstabellini, Ard Biesheuvel,
linux-kernel, xen-devel, Julien Grall, linux-efi,
Boris Ostrovsky, linux-arm-kernel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 84+ messages in thread
* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-19 19:29 ` Daniel Kiper
0 siblings, 0 replies; 84+ 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] 84+ messages in thread
* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-19 19:29 ` Daniel Kiper
0 siblings, 0 replies; 84+ messages in thread
From: Daniel Kiper @ 2017-04-19 19:29 UTC (permalink / raw)
To: linux-arm-kernel
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] 84+ messages in thread
* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-19 19:29 ` Daniel Kiper
0 siblings, 0 replies; 84+ 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-DgEjT+Ai2ygdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
xen-devel-GuqFBffKawuEi8DpZVb4nw, Ard Biesheuvel,
linux-efi-u79uwXL29TY76Z2rM5mHXA
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] 84+ messages in thread
* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-19 19:37 ` Matt Fleming
0 siblings, 0 replies; 84+ 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] 84+ messages in thread
* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-19 19:37 ` Matt Fleming
0 siblings, 0 replies; 84+ messages in thread
From: Matt Fleming @ 2017-04-19 19:37 UTC (permalink / raw)
To: linux-arm-kernel
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] 84+ messages in thread
* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-19 19:37 ` Matt Fleming
0 siblings, 0 replies; 84+ 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-DgEjT+Ai2ygdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
xen-devel-GuqFBffKawuEi8DpZVb4nw, Ard Biesheuvel,
linux-efi-u79uwXL29TY76Z2rM5mHXA
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] 84+ 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
-1 siblings, 0 replies; 84+ messages in thread
From: Daniel Kiper @ 2017-04-19 19:43 UTC (permalink / raw)
To: Matt Fleming
Cc: Mark Rutland, Juergen Gross, sstabellini, Ard Biesheuvel,
linux-kernel, xen-devel, Julien Grall, linux-efi,
Boris Ostrovsky, linux-arm-kernel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 84+ 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
-1 siblings, 0 replies; 84+ 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] 84+ messages in thread
* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-19 19:43 ` Daniel Kiper
0 siblings, 0 replies; 84+ messages in thread
From: Daniel Kiper @ 2017-04-19 19:43 UTC (permalink / raw)
To: linux-arm-kernel
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] 84+ messages in thread
* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-19 19:43 ` Daniel Kiper
0 siblings, 0 replies; 84+ messages in thread
From: Daniel Kiper @ 2017-04-19 19:43 UTC (permalink / raw)
To: Matt Fleming
Cc: Mark Rutland, Juergen Gross, sstabellini, Ard Biesheuvel,
linux-kernel, xen-devel, Julien Grall, linux-efi,
Boris Ostrovsky, linux-arm-kernel
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] 84+ messages in thread
* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
2017-04-19 19:29 ` Daniel Kiper
` (2 preceding siblings ...)
(?)
@ 2017-04-19 19:37 ` Matt Fleming
-1 siblings, 0 replies; 84+ messages in thread
From: Matt Fleming @ 2017-04-19 19:37 UTC (permalink / raw)
To: Daniel Kiper
Cc: Mark Rutland, Juergen Gross, sstabellini, Ard Biesheuvel,
linux-kernel, xen-devel, Julien Grall, linux-efi,
Boris Ostrovsky, linux-arm-kernel
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().
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 84+ messages in thread
* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
2017-04-06 15:39 ` Julien Grall
` (2 preceding siblings ...)
(?)
@ 2017-04-06 16:06 ` Daniel Kiper
-1 siblings, 0 replies; 84+ messages in thread
From: Daniel Kiper @ 2017-04-06 16:06 UTC (permalink / raw)
To: Julien Grall
Cc: Juergen Gross, sstabellini, linux-kernel, xen-devel,
Boris Ostrovsky, linux-arm-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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 84+ messages in thread
* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
2017-04-06 15:39 ` Julien Grall
@ 2017-04-06 16:06 ` Daniel Kiper
-1 siblings, 0 replies; 84+ 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] 84+ messages in thread
* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-06 16:06 ` Daniel Kiper
0 siblings, 0 replies; 84+ messages in thread
From: Daniel Kiper @ 2017-04-06 16:06 UTC (permalink / raw)
To: linux-arm-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] 84+ 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
-1 siblings, 0 replies; 84+ messages in thread
From: Juergen Gross @ 2017-04-06 16:22 UTC (permalink / raw)
To: Daniel Kiper, Julien Grall
Cc: Boris Ostrovsky, sstabellini, linux-kernel, linux-arm-kernel, xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 84+ 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
-1 siblings, 0 replies; 84+ 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] 84+ messages in thread
* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-06 16:22 ` Juergen Gross
0 siblings, 0 replies; 84+ messages in thread
From: Juergen Gross @ 2017-04-06 16:22 UTC (permalink / raw)
To: linux-arm-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] 84+ 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
-1 siblings, 0 replies; 84+ messages in thread
From: Daniel Kiper @ 2017-04-06 16:43 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, linux-kernel, xen-devel, Julien Grall,
Boris Ostrovsky, linux-arm-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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 84+ 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
-1 siblings, 0 replies; 84+ 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] 84+ messages in thread
* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-06 16:43 ` Daniel Kiper
0 siblings, 0 replies; 84+ messages in thread
From: Daniel Kiper @ 2017-04-06 16:43 UTC (permalink / raw)
To: linux-arm-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] 84+ 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
-1 siblings, 0 replies; 84+ 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] 84+ messages in thread
* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-06 17:39 ` Juergen Gross
0 siblings, 0 replies; 84+ messages in thread
From: Juergen Gross @ 2017-04-06 17:39 UTC (permalink / raw)
To: linux-arm-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] 84+ 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
-1 siblings, 0 replies; 84+ messages in thread
From: Stefano Stabellini @ 2017-04-18 18:37 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, Daniel Kiper, linux-kernel, xen-devel, Julien Grall,
Boris Ostrovsky, linux-arm-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?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 84+ 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
-1 siblings, 0 replies; 84+ 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] 84+ messages in thread
* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-18 18:37 ` Stefano Stabellini
0 siblings, 0 replies; 84+ messages in thread
From: Stefano Stabellini @ 2017-04-18 18:37 UTC (permalink / raw)
To: linux-arm-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 at arm.com) is good to go, right?
^ permalink raw reply [flat|nested] 84+ 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
-1 siblings, 0 replies; 84+ messages in thread
From: Juergen Gross @ 2017-04-18 18:43 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Daniel Kiper, linux-kernel, xen-devel, Julien Grall,
Boris Ostrovsky, linux-arm-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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 84+ 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
-1 siblings, 0 replies; 84+ 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] 84+ messages in thread
* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-18 18:43 ` Juergen Gross
0 siblings, 0 replies; 84+ messages in thread
From: Juergen Gross @ 2017-04-18 18:43 UTC (permalink / raw)
To: linux-arm-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 at 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] 84+ 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
-1 siblings, 0 replies; 84+ 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] 84+ messages in thread
* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-18 18:46 ` Stefano Stabellini
0 siblings, 0 replies; 84+ messages in thread
From: Stefano Stabellini @ 2017-04-18 18:46 UTC (permalink / raw)
To: linux-arm-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 at 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] 84+ 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
-1 siblings, 0 replies; 84+ 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] 84+ messages in thread
* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-18 18:51 ` Juergen Gross
0 siblings, 0 replies; 84+ messages in thread
From: Juergen Gross @ 2017-04-18 18:51 UTC (permalink / raw)
To: linux-arm-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 at 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] 84+ messages in thread
* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-20 18:09 ` Julien Grall
0 siblings, 0 replies; 84+ 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] 84+ messages in thread
* [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-20 18:09 ` Julien Grall
0 siblings, 0 replies; 84+ messages in thread
From: Julien Grall @ 2017-04-20 18:09 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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] 84+ messages in thread
* Re: [PATCH] arm64: xen: Implement EFI reset_system callback
@ 2017-04-20 18:09 ` Julien Grall
0 siblings, 0 replies; 84+ 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-GuqFBffKawuEi8DpZVb4nw,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Rutland,
ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
matt-mF/unelCI9GS6iBeEJttW/XRex20P6io
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-5wv7dgnIgG8@public.gmane.org) 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] 84+ 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
-1 siblings, 0 replies; 84+ messages in thread
From: Julien Grall @ 2017-04-20 18:09 UTC (permalink / raw)
To: Juergen Gross, Stefano Stabellini
Cc: Mark Rutland, linux-efi, ard.biesheuvel, matt, Daniel Kiper,
linux-kernel, xen-devel, Boris Ostrovsky, linux-arm-kernel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 84+ 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
-1 siblings, 0 replies; 84+ messages in thread
From: Juergen Gross @ 2017-04-18 18:51 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Daniel Kiper, linux-kernel, xen-devel, Julien Grall,
Boris Ostrovsky, linux-arm-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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 84+ 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
-1 siblings, 0 replies; 84+ messages in thread
From: Stefano Stabellini @ 2017-04-18 18:46 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Daniel Kiper, linux-kernel, xen-devel,
Julien Grall, Boris Ostrovsky, linux-arm-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?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 84+ 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
-1 siblings, 0 replies; 84+ messages in thread
From: Juergen Gross @ 2017-04-06 17:39 UTC (permalink / raw)
To: Daniel Kiper
Cc: sstabellini, linux-kernel, xen-devel, Julien Grall,
Boris Ostrovsky, linux-arm-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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 84+ messages in thread