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

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

On Wed, Nov 14, 2018 at 4:36 PM Mirela Simonovic
<mirela.simonovic@xxxxxxxxxx> wrote:
>
> 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).
> >
>

I wanted to write also that this is a very good example, thank you for
putting it together

>
>
> > Cheers,
> >
> > --
> > Julien Grall

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