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

[Mirela Simonovic <mirela.simonovic@aggios.com>]

On 11/13/2018 04:09 PM, Julien Grall wrote:
>
>
> On 13/11/2018 10:23, Julien Grall wrote:
>> Hi,
>>
>> On 12/11/2018 11:30, Mirela Simonovic wrote:
>>> +/*
>>> + * This function sets the context of current VCPU to the state 
>>> which is expected
>>> + * by the guest on resume. The expected VCPU state is:
>>> + * 1) pc to contain resume entry point (1st argument of PSCI 
>>> SYSTEM_SUSPEND)
>>> + * 2) r0/x0 to contain context ID (2nd argument of PSCI 
>>> SYSTEM_SUSPEND)
>>> + * 3) All other general purpose and system registers should have 
>>> reset values
>>> + *
>>> + * Note: this function has to return void because it has to always 
>>> succeed. In
>>> + * other words, this function is called from virtual PSCI 
>>> SYSTEM_SUSPEND
>>> + * implementation, which can return only a limited number of 
>>> possible errors,
>>> + * none of which could represent the fact that an error occurred 
>>> when preparing
>>> + * the domain for suspend.
>>> + * Consequently, dynamic memory allocation cannot be done within 
>>> this function,
>>> + * because if malloc fails the error has nowhere to propagate.
>>> + */
>>> +static void vcpu_suspend(register_t epoint, register_t cid)
>>> +{
>>> +    /* Static allocation because dynamic would need a non-void 
>>> return */
>>> +    static struct vcpu_guest_context ctxt;
>>> +    struct vcpu *v = current;
>>> +
>>> +    /* Make sure that VCPU guest regs are zeroied */
>>> +    memset(&ctxt, 0, sizeof(ctxt));
>>> +
>>> +    /* Set non-zero values to the registers prior to copying */
>>> +    ctxt.user_regs.pc64 = (u64)epoint;
>>> +
>>> +    if ( is_32bit_domain(current->domain) )
>>> +    {
>>> +        ctxt.user_regs.r0_usr = cid;
>>> +        ctxt.user_regs.cpsr = PSR_GUEST32_INIT;
>>
>> This is going to disable the MMU and Cache as requested by the PSCI 
>> spec. As the guest is not required to clean the cache when turning 
>> off the CPU/suspending, the data may not have reached the main memory.
>>
>> So do you need to perform cache maintenance to avoid stale information?
>
> Answering to myself, I have discussed about the cache with others Arm 
> folks today. SYSTEM_SUSPEND may exit early due to a pending event (BTW 
> you don't seem to handle it) and could jump to the specified entry 
> point address.
>

I believe it is handled - with the vcpu_block_unless_event_pending in 
domain_suspend

> In that case, the only guarantee is the data would not be lost, yet 
> can still be in the cache. This means that any data used before the 
> MMU & cache are enabled in the entry point should have been cleaned to 
> PoC and if you modify data make sure the cache is invalidated.
>
> Linux is doing that properly. Looking at Xen, we don't really properly 
> handle the cache in the boot path. So you may randomly crash.
>
> Cheers,
>


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