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

["Roger Pau Monné" <roger.pau@citrix.com>]

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@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
>  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.

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.

Thanks, Roger.