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

[Alejandro Vallejo <alejandro.garciavallejo@amd.com>]

On Mon Feb 9, 2026 at 3:48 PM CET, Jan Beulich wrote:
> On 09.02.2026 11:31, Alejandro Vallejo wrote:
>> --- 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
>>  	depends on PV || HVM
>>  	help
>
> Why would this be? IOW why would shadow imply log-dirty, but HAP wouldn't?

The logic is rather opaque. I admit I'm a bit fuzzy on the uses of logdirty.

I know what it's for and I could navigate the code if a problem arose, but I'm
less clear about which other elements of the hypervisor rely on it (pod? nsvm?
vvmx? shadow? hap?).

If it's strictly toolstack/DM-driven maybe it'd be more apt to have a separate
LIVE_MIGRATION and SAVE_RESTORE configs where LM selects SAVE_RESTORE, which
selects LOG_DIRTY. That's also improve some defaults auto-downgraded from the
max policy just in case a VM is migrated.

>
>> @@ -166,6 +167,14 @@ config SHADOW_PAGING
>>  config PAGING
>>  	def_bool HVM || SHADOW_PAGING
>>  
>> +config LOG_DIRTY
>
> PAGING_LOG_DIRTY?

sure.

>
>> --- 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;
>
> Can a HVM-only hypervisor create any guests with this? I simply fail to
> see how XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION would then make it through to
> hap_domctl().

xl doesn't seem to call it at all. hap_set_allocation() is implicitly called
through paging_enable() -> hap_enable() -> hap_set_allocation()

>
>> --- 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 */
>
> Such an #ifdef can likely replace the comment? Or else the comment would
> better also live inside the #ifdef?

true.

>
>> --- 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)
>
> Need wrapping in parentheses then.

true.

>
>> --- 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
>
> This wants to move further up.

sure

>
>> --- a/xen/include/hypercall-defs.c
>> +++ b/xen/include/hypercall-defs.c
>> @@ -194,7 +194,7 @@ dm_op(domid_t domid, unsigned int nr_bufs, xen_dm_op_buf_t *bufs)
>>  #ifdef CONFIG_SYSCTL
>>  sysctl(xen_sysctl_t *u_sysctl)
>>  #endif
>> -#if defined(CONFIG_X86) && defined(CONFIG_PAGING) && !defined(CONFIG_PV_SHIM_EXCLUSIVE)
>> +#if defined(CONFIG_LOG_DIRTY)
>>  paging_domctl_cont(xen_domctl_t *u_domctl)
>>  #endif
>>  #ifndef CONFIG_PV_SHIM_EXCLUSIVE
>> @@ -292,7 +292,7 @@ dm_op                              compat   do       compat   do       do
>>  hypfs_op                           do       do       do       do       do
>>  #endif
>>  mca                                do       do       -        -        -
>> -#if defined(CONFIG_X86) && defined(CONFIG_PAGING) && !defined(CONFIG_PV_SHIM_EXCLUSIVE)
>> +#if defined(CONFIG_LOG_DIRTY)
>>  paging_domctl_cont                 do       do       do       do       -
>>  #endif
>
> The CONFIG_X86 part of the checking wants to remain: Another port may also gain
> a setting of this name, without necessarily having this auxiliary hypercall.

Hmmm. Makes sense.

>
> Jan