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 <jul...@xen.org> 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
