Re: [PATCH 2/3] x86/shadow: VRAM last_dirty tagging

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Feb 03, 2026 at 05:49:55PM +0100, Jan Beulich wrote:
> ->last_dirty holding a valid value (one other than -1) is solely an
> indication of the bitmap being entirely clean. (The opposite isn't true,
> because of _sh_propagate() setting the field to a valid value without
> setting a bit in the bitmap.) As a consequence
> - setting the field to a valid value right after having allocated zero-
>   filled space is pointless,
> - copying the the all empty bitmap to the output array is pointless; with

Double 'the'?

>   the output array also having been allocated zero-filled, not even a
>   memset() is needed there,
> - after restoring bitmap contents when dealing with copy_to_guest() having
>   failed, the field needs setting to a valid value again.
> 
> Furthermore invoking NOW() in perhaps many loop iterations of the main
> loop is wasteful, too. Record whether any bit was set, and record a new
> ->last_dirty only once, after the loop. Then use the same NOW() value also
> for the subsequent check.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/mm/shadow/hvm.c
> +++ b/xen/arch/x86/mm/shadow/hvm.c
> @@ -1087,18 +1087,18 @@ int shadow_track_dirty_vram(struct domai
>          if ( (dirty_vram->dirty_bitmap = xzalloc_array(uint8_t, dirty_size)) 
> == NULL )
>              goto out_sl1ma;
>  
> -        dirty_vram->last_dirty = NOW();
> +        dirty_vram->last_dirty = -1;
>  
>          /* Tell the caller that this time we could not track dirty bits. */
>          rc = -ENODATA;
>      }
> -    else if ( dirty_vram->last_dirty == -1 )
> -        /* still completely clean, just copy our empty bitmap */
> -        memcpy(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size);
> -    else
> +    /* Nothing to do when the bitmap is still completely clean. */
> +    else if ( dirty_vram->last_dirty != -1 )
>      {
>          mfn_t map_mfn = INVALID_MFN;
>          void *map_sl1p = NULL;
> +        bool any_dirty = false;
> +        s_time_t now;
>  
>          /* Iterate over VRAM to track dirty bits. */
>          for ( i = 0; i < nr_frames; i++ )
> @@ -1174,16 +1174,20 @@ int shadow_track_dirty_vram(struct domai
>              if ( dirty )
>              {
>                  dirty_vram->dirty_bitmap[i / 8] |= 1 << (i % 8);
> -                dirty_vram->last_dirty = NOW();
> +                any_dirty = true;
>              }
>          }
>  
> +        now = NOW();
> +        if ( any_dirty )
> +            dirty_vram->last_dirty = now;

I'm a bit confused with the setting of ->last_dirty here ...

> +
>          if ( map_sl1p )
>              unmap_domain_page(map_sl1p);
>  
>          memcpy(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size);
>          memset(dirty_vram->dirty_bitmap, 0, dirty_size);

... as here the bitmap is zeroed, and hence ->last_dirty should be set
to -1?

> -        if ( dirty_vram->last_dirty + SECONDS(2) < NOW() )
> +        if ( dirty_vram->last_dirty + SECONDS(2) < now )
>          {
>              /*
>               * Was clean for more than two seconds, try to disable guest
> @@ -1216,6 +1220,7 @@ int shadow_track_dirty_vram(struct domai
>          paging_lock(d);
>          for ( i = 0; i < dirty_size; i++ )
>              dirty_vram->dirty_bitmap[i] |= dirty_bitmap[i];
> +        dirty_vram->last_dirty = NOW();

I think this is doesn't deserve a 'Fixes:' tag because the setting of
->last_dirty unconditionally to NOW() regardless of whether the bitmap
is zeroed?

Thanks, Roger.