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

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

[Roger Pau Monne]

Patch series has not changed substantially since v2, I've addressed both 
Andrew's and Tim's comments.

Thanks, Roger.

[PATCH v3 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>
---
Once migration v2 has been merged this patch can be reverted, since it
removes the need to use the MMU_MACHPHYS_UPDATE hypercall.
---
 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 bfde380..d3494ed 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 v3 1/3] xen/pvh: enable mmu_update hypercall

[Jan Beulich]

>>> On 10.04.15 at 19:29, <roger.pau@citrix.com> wrote:
> 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>
> ---
> Once migration v2 has been merged this patch can be reverted, since it
> removes the need to use the MMU_MACHPHYS_UPDATE hypercall.

Didn't earlier discussion end with the request to limit PVH access to
just this one sub-op?

Jan

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

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

[Roger Pau Monné]

El 14/04/15 a les 13.55, Jan Beulich ha escrit:
>>>> On 10.04.15 at 19:29, <roger.pau@citrix.com> wrote:
>> 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>
>> ---
>> Once migration v2 has been merged this patch can be reverted, since it
>> removes the need to use the MMU_MACHPHYS_UPDATE hypercall.
> 
> Didn't earlier discussion end with the request to limit PVH access to
> just this one sub-op?

My bad, from the last conversation I got the feeling that the other
sub-ops already had the needed checks so it was fine to enable them. I
know the checks are there, and using the other sub-ops from a PVH guest
should be fine, but I guess it's better to just enable what we really need.

Roger.


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

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

[Andrew Cooper]

On 14/04/15 13:03, Roger Pau Monné wrote:
> El 14/04/15 a les 13.55, Jan Beulich ha escrit:
>>>>> On 10.04.15 at 19:29, <roger.pau@citrix.com> wrote:
>>> 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>
>>> ---
>>> Once migration v2 has been merged this patch can be reverted, since it
>>> removes the need to use the MMU_MACHPHYS_UPDATE hypercall.
>> Didn't earlier discussion end with the request to limit PVH access to
>> just this one sub-op?
> My bad, from the last conversation I got the feeling that the other
> sub-ops already had the needed checks so it was fine to enable them. I
> know the checks are there, and using the other sub-ops from a PVH guest
> should be fine, but I guess it's better to just enable what we really need.

I would be in favour of dropping this patch entirely, as there is
agreement in principle from the toolstack maintainers to include
migration v2 in its current state.

PVH is still very much experimental, and I don't this it warrants the
risk of exposing mmu_update() to non-PV guests, especially as we know it
wont be needed in the near future.

~Andrew

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

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

[Tim Deegan]

At 14:10 +0100 on 14 Apr (1429020603), Andrew Cooper wrote:
> On 14/04/15 13:03, Roger Pau Monné wrote:
> > El 14/04/15 a les 13.55, Jan Beulich ha escrit:
> >>>>> On 10.04.15 at 19:29, <roger.pau@citrix.com> wrote:
> >>> 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>
> >>> ---
> >>> Once migration v2 has been merged this patch can be reverted, since it
> >>> removes the need to use the MMU_MACHPHYS_UPDATE hypercall.
> >> Didn't earlier discussion end with the request to limit PVH access to
> >> just this one sub-op?
> > My bad, from the last conversation I got the feeling that the other
> > sub-ops already had the needed checks so it was fine to enable them. I
> > know the checks are there, and using the other sub-ops from a PVH guest
> > should be fine, but I guess it's better to just enable what we really need.
> 
> I would be in favour of dropping this patch entirely, as there is
> agreement in principle from the toolstack maintainers to include
> migration v2 in its current state.

+1.

Tim.

[PATCH v3 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>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v2:
 - Remove checks against memcpy, it always returns the address passed as
   dst.
 - Join the nested ifs in the out path.
 - Add Andrew Cooper Reviewed-by.
---
 xen/arch/x86/mm/shadow/common.c | 44 ++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 2e43d6d..ae5855d 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 )
@@ -3584,12 +3591,8 @@ int shadow_track_dirty_vram(struct domain *d,
         rc = -ENODATA;
     }
     else if (dirty_vram->last_dirty == -1)
-    {
         /* still completely clean, just copy our empty bitmap */
-        rc = -EFAULT;
-        if ( copy_to_guest(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size) == 0 )
-            rc = 0;
-    }
+        memcpy(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size);
     else
     {
         unsigned long map_mfn = INVALID_MFN;
@@ -3668,21 +3671,18 @@ int shadow_track_dirty_vram(struct domain *d,
         if ( map_sl1p )
             sh_unmap_domain_page(map_sl1p);
 
-        rc = -EFAULT;
-        if ( copy_to_guest(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size) == 0 ) {
-            memset(dirty_vram->dirty_bitmap, 0, dirty_size);
-            if (dirty_vram->last_dirty + SECONDS(2) < NOW())
-            {
-                /* was clean for more than two seconds, try to disable guest
-                 * write access */
-                for ( i = begin_pfn; i < end_pfn; i++ ) {
-                    mfn_t mfn = get_gfn_query_unlocked(d, i, &t);
-                    if (mfn_x(mfn) != INVALID_MFN)
-                        flush_tlb |= sh_remove_write_access(d, mfn, 1, 0);
-                }
-                dirty_vram->last_dirty = -1;
+        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())
+        {
+            /* was clean for more than two seconds, try to disable guest
+             * write access */
+            for ( i = begin_pfn; i < end_pfn; i++ ) {
+                mfn_t mfn = get_gfn_query_unlocked(d, i, &t);
+                if (mfn_x(mfn) != INVALID_MFN)
+                    flush_tlb |= sh_remove_write_access(d, mfn, 1, 0);
             }
-            rc = 0;
+            dirty_vram->last_dirty = -1;
         }
     }
     if ( flush_tlb )
@@ -3697,6 +3697,10 @@ out_dirty_vram:
 
 out:
     paging_unlock(d);
+    if ( rc == 0 && dirty_bitmap != NULL &&
+         copy_to_guest(guest_dirty_bitmap, dirty_bitmap, dirty_size) )
+        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 v3 2/3] xen/shadow: fix shadow_track_dirty_vram to work on hvm guests

[Jan Beulich]

>>> On 10.04.15 at 19:29, <roger.pau@citrix.com> 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.

And hence introduces the same issue: The HVMOP_track_dirty_vram
handler explicitly allows for up to 1Gb of VRAM, yet here you
effectively limit things to 128Mb (one page worth of bits each taking
care of one guest page) considering heavily fragmented memory.

Apart from that the patch would need cleaning up for coding style.

Jan

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

[Roger Pau Monné]

El 14/04/15 a les 14.22, Jan Beulich ha escrit:
>>>> On 10.04.15 at 19:29, <roger.pau@citrix.com> 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.
> 
> And hence introduces the same issue: The HVMOP_track_dirty_vram
> handler explicitly allows for up to 1Gb of VRAM, yet here you
> effectively limit things to 128Mb (one page worth of bits each taking
> care of one guest page) considering heavily fragmented memory.

Where does this limitation come from? We allocate a temporary bitmap
that has enough size to accommodate for the number of entries passed to
the function in nr.

I guess there's some limitation I'm not seeing.

Roger.

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

[Jan Beulich]

>>> On 06.05.15 at 17:12, <roger.pau@citrix.com> wrote:
> El 14/04/15 a les 14.22, Jan Beulich ha escrit:
>>>>> On 10.04.15 at 19:29, <roger.pau@citrix.com> 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.
>> 
>> And hence introduces the same issue: The HVMOP_track_dirty_vram
>> handler explicitly allows for up to 1Gb of VRAM, yet here you
>> effectively limit things to 128Mb (one page worth of bits each taking
>> care of one guest page) considering heavily fragmented memory.
> 
> Where does this limitation come from? We allocate a temporary bitmap
> that has enough size to accommodate for the number of entries passed to
> the function in nr.
> 
> I guess there's some limitation I'm not seeing.

You allocate a buffer via xzalloc(). When memory is heavily
fragmented, the largest contiguous chunk you can expect to be
able to successfully allocate is 1 page. And 1 page is worth
128Mb of tracked guest address space (4k * 8 bits each covering
a 4k guest page).

Jan

[PATCH v3 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>
---
Changes since v2:
 - Add checks for p2m_is_ram and p2m_is_discard_write when mapping a guest
   page.
 - Remove error checking from memset/memcpy, they unconditionally return
   dst.
---
 xen/arch/x86/mm/paging.c | 103 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 91 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index b54d76a..cb5feb8 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_ram(p2mt) )
+    {
+        put_page(*page);
+        return NULL;
+    }
+    if ( p2m_is_paging(p2mt) )
+    {
+        put_page(*page);
+        p2m_mem_paging_populate(current->domain, gfn);
+        return NULL;
+    }
+    if ( p2m_is_shared(p2mt) || p2m_is_discard_write(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,15 +534,29 @@ 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 + PAGE_SHIFT) !=
+                         index_mapped >> (3 + PAGE_SHIFT) )
                     {
-                        rv = -EFAULT;
-                        goto out;
+                        /* 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);
+                    else
+                        memset(dirty_bitmap + ((pages >> 3) % PAGE_SIZE), 0,
+                               bytes);
                 }
                 pages += bytes << 3;
                 if ( l1 )
@@ -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 v3 3/3] xen: rework paging_log_dirty_op to work with hvm guests

[Roger Pau Monné]

El 10/04/15 a les 19.29, Roger Pau Monne ha escrit:
> 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>
> ---
> Changes since v2:
>  - Add checks for p2m_is_ram and p2m_is_discard_write when mapping a guest
>    page.
>  - Remove error checking from memset/memcpy, they unconditionally return
>    dst.
> ---
>  xen/arch/x86/mm/paging.c | 103 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 91 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
> index b54d76a..cb5feb8 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;

This should be:

uint32_t pfec = PFEC_page_present | PFEC_write_access;

Will wait for other's comments before resending the series.

Roger.

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

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

[Jan Beulich]

>>> On 10.04.15 at 19:29, <roger.pau@citrix.com> wrote:
> --- 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)),

I'd suggest dropping the parentheses around the inner cast - they
make this more difficult to read, and I think any C programmer
should know that unary ops have precedence over binary ones.

> +                            &pfec);
> +    if ( gfn == INVALID_GFN )
> +        return NULL;
> +
> +    *page = get_page_from_gfn(current->domain, gfn, &p2mt, P2M_UNSHARE);
> +
> +    if ( !p2m_is_ram(p2mt) )
> +    {
> +        put_page(*page);
> +        return NULL;
> +    }
> +    if ( p2m_is_paging(p2mt) )
> +    {
> +        put_page(*page);
> +        p2m_mem_paging_populate(current->domain, gfn);
> +        return NULL;
> +    }
> +    if ( p2m_is_shared(p2mt) || p2m_is_discard_write(p2mt) )
> +    {
> +        put_page(*page);
> +        return NULL;
> +    }
> +
> +    *mapped_page = pages;

You only ever store the passed in value of "pages" into
"*mapped_pages" - what's the point of this? If the caller needs
to track the value it passes here, it should simple make a copy
itself if so needed.

Apart from that both parameter names don't really seem to
express their purpose.

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

Pointless initializer.

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

Blank line above that one please.

> @@ -471,15 +534,29 @@ 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 + PAGE_SHIFT) !=
> +                         index_mapped >> (3 + PAGE_SHIFT) )
>                      {
> -                        rv = -EFAULT;
> -                        goto out;
> +                        /* 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;

This won't work: The paging lock protects all of
d->arch.paging.preempt.log_dirty, of which you hold cached values
in local variables.

> +                    BUG_ON(((pages >> 3) % PAGE_SIZE) + bytes > PAGE_SIZE);

I don't seem to be able to spot the original for this one. If there
was none, please make this an ASSERT() instead.

> +                    if ( l1 )
> +                        memcpy(dirty_bitmap + ((pages >> 3) % PAGE_SIZE),
> +                               (uint8_t *)l1, bytes);

Pointless cast.

Jan

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

[Tim Deegan]

At 13:14 +0100 on 14 Apr (1429017287), Jan Beulich wrote:
> > This won't work: The paging lock protects all of
> > d->arch.paging.preempt.log_dirty, of which you hold cached values
> > in local variables.
> 
> ... so how about reusing the existing restart code instead?  We could
> either move the label further up or tail-recurse into it. 
> 
> Tim.

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

[Jan Beulich]

>>> On 16.04.15 at 11:11, <tim@xen.org> wrote:
> At 13:14 +0100 on 14 Apr (1429017287), Jan Beulich wrote:
>> > This won't work: The paging lock protects all of
>> > d->arch.paging.preempt.log_dirty, of which you hold cached values
>> > in local variables.
>> 
>> ... so how about reusing the existing restart code instead?  We could
>> either move the label further up or tail-recurse into it. 

Possibly - I didn't look too closely though at how feasible this would be.

Jan

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

[Roger Pau Monné]

Hello,

I think I've fixed the rest of the comments, this one however is not
easy to deal with.

El 14/04/15 a les 14.14, Jan Beulich ha escrit:
>>>> On 10.04.15 at 19:29, <roger.pau@citrix.com> wrote:
>> +                    BUG_ON(((pages >> 3) % PAGE_SIZE) + bytes > PAGE_SIZE);
> 
> I don't seem to be able to spot the original for this one. If there
> was none, please make this an ASSERT() instead.

Yes, there's no previous BUG_ON because this was not a problem in the
past, since we could write to any position on dirty_bitmap, but that's
not the case any more. Since we only have one page mapped at a time we
need to make sure that what we are about to write doesn't cross a page
boundary.

I understand this is not an ideal solution, but AFAICT there's no easy
way to deal with writes that expand over a page boundary.

Roger.

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

[Jan Beulich]

>>> On 06.05.15 at 13:55, <roger.pau@citrix.com> wrote:
> El 14/04/15 a les 14.14, Jan Beulich ha escrit:
>>>>> On 10.04.15 at 19:29, <roger.pau@citrix.com> wrote:
>>> +                    BUG_ON(((pages >> 3) % PAGE_SIZE) + bytes > PAGE_SIZE);
>> 
>> I don't seem to be able to spot the original for this one. If there
>> was none, please make this an ASSERT() instead.
> 
> Yes, there's no previous BUG_ON because this was not a problem in the
> past, since we could write to any position on dirty_bitmap, but that's
> not the case any more. Since we only have one page mapped at a time we
> need to make sure that what we are about to write doesn't cross a page
> boundary.
> 
> I understand this is not an ideal solution, but AFAICT there's no easy
> way to deal with writes that expand over a page boundary.

I'm afraid I don't really see what you're asking for. Just to clarify -
I didn't put anything under question, all I asked for was to use
ASSERT() instead of BUG_ON() here. Yet what you wrote above
doesn't seem to related to that request.

Jan

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

[Roger Pau Monné]

El 06/05/15 a les 14.10, Jan Beulich ha escrit:
>>>> On 06.05.15 at 13:55, <roger.pau@citrix.com> wrote:
>> El 14/04/15 a les 14.14, Jan Beulich ha escrit:
>>>>>> On 10.04.15 at 19:29, <roger.pau@citrix.com> wrote:
>>>> +                    BUG_ON(((pages >> 3) % PAGE_SIZE) + bytes > PAGE_SIZE);
>>>
>>> I don't seem to be able to spot the original for this one. If there
>>> was none, please make this an ASSERT() instead.
>>
>> Yes, there's no previous BUG_ON because this was not a problem in the
>> past, since we could write to any position on dirty_bitmap, but that's
>> not the case any more. Since we only have one page mapped at a time we
>> need to make sure that what we are about to write doesn't cross a page
>> boundary.
>>
>> I understand this is not an ideal solution, but AFAICT there's no easy
>> way to deal with writes that expand over a page boundary.
> 
> I'm afraid I don't really see what you're asking for. Just to clarify -
> I didn't put anything under question, all I asked for was to use
> ASSERT() instead of BUG_ON() here. Yet what you wrote above
> doesn't seem to related to that request.

I don't think an ASSERT is appropriate here, because it means that on
non-debug versions we might write past the end of the mapped page
without noticing.

Roger.

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

[Jan Beulich]

>>> On 06.05.15 at 14:32, <roger.pau@citrix.com> wrote:
> El 06/05/15 a les 14.10, Jan Beulich ha escrit:
>>>>> On 06.05.15 at 13:55, <roger.pau@citrix.com> wrote:
>>> El 14/04/15 a les 14.14, Jan Beulich ha escrit:
>>>>>>> On 10.04.15 at 19:29, <roger.pau@citrix.com> wrote:
>>>>> +                    BUG_ON(((pages >> 3) % PAGE_SIZE) + bytes > PAGE_SIZE);
>>>>
>>>> I don't seem to be able to spot the original for this one. If there
>>>> was none, please make this an ASSERT() instead.
>>>
>>> Yes, there's no previous BUG_ON because this was not a problem in the
>>> past, since we could write to any position on dirty_bitmap, but that's
>>> not the case any more. Since we only have one page mapped at a time we
>>> need to make sure that what we are about to write doesn't cross a page
>>> boundary.
>>>
>>> I understand this is not an ideal solution, but AFAICT there's no easy
>>> way to deal with writes that expand over a page boundary.
>> 
>> I'm afraid I don't really see what you're asking for. Just to clarify -
>> I didn't put anything under question, all I asked for was to use
>> ASSERT() instead of BUG_ON() here. Yet what you wrote above
>> doesn't seem to related to that request.
> 
> I don't think an ASSERT is appropriate here, because it means that on
> non-debug versions we might write past the end of the mapped page
> without noticing.

With that argumentation there shouldn't be any ASSERT()s, but only
BUG_ON()s.

Jan