[PATCH RFC v2 0/3] xen/pvh: enable migration on PVH Dom0

[PATCH RFC v2 0/3] xen/pvh: enable migration on PVH Dom0

[Roger Pau Monne]

This series enables migration of guests (either PV or HVM) from a PVH Dom0.

Since the last round a new patch has been added (2/3) in order to fix
shadow_track_dirty_vram when called from a hvm guest. Patch 3/3 has also
been reworked according to Tim's comments in order to map the dirty_bitmap
destination on demand without the paging lock being held.

Thanks, Roger.

[PATCH RFC v2 1/3] xen/pvh: enable mmu_update hypercall

[Roger Pau Monne]

This is needed for performing save/restore of PV guests.

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>
---
 xen/arch/x86/hvm/hvm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 3ff87c6..11680ba 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4907,6 +4907,7 @@ static hvm_hypercall_t *const pvh_hypercall64_table[NR_hypercalls] = {
     [ __HYPERVISOR_grant_table_op ]  = (hvm_hypercall_t *)hvm_grant_table_op,
     HYPERCALL(vcpu_op),
     HYPERCALL(mmuext_op),
+    HYPERCALL(mmu_update),
     HYPERCALL(xsm_op),
     HYPERCALL(sched_op),
     HYPERCALL(event_channel_op),
-- 
1.9.5 (Apple Git-50.3)


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

Re: [PATCH RFC v2 1/3] xen/pvh: enable mmu_update hypercall

[Ian Campbell]

On Thu, 2015-04-02 at 12:26 +0200, Roger Pau Monne wrote:
> This is needed for performing save/restore of PV guests.

It's quite a big interface though, isn't it?

Could we restrict it to a subset of the operations perhaps? Or at least
justify here how it has been audited and found to be safe to allow an
HVM guest this access.

> 
> 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>
> ---
>  xen/arch/x86/hvm/hvm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 3ff87c6..11680ba 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4907,6 +4907,7 @@ static hvm_hypercall_t *const pvh_hypercall64_table[NR_hypercalls] = {
>      [ __HYPERVISOR_grant_table_op ]  = (hvm_hypercall_t *)hvm_grant_table_op,
>      HYPERCALL(vcpu_op),
>      HYPERCALL(mmuext_op),
> +    HYPERCALL(mmu_update),
>      HYPERCALL(xsm_op),
>      HYPERCALL(sched_op),
>      HYPERCALL(event_channel_op),



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

Re: [PATCH RFC v2 1/3] xen/pvh: enable mmu_update hypercall

[Roger Pau Monné]

El 02/04/15 a les 12.42, Ian Campbell ha escrit:
> On Thu, 2015-04-02 at 12:26 +0200, Roger Pau Monne wrote:
>> This is needed for performing save/restore of PV guests.
> 
> It's quite a big interface though, isn't it?

AFAICT it contains MMU_NORMAL_PT_UPDATE, MMU_PT_UPDATE_PRESERVE_AD and
MMU_MACHPHYS_UPDATE.

> Could we restrict it to a subset of the operations perhaps? Or at least
> justify here how it has been audited and found to be safe to allow an
> HVM guest this access.

XSA-109 should have fixed all issues with this operations. IIRC only
MMU_MACHPHYS_UPDATE is needed for save/restore of PV guests, but I will
have to check. If that's the case, I could restrict PVH domains to only
have access to that operation.

Roger.

Re: [PATCH RFC v2 1/3] xen/pvh: enable mmu_update hypercall

[Andrew Cooper]

On 02/04/15 11:42, Ian Campbell wrote:
> On Thu, 2015-04-02 at 12:26 +0200, Roger Pau Monne wrote:
>> This is needed for performing save/restore of PV guests.
> It's quite a big interface though, isn't it?
>
> Could we restrict it to a subset of the operations perhaps? Or at least
> justify here how it has been audited and found to be safe to allow an
> HVM guest this access.

It isn't actually very big, but does have quite a lot of PV knowledge
built in.

I would be happer with the safety of this patch if
v->arch.old_guest_table got moved into the pv union, to make the code
much clearer that it is specifically for PV guests.

If I recall, this change only needed for MMU_MACHPHYS_UPDATE against a
foreign domain.  Each of the 3 subops does check for
paging_mode_translate/refcounts() of the target, which does prevent the
hypercall being made against a non-PV domains.  From that point of view,
it should be safe for HVM guests to use, as it is the target domain,
rather than the source domain, which is important. 


However, with migration v2 dropping support for 2-level PV guests (which
died with the 32bit hypervisor build), I believe I have removed all need
for MMU_MACHPHYS_UPDATE hypercalls entirely (unless there are some done
behind the scenes from the kernel).

~Andrew

Re: [PATCH RFC v2 1/3] xen/pvh: enable mmu_update hypercall

[Jürgen Groß]

On 04/02/2015 01:50 PM, Andrew Cooper wrote:
> On 02/04/15 11:42, Ian Campbell wrote:
>> On Thu, 2015-04-02 at 12:26 +0200, Roger Pau Monne wrote:
>>> This is needed for performing save/restore of PV guests.
>> It's quite a big interface though, isn't it?
>>
>> Could we restrict it to a subset of the operations perhaps? Or at least
>> justify here how it has been audited and found to be safe to allow an
>> HVM guest this access.
>
> It isn't actually very big, but does have quite a lot of PV knowledge
> built in.
>
> I would be happer with the safety of this patch if
> v->arch.old_guest_table got moved into the pv union, to make the code
> much clearer that it is specifically for PV guests.
>
> If I recall, this change only needed for MMU_MACHPHYS_UPDATE against a
> foreign domain.  Each of the 3 subops does check for
> paging_mode_translate/refcounts() of the target, which does prevent the
> hypercall being made against a non-PV domains.  From that point of view,
> it should be safe for HVM guests to use, as it is the target domain,
> rather than the source domain, which is important.
>
>
> However, with migration v2 dropping support for 2-level PV guests (which
> died with the 32bit hypervisor build), I believe I have removed all need
> for MMU_MACHPHYS_UPDATE hypercalls entirely (unless there are some done
> behind the scenes from the kernel).

There is one usage in the kernel in file arch/x86/xen/setup.c targeting
DOMID_SELF.


Juergen

Re: [PATCH RFC v2 1/3] xen/pvh: enable mmu_update hypercall

[Andrew Cooper]

On 02/04/15 13:43, Jürgen Groß wrote:
> On 04/02/2015 01:50 PM, Andrew Cooper wrote:
>> On 02/04/15 11:42, Ian Campbell wrote:
>>> On Thu, 2015-04-02 at 12:26 +0200, Roger Pau Monne wrote:
>>>> This is needed for performing save/restore of PV guests.
>>> It's quite a big interface though, isn't it?
>>>
>>> Could we restrict it to a subset of the operations perhaps? Or at least
>>> justify here how it has been audited and found to be safe to allow an
>>> HVM guest this access.
>>
>> It isn't actually very big, but does have quite a lot of PV knowledge
>> built in.
>>
>> I would be happer with the safety of this patch if
>> v->arch.old_guest_table got moved into the pv union, to make the code
>> much clearer that it is specifically for PV guests.
>>
>> If I recall, this change only needed for MMU_MACHPHYS_UPDATE against a
>> foreign domain.  Each of the 3 subops does check for
>> paging_mode_translate/refcounts() of the target, which does prevent the
>> hypercall being made against a non-PV domains.  From that point of view,
>> it should be safe for HVM guests to use, as it is the target domain,
>> rather than the source domain, which is important.
>>
>>
>> However, with migration v2 dropping support for 2-level PV guests (which
>> died with the 32bit hypervisor build), I believe I have removed all need
>> for MMU_MACHPHYS_UPDATE hypercalls entirely (unless there are some done
>> behind the scenes from the kernel).
>
> There is one usage in the kernel in file arch/x86/xen/setup.c targeting
> DOMID_SELF.

Right, but that looks to be a codepath which is only used in PV guests.

All that matters (from the point of view of this patch) is whether any
toolstack actions result in the issue of mmu_update hypercalls.

If migration v2 has removed the need for any MMU_MACHPHYS_UPDATE ops
(which I believe it has), then Xen need not expose the do_mmu_update()
hypercall to non-PV guests.

~Andrew

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

[Roger Pau Monne]

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>
---
 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 )
             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 )
+        {
             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;
+    xfree(dirty_bitmap);
     p2m_unlock(p2m_get_hostp2m(d));
     return rc;
 }
-- 
1.9.5 (Apple Git-50.3)


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

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

[Andrew Cooper]

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.  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@lists.xen.org
http://lists.xen.org/xen-devel

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

[Tim Deegan]

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.

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

[Andrew Cooper]

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

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

You are completely correct.

~Andrew

[PATCH RFC v2 3/3] xen: rework paging_log_dirty_op to work with hvm guests

[Roger Pau Monne]

When the caller of paging_log_dirty_op is a hvm guest Xen would choke when
trying to copy the dirty bitmap to the guest because the paging lock is
already held.

Fix this by independently mapping each page of the guest bitmap as needed
without the paging lock held.

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>
---
 xen/arch/x86/mm/paging.c | 99 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 89 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index b54d76a..4dcf942 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -397,6 +397,53 @@ int paging_mfn_is_dirty(struct domain *d, mfn_t gmfn)
     return rv;
 }
 
+static inline void *map_dirty_bitmap(XEN_GUEST_HANDLE_64(uint8) dirty_bitmap,
+                                     unsigned long pages,
+                                     struct page_info **page,
+                                     unsigned long *mapped_page)
+{
+    p2m_type_t p2mt;
+    uint32_t pfec;
+    unsigned long gfn;
+
+    gfn = paging_gva_to_gfn(current,
+                            (paddr_t)(((char *)dirty_bitmap.p) + (pages >> 3)),
+                            &pfec);
+    if ( gfn == INVALID_GFN )
+        return NULL;
+
+    *page = get_page_from_gfn(current->domain, gfn, &p2mt, P2M_UNSHARE);
+
+    if ( p2m_is_paging(p2mt) )
+    {
+        put_page(*page);
+        p2m_mem_paging_populate(current->domain, gfn);
+        return NULL;
+    }
+    if ( p2m_is_shared(p2mt) )
+    {
+        put_page(*page);
+        return NULL;
+    }
+    if ( p2m_is_grant(p2mt) )
+    {
+        put_page(*page);
+        return NULL;
+    }
+
+    *mapped_page = pages;
+    return __map_domain_page(*page);
+}
+
+static inline void unmap_dirty_bitmap(void *addr, struct page_info *page)
+{
+    if ( addr != NULL )
+    {
+        unmap_domain_page(addr);
+        put_page(page);
+    }
+}
+
 
 /* Read a domain's log-dirty bitmap and stats.  If the operation is a CLEAN,
  * clear the bitmap and stats as well. */
@@ -409,9 +456,23 @@ static int paging_log_dirty_op(struct domain *d,
     mfn_t *l4 = NULL, *l3 = NULL, *l2 = NULL;
     unsigned long *l1 = NULL;
     int i4, i3, i2;
+    uint8_t *dirty_bitmap = NULL;
+    struct page_info *page;
+    unsigned long index_mapped = 0;
 
     if ( !resuming )
         domain_pause(d);
+
+    dirty_bitmap = map_dirty_bitmap(sc->dirty_bitmap,
+                                    resuming ?
+                                        d->arch.paging.preempt.log_dirty.done :
+                                        0,
+                                    &page, &index_mapped);
+    if ( dirty_bitmap == NULL )
+    {
+        domain_unpause(d);
+        return -EFAULT;
+    }
     paging_lock(d);
 
     if ( !d->arch.paging.preempt.dom )
@@ -448,21 +509,23 @@ static int paging_log_dirty_op(struct domain *d,
         goto out;
     }
 
-    l4 = paging_map_log_dirty_bitmap(d);
     i4 = d->arch.paging.preempt.log_dirty.i4;
     i3 = d->arch.paging.preempt.log_dirty.i3;
+    i2 = 0;
     pages = d->arch.paging.preempt.log_dirty.done;
 
+ again:
+    l4 = paging_map_log_dirty_bitmap(d);
+
     for ( ; (pages < sc->pages) && (i4 < LOGDIRTY_NODE_ENTRIES); i4++, i3 = 0 )
     {
         l3 = (l4 && mfn_valid(l4[i4])) ? map_domain_page(mfn_x(l4[i4])) : NULL;
-        for ( ; (pages < sc->pages) && (i3 < LOGDIRTY_NODE_ENTRIES); i3++ )
+        for ( ; (pages < sc->pages) && (i3 < LOGDIRTY_NODE_ENTRIES);
+             i3++, i2 = 0 )
         {
             l2 = ((l3 && mfn_valid(l3[i3])) ?
                   map_domain_page(mfn_x(l3[i3])) : NULL);
-            for ( i2 = 0;
-                  (pages < sc->pages) && (i2 < LOGDIRTY_NODE_ENTRIES);
-                  i2++ )
+            for ( ; (pages < sc->pages) && (i2 < LOGDIRTY_NODE_ENTRIES); i2++ )
             {
                 unsigned int bytes = PAGE_SIZE;
                 l1 = ((l2 && mfn_valid(l2[i2])) ?
@@ -471,11 +534,25 @@ static int paging_log_dirty_op(struct domain *d,
                     bytes = (unsigned int)((sc->pages - pages + 7) >> 3);
                 if ( likely(peek) )
                 {
-                    if ( (l1 ? copy_to_guest_offset(sc->dirty_bitmap,
-                                                    pages >> 3, (uint8_t *)l1,
-                                                    bytes)
-                             : clear_guest_offset(sc->dirty_bitmap,
-                                                  pages >> 3, bytes)) != 0 )
+                    if ( (pages >> 3) >= (index_mapped >> 3) + 4096 ) {
+                        /* We need to map next page */
+                        paging_unlock(d);
+                        unmap_dirty_bitmap(dirty_bitmap, page);
+                        dirty_bitmap = map_dirty_bitmap(sc->dirty_bitmap, pages,
+                                                        &page, &index_mapped);
+                        paging_lock(d);
+                        if ( dirty_bitmap == NULL )
+                        {
+                            rv = -EFAULT;
+                            goto out;
+                        }
+                        goto again;
+                    }
+                    BUG_ON(((pages >> 3) % PAGE_SIZE) + bytes > PAGE_SIZE);
+                    if ( (l1 ? memcpy(dirty_bitmap + ((pages >> 3) % PAGE_SIZE),
+                                      (uint8_t *)l1, bytes)
+                             : memset(dirty_bitmap + ((pages >> 3) % PAGE_SIZE),
+                                      0, bytes)) == NULL )
                     {
                         rv = -EFAULT;
                         goto out;
@@ -549,12 +626,14 @@ static int paging_log_dirty_op(struct domain *d,
          * paging modes (shadow or hap).  Safe because the domain is paused. */
         d->arch.paging.log_dirty.clean_dirty_bitmap(d);
     }
+    unmap_dirty_bitmap(dirty_bitmap, page);
     domain_unpause(d);
     return rv;
 
  out:
     d->arch.paging.preempt.dom = NULL;
     paging_unlock(d);
+    unmap_dirty_bitmap(dirty_bitmap, page);
     domain_unpause(d);
 
     if ( l1 )
-- 
1.9.5 (Apple Git-50.3)


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

Re: [PATCH RFC v2 3/3] xen: rework paging_log_dirty_op to work with hvm guests

[Andrew Cooper]

On 02/04/15 11:26, Roger Pau Monne wrote:
> When the caller of paging_log_dirty_op is a hvm guest Xen would choke when
> trying to copy the dirty bitmap to the guest because the paging lock is
> already held.

Are you sure? Presumably you get an mm lock ordering violation, because
paging_log_dirty_op() should take the target domains paging lock, rather
than your own (which is prohibited by the current check at the top of
paging_domctl()).

Unfortunately, dropping the paging_lock() here is unsafe, as it will
result in corruption of the logdirty bitmap from non-domain sources such
as HVMOP_modified_memory.

I will need to find some time with a large pot of coffee and a
whiteboard, but I suspect it might actually be safe to alter the current
mm_lock() enforcement to maintain independent levels for a source and
destination domain.

Up until now, the toolstack domain has always been PV (with very little
in the way of locking), and I don't believe our current locking model is
suitable for an HVM domain performing toolstack operations on another,
where both the source and destination need locking.

~Andrew

>
> Fix this by independently mapping each page of the guest bitmap as needed
> without the paging lock held.
>
> 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>
> ---
>  xen/arch/x86/mm/paging.c | 99 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 89 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
> index b54d76a..4dcf942 100644
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -397,6 +397,53 @@ int paging_mfn_is_dirty(struct domain *d, mfn_t gmfn)
>      return rv;
>  }
>  
> +static inline void *map_dirty_bitmap(XEN_GUEST_HANDLE_64(uint8) dirty_bitmap,
> +                                     unsigned long pages,
> +                                     struct page_info **page,
> +                                     unsigned long *mapped_page)
> +{
> +    p2m_type_t p2mt;
> +    uint32_t pfec;
> +    unsigned long gfn;
> +
> +    gfn = paging_gva_to_gfn(current,
> +                            (paddr_t)(((char *)dirty_bitmap.p) + (pages >> 3)),
> +                            &pfec);
> +    if ( gfn == INVALID_GFN )
> +        return NULL;
> +
> +    *page = get_page_from_gfn(current->domain, gfn, &p2mt, P2M_UNSHARE);
> +
> +    if ( p2m_is_paging(p2mt) )
> +    {
> +        put_page(*page);
> +        p2m_mem_paging_populate(current->domain, gfn);
> +        return NULL;
> +    }
> +    if ( p2m_is_shared(p2mt) )
> +    {
> +        put_page(*page);
> +        return NULL;
> +    }
> +    if ( p2m_is_grant(p2mt) )
> +    {
> +        put_page(*page);
> +        return NULL;
> +    }
> +
> +    *mapped_page = pages;
> +    return __map_domain_page(*page);
> +}
> +
> +static inline void unmap_dirty_bitmap(void *addr, struct page_info *page)
> +{
> +    if ( addr != NULL )
> +    {
> +        unmap_domain_page(addr);
> +        put_page(page);
> +    }
> +}
> +
>  
>  /* Read a domain's log-dirty bitmap and stats.  If the operation is a CLEAN,
>   * clear the bitmap and stats as well. */
> @@ -409,9 +456,23 @@ static int paging_log_dirty_op(struct domain *d,
>      mfn_t *l4 = NULL, *l3 = NULL, *l2 = NULL;
>      unsigned long *l1 = NULL;
>      int i4, i3, i2;
> +    uint8_t *dirty_bitmap = NULL;
> +    struct page_info *page;
> +    unsigned long index_mapped = 0;
>  
>      if ( !resuming )
>          domain_pause(d);
> +
> +    dirty_bitmap = map_dirty_bitmap(sc->dirty_bitmap,
> +                                    resuming ?
> +                                        d->arch.paging.preempt.log_dirty.done :
> +                                        0,
> +                                    &page, &index_mapped);
> +    if ( dirty_bitmap == NULL )
> +    {
> +        domain_unpause(d);
> +        return -EFAULT;
> +    }
>      paging_lock(d);
>  
>      if ( !d->arch.paging.preempt.dom )
> @@ -448,21 +509,23 @@ static int paging_log_dirty_op(struct domain *d,
>          goto out;
>      }
>  
> -    l4 = paging_map_log_dirty_bitmap(d);
>      i4 = d->arch.paging.preempt.log_dirty.i4;
>      i3 = d->arch.paging.preempt.log_dirty.i3;
> +    i2 = 0;
>      pages = d->arch.paging.preempt.log_dirty.done;
>  
> + again:
> +    l4 = paging_map_log_dirty_bitmap(d);
> +
>      for ( ; (pages < sc->pages) && (i4 < LOGDIRTY_NODE_ENTRIES); i4++, i3 = 0 )
>      {
>          l3 = (l4 && mfn_valid(l4[i4])) ? map_domain_page(mfn_x(l4[i4])) : NULL;
> -        for ( ; (pages < sc->pages) && (i3 < LOGDIRTY_NODE_ENTRIES); i3++ )
> +        for ( ; (pages < sc->pages) && (i3 < LOGDIRTY_NODE_ENTRIES);
> +             i3++, i2 = 0 )
>          {
>              l2 = ((l3 && mfn_valid(l3[i3])) ?
>                    map_domain_page(mfn_x(l3[i3])) : NULL);
> -            for ( i2 = 0;
> -                  (pages < sc->pages) && (i2 < LOGDIRTY_NODE_ENTRIES);
> -                  i2++ )
> +            for ( ; (pages < sc->pages) && (i2 < LOGDIRTY_NODE_ENTRIES); i2++ )
>              {
>                  unsigned int bytes = PAGE_SIZE;
>                  l1 = ((l2 && mfn_valid(l2[i2])) ?
> @@ -471,11 +534,25 @@ static int paging_log_dirty_op(struct domain *d,
>                      bytes = (unsigned int)((sc->pages - pages + 7) >> 3);
>                  if ( likely(peek) )
>                  {
> -                    if ( (l1 ? copy_to_guest_offset(sc->dirty_bitmap,
> -                                                    pages >> 3, (uint8_t *)l1,
> -                                                    bytes)
> -                             : clear_guest_offset(sc->dirty_bitmap,
> -                                                  pages >> 3, bytes)) != 0 )
> +                    if ( (pages >> 3) >= (index_mapped >> 3) + 4096 ) {
> +                        /* We need to map next page */
> +                        paging_unlock(d);
> +                        unmap_dirty_bitmap(dirty_bitmap, page);
> +                        dirty_bitmap = map_dirty_bitmap(sc->dirty_bitmap, pages,
> +                                                        &page, &index_mapped);
> +                        paging_lock(d);
> +                        if ( dirty_bitmap == NULL )
> +                        {
> +                            rv = -EFAULT;
> +                            goto out;
> +                        }
> +                        goto again;
> +                    }
> +                    BUG_ON(((pages >> 3) % PAGE_SIZE) + bytes > PAGE_SIZE);
> +                    if ( (l1 ? memcpy(dirty_bitmap + ((pages >> 3) % PAGE_SIZE),
> +                                      (uint8_t *)l1, bytes)
> +                             : memset(dirty_bitmap + ((pages >> 3) % PAGE_SIZE),
> +                                      0, bytes)) == NULL )
>                      {
>                          rv = -EFAULT;
>                          goto out;
> @@ -549,12 +626,14 @@ static int paging_log_dirty_op(struct domain *d,
>           * paging modes (shadow or hap).  Safe because the domain is paused. */
>          d->arch.paging.log_dirty.clean_dirty_bitmap(d);
>      }
> +    unmap_dirty_bitmap(dirty_bitmap, page);
>      domain_unpause(d);
>      return rv;
>  
>   out:
>      d->arch.paging.preempt.dom = NULL;
>      paging_unlock(d);
> +    unmap_dirty_bitmap(dirty_bitmap, page);
>      domain_unpause(d);
>  
>      if ( l1 )


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

Re: [PATCH RFC v2 3/3] xen: rework paging_log_dirty_op to work with hvm guests

[Tim Deegan]

Hi,

At 20:46 +0100 on 02 Apr (1428007593), Andrew Cooper wrote:
> On 02/04/15 11:26, Roger Pau Monne wrote:
> > When the caller of paging_log_dirty_op is a hvm guest Xen would choke when
> > trying to copy the dirty bitmap to the guest because the paging lock is
> > already held.
> 
> Are you sure? Presumably you get an mm lock ordering violation, because
> paging_log_dirty_op() should take the target domains paging lock, rather
> than your own (which is prohibited by the current check at the top of
> paging_domctl()).
> 
> Unfortunately, dropping the paging_lock() here is unsafe, as it will
> result in corruption of the logdirty bitmap from non-domain sources such
> as HVMOP_modified_memory.
> 
> I will need to find some time with a large pot of coffee and a
> whiteboard, but I suspect it might actually be safe to alter the current
> mm_lock() enforcement to maintain independent levels for a source and
> destination domain.

We discussed this in an earlier thread and agreed it would be better
to try to do this work in batches rather than add more complexity to
the mm locking rules.  (I'm AFK this week so I haven't had a chance to
review the actual pacth yet.)

> Up until now, the toolstack domain has always been PV (with very little
> in the way of locking), and I don't believe our current locking model is
> suitable for an HVM domain performing toolstack operations on another,
> where both the source and destination need locking.

AFAICT this is the only place in the hypervisor where a hypercall
copies data back with target domain's paging locks held.

Tim.

Re: [PATCH RFC v2 3/3] xen: rework paging_log_dirty_op to work with hvm guests

[Roger Pau Monné]

Hello,

El 03/04/15 a les 16.12, Tim Deegan ha escrit:
> Hi,
> 
> At 20:46 +0100 on 02 Apr (1428007593), Andrew Cooper wrote:
>> On 02/04/15 11:26, Roger Pau Monne wrote:
>>> When the caller of paging_log_dirty_op is a hvm guest Xen would choke when
>>> trying to copy the dirty bitmap to the guest because the paging lock is
>>> already held.
>>
>> Are you sure? Presumably you get an mm lock ordering violation, because
>> paging_log_dirty_op() should take the target domains paging lock, rather
>> than your own (which is prohibited by the current check at the top of
>> paging_domctl()).
>>
>> Unfortunately, dropping the paging_lock() here is unsafe, as it will
>> result in corruption of the logdirty bitmap from non-domain sources such
>> as HVMOP_modified_memory.
>>
>> I will need to find some time with a large pot of coffee and a
>> whiteboard, but I suspect it might actually be safe to alter the current
>> mm_lock() enforcement to maintain independent levels for a source and
>> destination domain.
> 
> We discussed this in an earlier thread and agreed it would be better
> to try to do this work in batches rather than add more complexity to
> the mm locking rules.  (I'm AFK this week so I haven't had a chance to
> review the actual pacth yet.)

I don't know about the locking rules or how much complexity would
permitting this kind of accesses add to it, but IMHO this patch makes
the code quite more complex and possibly error prone, so finding a
simpler approach seems like a good option to me.

Roger.

Re: [PATCH RFC v2 3/3] xen: rework paging_log_dirty_op to work with hvm guests

[Tim Deegan]

At 12:09 +0200 on 07 Apr (1428408556), Roger Pau Monné wrote:
> Hello,
> 
> El 03/04/15 a les 16.12, Tim Deegan ha escrit:
> > Hi,
> > 
> > At 20:46 +0100 on 02 Apr (1428007593), Andrew Cooper wrote:
> >> On 02/04/15 11:26, Roger Pau Monne wrote:
> >>> When the caller of paging_log_dirty_op is a hvm guest Xen would choke when
> >>> trying to copy the dirty bitmap to the guest because the paging lock is
> >>> already held.
> >>
> >> Are you sure? Presumably you get an mm lock ordering violation, because
> >> paging_log_dirty_op() should take the target domains paging lock, rather
> >> than your own (which is prohibited by the current check at the top of
> >> paging_domctl()).
> >>
> >> Unfortunately, dropping the paging_lock() here is unsafe, as it will
> >> result in corruption of the logdirty bitmap from non-domain sources such
> >> as HVMOP_modified_memory.
> >>
> >> I will need to find some time with a large pot of coffee and a
> >> whiteboard, but I suspect it might actually be safe to alter the current
> >> mm_lock() enforcement to maintain independent levels for a source and
> >> destination domain.
> > 
> > We discussed this in an earlier thread and agreed it would be better
> > to try to do this work in batches rather than add more complexity to
> > the mm locking rules.  (I'm AFK this week so I haven't had a chance to
> > review the actual pacth yet.)
> 
> I don't know about the locking rules or how much complexity would
> permitting this kind of accesses add to it, but IMHO this patch makes
> the code quite more complex and possibly error prone, so finding a
> simpler approach seems like a good option to me.

I'm happier with this (relatively contained) complexity than with
adding yet more logic to the mm_locks code.

I don't think there are any new races introduced here that weren't
already present in the -ERESTART case.

Cheers,

Tim.

Re: [PATCH RFC v2 3/3] xen: rework paging_log_dirty_op to work with hvm guests

[Tim Deegan]

Hi,

At 12:26 +0200 on 02 Apr (1427977595), Roger Pau Monne wrote:
> +static inline void *map_dirty_bitmap(XEN_GUEST_HANDLE_64(uint8) dirty_bitmap,
> +                                     unsigned long pages,
> +                                     struct page_info **page,
> +                                     unsigned long *mapped_page)
> +{
> +    p2m_type_t p2mt;
> +    uint32_t pfec;
> +    unsigned long gfn;
> +
> +    gfn = paging_gva_to_gfn(current,
> +                            (paddr_t)(((char *)dirty_bitmap.p) + (pages >> 3)),
> +                            &pfec);
> +    if ( gfn == INVALID_GFN )
> +        return NULL;
> +
> +    *page = get_page_from_gfn(current->domain, gfn, &p2mt, P2M_UNSHARE);
> +
> +    if ( p2m_is_paging(p2mt) )
> +    {
> +        put_page(*page);
> +        p2m_mem_paging_populate(current->domain, gfn);
> +        return NULL;
> +    }
> +    if ( p2m_is_shared(p2mt) )
> +    {
> +        put_page(*page);
> +        return NULL;
> +    }
> +    if ( p2m_is_grant(p2mt) )
> +    {
> +        put_page(*page);
> +        return NULL;
> +    }
> +
> +    *mapped_page = pages;
> +    return __map_domain_page(*page);
> +}

It's a shame to have to copy this logic from __hvm_copy() but I can't
see an easy way of avoiding it.  We might want to wrap it up in case
some other path wants to do the same thing?

But since we are doing these checks, we also need to check that the
destination buffer isn't supposed to be read-only -- __hvm_copy()
uses p2m_is_discard_write() for that.  It would probably be a good
idea to add a check of p2m_is_ram too, just to avoid having to think
about the MMIO cases.

Also, I think the _is_shared() and _is_grant() clauses should be folded
together, since the action taken is the same.

> @@ -471,11 +534,25 @@ static int paging_log_dirty_op(struct domain *d,
>                      bytes = (unsigned int)((sc->pages - pages + 7) >> 3);
>                  if ( likely(peek) )
>                  {
> -                    if ( (l1 ? copy_to_guest_offset(sc->dirty_bitmap,
> -                                                    pages >> 3, (uint8_t *)l1,
> -                                                    bytes)
> -                             : clear_guest_offset(sc->dirty_bitmap,
> -                                                  pages >> 3, bytes)) != 0 )
> +                    if ( (pages >> 3) >= (index_mapped >> 3) + 4096 ) {

This is a bit odd.  Maybe s/4096/PAGE_SIZE/ ?
Or test (pages >> (3 + PAGE_SHIFT) != index_mapped >> (3 + PAGE_SHIFT) ?

> +                        /* We need to map next page */
> +                        paging_unlock(d);

> +                        unmap_dirty_bitmap(dirty_bitmap, page);
> +                        dirty_bitmap = map_dirty_bitmap(sc->dirty_bitmap, pages,
> +                                                        &page, &index_mapped);
> +                        paging_lock(d);
> +                        if ( dirty_bitmap == NULL )
> +                        {
> +                            rv = -EFAULT;
> +                            goto out;
> +                        }
> +                        goto again;
> +                    }
> +                    BUG_ON(((pages >> 3) % PAGE_SIZE) + bytes > PAGE_SIZE);
> +                    if ( (l1 ? memcpy(dirty_bitmap + ((pages >> 3) % PAGE_SIZE),
> +                                      (uint8_t *)l1, bytes)
> +                             : memset(dirty_bitmap + ((pages >> 3) % PAGE_SIZE),
> +                                      0, bytes)) == NULL )

Like in the previous patch, memcpy() and memset() never return errors,
so there's no need to check them here.

Cheers,

Tim.