Re: [Xen-devel] [PATCH] arm64: xen: Implement EFI reset_system callback

[Juergen Gross <jgross@xxxxxxxx>]

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

Didn't Julien say he would do it?


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel