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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>> @@ -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?

Jan