Re: [PATCH] xen/domctl: add domain_lock in XEN_DOMCTL_setvcpucontext

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Thu, Aug 7, 2025 at 8:47 AM Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
>
> Hi Jan,
>
> On Tue, Jul 29, 2025 at 10:56 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >
> > On 28.07.2025 20:40, Mykola Kvach wrote:
> > > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > >
> > > Add domain_{lock,unlock} in the XEN_DOMCTL_setvcpucontext operation
> > > for protecting arch_set_info_guest.
> > >
> > > This aligns with the locking pattern used by other operations that
> > > modify vCPU state.
> > >
> > > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > I think this requires more of a description / justification. May I in
> > particular turn your attention to this comment that we have in x86'es
> > handling of HVM_PARAM_IDENT_PT (disregard the 1st sentence for the
> > purpose here):
> >
> >         /*
> >          * Update GUEST_CR3 in each VMCS to point at identity map.
> >          * All foreign updates to guest state must synchronise on
> >          * the domctl_lock.
> >          */
> >         rc = -ERESTART;
> >         if ( !domctl_lock_acquire(d) )
> >             break;
> >
> > IOW in particular I'd expect you to explain why holding the domctl
> > lock isn't sufficient here, and hence what (theoretical?) race it is
> > you're concerned about. That may in turn clarify whether a Fixes: tag
> > would actually be appropriate here.
>
> For example, on ARM systems, we bring up vCPUs via PSCI. At the same time,
> from another domain, XEN_DOMCTL_setvcpucontext may be called. In the PSCI
> path, access is protected by domain_lock, but domain_pause alone is not
> sufficient to prevent races during modification of the vCPU context from
> XEN_DOMCTL_setvcpucontext.

Sorry, we don't need any extra locking here, because domain_pause
performs vCPU blocking synchronously. It waits until the vCPU becomes
non-runnable, and only after that the context is updated.

So this patch isn't needed.

>
> >
> > Jan
> >
> > > --- a/xen/common/domctl.c
> > > +++ b/xen/common/domctl.c
> > > @@ -392,7 +392,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> > > u_domctl)
> > >          if ( ret == 0 )
> > >          {
> > >              domain_pause(d);
> > > +            domain_lock(d);
> > >              ret = arch_set_info_guest(v, c);
> > > +            domain_unlock(d);
> > >              domain_unpause(d);
> > >
> > >              if ( ret == -ERESTART )
> >
>
> Best regards,
> Mykola