Re: [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)

[Julien Grall <julien.grall@arm.com>]

Hi Mirela,

On 14/11/2018 15:36, Mirela Simonovic wrote:
> On Wed, Nov 14, 2018 at 3:49 PM Julien Grall <julien.grall@arm.com> wrote:
>> On 14/11/2018 13:05, Julien Grall wrote:
>>> On 14/11/2018 12:35, Mirela Simonovic wrote:
>>>> On 11/14/2018 11:45 AM, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 13/11/2018 20:39, Stefano Stabellini wrote:
>>>>>> On Mon, 12 Nov 2018, Julien Grall wrote:
>>>>>>>>> However, what is the issue with saving all the registers here?
>>>>>>>>>
>>>>>>>>
>>>>>>>> We need to save arguments that are provided by a guest with system
>>>>>>>> suspend PSCI call. These arguments are the entry point that needs to
>>>>>>>> be saved in program counter and context ID that needs to be saved in
>>>>>>>> x0/r0. We don't have these arguments here. Context switch happens
>>>>>>>> after processing the system suspend PSCI call, so it's too late.
>>>>>>>
>>>>>>> It does not feel right to modify ctxt_switch{from,to} for suspend/resume. If
>>>>>>> you want to reset the vCPU state before blocking the vCPU, then you should
>>>>>>> instead
>>>>>>
>>>>
>>>> I think it's not clear what problem are we discussing here, at least it's not
>>>> to me. So I'll make an assumption, and please correct me if I'm wrong.
>>>> In the patches we submitted, the VCPU context is not reset in
>>>> ctxt_switch{from,to}. My understanding is that you suggested/asked to reset
>>>> the VCPU context when switch happens, and I explained why is that not possible
>>>> - at least not without additional code changes, that may not be so small. I
>>>> agree with Andrew's comment in this perspective - reset of VCPU should not
>>>> (and right now cannot) be done when the context is switched.
>>>
>>> I didn't ask to reset the vCPU context in the switch. Instead we should make
>>> sure the vCPU context is synced to memory before modifying it.
>>>
>>> It seems that solution works on x86 using domain_pause (see
>>> hvm_s3_{resume,suspend}). So I am not sure why it cannot be use on Arm. Note
>>> that it may require more work.
>>>
>>>>
>>>>>> You missed the end of the suggestion here
>>>>>
>>>>> Whoops. I meant that instead you should save the context of the vCPU in
>>>>> advance or reset the vCPU using the system registers directly.
>>>>>
>>>>> But my preference is to reset the vCPU when you receive the wake-up interrupt.
>>>>>
>>>>
>>>> Without you presenting more details how would that work I cannot really
>>>> provide any comment, nor say that your preference could work or be better
>>>> compared to what is in this series. Honestly, I don't understand what exactly
>>>> you're proposing, because more things needs to be think-through beyond the
>>>> place to put a code.
>>>> We submitted a code that works, which is very elegant and nice in my opinion
>>>> (fair to say we may not share opinions here), and does not require lots of
>>>> code changes. So there's the reference.
>>>> Could you please clarify why do you think the proposed solution is not good?
>>>
>>> The context switch is about saving/restore the context from the hardware. We can
>>> decide to optimize it in the suspend case (though it might be premature), but it
>>> is clearly the wrong place to decide to resume a domain.
>>
>> Actually, I just found a good example of why I think it is wrong and broken. You
>> rely on a context switch to always happen after suspending and on resuming the
>> guest. There are path where context/switch will not happen. An example is if you
>> have interrupt pending, you may return to the guest directly if the scheduler
>> does not have anything else to schedule.
>>
> 
> Can we check whether the vcpu blocked, right? Let me be more specific
> - after calling vcpu_block_unless_event_pending we can check whether
> the vcpu is indeed blocked?
This would be racy. The vCPU might be blocked when you check it. However, it can 
  get unblocked before you reached the schedule softirq (where schedule is done).

Cheers,

-- 
Julien Grall

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