Re: [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends

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

On 11/12/18 4:52 PM, Mirela Simonovic wrote:
> Hi Julien,

Hi,

> Thanks for the feedback.
> 
> On Mon, Nov 12, 2018 at 4:36 PM Julien Grall <julien.grall@arm.com> wrote:
>>
>> Hi Mirela,
>>
>> On 11/12/18 11:30 AM, Mirela Simonovic wrote:
>>> GIC and virtual timer context must be saved when the domain suspends.
>>
>> Please provide the rationale for this. Is it required by the spec?
>>
> 
> This is required for GIC because a guest leaves enabled interrupts in
> the GIC that could wake it up, and on resume it should be able to
> detect which interrupt woke it up. Without saving/restoring the state
> of GIC this would not be possible.

I am confused. In patch #10, you save the GIC host because the GIC can 
be powered-down. Linux is also saving the GIC state. So how the 
interrupt can come up if the GIC is powered down?

> For timer, my understanding is that vtimer accounts for how long the
> guest was executing and this tracking should not be affected by the
> suspend/resume sequence. Without saving/restoring the state of vtimer,
> the time tracking would be affected by the suspend/resume.

Currently, vtimer does not take such things into account. It only setup 
a timer in Xen to wake up the guest later on.

> Hope this is ok, I'll add it to the commit message.
> 
>>> This is done by moving the respective code in ctxt_switch_from()
>>> before the return that happens if the domain suspended.
>>>
>>> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>>> Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xilinx.com>
>>> ---
>>>    xen/arch/arm/domain.c | 14 +++++++-------
>>>    1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>> index 7f8105465c..bebe3238e8 100644
>>> --- a/xen/arch/arm/domain.c
>>> +++ b/xen/arch/arm/domain.c
>>> @@ -97,6 +97,13 @@ static void ctxt_switch_from(struct vcpu *p)
>>>        if ( is_idle_vcpu(p) )
>>>            return;
>>>
>>> +    /* VGIC */
>>> +    gic_save_state(p);
>>> +
>>> +    /* Arch timer */
>>> +    p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
>>> +    virt_timer_save(p);
>>> +
>>>        /* VCPU's context should not be saved if its domain is suspended */
>>
>> The GIC and timer are part of the vCPU context. So this comment looks a
>> bit stale.
> 
> It's not stale, but incorrect in that perspective. The comment should
> be updated to "VCPU architecture specific context should ..."

But the timer is part of the architecture...

Cheers,

-- 
Julien Grall

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