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

[Tim Deegan <tim@xen.org>]

At 20:06 +0100 on 02 Apr (1428005197), Andrew Cooper wrote:
> 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@citrix.com>
> > Cc: Tim Deegan <tim@xen.org>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, 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.

No, memcpy() returns a pointer, but it's always == its first argument
so there's no need to check it at all. :)

> > +    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;

+1.

Cheers,

Tim.