Re: [Xen-devel] [PATCH RFC v2 2/3] xen/shadow: fix shadow_track_dirty_vram to work on hvm guests

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/04/15 11:26, Roger Pau Monne wrote:
> Modify shadow_track_dirty_vram to use a local buffer and then flush to the
> guest without the paging_lock held. This is modeled after
> hap_track_dirty_vram.
>
> Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, subject to two
corrections and a suggestion.

> ---
>  xen/arch/x86/mm/shadow/common.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
> index 2e43d6d..8fff43a 100644
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -3516,7 +3516,7 @@ static void sh_clean_dirty_bitmap(struct domain *d)
>  int shadow_track_dirty_vram(struct domain *d,
>                              unsigned long begin_pfn,
>                              unsigned long nr,
> -                            XEN_GUEST_HANDLE_64(uint8) dirty_bitmap)
> +                            XEN_GUEST_HANDLE_64(uint8) guest_dirty_bitmap)
>  {
>      int rc;
>      unsigned long end_pfn = begin_pfn + nr;
> @@ -3526,6 +3526,7 @@ int shadow_track_dirty_vram(struct domain *d,
>      p2m_type_t t;
>      struct sh_dirty_vram *dirty_vram;
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    uint8_t *dirty_bitmap = NULL;
>  
>      if ( end_pfn < begin_pfn || end_pfn > p2m->max_mapped_pfn + 1 )
>          return -EINVAL;
> @@ -3554,6 +3555,12 @@ int shadow_track_dirty_vram(struct domain *d,
>          goto out;
>      }
>  
> +    dirty_bitmap = xzalloc_bytes(dirty_size);
> +    if ( dirty_bitmap == NULL )
> +    {
> +        rc = -ENOMEM;
> +        goto out;
> +    }
>      /* This should happen seldomly (Video mode change),
>       * no need to be careful. */
>      if ( !dirty_vram )
> @@ -3587,7 +3594,7 @@ int shadow_track_dirty_vram(struct domain *d,
>      {
>          /* still completely clean, just copy our empty bitmap */
>          rc = -EFAULT;
> -        if ( copy_to_guest(dirty_bitmap, dirty_vram->dirty_bitmap, 
> dirty_size) == 0 )
> +        if ( memcpy(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size) != 
> NULL )

memcpy() returns an int, not a pointer.  You should use != 0

>              rc = 0;
>      }
>      else
> @@ -3669,7 +3676,8 @@ int shadow_track_dirty_vram(struct domain *d,
>              sh_unmap_domain_page(map_sl1p);
>  
>          rc = -EFAULT;
> -        if ( copy_to_guest(dirty_bitmap, dirty_vram->dirty_bitmap, 
> dirty_size) == 0 ) {
> +        if ( memcpy(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size) != 
> NULL )
> +        {

And here.

>              memset(dirty_vram->dirty_bitmap, 0, dirty_size);
>              if (dirty_vram->last_dirty + SECONDS(2) < NOW())
>              {
> @@ -3697,6 +3705,10 @@ out_dirty_vram:
>  
>  out:
>      paging_unlock(d);
> +    if ( rc == 0 && dirty_bitmap != NULL )
> +        if ( copy_to_guest(guest_dirty_bitmap, dirty_bitmap, dirty_size) != 
> 0 )
> +            rc = -EFAULT;

These two if()s can be joined, and shorted for brevity.

if ( !rc && dirty_bitmap && copy_to_guest(guest_dirty_bitmap,
dirty_bitmap, dirty_size) )
    rc = -EFAULT;

~Andrew

> +    xfree(dirty_bitmap);
>      p2m_unlock(p2m_get_hostp2m(d));
>      return rc;
>  }


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel