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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Jan,

I have completely changed the implementation so that the common parts
of the code are almost untouched. Therefore, there is no longer a need
to address the previous issues with comments and other concerns.

Thank you for your review. Please take a look at the new version of
this patch series. I hope there are no issues remaining.

On Thu, Aug 28, 2025 at 10:40 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 27.08.2025 23:21, Mykola Kvach wrote:
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -1349,16 +1349,10 @@ int domain_shutdown(struct domain *d, u8 reason)
> >      return 0;
> >  }
> >
> > -void domain_resume(struct domain *d)
> > +static void domain_resume_nopause(struct domain *d)
> >  {
> >      struct vcpu *v;
> >
> > -    /*
> > -     * Some code paths assume that shutdown status does not get reset under
> > -     * their feet (e.g., some assertions make this assumption).
> > -     */
> > -    domain_pause(d);
> > -
> >      spin_lock(&d->shutdown_lock);
> >
> >      d->is_shutting_down = d->is_shut_down = 0;
> > @@ -1366,13 +1360,40 @@ void domain_resume(struct domain *d)
> >
> >      for_each_vcpu ( d, v )
> >      {
> > +        /*
> > +         * No need to conditionally clear _VPF_suspended here:
> > +         * - This bit is only set on Arm, and only after a successful 
> > suspend.
>
> How likely do you think it is that, if e.g. RISC-V or PPC clone Arm's
> code, this comment would then be updated accordingly? IOW I don't think
> a justification like this one should be written in such terms.
>
> > +         * - domain_resume_nopause() may also be called from paths other 
> > than
> > +         *   the suspend/resume flow, such as "soft-reset" actions (e.g.,
> > +         *   on_poweroff), as part of the Xenstore control/shutdown 
> > protocol.
> > +         *   These require guest acknowledgement to complete the operation.
>
> I'm having trouble connecting "require guest acknowledgement" to ...
>
> > +         * So clearing the bit unconditionally is safe.
>
> ... the safety of the unconditional clearing.
>
> > +         */
> > +        clear_bit(_VPF_suspended, &v->pause_flags);
> > +
> >          if ( v->paused_for_shutdown )
> >              vcpu_unpause(v);
> >          v->paused_for_shutdown = 0;
> >      }
> >
> >      spin_unlock(&d->shutdown_lock);
> > +}
> >
> > +#ifdef CONFIG_ARM
> > +void domain_resume_nopause_helper(struct domain *d)
>
> This is an odd name to use from code meaning to invoke 
> domain_resume_nopause().
> Why isn't this called domain_resume_nopause(), and ...
>
> > +{
> > +    domain_resume_nopause(d);
>
> ... the static function simply _domain_resume_nopause() (in full accordance
> to the C standard's naming rules)?
>
> > +}
> > +#endif
> > +
> > +void domain_resume(struct domain *d)
> > +{
> > +    /*
> > +     * Some code paths assume that shutdown status does not get reset under
> > +     * their feet (e.g., some assertions make this assumption).
> > +     */
> > +    domain_pause(d);
>
> As you move the comment - no such assumptions exist when the code path
> through domain_resume_nopause_helper() is taken?
>
> Jan

Best regards,
Mykola