Re: [PATCH v1 2/2] domctl: Handle some of XEN_DOMCTL_shadow_op without the domctl lock

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 6/11/26 10:20 AM, Roger Pau Monné wrote:
> On Thu, Jun 11, 2026 at 09:18:15AM -0400, Daniel P. Smith wrote:
> > On 6/9/26 11:15 AM, Ross Lagerwall wrote:
> > > Handle XEN_DOMCTL_SHADOW_OP_{CLEAN,PEEK} without taking the domctl lock.
> > > This is safe because for these subops, the paging lock is mostly held
> > > which prevents it from operating concurrently on the same domain. There
> > > are some parts that are called without the paging lock held:
> > >
> > > * hvm_mapped_guest_frames_mark_dirty() - The function itself takes a
> > >     spinlock so is protected from concurrent calls. In any case, it will
> > >     mark all the pages dirty as required.
> > >
> > > * domain_pause() - The toolstack cannot unpause the domain while in
> > >     paging_log_dirty_op() because the toolstack's pause/unpause ops have
> > >     a separate ref count.
> > >
> > > * p2m_flush_hardware_cached_dirty() - This is called elsewhere without
> > >     the domctl lock held so holding it wouldn't achieve anything. It
> > >     should be fine as long as it is called at least once.
> > >
> > > * log_dirty.ops->clean() - If the callback is hap_clean_dirty_bitmap(),
> > >     then it will hold the p2m lock while modifying the table. If the
> > >     callback is sh_clean_dirty_bitmap(), it will hold the paging lock
> > >     while modifying the table. In both cases, this is OK.
> > >
> > > * domain_unpause() - Same as the earlier domain_pause().
> > Please add a comment that that xsm check is to continue protecting the
> > sub-ops with XS_PRIV.
> >
> >
> > > Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> > > ---
> > >    xen/arch/x86/mm/paging.c |  8 ++++++--
> > >    xen/common/domctl.c      | 12 ++++++++++++
> > >    2 files changed, 18 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
> > > index 1a5822808620..bfb5b423a0dd 100644
> > > --- a/xen/arch/x86/mm/paging.c
> > > +++ b/xen/arch/x86/mm/paging.c
> > > @@ -746,11 +746,15 @@ long do_paging_domctl_cont(
> > >        ret = xsm_domctl(XSM_OTHER, d, &op);
> > >        if ( !ret )
> > >        {
> > > -        if ( domctl_lock_acquire() )
> > > +        bool lock = !(op.u.shadow_op.op == XEN_DOMCTL_SHADOW_OP_CLEAN ||
> > > +                      op.u.shadow_op.op == XEN_DOMCTL_SHADOW_OP_PEEK);
> > > +
> > > +        if ( !lock || domctl_lock_acquire() )
> > >            {
> > >                ret = paging_domctl(d, &op.u.shadow_op, u_domctl, 1);
> > > -            domctl_lock_release();
> > > +            if ( lock )
> > > +                domctl_lock_release();
> > >            }
> > >            else
> > >                ret = -ERESTART;
> > > diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> > > index 35144d95b808..a3888c4e87d4 100644
> > > --- a/xen/common/domctl.c
> > > +++ b/xen/common/domctl.c
> > > @@ -559,6 +559,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> > > u_domctl)
> > >            ret = arch_do_domctl(op, d, u_domctl);
> > >            goto domctl_out_unlock_domonly;
> > > +    case XEN_DOMCTL_shadow_op:
> > > +        if ( op->u.shadow_op.op == XEN_DOMCTL_SHADOW_OP_CLEAN ||
> > > +             op->u.shadow_op.op == XEN_DOMCTL_SHADOW_OP_PEEK )
> > > +        {
> > > +            ret = xsm_domctl(XSM_OTHER, d, op);
> > > +            if ( ret )
> > > +                goto domctl_out_unlock_domonly;
> > > +
> > > +            ret = arch_do_domctl(op, d, u_domctl);
> > > +            goto domctl_out_unlock_domonly;
> > > +        }
> > > +        fallthrough;
> > >        default:
> > >            /* Everything else handled further down. */
> > >            break;
> > After commit message change,
> >
> > Acked-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> Sorry, this was already picked up in a rush to get it into 4.22 and I
> didn't realize it was missing an XSM maintainer Ack.  That's entirely
> my fault, there was no intention to bypass or overrule your opinion.
I fully understand and take no offense.


> Given it's already committed, and there are no objections aside from
> the commit message adjustment my preference would be to leave it
> alone.
Hmm, I saw patch 1 but not patch 2. Sorry for the extra noise.

v/r,
dps