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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On 6/10/26 9:35 AM, Roger Pau Monné wrote:
> On Tue, Jun 09, 2026 at 04:15:28PM +0100, 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().
> You could join both into a single domain_{,un}pause() bullet point.
>
> > Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> Reviewed-by: Roger Pau Monné <roger.pau@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;
> Newline, and I would use break rather than fallthrough, if further
> cases are added below you don't what to fallthrough, and there's
> nothing to do in the default case anyway.
Yes, not sure what I was thinking here. break makes far more sense.

Can this adjustment be done when committing?

> See for example how this is similar to XEN_DOMCTL_vm_event_op which
> also handles some sub-ops without a lock and uses a break instead of a
> fallthrough.
>
> FWIW, I would also put the XEN_DOMCTL_shadow_op case after
> XEN_DOMCTL_get_device_group and ahead of the
> XEN_DOMCTL_ioport_permission block, but that's just my taste.
I don't have a preference here, either is fine.

Thanks,
Ross