Re: [Xen-devel] [PATCH 08/18] xen/arm: Disable/enable non-boot physical CPUs on suspend/resume

[Julien Grall <Julien.Grall@xxxxxxx>]

On 14/11/2018 23:04, Stefano Stabellini wrote:
> On Wed, 14 Nov 2018, Julien Grall wrote:
>> Hi Mirela,
>>
>> On 14/11/2018 13:00, Mirela Simonovic wrote:
>>>
>>>
>>> On 11/14/2018 11:52 AM, Julien Grall wrote:
>>>> Hi Mirela,
>>>>
>>>> On 12/11/2018 11:30, Mirela Simonovic wrote:
>>>>> Non-boot CPUs have to be disabled on suspend and enabled on resume
>>>>> (hotplug-based mechanism). Disabling non-boot CPUs will lead to PSCI
>>>>> CPU_OFF to be called by each non-boot CPU. Depending on the underlying
>>>>> platform capabilities, this may lead to the physical powering down of
>>>>> CPUs. Tested on Xilinx Zynq Ultrascale+ MPSoC (including power down of
>>>>> each non-boot CPU).
>>>>>
>>>>> Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
>>>>> Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xxxxxxxxxx>
>>>>> ---
>>>>>    xen/arch/arm/suspend.c | 15 ++++++++++++++-
>>>>>    1 file changed, 14 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
>>>>> index 575afd5eb8..dae1b1f7d6 100644
>>>>> --- a/xen/arch/arm/suspend.c
>>>>> +++ b/xen/arch/arm/suspend.c
>>>>> @@ -1,4 +1,5 @@
>>>>>    #include <xen/sched.h>
>>>>> +#include <xen/cpu.h>
>>>>>    #include <asm/cpufeature.h>
>>>>>    #include <asm/event.h>
>>>>>    #include <asm/psci.h>
>>>>> @@ -115,17 +116,29 @@ static void vcpu_suspend(register_t epoint,
>>>>> register_t cid)
>>>>>    /* Xen suspend. Note: data is not used (suspend is the suspend to RAM)
>>>>> */
>>>>>    static long system_suspend(void *data)
>>>>>    {
>>>>> +    int status;
>>>>> +
>>>>>        BUG_ON(system_state != SYS_STATE_active);
>>>>>          system_state = SYS_STATE_suspend;
>>>>>        freeze_domains();
>>>>>    +    status = disable_nonboot_cpus();
>>>>> +    if ( status )
>>>>> +    {
>>>>> +        system_state = SYS_STATE_resume;
>>>>> +        goto resume_nonboot_cpus;
>>>>> +    }
>>>>> +
>>>>>        system_state = SYS_STATE_resume;
>>>>>    +resume_nonboot_cpus:
>>>>> +    enable_nonboot_cpus();
>>>>>        thaw_domains();
>>>>>        system_state = SYS_STATE_active;
>>>>> +    dsb(sy);
>>>>
>>>> Why do you need a dsb(sy) here?
>>>>
>>>
>>> Updated value of system_state variable needs to be visible to other CPUs
>>> before we move on
>>
>> We tend to write the reason on top of barrier why they are necessary. But I 
>> am
>> still unsure to understand why this is important. What would happen if move 
>> on
>> without it?
> 
> That is a good question. I went through the code and it seems that the
> only effect could be potentially taking the wrong path in
> cpupool_cpu_add, but since that's called from a .notifier_call I don't
> think it can happen in practice. It is always difficult to prove that
> we don't need a barrier, it is easier to prove when we need a barrier,
> but in this case it does look like we do not need it after all.

It is also very easy to add barrier everywhere so we are sure what to do 
;). If you need a barrier, then you need to give plausible explanation.

In that case, if you need barrier here for system_state. Then what 
wouldn't you need it in other places where system_state is updated/read?
> 
>   
>>>>>    -    return -ENOSYS;
>>>>
>>>> Why do you return -ENOSYS until this patch? Should not have it be 0?
>>>>
>>>
>>> To distinguish that Xen suspend wasn't supported until we at least do
>>> something useful in suspend procedure. This is not so important, we can
>>> return 0 but needs to be fixed in previous patches.
>>
>> If you return 0 before hand you can more easily bisect this series and know
>> where it suspend/resume breaks.
> 
> Why 0 improves bisectability? 0 prevents other checks from figuring out
> that there was an error.

But this code is exactly replacing -ENOSYS by state (that would be 0 in 
success. So what's wrong to return 0 rather than -ENOSYS in that patch 
implement the dummy system_state?

> Also, the feature is not bisectable until the
> full series is applied.

Really? I was under the impression you can still do some sort of 
suspend/resume patch by patch. Although, you would not do a full 
suspend/resume.

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel