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 09/04/15 13:41, Tim Deegan wrote:
> 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@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.
> No, memcpy() returns a pointer, but it's always == its first argument
> so there's no need to check it at all. :)

D'oh - I had mixed up memcmp() and memcpy().

You are completely correct.

~Andrew

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