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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.05.2026 15:54, Roger Pau Monné wrote:
> 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?

Maybe (in a separate patch), but then how I understand the field is used
may still not be quite correct. I.e. by adding a comment I may further
confuse things.

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

Thanks.

Jan