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

[Julien Grall <julien.grall@xxxxxxx>]

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.
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,

--
Julien Grall

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