Re: [PATCH] x86: Add Kconfig option for log-dirty tracking

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

On Mon, Feb 09, 2026 at 11:31:15AM +0100, Alejandro Vallejo wrote:
> Creates a CONFIG_LOG_DIRTY Kconfig option with the following effects
> when disabled:
> 
>   * paging_domctl{,_cont} return -EOPNOTSUPP (XEN_DOMCTL_shadow_op).
>   * VRAM tracking via DMOP returns EOPNOTSUPP.
> 
> And compiles out all log-dirty tracking infra.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
> ---
> RFC for the Kconfig help message.
> ---
>  xen/arch/x86/Kconfig              |  9 +++++++++
>  xen/arch/x86/domctl.c             |  8 ++++----
>  xen/arch/x86/hvm/dm.c             |  3 +++
>  xen/arch/x86/include/asm/domain.h |  2 ++
>  xen/arch/x86/include/asm/p2m.h    |  2 ++
>  xen/arch/x86/include/asm/paging.h |  8 +++-----
>  xen/arch/x86/mm/hap/hap.c         | 22 +++++++++++++---------
>  xen/arch/x86/mm/p2m.c             |  2 ++
>  xen/arch/x86/mm/paging.c          |  2 ++
>  xen/include/hypercall-defs.c      |  4 ++--
>  10 files changed, 42 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 61f58aa829..fbf044aa4d 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -146,6 +146,7 @@ config XEN_IBT
>  config SHADOW_PAGING
>  	bool "Shadow Paging"
>  	default !PV_SHIM_EXCLUSIVE
> +	select LOG_DIRTY

I think I'm missing something, why this dependency of shadow on log
dirty?  Isn't HAP equally dependent on it?

Oh, I see, You adjust HAP code to deal with the !CONFIG_LOG_DIRTY
scenario, but not the SHADOW counterpart, that still has a hard
dependency on LOG_DIRTY being enabled.

>  	depends on PV || HVM
>  	help
>  
> @@ -166,6 +167,14 @@ config SHADOW_PAGING
>  config PAGING
>  	def_bool HVM || SHADOW_PAGING
>  
> +config LOG_DIRTY
> +	bool "Log-dirty page tracking" if EXPERT
> +	depends on PAGING
> +	default !PV_SHIM_EXCLUSIVE
> +	help
> +	  Enable log-dirty infrastructure so Xen can track domain memory writes and
> +	  the dirty state of VRAM for device models and live migrations.
> +
>  config BIGMEM
>  	bool "big memory support"
>  	default n
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index d9521808dc..61d43a21d0 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -220,15 +220,15 @@ long arch_do_domctl(
>      {
>  
>      case XEN_DOMCTL_shadow_op:
> -#ifdef CONFIG_PAGING
> +        ret = -EOPNOTSUPP;
> +        if ( !IS_ENABLED(CONFIG_LOG_DIRTY) )
> +            break;
> +
>          ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0);
>          if ( ret == -ERESTART )
>              return hypercall_create_continuation(
>                         __HYPERVISOR_paging_domctl_cont, "h", u_domctl);
>          copyback = true;
> -#else
> -        ret = -EOPNOTSUPP;
> -#endif
>          break;
>  
>      case XEN_DOMCTL_ioport_permission:
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index 3b53471af0..94216aecc2 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -48,6 +48,9 @@ static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
>                              unsigned int nr_frames,
>                              const struct xen_dm_op_buf *buf)
>  {
> +    if ( !IS_ENABLED(CONFIG_LOG_DIRTY) )
> +        return -EOPNOTSUPP;
> +
>      if ( nr_frames > (GB(1) >> PAGE_SHIFT) )
>          return -EINVAL;
>  
> diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
> index 94b0cf7f1d..f09c13909f 100644
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -226,7 +226,9 @@ struct paging_domain {
>      unsigned int            p2m_pages;    /* number of pages allocated to p2m */
>  
>      /* log dirty support */
> +#ifdef CONFIG_LOG_DIRTY
>      struct log_dirty_domain log_dirty;
> +#endif /* CONFIG_LOG_DIRTY */

Can also ifdef out the declaration of the log_dirty_domain struct, and
just provide a forward pointer declaration for it?  Would that satisfy
the paging_log_dirty_init() prototype?  Hm, I guess that won't work
with the usage in hap_domain_init().

>      /* preemption handling */
>      struct {
> diff --git a/xen/arch/x86/include/asm/p2m.h b/xen/arch/x86/include/asm/p2m.h
> index 9016e88411..3c2dcacfa5 100644
> --- a/xen/arch/x86/include/asm/p2m.h
> +++ b/xen/arch/x86/include/asm/p2m.h
> @@ -253,9 +253,11 @@ struct p2m_domain {
>                                      bool *sve);
>      int                (*recalc)(struct p2m_domain *p2m,
>                                   unsigned long gfn);
> +#ifdef CONFIG_LOG_DIRTY
>      void               (*enable_hardware_log_dirty)(struct p2m_domain *p2m);
>      void               (*disable_hardware_log_dirty)(struct p2m_domain *p2m);
>      void               (*flush_hardware_cached_dirty)(struct p2m_domain *p2m);
> +#endif /* CONFIG_LOG_DIRTY */

Will this build when VMX is enabled?  I think it's missing an
adjustment to ept_p2m_init() to not initialize those fields if
!CONFIG_LOG_DIRTY.

>      void               (*change_entry_type_global)(struct p2m_domain *p2m,
>                                                     p2m_type_t ot,
>                                                     p2m_type_t nt);
> diff --git a/xen/arch/x86/include/asm/paging.h b/xen/arch/x86/include/asm/paging.h
> index 291ab386e8..980cdfa455 100644
> --- a/xen/arch/x86/include/asm/paging.h
> +++ b/xen/arch/x86/include/asm/paging.h
> @@ -55,12 +55,9 @@
>  #define PG_translate   0
>  #define PG_external    0
>  #endif
> -#if defined(CONFIG_PAGING) && !defined(CONFIG_PV_SHIM_EXCLUSIVE)
>  /* Enable log dirty mode */
> -#define PG_log_dirty   (XEN_DOMCTL_SHADOW_ENABLE_LOG_DIRTY << PG_mode_shift)
> -#else
> -#define PG_log_dirty   0
> -#endif
> +#define PG_log_dirty   IS_ENABLED(CONFIG_LOG_DIRTY) * \
> +                       (XEN_DOMCTL_SHADOW_ENABLE_LOG_DIRTY << PG_mode_shift)

You want an extra set of parentheses around this define I think, just
in case.

Quite a lot of the code is also protected using #if PG_log_dirty.  I
think if you introduce a CONFIG_LOG_DIRTY it would best to also guard
the logdirty code using that new define, so it's consistent (iow:
replace `#if PG_log_dirty` and similar usages).

>  
>  /* All paging modes. */
>  #define PG_MASK (PG_refcounts | PG_log_dirty | PG_translate | PG_external)
> @@ -174,6 +171,7 @@ static inline void paging_log_dirty_init(struct domain *d,
>                                           const struct log_dirty_ops *ops) {}
>  static inline void paging_mark_dirty(struct domain *d, mfn_t gmfn) {}
>  static inline void paging_mark_pfn_dirty(struct domain *d, pfn_t pfn) {}
> +static inline void paging_mark_pfn_clean(struct domain *d, pfn_t pfn) {}
>  static inline bool paging_mfn_is_dirty(struct domain *d, mfn_t gmfn) { return false; }
>  
>  #endif /* PG_log_dirty */
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index a337752bf4..21672db011 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -50,7 +50,7 @@ struct hap_dirty_vram {
>   * calling p2m_log_dirty_range(), which interrogates each vram
>   * page's p2m type looking for pages that have been made writable.
>   */
> -
> +#ifdef CONFIG_LOG_DIRTY

Nit: I would leave the newline as-is.

>  int hap_track_dirty_vram(struct domain *d,
>                           unsigned long begin_pfn,
>                           unsigned int nr_frames,
> @@ -161,6 +161,7 @@ out:
>  
>      return rc;
>  }
> +#endif /* CONFIG_LOG_DIRTY */
>  
>  /************************************************/
>  /*            HAP LOG DIRTY SUPPORT             */
> @@ -440,14 +441,17 @@ static bool cf_check flush_tlb(const unsigned long *vcpu_bitmap);
>  
>  void hap_domain_init(struct domain *d)
>  {
> -    static const struct log_dirty_ops hap_ops = {
> -        .enable  = hap_enable_log_dirty,
> -        .disable = hap_disable_log_dirty,
> -        .clean   = hap_clean_dirty_bitmap,
> -    };
> -
> -    /* Use HAP logdirty mechanism. */
> -    paging_log_dirty_init(d, &hap_ops);
> +    if ( IS_ENABLED(CONFIG_LOG_DIRTY) )
> +    {
> +        static const struct log_dirty_ops hap_ops = {
> +            .enable  = hap_enable_log_dirty,
> +            .disable = hap_disable_log_dirty,
> +            .clean   = hap_clean_dirty_bitmap,
> +        };
> +
> +        /* Use HAP logdirty mechanism. */
> +        paging_log_dirty_init(d, &hap_ops);
> +    }
>  
>      d->arch.paging.update_paging_modes = hap_update_paging_modes;
>      d->arch.paging.flush_tlb           = flush_tlb;
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index e915da26a8..373382c28c 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -236,6 +236,7 @@ struct ioreq_server *p2m_get_ioreq_server(struct domain *d,
>      return s;
>  }
>  
> +#ifdef CONFIG_LOG_DIRTY
>  void p2m_enable_hardware_log_dirty(struct domain *d)
>  {
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
> @@ -263,6 +264,7 @@ void p2m_flush_hardware_cached_dirty(struct domain *d)
>          p2m_unlock(p2m);
>      }
>  }
> +#endif /* CONFIG_LOG_DIRTY */
>  
>  /*
>   * Force a synchronous P2M TLB flush if a deferred flush is pending.
> diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
> index 2396f81ad5..13ee137db9 100644
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -623,10 +623,12 @@ int paging_domain_init(struct domain *d)
>      INIT_PAGE_LIST_HEAD(&d->arch.paging.freelist);
>      mm_lock_init(&d->arch.paging.lock);
>  
> +#ifdef CONFIG_LOG_DIRTY
>      /* This must be initialized separately from the rest of the
>       * log-dirty init code as that can be called more than once and we
>       * don't want to leak any active log-dirty bitmaps */
>      d->arch.paging.log_dirty.top = INVALID_MFN;
> +#endif /* CONFIG_LOG_DIRTY */

Could you possibly init this field from paging_log_dirty_init()?  As
to avoid having more ifdef churn in paging_domain_init().

Thanks, Roger.