Re: [PATCH v13 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests

[Julien Grall <julien@xxxxxxx>]

Hi Mykola,

On 02/09/2025 10:03, Mykola Kvach wrote:
> @@ -880,6 +883,40 @@ void arch_domain_creation_finished(struct domain *d)
>       p2m_domain_creation_finished(d);
>   }
>   
> +int arch_domain_resume(struct domain *d)
> +{
> +    int rc;
> +    typeof(d->arch.resume_ctx) *ctx = &d->arch.resume_ctx;
I know this is v13, but I don't think we should use typeof() is in this 
context. "struct resume_info" is much shorter and help the review (I 
don't have to grep resume_ctx to figure out the type).
> +
> +    if ( !d->is_shutting_down || d->shutdown_code != SHUTDOWN_suspend )
> +    {
> +        dprintk(XENLOG_WARNING,
> +                "%pd: Invalid domain state for resume: is_shutting_down=%d, 
> shutdown_code=%d\n",
> +                d, d->is_shutting_down, d->shutdown_code);
> +        return -EINVAL;
> +    }
> +
> +    /*
> +     * It is still possible to call domain_shutdown() with a suspend reason
> +     * via some hypercalls, such as SCHEDOP_shutdown or 
> SCHEDOP_remote_shutdown.
> +     * In these cases, the resume context will be empty.
> +     * This is not expected to cause any issues, so we just warn about the
> +     * situation and return without error, allowing the existing logic to
> +     * proceed as expected.
I think this odd to warn if something is expected. It would be best to 
use "XENLOG_INFO".
> +     */
> +    if ( !ctx->wake_cpu )
> +    {
> +        dprintk(XENLOG_WARNING, "%pd: Invalid wake CPU pointer for resume\n",
As you wrote above, there is nothing wrong. So "Invalid" is not correct. 
I think a better wording is "Wake CPU pointer context was not provided").
Also note that dprintk() will be a NOP in release build. I am guessing 
this is intended?
I will answer you Jan's comment separately. The rest looks good to me.

Cheers,

--
Julien Grall