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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Julien,

Thank you for reviewing this patch series and for your valuable feedback.

On Sat, Sep 13, 2025 at 1:38 AM Julien Grall <julien@xxxxxxx> wrote:
>
> 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).

Ack

>
> > +
> > +    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".

Ack.

>
> > +     */
> > +    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").

Thanks for the suggestion. I'll change the message.

>
> Also note that dprintk() will be a NOP in release build. I am guessing
> this is intended?

Yep. In any case, the status is checked after the call to arch_domain_resume
(see domain_resume), so we notify the user about the situation. Detailed
prints are only available in debug builds.

>
> I will answer you Jan's comment separately. The rest looks good to me.
>
> Cheers,
>
> --
> Julien Grall
>

Best regards,
Mykola