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

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

Hi Julien,

On Wed, Nov 14, 2018 at 3:49 PM Julien Grall <julien.grall@xxxxxxx> wrote:
>
>
>
> On 14/11/2018 13:05, Julien Grall wrote:
> >
> >
> > On 14/11/2018 12:35, Mirela Simonovic wrote:
> >> Hi Julien,
> >
> > Hi,
> >
> >>
> >> 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?

> The problem is the variable is_shut_down and shutdown_code are only be reset 
> on
> restoring the vCPU. This means the next context switch will skip the saving 
> part
> and result to vCPU corruption.
>
> In general, you cannot rely on the context/switch code as it may or may not
> happen (that's up to the scheduler).
>



> Cheers,
>
> --
> Julien Grall

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