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

From: Mirela Simonovic <mirela.simonovic@aggios.com>
To: Julien Grall <julien.grall@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>, Xen Devel <xen-devel@lists.xen.org>,
	Wei Liu <wei.liu2@citrix.com>, Davorin Mista <dm@aggios.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Saeed Nowshadi <saeed.nowshadi@xilinx.com>,
	Jan Beulich <jbeulich@suse.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Stefano Stabellini <stefano.stabellini@xilinx.com>
Subject: Re: [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Date: Wed, 14 Nov 2018 13:35:27 +0100	[thread overview]
Message-ID: <0160c922-22fb-9ed9-5460-80c133516d4b@aggios.com> (raw)
In-Reply-To: <508d2a46-a275-8a8f-087e-4b953b942425@arm.com>

Hi Julien,

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.

>> 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?
And why do you think that what you're proposing is better? Lets be more 
clear here - how exactly you propose to implement that?

I haven't understood so far why do you think that the proposed approach 
is not good. Maybe the whole discussion drifted a bit for no reason.

Thanks,
Mirela

> Cheers,
>

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