[PATCH 0/3] x86/shadow: tidy VRAM tracking a little

[PATCH 0/3] x86/shadow: tidy VRAM tracking a little

[Jan Beulich]

While dealing with Roger's comments on "x86/mm: split struct sh_dirty_vram and
make results private" I spotted more aspects which may want changing.

1: unlock P2M slightly earlier in shadow_track_dirty_vram()
2: VRAM last_dirty tagging
3: reduce flush_tlb's scope in shadow_track_dirty_vram()

Jan

[PATCH 1/3] x86/shadow: unlock P2M slightly earlier in shadow_track_dirty_vram()

[Jan Beulich]

There's no need to call vfree() with the lock still held.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
In fact for the purposes of the function the P2M lock could really be
obtained merely in read mode, and it could be dropped immediately in both
the main "if()" and its "else if()". If only there wasn't the error
handling after copy_to_guest(): Dropping the paging lock ahead of that
call, we rely solely on the P2M lock to also guard the changing of
d->arch.hvm.dirty_vram.sh and what it points to. Question is why dropping
the paging lock (but continuing to hold the P2M lock) is necessary there
in the first place.

--- a/xen/arch/x86/mm/shadow/hvm.c
+++ b/xen/arch/x86/mm/shadow/hvm.c
@@ -1219,8 +1219,8 @@ int shadow_track_dirty_vram(struct domai
         paging_unlock(d);
         rc = -EFAULT;
     }
-    vfree(dirty_bitmap);
     p2m_unlock(p2m_get_hostp2m(d));
+    vfree(dirty_bitmap);
     return rc;
 }
 

Re: [PATCH 1/3] x86/shadow: unlock P2M slightly earlier in shadow_track_dirty_vram()

[Roger Pau Monné]

On Tue, Feb 03, 2026 at 05:49:35PM +0100, Jan Beulich wrote:
> There's no need to call vfree() with the lock still held.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

> ---
> In fact for the purposes of the function the P2M lock could really be
> obtained merely in read mode, and it could be dropped immediately in both
> the main "if()" and its "else if()". If only there wasn't the error
> handling after copy_to_guest(): Dropping the paging lock ahead of that
> call, we rely solely on the P2M lock to also guard the changing of
> d->arch.hvm.dirty_vram.sh and what it points to. Question is why dropping
> the paging lock (but continuing to hold the P2M lock) is necessary there
> in the first place.

I wouldn't spend a lot of time trying to optimize this, we already
know HVM shadow is ATM not very optimized, and generally recommend
HAP.

If we could turn the p2m lock into read-mode, maybe at the expense of
expanding the paging locked region that would likely be slightly better?

Thanks, Roger.

Re: [PATCH 1/3] x86/shadow: unlock P2M slightly earlier in shadow_track_dirty_vram()

[Jan Beulich]

On 27.04.2026 10:47, Roger Pau Monné wrote:
> On Tue, Feb 03, 2026 at 05:49:35PM +0100, Jan Beulich wrote:
>> There's no need to call vfree() with the lock still held.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> In fact for the purposes of the function the P2M lock could really be
>> obtained merely in read mode, and it could be dropped immediately in both
>> the main "if()" and its "else if()". If only there wasn't the error
>> handling after copy_to_guest(): Dropping the paging lock ahead of that
>> call, we rely solely on the P2M lock to also guard the changing of
>> d->arch.hvm.dirty_vram.sh and what it points to. Question is why dropping
>> the paging lock (but continuing to hold the P2M lock) is necessary there
>> in the first place.
> 
> I wouldn't spend a lot of time trying to optimize this, we already
> know HVM shadow is ATM not very optimized, and generally recommend
> HAP.
> 
> If we could turn the p2m lock into read-mode, maybe at the expense of
> expanding the paging locked region that would likely be slightly better?

I fear holding the paging lock across copy_to_guest() is a no-go, as a
pagefault there could invoke shadow code. Thinking about it, that lock
is a recursive one, so it may be possible to leverage that. Would
require making sure that all code paths potentially involved in #PF
handling would also use the recursive locking form.

Jan

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

[Jan Beulich]

->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
  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@suse.com>

--- 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;
+
         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);
-        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();
         paging_unlock(d);
         rc = -EFAULT;
     }

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

[Roger Pau Monné]

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@suse.com>
> 
> --- 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.

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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@citrix.com>

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

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

[Jan Beulich]

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@citrix.com>

Thanks.

Jan

[PATCH 3/3] x86/shadow: reduce flush_tlb's scope in shadow_track_dirty_vram()

[Jan Beulich]

It's set only in the main "else", so the declaration as well as the sole
consumer can also move into that more narrow scope. This may in particular
help with possible future locking changes.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/hvm.c
+++ b/xen/arch/x86/mm/shadow/hvm.c
@@ -1022,7 +1022,6 @@ int shadow_track_dirty_vram(struct domai
     int rc = 0;
     unsigned long end_pfn = begin_pfn + nr_frames;
     unsigned int dirty_size = DIV_ROUND_UP(nr_frames, BITS_PER_BYTE);
-    int flush_tlb = 0;
     unsigned long i;
     p2m_type_t t;
     struct sh_dirty_vram *dirty_vram;
@@ -1097,7 +1096,7 @@ int shadow_track_dirty_vram(struct domai
     {
         mfn_t map_mfn = INVALID_MFN;
         void *map_sl1p = NULL;
-        bool any_dirty = false;
+        bool any_dirty = false, flush_tlb = false;
         s_time_t now;
 
         /* Iterate over VRAM to track dirty bits. */
@@ -1158,7 +1157,7 @@ int shadow_track_dirty_vram(struct domai
                              * _PAGE_ACCESSED set by another processor.
                              */
                             l1e_remove_flags(*sl1e, _PAGE_DIRTY);
-                            flush_tlb = 1;
+                            flush_tlb = true;
                         }
                     }
                     break;
@@ -1201,9 +1200,10 @@ int shadow_track_dirty_vram(struct domai
             }
             dirty_vram->last_dirty = -1;
         }
+
+        if ( flush_tlb )
+            guest_flush_tlb_mask(d, d->dirty_cpumask);
     }
-    if ( flush_tlb )
-        guest_flush_tlb_mask(d, d->dirty_cpumask);
     goto out;
 
  out_sl1ma:

Re: [PATCH 3/3] x86/shadow: reduce flush_tlb's scope in shadow_track_dirty_vram()

[Roger Pau Monné]

On Tue, Feb 03, 2026 at 05:50:31PM +0100, Jan Beulich wrote:
> It's set only in the main "else", so the declaration as well as the sole
> consumer can also move into that more narrow scope. This may in particular
> help with possible future locking changes.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Thanks, Roger.