[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 19/25] arm/altp2m: Add altp2m_propagate_change.



Hello Sergej,

On 01/08/16 18:10, Sergej Proskurin wrote:
This commit introduces the function "altp2m_propagate_change" that is
responsible to propagate changes applied to the host's p2m to a specific
or even all altp2m views. In this way, Xen can in-/decrease the guest's
physmem at run-time without leaving the altp2m views with
stalled/invalid entries.

Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
---
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxx>
---
 xen/arch/arm/altp2m.c        | 75 ++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/p2m.c           | 14 +++++++++
 xen/include/asm-arm/altp2m.h |  9 ++++++
 xen/include/asm-arm/p2m.h    |  5 +++
 4 files changed, 103 insertions(+)

diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
index f98fd73..f3c1cff 100644
--- a/xen/arch/arm/altp2m.c
+++ b/xen/arch/arm/altp2m.c
@@ -133,6 +133,81 @@ out:
     return rc;
 }

+static inline void altp2m_reset(struct p2m_domain *p2m)
+{
+    read_lock(&p2m->lock);

Again read_lock does not protect you against concurrent access. Only against someone else update the page table.

This should be p2m_write_lock.

+
+    p2m_flush_table(p2m);
+    p2m_flush_tlb(p2m);

altp2m_reset may be called on a p2m used by a running vCPU. What this code does is:
    1) clearing root page table
    2) free intermediate page table
    3) invalidate the TLB

Until step 3, the other TLBs may contain entries pointing the intermediate page table. But they were freed and could therefore be re-used for another purpose. So step 2 and 3 should be inverted.

I will re-iterate same message as in the previous series. Please have a think about the locking and memory ordering of all this series. I found a lot of race condition and I may have miss someone. If you have a doubt don't hesitate to ask.

+
+    p2m->lowest_mapped_gfn = INVALID_GFN;
+    p2m->max_mapped_gfn = _gfn(0);
+
+    read_unlock(&p2m->lock);
+}
+
+void altp2m_propagate_change(struct domain *d,
+                             gfn_t sgfn,
+                             unsigned long nr,
+                             mfn_t smfn,
+                             uint32_t mask,
+                             p2m_type_t p2mt,
+                             p2m_access_t p2ma)
+{
+    struct p2m_domain *p2m;
+    mfn_t m;
+    unsigned int i;
+    unsigned int reset_count = 0;
+    unsigned int last_reset_idx = ~0;
+
+    if ( !altp2m_active(d) )

This is not safe. d->arch.altp2m_active maybe be turn off just after you read. Maybe you want to protect it with altp2m_lock.

+        return;
+
+    altp2m_lock(d);
+
+    for ( i = 0; i < MAX_ALTP2M; i++ )
+    {
+        if ( d->arch.altp2m_vttbr[i] == INVALID_VTTBR )
+            continue;
+
+        p2m = d->arch.altp2m_p2m[i];
+
+        m = p2m_lookup_attr(p2m, sgfn, NULL, NULL, NULL, NULL);

What is the benefits to check if it already exists in the altp2m?

+
+        /* Check for a dropped page that may impact this altp2m. */
+        if ( (mfn_eq(smfn, INVALID_MFN) || p2mt == p2m_invalid) &&

Why the check to p2mt against p2m_invalid?

+             gfn_x(sgfn) >= gfn_x(p2m->lowest_mapped_gfn) &&
+             gfn_x(sgfn) <= gfn_x(p2m->max_mapped_gfn) )
+        {
+            if ( !reset_count++ )
+            {
+                altp2m_reset(p2m);
+                last_reset_idx = i;
+            }
+            else
+            {
+                /* At least 2 altp2m's impacted, so reset everything. */

So if you remove a 4KB page in more than 2 altp2m, you will flush all the p2m. This sounds really more time consuming (you have to free all the intermediate page table) than removing a single 4KB page.

+                for ( i = 0; i < MAX_ALTP2M; i++ )
+                {
+                    if ( i == last_reset_idx ||
+                         d->arch.altp2m_vttbr[i] == INVALID_VTTBR )
+                        continue;
+
+                    p2m = d->arch.altp2m_p2m[i];
+                    altp2m_reset(p2m);
+                }
+                goto out;
+            }
+        }
+        else if ( !mfn_eq(m, INVALID_MFN) )
+            modify_altp2m_range(d, p2m, sgfn, nr, smfn,
+                                mask, p2mt, p2ma);

I am a bit concerned about this function. We decided to limit the size of the mapping to avoid long running memory operations (see XSA-158).

With this function you multiply up to 10 times the duration of the operation.

Also, what is modify_altp2m_range has failed?


+    }
+
+out:
+    altp2m_unlock(d);
+}
+
 static void altp2m_vcpu_reset(struct vcpu *v)
 {
     struct altp2mvcpu *av = &vcpu_altp2m(v);
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index e0a7f38..31810e6 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -992,6 +992,7 @@ static int apply_p2m_changes(struct domain *d,
     const bool_t preempt = !is_idle_vcpu(current);
     bool_t flush = false;
     bool_t flush_pt;
+    bool_t entry_written = false;
     PAGE_LIST_HEAD(free_pages);
     struct page_info *pg;

@@ -1112,6 +1113,7 @@ static int apply_p2m_changes(struct domain *d,
                                   &addr, &maddr, &flush,
                                   t, a);
             if ( ret < 0 ) { rc = ret ; goto out; }
+            if ( ret ) entry_written = 1;

Please don't mix false and 1. This should be true here.

             count += ret;

             if ( ret != P2M_ONE_PROGRESS_NOP )
@@ -1208,6 +1210,9 @@ out:

     p2m_write_unlock(p2m);

+    if ( rc >= 0 && entry_written && p2m_is_hostp2m(p2m) )
+        altp2m_propagate_change(d, sgfn, nr, smfn, mask, t, a);

There are operation which does not require propagation (for instance RELINQUISH and CACHEFLUSH).

+
     if ( rc < 0 && ( op == INSERT ) &&
          addr != start_gpaddr )
     {
@@ -1331,6 +1336,15 @@ int modify_altp2m_entry(struct domain *d, struct 
p2m_domain *ap2m,
     return apply_p2m_changes(d, ap2m, INSERT, gfn, nr, mfn, 0, t, a);
 }

+int modify_altp2m_range(struct domain *d, struct p2m_domain *ap2m,
+                        gfn_t sgfn, unsigned long nr, mfn_t smfn,
+                        uint32_t m, p2m_type_t t, p2m_access_t a)
+{
+    ASSERT(p2m_is_altp2m(ap2m));
+
+    return apply_p2m_changes(d, ap2m, INSERT, sgfn, nr, smfn, m, t, a);
+}
+
 int p2m_alloc_table(struct p2m_domain *p2m)
 {
     unsigned int i;
diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
index dc41f93..9aeb7d6 100644
--- a/xen/include/asm-arm/altp2m.h
+++ b/xen/include/asm-arm/altp2m.h
@@ -81,4 +81,13 @@ int altp2m_set_mem_access(struct domain *d,
                           p2m_access_t a,
                           gfn_t gfn);

+/* Propagates changes made to hostp2m to affected altp2m views. */
+void altp2m_propagate_change(struct domain *d,
+                             gfn_t sgfn,
+                             unsigned long nr,
+                             mfn_t smfn,
+                             uint32_t mask,
+                             p2m_type_t p2mt,
+                             p2m_access_t p2ma);
+
 #endif /* __ASM_ARM_ALTP2M_H */
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 9859ad1..59186c9 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -191,6 +191,11 @@ int modify_altp2m_entry(struct domain *d, struct 
p2m_domain *p2m,
                         paddr_t gpa, paddr_t maddr, unsigned int level,
                         p2m_type_t t, p2m_access_t a);

+/* Modify an altp2m view's range of entries or their attributes. */
+int modify_altp2m_range(struct domain *d, struct p2m_domain *p2m,
+                        gfn_t sgfn, unsigned long nr, mfn_t smfn,
+                        uint32_t mask, p2m_type_t t, p2m_access_t a);
+
 /* Clean & invalidate caches corresponding to a region of guest address space 
*/
 int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr);



--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.