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

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

On Mon, May 04, 2026 at 10:39:53AM +0200, Jan Beulich wrote:
> On 27.04.2026 11:28, Roger Pau Monné wrote:
> > On Tue, Feb 03, 2026 at 05:49:55PM +0100, Jan Beulich wrote:
> >> --- 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?
> 
> That's not how I understand the field is used. Aiui it identifies "was
> clean for more than 2 seconds". That's not the case here. Hence the
> setting to -1 only conditionally a few lines down from here.

Hm, OK, it seems like a very complicated way to signal this.  Won't it
be easier to unconditionally store the last write time in
->last_dirty, and let the consumer decide whether it's been more than
2s or not?

Maybe you could write a comment next to the field in the struct
declaration?

Either way:

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> >> @@ -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?
> 
> There was (and is) no unconditional setting of ->last_dirty. Technically
> maybe a Fixes: tag might be appropriate, but this is an error path which
> should never be taken (assuming a well behaved DM). Do you think I should
> dig out the offending commit?

I'm not specially fuzzed about backporting this, I think it's fine to
go in without a Fixes tag (and then no backport).

Thanks, Roger.