[PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Alexandru Stefan ISAILA]

This patch moves common code from p2m_set_altp2m_mem_access() and
p2m_change_altp2m_gfn() into one function

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

---
Changes since V2:
	- Change var name from found_in_hostp2m to copied_from_hostp2m
	- Move the type check from altp2m_get_gfn_type_access() to the
	callers.
---
 xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
 xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
 xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
 3 files changed, 49 insertions(+), 43 deletions(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 56c06a4fc6..bf67ddb15a 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
     unsigned int page_order;
     unsigned long gfn_l = gfn_x(gfn);
     int rc;
+    bool copied_from_hostp2m;
 
-    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
+    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
 
-    /* Check host p2m if no valid entry in alternate */
     if ( !mfn_valid(mfn) )
+        return -ESRCH;
+
+    /* If this is a superpage, copy that first */
+    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
     {
+        unsigned long mask = ~((1UL << page_order) - 1);
+        gfn_t gfn2 = _gfn(gfn_l & mask);
+        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
 
-        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
-                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
+        /* Note: currently it is not safe to remap to a shared entry */
+        if ( t != p2m_ram_rw )
+            return -ESRCH;
 
-        rc = -ESRCH;
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
+        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
+        if ( rc )
             return rc;
-
-        /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
-        {
-            unsigned long mask = ~((1UL << page_order) - 1);
-            gfn_t gfn2 = _gfn(gfn_l & mask);
-            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
-
-            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
-            if ( rc )
-                return rc;
-        }
     }
 
     /*
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b9bbb8f485..d38d7c29ca 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     mfn_t mfn;
     unsigned int page_order;
     int rc = -EINVAL;
+    bool copied_from_hostp2m;
 
     if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
         return rc;
@@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     p2m_lock(hp2m);
     p2m_lock(ap2m);
 
-    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
+    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
 
     if ( gfn_eq(new_gfn, INVALID_GFN) )
     {
@@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
         goto out;
     }
 
-    /* Check host p2m if no valid entry in alternate */
-    if ( !mfn_valid(mfn) )
-    {
-        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
-                                    P2M_ALLOC, &page_order, 0);
+    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
+         goto out;
 
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
-            goto out;
-
-        /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
-        {
-            gfn_t gfn;
-            unsigned long mask;
+    /* If this is a superpage, copy that first */
+    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
+    {
+        gfn_t gfn;
+        unsigned long mask;
 
-            mask = ~((1UL << page_order) - 1);
-            gfn = _gfn(gfn_x(old_gfn) & mask);
-            mfn = _mfn(mfn_x(mfn) & mask);
+        mask = ~((1UL << page_order) - 1);
+        gfn = _gfn(gfn_x(old_gfn) & mask);
+        mfn = _mfn(mfn_x(mfn) & mask);
 
-            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
-                goto out;
-        }
+        if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
+            goto out;
     }
 
-    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
-
-    if ( !mfn_valid(mfn) )
-        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
+    mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &copied_from_hostp2m);
 
     /* Note: currently it is not safe to remap to a shared entry */
-    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
+    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
         goto out;
 
     if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 2801a8ccca..6de1546d76 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -448,6 +448,25 @@ static inline mfn_t __nonnull(3) get_gfn_type(
     return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL);
 }
 
+static inline mfn_t altp2m_get_gfn_type_access(
+    struct p2m_domain *ap2m, gfn_t gfn, p2m_type_t *t, p2m_access_t *a,
+    unsigned int *page_order, bool *copied_from_hostp2m)
+{
+    mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
+
+    *copied_from_hostp2m = false;
+
+    /* Check host p2m if no valid entry in alternate */
+    if ( !mfn_valid(mfn) )
+    {
+        mfn = __get_gfn_type_access(p2m_get_hostp2m(ap2m->domain), gfn_x(gfn), t, a,
+                                    P2M_ALLOC | P2M_UNSHARE, page_order, false);
+        *copied_from_hostp2m = mfn_valid(mfn);
+    }
+
+    return mfn;
+}
+
 /* Syntactic sugar: most callers will use one of these. */
 #define get_gfn(d, g, t)         get_gfn_type((d), (g), (t), P2M_ALLOC)
 #define get_gfn_query(d, g, t)   get_gfn_type((d), (g), (t), 0)
-- 
2.17.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Alexandru Stefan ISAILA]

This patch moves common code from p2m_set_altp2m_mem_access() and
p2m_change_altp2m_gfn() into one function

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

---
Changes since V2:
	- Change var name from found_in_hostp2m to copied_from_hostp2m
	- Move the type check from altp2m_get_gfn_type_access() to the
	callers.
---
 xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
 xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
 xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
 3 files changed, 49 insertions(+), 43 deletions(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 56c06a4fc6..bf67ddb15a 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
     unsigned int page_order;
     unsigned long gfn_l = gfn_x(gfn);
     int rc;
+    bool copied_from_hostp2m;
 
-    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
+    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
 
-    /* Check host p2m if no valid entry in alternate */
     if ( !mfn_valid(mfn) )
+        return -ESRCH;
+
+    /* If this is a superpage, copy that first */
+    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
     {
+        unsigned long mask = ~((1UL << page_order) - 1);
+        gfn_t gfn2 = _gfn(gfn_l & mask);
+        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
 
-        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
-                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
+        /* Note: currently it is not safe to remap to a shared entry */
+        if ( t != p2m_ram_rw )
+            return -ESRCH;
 
-        rc = -ESRCH;
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
+        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
+        if ( rc )
             return rc;
-
-        /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
-        {
-            unsigned long mask = ~((1UL << page_order) - 1);
-            gfn_t gfn2 = _gfn(gfn_l & mask);
-            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
-
-            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
-            if ( rc )
-                return rc;
-        }
     }
 
     /*
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b9bbb8f485..d38d7c29ca 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     mfn_t mfn;
     unsigned int page_order;
     int rc = -EINVAL;
+    bool copied_from_hostp2m;
 
     if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
         return rc;
@@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     p2m_lock(hp2m);
     p2m_lock(ap2m);
 
-    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
+    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
 
     if ( gfn_eq(new_gfn, INVALID_GFN) )
     {
@@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
         goto out;
     }
 
-    /* Check host p2m if no valid entry in alternate */
-    if ( !mfn_valid(mfn) )
-    {
-        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
-                                    P2M_ALLOC, &page_order, 0);
+    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
+         goto out;
 
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
-            goto out;
-
-        /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
-        {
-            gfn_t gfn;
-            unsigned long mask;
+    /* If this is a superpage, copy that first */
+    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
+    {
+        gfn_t gfn;
+        unsigned long mask;
 
-            mask = ~((1UL << page_order) - 1);
-            gfn = _gfn(gfn_x(old_gfn) & mask);
-            mfn = _mfn(mfn_x(mfn) & mask);
+        mask = ~((1UL << page_order) - 1);
+        gfn = _gfn(gfn_x(old_gfn) & mask);
+        mfn = _mfn(mfn_x(mfn) & mask);
 
-            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
-                goto out;
-        }
+        if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
+            goto out;
     }
 
-    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
-
-    if ( !mfn_valid(mfn) )
-        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
+    mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &copied_from_hostp2m);
 
     /* Note: currently it is not safe to remap to a shared entry */
-    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
+    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
         goto out;
 
     if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 2801a8ccca..6de1546d76 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -448,6 +448,25 @@ static inline mfn_t __nonnull(3) get_gfn_type(
     return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL);
 }
 
+static inline mfn_t altp2m_get_gfn_type_access(
+    struct p2m_domain *ap2m, gfn_t gfn, p2m_type_t *t, p2m_access_t *a,
+    unsigned int *page_order, bool *copied_from_hostp2m)
+{
+    mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
+
+    *copied_from_hostp2m = false;
+
+    /* Check host p2m if no valid entry in alternate */
+    if ( !mfn_valid(mfn) )
+    {
+        mfn = __get_gfn_type_access(p2m_get_hostp2m(ap2m->domain), gfn_x(gfn), t, a,
+                                    P2M_ALLOC | P2M_UNSHARE, page_order, false);
+        *copied_from_hostp2m = mfn_valid(mfn);
+    }
+
+    return mfn;
+}
+
 /* Syntactic sugar: most callers will use one of these. */
 #define get_gfn(d, g, t)         get_gfn_type((d), (g), (t), P2M_ALLOC)
 #define get_gfn_query(d, g, t)   get_gfn_type((d), (g), (t), 0)
-- 
2.17.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 2/3] x86/mm: Introduce altp2m_set_entry_by_page_order

[Alexandru Stefan ISAILA]

This patch moves common code from p2m_set_altp2m_mem_access() and
p2m_change_altp2m_gfn() into one function

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
 xen/arch/x86/mm/mem_access.c |  2 +-
 xen/include/asm-x86/p2m.h    | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index bf67ddb15a..6a22512555 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -279,7 +279,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
         gfn_t gfn2 = _gfn(gfn_l & mask);
         mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
 
-        /* Note: currently it is not safe to remap to a shared entry */
+	/* Note: currently it is not safe to remap to a shared entry */
         if ( t != p2m_ram_rw )
             return -ESRCH;
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 6de1546d76..90a6c135a7 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -467,6 +467,17 @@ static inline mfn_t altp2m_get_gfn_type_access(
     return mfn;
 }
 
+static inline int altp2m_set_entry_by_page_order(
+    struct p2m_domain *ap2m, unsigned long gfn,  mfn_t mfn,
+    unsigned int page_order, p2m_type_t t, p2m_access_t a)
+{
+    unsigned long mask = ~((1UL << page_order) - 1);
+    gfn_t gfn2 = _gfn(gfn & mask);
+    mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
+
+    return ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, a, 1);
+}
+
 /* Syntactic sugar: most callers will use one of these. */
 #define get_gfn(d, g, t)         get_gfn_type((d), (g), (t), P2M_ALLOC)
 #define get_gfn_query(d, g, t)   get_gfn_type((d), (g), (t), 0)
-- 
2.17.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 2/3] x86/mm: Introduce altp2m_set_entry_by_page_order

[Alexandru Stefan ISAILA]

This patch moves common code from p2m_set_altp2m_mem_access() and
p2m_change_altp2m_gfn() into one function

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
 xen/arch/x86/mm/mem_access.c |  2 +-
 xen/include/asm-x86/p2m.h    | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index bf67ddb15a..6a22512555 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -279,7 +279,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
         gfn_t gfn2 = _gfn(gfn_l & mask);
         mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
 
-        /* Note: currently it is not safe to remap to a shared entry */
+	/* Note: currently it is not safe to remap to a shared entry */
         if ( t != p2m_ram_rw )
             return -ESRCH;
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 6de1546d76..90a6c135a7 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -467,6 +467,17 @@ static inline mfn_t altp2m_get_gfn_type_access(
     return mfn;
 }
 
+static inline int altp2m_set_entry_by_page_order(
+    struct p2m_domain *ap2m, unsigned long gfn,  mfn_t mfn,
+    unsigned int page_order, p2m_type_t t, p2m_access_t a)
+{
+    unsigned long mask = ~((1UL << page_order) - 1);
+    gfn_t gfn2 = _gfn(gfn & mask);
+    mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
+
+    return ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, a, 1);
+}
+
 /* Syntactic sugar: most callers will use one of these. */
 #define get_gfn(d, g, t)         get_gfn_type((d), (g), (t), P2M_ALLOC)
 #define get_gfn_query(d, g, t)   get_gfn_type((d), (g), (t), 0)
-- 
2.17.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 3/3] x86/mm: Fix p2m_set_suppress_ve

[Alexandru Stefan ISAILA]

On a new altp2m view the p2m_set_suppress_ve() func will fail with invalid mfn
from p2m->get_entry() if p2m->set_entry() was not called before.

This patch solves the problem by getting the mfn from hostp2m.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
 xen/arch/x86/mm/p2m.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index d38d7c29ca..aee82f814d 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2982,6 +2982,7 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
     p2m_access_t a;
     p2m_type_t t;
     int rc;
+    bool found_in_hostp2m;
 
     if ( altp2m_idx > 0 )
     {
@@ -2999,7 +3000,7 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
     if ( ap2m )
         p2m_lock(ap2m);
 
-    mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
+    mfn = altp2m_get_gfn_type_access(p2m, gfn, &t, &a, NULL, &found_in_hostp2m);
     if ( !mfn_valid(mfn) )
     {
         rc = -ESRCH;
-- 
2.17.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 3/3] x86/mm: Fix p2m_set_suppress_ve

[Alexandru Stefan ISAILA]

On a new altp2m view the p2m_set_suppress_ve() func will fail with invalid mfn
from p2m->get_entry() if p2m->set_entry() was not called before.

This patch solves the problem by getting the mfn from hostp2m.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
 xen/arch/x86/mm/p2m.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index d38d7c29ca..aee82f814d 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2982,6 +2982,7 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
     p2m_access_t a;
     p2m_type_t t;
     int rc;
+    bool found_in_hostp2m;
 
     if ( altp2m_idx > 0 )
     {
@@ -2999,7 +3000,7 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
     if ( ap2m )
         p2m_lock(ap2m);
 
-    mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
+    mfn = altp2m_get_gfn_type_access(p2m, gfn, &t, &a, NULL, &found_in_hostp2m);
     if ( !mfn_valid(mfn) )
     {
         rc = -ESRCH;
-- 
2.17.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/3] x86/mm: Introduce altp2m_set_entry_by_page_order

[Tamas K Lengyel]

On Tue, Apr 9, 2019 at 6:04 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
> This patch moves common code from p2m_set_altp2m_mem_access() and
> p2m_change_altp2m_gfn() into one function
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> ---
>  xen/arch/x86/mm/mem_access.c |  2 +-
>  xen/include/asm-x86/p2m.h    | 11 +++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index bf67ddb15a..6a22512555 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -279,7 +279,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>          gfn_t gfn2 = _gfn(gfn_l & mask);
>          mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>
> -        /* Note: currently it is not safe to remap to a shared entry */
> +       /* Note: currently it is not safe to remap to a shared entry */

This looks like an unrelated change. And I guess this comment should
also be fixed as this is the mem_access setting function, not the
remap function.

>          if ( t != p2m_ram_rw )
>              return -ESRCH;
>
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 6de1546d76..90a6c135a7 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -467,6 +467,17 @@ static inline mfn_t altp2m_get_gfn_type_access(
>      return mfn;
>  }
>
> +static inline int altp2m_set_entry_by_page_order(
> +    struct p2m_domain *ap2m, unsigned long gfn,  mfn_t mfn,
> +    unsigned int page_order, p2m_type_t t, p2m_access_t a)
> +{
> +    unsigned long mask = ~((1UL << page_order) - 1);
> +    gfn_t gfn2 = _gfn(gfn & mask);
> +    mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> +
> +    return ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, a, 1);
> +}
> +
>  /* Syntactic sugar: most callers will use one of these. */
>  #define get_gfn(d, g, t)         get_gfn_type((d), (g), (t), P2M_ALLOC)
>  #define get_gfn_query(d, g, t)   get_gfn_type((d), (g), (t), 0)
> --
> 2.17.1
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/3] x86/mm: Introduce altp2m_set_entry_by_page_order

[Tamas K Lengyel]

On Tue, Apr 9, 2019 at 6:04 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
> This patch moves common code from p2m_set_altp2m_mem_access() and
> p2m_change_altp2m_gfn() into one function
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> ---
>  xen/arch/x86/mm/mem_access.c |  2 +-
>  xen/include/asm-x86/p2m.h    | 11 +++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index bf67ddb15a..6a22512555 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -279,7 +279,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>          gfn_t gfn2 = _gfn(gfn_l & mask);
>          mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>
> -        /* Note: currently it is not safe to remap to a shared entry */
> +       /* Note: currently it is not safe to remap to a shared entry */

This looks like an unrelated change. And I guess this comment should
also be fixed as this is the mem_access setting function, not the
remap function.

>          if ( t != p2m_ram_rw )
>              return -ESRCH;
>
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 6de1546d76..90a6c135a7 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -467,6 +467,17 @@ static inline mfn_t altp2m_get_gfn_type_access(
>      return mfn;
>  }
>
> +static inline int altp2m_set_entry_by_page_order(
> +    struct p2m_domain *ap2m, unsigned long gfn,  mfn_t mfn,
> +    unsigned int page_order, p2m_type_t t, p2m_access_t a)
> +{
> +    unsigned long mask = ~((1UL << page_order) - 1);
> +    gfn_t gfn2 = _gfn(gfn & mask);
> +    mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> +
> +    return ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, a, 1);
> +}
> +
>  /* Syntactic sugar: most callers will use one of these. */
>  #define get_gfn(d, g, t)         get_gfn_type((d), (g), (t), P2M_ALLOC)
>  #define get_gfn_query(d, g, t)   get_gfn_type((d), (g), (t), 0)
> --
> 2.17.1
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Tamas K Lengyel]

On Tue, Apr 9, 2019 at 6:04 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
> This patch moves common code from p2m_set_altp2m_mem_access() and
> p2m_change_altp2m_gfn() into one function
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>
> ---
> Changes since V2:
>         - Change var name from found_in_hostp2m to copied_from_hostp2m
>         - Move the type check from altp2m_get_gfn_type_access() to the
>         callers.
> ---
>  xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
>  xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
>  xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
>  3 files changed, 49 insertions(+), 43 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 56c06a4fc6..bf67ddb15a 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>      unsigned int page_order;
>      unsigned long gfn_l = gfn_x(gfn);
>      int rc;
> +    bool copied_from_hostp2m;
>
> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
>
> -    /* Check host p2m if no valid entry in alternate */
>      if ( !mfn_valid(mfn) )
> +        return -ESRCH;
> +
> +    /* If this is a superpage, copy that first */
> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
>      {
> +        unsigned long mask = ~((1UL << page_order) - 1);
> +        gfn_t gfn2 = _gfn(gfn_l & mask);
> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>
> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> +        /* Note: currently it is not safe to remap to a shared entry */
> +        if ( t != p2m_ram_rw )
> +            return -ESRCH;
>
> -        rc = -ESRCH;
> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> +        if ( rc )
>              return rc;
> -
> -        /* If this is a superpage, copy that first */
> -        if ( page_order != PAGE_ORDER_4K )
> -        {
> -            unsigned long mask = ~((1UL << page_order) - 1);
> -            gfn_t gfn2 = _gfn(gfn_l & mask);
> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> -
> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> -            if ( rc )
> -                return rc;
> -        }
>      }
>
>      /*
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index b9bbb8f485..d38d7c29ca 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>      mfn_t mfn;
>      unsigned int page_order;
>      int rc = -EINVAL;
> +    bool copied_from_hostp2m;
>
>      if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>          return rc;
> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>      p2m_lock(hp2m);
>      p2m_lock(ap2m);
>
> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
>
>      if ( gfn_eq(new_gfn, INVALID_GFN) )
>      {
> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>          goto out;
>      }
>
> -    /* Check host p2m if no valid entry in alternate */
> -    if ( !mfn_valid(mfn) )
> -    {
> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
> -                                    P2M_ALLOC, &page_order, 0);
> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )

Is this check correct? Why do you want to get out only when type is
non-rw *and* it's copied from the hostp2m? You could have non-rw
entries like mmio in the altp2m that were lazily copied and I don't
think we want to allow remapping to those either.

> +         goto out;
>
> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> -            goto out;
> -
> -        /* If this is a superpage, copy that first */
> -        if ( page_order != PAGE_ORDER_4K )
> -        {
> -            gfn_t gfn;
> -            unsigned long mask;
> +    /* If this is a superpage, copy that first */
> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
> +    {
> +        gfn_t gfn;
> +        unsigned long mask;
>
> -            mask = ~((1UL << page_order) - 1);
> -            gfn = _gfn(gfn_x(old_gfn) & mask);
> -            mfn = _mfn(mfn_x(mfn) & mask);
> +        mask = ~((1UL << page_order) - 1);
> +        gfn = _gfn(gfn_x(old_gfn) & mask);
> +        mfn = _mfn(mfn_x(mfn) & mask);
>
> -            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
> -                goto out;
> -        }
> +        if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
> +            goto out;
>      }
>
> -    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
> -
> -    if ( !mfn_valid(mfn) )
> -        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &copied_from_hostp2m);
>
>      /* Note: currently it is not safe to remap to a shared entry */
> -    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
>          goto out;
>
>      if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 2801a8ccca..6de1546d76 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -448,6 +448,25 @@ static inline mfn_t __nonnull(3) get_gfn_type(
>      return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL);
>  }
>
> +static inline mfn_t altp2m_get_gfn_type_access(
> +    struct p2m_domain *ap2m, gfn_t gfn, p2m_type_t *t, p2m_access_t *a,
> +    unsigned int *page_order, bool *copied_from_hostp2m)
> +{
> +    mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
> +
> +    *copied_from_hostp2m = false;
> +
> +    /* Check host p2m if no valid entry in alternate */
> +    if ( !mfn_valid(mfn) )
> +    {
> +        mfn = __get_gfn_type_access(p2m_get_hostp2m(ap2m->domain), gfn_x(gfn), t, a,
> +                                    P2M_ALLOC | P2M_UNSHARE, page_order, false);
> +        *copied_from_hostp2m = mfn_valid(mfn);
> +    }
> +
> +    return mfn;
> +}
> +
>  /* Syntactic sugar: most callers will use one of these. */
>  #define get_gfn(d, g, t)         get_gfn_type((d), (g), (t), P2M_ALLOC)
>  #define get_gfn_query(d, g, t)   get_gfn_type((d), (g), (t), 0)
> --
> 2.17.1
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Tamas K Lengyel]

On Tue, Apr 9, 2019 at 6:04 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
> This patch moves common code from p2m_set_altp2m_mem_access() and
> p2m_change_altp2m_gfn() into one function
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>
> ---
> Changes since V2:
>         - Change var name from found_in_hostp2m to copied_from_hostp2m
>         - Move the type check from altp2m_get_gfn_type_access() to the
>         callers.
> ---
>  xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
>  xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
>  xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
>  3 files changed, 49 insertions(+), 43 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 56c06a4fc6..bf67ddb15a 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>      unsigned int page_order;
>      unsigned long gfn_l = gfn_x(gfn);
>      int rc;
> +    bool copied_from_hostp2m;
>
> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
>
> -    /* Check host p2m if no valid entry in alternate */
>      if ( !mfn_valid(mfn) )
> +        return -ESRCH;
> +
> +    /* If this is a superpage, copy that first */
> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
>      {
> +        unsigned long mask = ~((1UL << page_order) - 1);
> +        gfn_t gfn2 = _gfn(gfn_l & mask);
> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>
> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> +        /* Note: currently it is not safe to remap to a shared entry */
> +        if ( t != p2m_ram_rw )
> +            return -ESRCH;
>
> -        rc = -ESRCH;
> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> +        if ( rc )
>              return rc;
> -
> -        /* If this is a superpage, copy that first */
> -        if ( page_order != PAGE_ORDER_4K )
> -        {
> -            unsigned long mask = ~((1UL << page_order) - 1);
> -            gfn_t gfn2 = _gfn(gfn_l & mask);
> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> -
> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> -            if ( rc )
> -                return rc;
> -        }
>      }
>
>      /*
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index b9bbb8f485..d38d7c29ca 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>      mfn_t mfn;
>      unsigned int page_order;
>      int rc = -EINVAL;
> +    bool copied_from_hostp2m;
>
>      if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>          return rc;
> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>      p2m_lock(hp2m);
>      p2m_lock(ap2m);
>
> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
>
>      if ( gfn_eq(new_gfn, INVALID_GFN) )
>      {
> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>          goto out;
>      }
>
> -    /* Check host p2m if no valid entry in alternate */
> -    if ( !mfn_valid(mfn) )
> -    {
> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
> -                                    P2M_ALLOC, &page_order, 0);
> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )

Is this check correct? Why do you want to get out only when type is
non-rw *and* it's copied from the hostp2m? You could have non-rw
entries like mmio in the altp2m that were lazily copied and I don't
think we want to allow remapping to those either.

> +         goto out;
>
> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> -            goto out;
> -
> -        /* If this is a superpage, copy that first */
> -        if ( page_order != PAGE_ORDER_4K )
> -        {
> -            gfn_t gfn;
> -            unsigned long mask;
> +    /* If this is a superpage, copy that first */
> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
> +    {
> +        gfn_t gfn;
> +        unsigned long mask;
>
> -            mask = ~((1UL << page_order) - 1);
> -            gfn = _gfn(gfn_x(old_gfn) & mask);
> -            mfn = _mfn(mfn_x(mfn) & mask);
> +        mask = ~((1UL << page_order) - 1);
> +        gfn = _gfn(gfn_x(old_gfn) & mask);
> +        mfn = _mfn(mfn_x(mfn) & mask);
>
> -            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
> -                goto out;
> -        }
> +        if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
> +            goto out;
>      }
>
> -    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
> -
> -    if ( !mfn_valid(mfn) )
> -        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &copied_from_hostp2m);
>
>      /* Note: currently it is not safe to remap to a shared entry */
> -    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
>          goto out;
>
>      if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 2801a8ccca..6de1546d76 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -448,6 +448,25 @@ static inline mfn_t __nonnull(3) get_gfn_type(
>      return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL);
>  }
>
> +static inline mfn_t altp2m_get_gfn_type_access(
> +    struct p2m_domain *ap2m, gfn_t gfn, p2m_type_t *t, p2m_access_t *a,
> +    unsigned int *page_order, bool *copied_from_hostp2m)
> +{
> +    mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
> +
> +    *copied_from_hostp2m = false;
> +
> +    /* Check host p2m if no valid entry in alternate */
> +    if ( !mfn_valid(mfn) )
> +    {
> +        mfn = __get_gfn_type_access(p2m_get_hostp2m(ap2m->domain), gfn_x(gfn), t, a,
> +                                    P2M_ALLOC | P2M_UNSHARE, page_order, false);
> +        *copied_from_hostp2m = mfn_valid(mfn);
> +    }
> +
> +    return mfn;
> +}
> +
>  /* Syntactic sugar: most callers will use one of these. */
>  #define get_gfn(d, g, t)         get_gfn_type((d), (g), (t), P2M_ALLOC)
>  #define get_gfn_query(d, g, t)   get_gfn_type((d), (g), (t), 0)
> --
> 2.17.1
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Alexandru Stefan ISAILA]

On 09.04.2019 16:48, Tamas K Lengyel wrote:
> On Tue, Apr 9, 2019 at 6:04 AM Alexandru Stefan ISAILA
> <aisaila@bitdefender.com> wrote:
>>
>> This patch moves common code from p2m_set_altp2m_mem_access() and
>> p2m_change_altp2m_gfn() into one function
>>
>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>
>> ---
>> Changes since V2:
>>          - Change var name from found_in_hostp2m to copied_from_hostp2m
>>          - Move the type check from altp2m_get_gfn_type_access() to the
>>          callers.
>> ---
>>   xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
>>   xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
>>   xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
>>   3 files changed, 49 insertions(+), 43 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index 56c06a4fc6..bf67ddb15a 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>       unsigned int page_order;
>>       unsigned long gfn_l = gfn_x(gfn);
>>       int rc;
>> +    bool copied_from_hostp2m;
>>
>> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
>>
>> -    /* Check host p2m if no valid entry in alternate */
>>       if ( !mfn_valid(mfn) )
>> +        return -ESRCH;
>> +
>> +    /* If this is a superpage, copy that first */
>> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
>>       {
>> +        unsigned long mask = ~((1UL << page_order) - 1);
>> +        gfn_t gfn2 = _gfn(gfn_l & mask);
>> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>
>> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>> +        /* Note: currently it is not safe to remap to a shared entry */
>> +        if ( t != p2m_ram_rw )
>> +            return -ESRCH;
>>
>> -        rc = -ESRCH;
>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>> +        if ( rc )
>>               return rc;
>> -
>> -        /* If this is a superpage, copy that first */
>> -        if ( page_order != PAGE_ORDER_4K )
>> -        {
>> -            unsigned long mask = ~((1UL << page_order) - 1);
>> -            gfn_t gfn2 = _gfn(gfn_l & mask);
>> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>> -
>> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>> -            if ( rc )
>> -                return rc;
>> -        }
>>       }
>>
>>       /*
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index b9bbb8f485..d38d7c29ca 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>       mfn_t mfn;
>>       unsigned int page_order;
>>       int rc = -EINVAL;
>> +    bool copied_from_hostp2m;
>>
>>       if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>           return rc;
>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>       p2m_lock(hp2m);
>>       p2m_lock(ap2m);
>>
>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
>>
>>       if ( gfn_eq(new_gfn, INVALID_GFN) )
>>       {
>> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>           goto out;
>>       }
>>
>> -    /* Check host p2m if no valid entry in alternate */
>> -    if ( !mfn_valid(mfn) )
>> -    {
>> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
>> -                                    P2M_ALLOC, &page_order, 0);
>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
> 
> Is this check correct? Why do you want to get out only when type is
> non-rw *and* it's copied from the hostp2m? You could have non-rw
> entries like mmio in the altp2m that were lazily copied and I don't
> think we want to allow remapping to those either.

I just copied the functionality. If this is needed I will add a || t != 
p2m_mmio_dm and p2m_mmio_direct.

Alex

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Alexandru Stefan ISAILA]

On 09.04.2019 16:48, Tamas K Lengyel wrote:
> On Tue, Apr 9, 2019 at 6:04 AM Alexandru Stefan ISAILA
> <aisaila@bitdefender.com> wrote:
>>
>> This patch moves common code from p2m_set_altp2m_mem_access() and
>> p2m_change_altp2m_gfn() into one function
>>
>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>
>> ---
>> Changes since V2:
>>          - Change var name from found_in_hostp2m to copied_from_hostp2m
>>          - Move the type check from altp2m_get_gfn_type_access() to the
>>          callers.
>> ---
>>   xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
>>   xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
>>   xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
>>   3 files changed, 49 insertions(+), 43 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index 56c06a4fc6..bf67ddb15a 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>       unsigned int page_order;
>>       unsigned long gfn_l = gfn_x(gfn);
>>       int rc;
>> +    bool copied_from_hostp2m;
>>
>> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
>>
>> -    /* Check host p2m if no valid entry in alternate */
>>       if ( !mfn_valid(mfn) )
>> +        return -ESRCH;
>> +
>> +    /* If this is a superpage, copy that first */
>> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
>>       {
>> +        unsigned long mask = ~((1UL << page_order) - 1);
>> +        gfn_t gfn2 = _gfn(gfn_l & mask);
>> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>
>> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>> +        /* Note: currently it is not safe to remap to a shared entry */
>> +        if ( t != p2m_ram_rw )
>> +            return -ESRCH;
>>
>> -        rc = -ESRCH;
>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>> +        if ( rc )
>>               return rc;
>> -
>> -        /* If this is a superpage, copy that first */
>> -        if ( page_order != PAGE_ORDER_4K )
>> -        {
>> -            unsigned long mask = ~((1UL << page_order) - 1);
>> -            gfn_t gfn2 = _gfn(gfn_l & mask);
>> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>> -
>> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>> -            if ( rc )
>> -                return rc;
>> -        }
>>       }
>>
>>       /*
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index b9bbb8f485..d38d7c29ca 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>       mfn_t mfn;
>>       unsigned int page_order;
>>       int rc = -EINVAL;
>> +    bool copied_from_hostp2m;
>>
>>       if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>           return rc;
>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>       p2m_lock(hp2m);
>>       p2m_lock(ap2m);
>>
>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
>>
>>       if ( gfn_eq(new_gfn, INVALID_GFN) )
>>       {
>> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>           goto out;
>>       }
>>
>> -    /* Check host p2m if no valid entry in alternate */
>> -    if ( !mfn_valid(mfn) )
>> -    {
>> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
>> -                                    P2M_ALLOC, &page_order, 0);
>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
> 
> Is this check correct? Why do you want to get out only when type is
> non-rw *and* it's copied from the hostp2m? You could have non-rw
> entries like mmio in the altp2m that were lazily copied and I don't
> think we want to allow remapping to those either.

I just copied the functionality. If this is needed I will add a || t != 
p2m_mmio_dm and p2m_mmio_direct.

Alex

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Tamas K Lengyel]

On Tue, Apr 9, 2019 at 8:03 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
>
>
> On 09.04.2019 16:48, Tamas K Lengyel wrote:
> > On Tue, Apr 9, 2019 at 6:04 AM Alexandru Stefan ISAILA
> > <aisaila@bitdefender.com> wrote:
> >>
> >> This patch moves common code from p2m_set_altp2m_mem_access() and
> >> p2m_change_altp2m_gfn() into one function
> >>
> >> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> >>
> >> ---
> >> Changes since V2:
> >>          - Change var name from found_in_hostp2m to copied_from_hostp2m
> >>          - Move the type check from altp2m_get_gfn_type_access() to the
> >>          callers.
> >> ---
> >>   xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
> >>   xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
> >>   xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
> >>   3 files changed, 49 insertions(+), 43 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> >> index 56c06a4fc6..bf67ddb15a 100644
> >> --- a/xen/arch/x86/mm/mem_access.c
> >> +++ b/xen/arch/x86/mm/mem_access.c
> >> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
> >>       unsigned int page_order;
> >>       unsigned long gfn_l = gfn_x(gfn);
> >>       int rc;
> >> +    bool copied_from_hostp2m;
> >>
> >> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
> >> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
> >>
> >> -    /* Check host p2m if no valid entry in alternate */
> >>       if ( !mfn_valid(mfn) )
> >> +        return -ESRCH;
> >> +
> >> +    /* If this is a superpage, copy that first */
> >> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
> >>       {
> >> +        unsigned long mask = ~((1UL << page_order) - 1);
> >> +        gfn_t gfn2 = _gfn(gfn_l & mask);
> >> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> >>
> >> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> >> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> >> +        /* Note: currently it is not safe to remap to a shared entry */
> >> +        if ( t != p2m_ram_rw )
> >> +            return -ESRCH;
> >>
> >> -        rc = -ESRCH;
> >> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> >> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> >> +        if ( rc )
> >>               return rc;
> >> -
> >> -        /* If this is a superpage, copy that first */
> >> -        if ( page_order != PAGE_ORDER_4K )
> >> -        {
> >> -            unsigned long mask = ~((1UL << page_order) - 1);
> >> -            gfn_t gfn2 = _gfn(gfn_l & mask);
> >> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> >> -
> >> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> >> -            if ( rc )
> >> -                return rc;
> >> -        }
> >>       }
> >>
> >>       /*
> >> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> >> index b9bbb8f485..d38d7c29ca 100644
> >> --- a/xen/arch/x86/mm/p2m.c
> >> +++ b/xen/arch/x86/mm/p2m.c
> >> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>       mfn_t mfn;
> >>       unsigned int page_order;
> >>       int rc = -EINVAL;
> >> +    bool copied_from_hostp2m;
> >>
> >>       if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
> >>           return rc;
> >> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>       p2m_lock(hp2m);
> >>       p2m_lock(ap2m);
> >>
> >> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
> >> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
> >>
> >>       if ( gfn_eq(new_gfn, INVALID_GFN) )
> >>       {
> >> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>           goto out;
> >>       }
> >>
> >> -    /* Check host p2m if no valid entry in alternate */
> >> -    if ( !mfn_valid(mfn) )
> >> -    {
> >> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
> >> -                                    P2M_ALLOC, &page_order, 0);
> >> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
> >
> > Is this check correct? Why do you want to get out only when type is
> > non-rw *and* it's copied from the hostp2m? You could have non-rw
> > entries like mmio in the altp2m that were lazily copied and I don't
> > think we want to allow remapping to those either.
>
> I just copied the functionality. If this is needed I will add a || t !=
> p2m_mmio_dm and p2m_mmio_direct.

My problem is with the && copied_form_hostp2m part. Why is that a criteria?

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Tamas K Lengyel]

On Tue, Apr 9, 2019 at 8:03 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
>
>
> On 09.04.2019 16:48, Tamas K Lengyel wrote:
> > On Tue, Apr 9, 2019 at 6:04 AM Alexandru Stefan ISAILA
> > <aisaila@bitdefender.com> wrote:
> >>
> >> This patch moves common code from p2m_set_altp2m_mem_access() and
> >> p2m_change_altp2m_gfn() into one function
> >>
> >> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> >>
> >> ---
> >> Changes since V2:
> >>          - Change var name from found_in_hostp2m to copied_from_hostp2m
> >>          - Move the type check from altp2m_get_gfn_type_access() to the
> >>          callers.
> >> ---
> >>   xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
> >>   xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
> >>   xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
> >>   3 files changed, 49 insertions(+), 43 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> >> index 56c06a4fc6..bf67ddb15a 100644
> >> --- a/xen/arch/x86/mm/mem_access.c
> >> +++ b/xen/arch/x86/mm/mem_access.c
> >> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
> >>       unsigned int page_order;
> >>       unsigned long gfn_l = gfn_x(gfn);
> >>       int rc;
> >> +    bool copied_from_hostp2m;
> >>
> >> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
> >> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
> >>
> >> -    /* Check host p2m if no valid entry in alternate */
> >>       if ( !mfn_valid(mfn) )
> >> +        return -ESRCH;
> >> +
> >> +    /* If this is a superpage, copy that first */
> >> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
> >>       {
> >> +        unsigned long mask = ~((1UL << page_order) - 1);
> >> +        gfn_t gfn2 = _gfn(gfn_l & mask);
> >> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> >>
> >> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> >> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> >> +        /* Note: currently it is not safe to remap to a shared entry */
> >> +        if ( t != p2m_ram_rw )
> >> +            return -ESRCH;
> >>
> >> -        rc = -ESRCH;
> >> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> >> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> >> +        if ( rc )
> >>               return rc;
> >> -
> >> -        /* If this is a superpage, copy that first */
> >> -        if ( page_order != PAGE_ORDER_4K )
> >> -        {
> >> -            unsigned long mask = ~((1UL << page_order) - 1);
> >> -            gfn_t gfn2 = _gfn(gfn_l & mask);
> >> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> >> -
> >> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> >> -            if ( rc )
> >> -                return rc;
> >> -        }
> >>       }
> >>
> >>       /*
> >> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> >> index b9bbb8f485..d38d7c29ca 100644
> >> --- a/xen/arch/x86/mm/p2m.c
> >> +++ b/xen/arch/x86/mm/p2m.c
> >> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>       mfn_t mfn;
> >>       unsigned int page_order;
> >>       int rc = -EINVAL;
> >> +    bool copied_from_hostp2m;
> >>
> >>       if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
> >>           return rc;
> >> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>       p2m_lock(hp2m);
> >>       p2m_lock(ap2m);
> >>
> >> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
> >> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
> >>
> >>       if ( gfn_eq(new_gfn, INVALID_GFN) )
> >>       {
> >> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>           goto out;
> >>       }
> >>
> >> -    /* Check host p2m if no valid entry in alternate */
> >> -    if ( !mfn_valid(mfn) )
> >> -    {
> >> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
> >> -                                    P2M_ALLOC, &page_order, 0);
> >> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
> >
> > Is this check correct? Why do you want to get out only when type is
> > non-rw *and* it's copied from the hostp2m? You could have non-rw
> > entries like mmio in the altp2m that were lazily copied and I don't
> > think we want to allow remapping to those either.
>
> I just copied the functionality. If this is needed I will add a || t !=
> p2m_mmio_dm and p2m_mmio_direct.

My problem is with the && copied_form_hostp2m part. Why is that a criteria?

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Alexandru Stefan ISAILA]

On 09.04.2019 17:37, Tamas K Lengyel wrote:
> On Tue, Apr 9, 2019 at 8:03 AM Alexandru Stefan ISAILA
> <aisaila@bitdefender.com> wrote:
>>
>>
>>
>> On 09.04.2019 16:48, Tamas K Lengyel wrote:
>>> On Tue, Apr 9, 2019 at 6:04 AM Alexandru Stefan ISAILA
>>> <aisaila@bitdefender.com> wrote:
>>>>
>>>> This patch moves common code from p2m_set_altp2m_mem_access() and
>>>> p2m_change_altp2m_gfn() into one function
>>>>
>>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>>>
>>>> ---
>>>> Changes since V2:
>>>>           - Change var name from found_in_hostp2m to copied_from_hostp2m
>>>>           - Move the type check from altp2m_get_gfn_type_access() to the
>>>>           callers.
>>>> ---
>>>>    xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
>>>>    xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
>>>>    xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
>>>>    3 files changed, 49 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>>> index 56c06a4fc6..bf67ddb15a 100644
>>>> --- a/xen/arch/x86/mm/mem_access.c
>>>> +++ b/xen/arch/x86/mm/mem_access.c
>>>> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>>>        unsigned int page_order;
>>>>        unsigned long gfn_l = gfn_x(gfn);
>>>>        int rc;
>>>> +    bool copied_from_hostp2m;
>>>>
>>>> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
>>>> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
>>>>
>>>> -    /* Check host p2m if no valid entry in alternate */
>>>>        if ( !mfn_valid(mfn) )
>>>> +        return -ESRCH;
>>>> +
>>>> +    /* If this is a superpage, copy that first */
>>>> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
>>>>        {
>>>> +        unsigned long mask = ~((1UL << page_order) - 1);
>>>> +        gfn_t gfn2 = _gfn(gfn_l & mask);
>>>> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>>>
>>>> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>>>> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>>>> +        /* Note: currently it is not safe to remap to a shared entry */
>>>> +        if ( t != p2m_ram_rw )
>>>> +            return -ESRCH;
>>>>
>>>> -        rc = -ESRCH;
>>>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>>>> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>>>> +        if ( rc )
>>>>                return rc;
>>>> -
>>>> -        /* If this is a superpage, copy that first */
>>>> -        if ( page_order != PAGE_ORDER_4K )
>>>> -        {
>>>> -            unsigned long mask = ~((1UL << page_order) - 1);
>>>> -            gfn_t gfn2 = _gfn(gfn_l & mask);
>>>> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>>> -
>>>> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>>>> -            if ( rc )
>>>> -                return rc;
>>>> -        }
>>>>        }
>>>>
>>>>        /*
>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>>> index b9bbb8f485..d38d7c29ca 100644
>>>> --- a/xen/arch/x86/mm/p2m.c
>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>        mfn_t mfn;
>>>>        unsigned int page_order;
>>>>        int rc = -EINVAL;
>>>> +    bool copied_from_hostp2m;
>>>>
>>>>        if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>>>            return rc;
>>>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>        p2m_lock(hp2m);
>>>>        p2m_lock(ap2m);
>>>>
>>>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
>>>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
>>>>
>>>>        if ( gfn_eq(new_gfn, INVALID_GFN) )
>>>>        {
>>>> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>            goto out;
>>>>        }
>>>>
>>>> -    /* Check host p2m if no valid entry in alternate */
>>>> -    if ( !mfn_valid(mfn) )
>>>> -    {
>>>> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
>>>> -                                    P2M_ALLOC, &page_order, 0);
>>>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
>>>
>>> Is this check correct? Why do you want to get out only when type is
>>> non-rw *and* it's copied from the hostp2m? You could have non-rw
>>> entries like mmio in the altp2m that were lazily copied and I don't
>>> think we want to allow remapping to those either.
>>
>> I just copied the functionality. If this is needed I will add a || t !=
>> p2m_mmio_dm and p2m_mmio_direct.
> 
> My problem is with the && copied_form_hostp2m part. Why is that a criteria?

The (t != p2m_ram_rw) check was done only for the get from hostp2m.

If you think that I should do the check for all mfns (hostp2 and altp2m) 
then I can drop the copied_from_hostp2m bool and add mmio check.

I hope I understand the problem correctly.

Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Alexandru Stefan ISAILA]

On 09.04.2019 17:37, Tamas K Lengyel wrote:
> On Tue, Apr 9, 2019 at 8:03 AM Alexandru Stefan ISAILA
> <aisaila@bitdefender.com> wrote:
>>
>>
>>
>> On 09.04.2019 16:48, Tamas K Lengyel wrote:
>>> On Tue, Apr 9, 2019 at 6:04 AM Alexandru Stefan ISAILA
>>> <aisaila@bitdefender.com> wrote:
>>>>
>>>> This patch moves common code from p2m_set_altp2m_mem_access() and
>>>> p2m_change_altp2m_gfn() into one function
>>>>
>>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>>>
>>>> ---
>>>> Changes since V2:
>>>>           - Change var name from found_in_hostp2m to copied_from_hostp2m
>>>>           - Move the type check from altp2m_get_gfn_type_access() to the
>>>>           callers.
>>>> ---
>>>>    xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
>>>>    xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
>>>>    xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
>>>>    3 files changed, 49 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>>> index 56c06a4fc6..bf67ddb15a 100644
>>>> --- a/xen/arch/x86/mm/mem_access.c
>>>> +++ b/xen/arch/x86/mm/mem_access.c
>>>> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>>>        unsigned int page_order;
>>>>        unsigned long gfn_l = gfn_x(gfn);
>>>>        int rc;
>>>> +    bool copied_from_hostp2m;
>>>>
>>>> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
>>>> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
>>>>
>>>> -    /* Check host p2m if no valid entry in alternate */
>>>>        if ( !mfn_valid(mfn) )
>>>> +        return -ESRCH;
>>>> +
>>>> +    /* If this is a superpage, copy that first */
>>>> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
>>>>        {
>>>> +        unsigned long mask = ~((1UL << page_order) - 1);
>>>> +        gfn_t gfn2 = _gfn(gfn_l & mask);
>>>> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>>>
>>>> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>>>> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>>>> +        /* Note: currently it is not safe to remap to a shared entry */
>>>> +        if ( t != p2m_ram_rw )
>>>> +            return -ESRCH;
>>>>
>>>> -        rc = -ESRCH;
>>>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>>>> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>>>> +        if ( rc )
>>>>                return rc;
>>>> -
>>>> -        /* If this is a superpage, copy that first */
>>>> -        if ( page_order != PAGE_ORDER_4K )
>>>> -        {
>>>> -            unsigned long mask = ~((1UL << page_order) - 1);
>>>> -            gfn_t gfn2 = _gfn(gfn_l & mask);
>>>> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>>> -
>>>> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>>>> -            if ( rc )
>>>> -                return rc;
>>>> -        }
>>>>        }
>>>>
>>>>        /*
>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>>> index b9bbb8f485..d38d7c29ca 100644
>>>> --- a/xen/arch/x86/mm/p2m.c
>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>        mfn_t mfn;
>>>>        unsigned int page_order;
>>>>        int rc = -EINVAL;
>>>> +    bool copied_from_hostp2m;
>>>>
>>>>        if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>>>            return rc;
>>>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>        p2m_lock(hp2m);
>>>>        p2m_lock(ap2m);
>>>>
>>>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
>>>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
>>>>
>>>>        if ( gfn_eq(new_gfn, INVALID_GFN) )
>>>>        {
>>>> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>            goto out;
>>>>        }
>>>>
>>>> -    /* Check host p2m if no valid entry in alternate */
>>>> -    if ( !mfn_valid(mfn) )
>>>> -    {
>>>> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
>>>> -                                    P2M_ALLOC, &page_order, 0);
>>>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
>>>
>>> Is this check correct? Why do you want to get out only when type is
>>> non-rw *and* it's copied from the hostp2m? You could have non-rw
>>> entries like mmio in the altp2m that were lazily copied and I don't
>>> think we want to allow remapping to those either.
>>
>> I just copied the functionality. If this is needed I will add a || t !=
>> p2m_mmio_dm and p2m_mmio_direct.
> 
> My problem is with the && copied_form_hostp2m part. Why is that a criteria?

The (t != p2m_ram_rw) check was done only for the get from hostp2m.

If you think that I should do the check for all mfns (hostp2 and altp2m) 
then I can drop the copied_from_hostp2m bool and add mmio check.

I hope I understand the problem correctly.

Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Tamas K Lengyel]

On Tue, Apr 9, 2019 at 8:48 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
>
>
> On 09.04.2019 17:37, Tamas K Lengyel wrote:
> > On Tue, Apr 9, 2019 at 8:03 AM Alexandru Stefan ISAILA
> > <aisaila@bitdefender.com> wrote:
> >>
> >>
> >>
> >> On 09.04.2019 16:48, Tamas K Lengyel wrote:
> >>> On Tue, Apr 9, 2019 at 6:04 AM Alexandru Stefan ISAILA
> >>> <aisaila@bitdefender.com> wrote:
> >>>>
> >>>> This patch moves common code from p2m_set_altp2m_mem_access() and
> >>>> p2m_change_altp2m_gfn() into one function
> >>>>
> >>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> >>>>
> >>>> ---
> >>>> Changes since V2:
> >>>>           - Change var name from found_in_hostp2m to copied_from_hostp2m
> >>>>           - Move the type check from altp2m_get_gfn_type_access() to the
> >>>>           callers.
> >>>> ---
> >>>>    xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
> >>>>    xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
> >>>>    xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
> >>>>    3 files changed, 49 insertions(+), 43 deletions(-)
> >>>>
> >>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> >>>> index 56c06a4fc6..bf67ddb15a 100644
> >>>> --- a/xen/arch/x86/mm/mem_access.c
> >>>> +++ b/xen/arch/x86/mm/mem_access.c
> >>>> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
> >>>>        unsigned int page_order;
> >>>>        unsigned long gfn_l = gfn_x(gfn);
> >>>>        int rc;
> >>>> +    bool copied_from_hostp2m;
> >>>>
> >>>> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
> >>>> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
> >>>>
> >>>> -    /* Check host p2m if no valid entry in alternate */
> >>>>        if ( !mfn_valid(mfn) )
> >>>> +        return -ESRCH;
> >>>> +
> >>>> +    /* If this is a superpage, copy that first */
> >>>> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
> >>>>        {
> >>>> +        unsigned long mask = ~((1UL << page_order) - 1);
> >>>> +        gfn_t gfn2 = _gfn(gfn_l & mask);
> >>>> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> >>>>
> >>>> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> >>>> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> >>>> +        /* Note: currently it is not safe to remap to a shared entry */
> >>>> +        if ( t != p2m_ram_rw )
> >>>> +            return -ESRCH;
> >>>>
> >>>> -        rc = -ESRCH;
> >>>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> >>>> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> >>>> +        if ( rc )
> >>>>                return rc;
> >>>> -
> >>>> -        /* If this is a superpage, copy that first */
> >>>> -        if ( page_order != PAGE_ORDER_4K )
> >>>> -        {
> >>>> -            unsigned long mask = ~((1UL << page_order) - 1);
> >>>> -            gfn_t gfn2 = _gfn(gfn_l & mask);
> >>>> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> >>>> -
> >>>> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> >>>> -            if ( rc )
> >>>> -                return rc;
> >>>> -        }
> >>>>        }
> >>>>
> >>>>        /*
> >>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> >>>> index b9bbb8f485..d38d7c29ca 100644
> >>>> --- a/xen/arch/x86/mm/p2m.c
> >>>> +++ b/xen/arch/x86/mm/p2m.c
> >>>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>>>        mfn_t mfn;
> >>>>        unsigned int page_order;
> >>>>        int rc = -EINVAL;
> >>>> +    bool copied_from_hostp2m;
> >>>>
> >>>>        if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
> >>>>            return rc;
> >>>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>>>        p2m_lock(hp2m);
> >>>>        p2m_lock(ap2m);
> >>>>
> >>>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
> >>>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
> >>>>
> >>>>        if ( gfn_eq(new_gfn, INVALID_GFN) )
> >>>>        {
> >>>> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>>>            goto out;
> >>>>        }
> >>>>
> >>>> -    /* Check host p2m if no valid entry in alternate */
> >>>> -    if ( !mfn_valid(mfn) )
> >>>> -    {
> >>>> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
> >>>> -                                    P2M_ALLOC, &page_order, 0);
> >>>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
> >>>
> >>> Is this check correct? Why do you want to get out only when type is
> >>> non-rw *and* it's copied from the hostp2m? You could have non-rw
> >>> entries like mmio in the altp2m that were lazily copied and I don't
> >>> think we want to allow remapping to those either.
> >>
> >> I just copied the functionality. If this is needed I will add a || t !=
> >> p2m_mmio_dm and p2m_mmio_direct.
> >
> > My problem is with the && copied_form_hostp2m part. Why is that a criteria?
>
> The (t != p2m_ram_rw) check was done only for the get from hostp2m.
>
> If you think that I should do the check for all mfns (hostp2 and altp2m)
> then I can drop the copied_from_hostp2m bool and add mmio check.

I think you should just drop the && copied_from_hostp2m part of it.
Remappings should only be allowed for p2m_ram_rw type, no matter which
p2m its coming from.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Tamas K Lengyel]

On Tue, Apr 9, 2019 at 8:48 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
>
>
> On 09.04.2019 17:37, Tamas K Lengyel wrote:
> > On Tue, Apr 9, 2019 at 8:03 AM Alexandru Stefan ISAILA
> > <aisaila@bitdefender.com> wrote:
> >>
> >>
> >>
> >> On 09.04.2019 16:48, Tamas K Lengyel wrote:
> >>> On Tue, Apr 9, 2019 at 6:04 AM Alexandru Stefan ISAILA
> >>> <aisaila@bitdefender.com> wrote:
> >>>>
> >>>> This patch moves common code from p2m_set_altp2m_mem_access() and
> >>>> p2m_change_altp2m_gfn() into one function
> >>>>
> >>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> >>>>
> >>>> ---
> >>>> Changes since V2:
> >>>>           - Change var name from found_in_hostp2m to copied_from_hostp2m
> >>>>           - Move the type check from altp2m_get_gfn_type_access() to the
> >>>>           callers.
> >>>> ---
> >>>>    xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
> >>>>    xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
> >>>>    xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
> >>>>    3 files changed, 49 insertions(+), 43 deletions(-)
> >>>>
> >>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> >>>> index 56c06a4fc6..bf67ddb15a 100644
> >>>> --- a/xen/arch/x86/mm/mem_access.c
> >>>> +++ b/xen/arch/x86/mm/mem_access.c
> >>>> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
> >>>>        unsigned int page_order;
> >>>>        unsigned long gfn_l = gfn_x(gfn);
> >>>>        int rc;
> >>>> +    bool copied_from_hostp2m;
> >>>>
> >>>> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
> >>>> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
> >>>>
> >>>> -    /* Check host p2m if no valid entry in alternate */
> >>>>        if ( !mfn_valid(mfn) )
> >>>> +        return -ESRCH;
> >>>> +
> >>>> +    /* If this is a superpage, copy that first */
> >>>> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
> >>>>        {
> >>>> +        unsigned long mask = ~((1UL << page_order) - 1);
> >>>> +        gfn_t gfn2 = _gfn(gfn_l & mask);
> >>>> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> >>>>
> >>>> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> >>>> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> >>>> +        /* Note: currently it is not safe to remap to a shared entry */
> >>>> +        if ( t != p2m_ram_rw )
> >>>> +            return -ESRCH;
> >>>>
> >>>> -        rc = -ESRCH;
> >>>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> >>>> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> >>>> +        if ( rc )
> >>>>                return rc;
> >>>> -
> >>>> -        /* If this is a superpage, copy that first */
> >>>> -        if ( page_order != PAGE_ORDER_4K )
> >>>> -        {
> >>>> -            unsigned long mask = ~((1UL << page_order) - 1);
> >>>> -            gfn_t gfn2 = _gfn(gfn_l & mask);
> >>>> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> >>>> -
> >>>> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> >>>> -            if ( rc )
> >>>> -                return rc;
> >>>> -        }
> >>>>        }
> >>>>
> >>>>        /*
> >>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> >>>> index b9bbb8f485..d38d7c29ca 100644
> >>>> --- a/xen/arch/x86/mm/p2m.c
> >>>> +++ b/xen/arch/x86/mm/p2m.c
> >>>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>>>        mfn_t mfn;
> >>>>        unsigned int page_order;
> >>>>        int rc = -EINVAL;
> >>>> +    bool copied_from_hostp2m;
> >>>>
> >>>>        if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
> >>>>            return rc;
> >>>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>>>        p2m_lock(hp2m);
> >>>>        p2m_lock(ap2m);
> >>>>
> >>>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
> >>>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
> >>>>
> >>>>        if ( gfn_eq(new_gfn, INVALID_GFN) )
> >>>>        {
> >>>> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>>>            goto out;
> >>>>        }
> >>>>
> >>>> -    /* Check host p2m if no valid entry in alternate */
> >>>> -    if ( !mfn_valid(mfn) )
> >>>> -    {
> >>>> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
> >>>> -                                    P2M_ALLOC, &page_order, 0);
> >>>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
> >>>
> >>> Is this check correct? Why do you want to get out only when type is
> >>> non-rw *and* it's copied from the hostp2m? You could have non-rw
> >>> entries like mmio in the altp2m that were lazily copied and I don't
> >>> think we want to allow remapping to those either.
> >>
> >> I just copied the functionality. If this is needed I will add a || t !=
> >> p2m_mmio_dm and p2m_mmio_direct.
> >
> > My problem is with the && copied_form_hostp2m part. Why is that a criteria?
>
> The (t != p2m_ram_rw) check was done only for the get from hostp2m.
>
> If you think that I should do the check for all mfns (hostp2 and altp2m)
> then I can drop the copied_from_hostp2m bool and add mmio check.

I think you should just drop the && copied_from_hostp2m part of it.
Remappings should only be allowed for p2m_ram_rw type, no matter which
p2m its coming from.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Alexandru Stefan ISAILA]

On 09.04.2019 18:26, Tamas K Lengyel wrote:
> On Tue, Apr 9, 2019 at 8:48 AM Alexandru Stefan ISAILA
> <aisaila@bitdefender.com> wrote:
>>
>>
>>
>> On 09.04.2019 17:37, Tamas K Lengyel wrote:
>>> On Tue, Apr 9, 2019 at 8:03 AM Alexandru Stefan ISAILA
>>> <aisaila@bitdefender.com> wrote:
>>>>
>>>>
>>>>
>>>> On 09.04.2019 16:48, Tamas K Lengyel wrote:
>>>>> On Tue, Apr 9, 2019 at 6:04 AM Alexandru Stefan ISAILA
>>>>> <aisaila@bitdefender.com> wrote:
>>>>>>
>>>>>> This patch moves common code from p2m_set_altp2m_mem_access() and
>>>>>> p2m_change_altp2m_gfn() into one function
>>>>>>
>>>>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>>>>>
>>>>>> ---
>>>>>> Changes since V2:
>>>>>>            - Change var name from found_in_hostp2m to copied_from_hostp2m
>>>>>>            - Move the type check from altp2m_get_gfn_type_access() to the
>>>>>>            callers.
>>>>>> ---
>>>>>>     xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
>>>>>>     xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
>>>>>>     xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
>>>>>>     3 files changed, 49 insertions(+), 43 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>>>>> index 56c06a4fc6..bf67ddb15a 100644
>>>>>> --- a/xen/arch/x86/mm/mem_access.c
>>>>>> +++ b/xen/arch/x86/mm/mem_access.c
>>>>>> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>>>>>         unsigned int page_order;
>>>>>>         unsigned long gfn_l = gfn_x(gfn);
>>>>>>         int rc;
>>>>>> +    bool copied_from_hostp2m;
>>>>>>
>>>>>> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
>>>>>> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
>>>>>>
>>>>>> -    /* Check host p2m if no valid entry in alternate */
>>>>>>         if ( !mfn_valid(mfn) )
>>>>>> +        return -ESRCH;
>>>>>> +
>>>>>> +    /* If this is a superpage, copy that first */
>>>>>> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )

Regarding the drop of copied_from_hostp2m should this be dropped here as 
well and by that drop it all together?

>>>>>>         {
>>>>>> +        unsigned long mask = ~((1UL << page_order) - 1);
>>>>>> +        gfn_t gfn2 = _gfn(gfn_l & mask);
>>>>>> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>>>>>
>>>>>> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>>>>>> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>>>>>> +        /* Note: currently it is not safe to remap to a shared entry */
>>>>>> +        if ( t != p2m_ram_rw )
>>>>>> +            return -ESRCH;

And if so and regarding the comments from p2m_change_altp2m_gfn should I 
move the ( t != p2m_ram_rw ) check up with the ( !mfn_valid(mfn) ) check?

Alex

>>>>>>
>>>>>> -        rc = -ESRCH;
>>>>>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>>>>>> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>>>>>> +        if ( rc )
>>>>>>                 return rc;
>>>>>> -
>>>>>> -        /* If this is a superpage, copy that first */
>>>>>> -        if ( page_order != PAGE_ORDER_4K )
>>>>>> -        {
>>>>>> -            unsigned long mask = ~((1UL << page_order) - 1);
>>>>>> -            gfn_t gfn2 = _gfn(gfn_l & mask);
>>>>>> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>>>>> -
>>>>>> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>>>>>> -            if ( rc )
>>>>>> -                return rc;
>>>>>> -        }
>>>>>>         }
>>>>>>
>>>>>>         /*
>>>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>>>>> index b9bbb8f485..d38d7c29ca 100644
>>>>>> --- a/xen/arch/x86/mm/p2m.c
>>>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>>>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>>>         mfn_t mfn;
>>>>>>         unsigned int page_order;
>>>>>>         int rc = -EINVAL;
>>>>>> +    bool copied_from_hostp2m;
>>>>>>
>>>>>>         if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>>>>>             return rc;
>>>>>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>>>         p2m_lock(hp2m);
>>>>>>         p2m_lock(ap2m);
>>>>>>
>>>>>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
>>>>>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
>>>>>>
>>>>>>         if ( gfn_eq(new_gfn, INVALID_GFN) )
>>>>>>         {
>>>>>> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>>>             goto out;
>>>>>>         }
>>>>>>
>>>>>> -    /* Check host p2m if no valid entry in alternate */
>>>>>> -    if ( !mfn_valid(mfn) )
>>>>>> -    {
>>>>>> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
>>>>>> -                                    P2M_ALLOC, &page_order, 0);
>>>>>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
>>>>>
>>>>> Is this check correct? Why do you want to get out only when type is
>>>>> non-rw *and* it's copied from the hostp2m? You could have non-rw
>>>>> entries like mmio in the altp2m that were lazily copied and I don't
>>>>> think we want to allow remapping to those either.
>>>>
>>>> I just copied the functionality. If this is needed I will add a || t !=
>>>> p2m_mmio_dm and p2m_mmio_direct.
>>>
>>> My problem is with the && copied_form_hostp2m part. Why is that a criteria?
>>
>> The (t != p2m_ram_rw) check was done only for the get from hostp2m.
>>
>> If you think that I should do the check for all mfns (hostp2 and altp2m)
>> then I can drop the copied_from_hostp2m bool and add mmio check.
> 
> I think you should just drop the && copied_from_hostp2m part of it.
> Remappings should only be allowed for p2m_ram_rw type, no matter which
> p2m its coming from.
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Alexandru Stefan ISAILA]

On 09.04.2019 18:26, Tamas K Lengyel wrote:
> On Tue, Apr 9, 2019 at 8:48 AM Alexandru Stefan ISAILA
> <aisaila@bitdefender.com> wrote:
>>
>>
>>
>> On 09.04.2019 17:37, Tamas K Lengyel wrote:
>>> On Tue, Apr 9, 2019 at 8:03 AM Alexandru Stefan ISAILA
>>> <aisaila@bitdefender.com> wrote:
>>>>
>>>>
>>>>
>>>> On 09.04.2019 16:48, Tamas K Lengyel wrote:
>>>>> On Tue, Apr 9, 2019 at 6:04 AM Alexandru Stefan ISAILA
>>>>> <aisaila@bitdefender.com> wrote:
>>>>>>
>>>>>> This patch moves common code from p2m_set_altp2m_mem_access() and
>>>>>> p2m_change_altp2m_gfn() into one function
>>>>>>
>>>>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>>>>>
>>>>>> ---
>>>>>> Changes since V2:
>>>>>>            - Change var name from found_in_hostp2m to copied_from_hostp2m
>>>>>>            - Move the type check from altp2m_get_gfn_type_access() to the
>>>>>>            callers.
>>>>>> ---
>>>>>>     xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
>>>>>>     xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
>>>>>>     xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
>>>>>>     3 files changed, 49 insertions(+), 43 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>>>>> index 56c06a4fc6..bf67ddb15a 100644
>>>>>> --- a/xen/arch/x86/mm/mem_access.c
>>>>>> +++ b/xen/arch/x86/mm/mem_access.c
>>>>>> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>>>>>         unsigned int page_order;
>>>>>>         unsigned long gfn_l = gfn_x(gfn);
>>>>>>         int rc;
>>>>>> +    bool copied_from_hostp2m;
>>>>>>
>>>>>> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
>>>>>> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
>>>>>>
>>>>>> -    /* Check host p2m if no valid entry in alternate */
>>>>>>         if ( !mfn_valid(mfn) )
>>>>>> +        return -ESRCH;
>>>>>> +
>>>>>> +    /* If this is a superpage, copy that first */
>>>>>> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )

Regarding the drop of copied_from_hostp2m should this be dropped here as 
well and by that drop it all together?

>>>>>>         {
>>>>>> +        unsigned long mask = ~((1UL << page_order) - 1);
>>>>>> +        gfn_t gfn2 = _gfn(gfn_l & mask);
>>>>>> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>>>>>
>>>>>> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>>>>>> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>>>>>> +        /* Note: currently it is not safe to remap to a shared entry */
>>>>>> +        if ( t != p2m_ram_rw )
>>>>>> +            return -ESRCH;

And if so and regarding the comments from p2m_change_altp2m_gfn should I 
move the ( t != p2m_ram_rw ) check up with the ( !mfn_valid(mfn) ) check?

Alex

>>>>>>
>>>>>> -        rc = -ESRCH;
>>>>>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>>>>>> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>>>>>> +        if ( rc )
>>>>>>                 return rc;
>>>>>> -
>>>>>> -        /* If this is a superpage, copy that first */
>>>>>> -        if ( page_order != PAGE_ORDER_4K )
>>>>>> -        {
>>>>>> -            unsigned long mask = ~((1UL << page_order) - 1);
>>>>>> -            gfn_t gfn2 = _gfn(gfn_l & mask);
>>>>>> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>>>>> -
>>>>>> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>>>>>> -            if ( rc )
>>>>>> -                return rc;
>>>>>> -        }
>>>>>>         }
>>>>>>
>>>>>>         /*
>>>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>>>>> index b9bbb8f485..d38d7c29ca 100644
>>>>>> --- a/xen/arch/x86/mm/p2m.c
>>>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>>>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>>>         mfn_t mfn;
>>>>>>         unsigned int page_order;
>>>>>>         int rc = -EINVAL;
>>>>>> +    bool copied_from_hostp2m;
>>>>>>
>>>>>>         if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>>>>>             return rc;
>>>>>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>>>         p2m_lock(hp2m);
>>>>>>         p2m_lock(ap2m);
>>>>>>
>>>>>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
>>>>>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
>>>>>>
>>>>>>         if ( gfn_eq(new_gfn, INVALID_GFN) )
>>>>>>         {
>>>>>> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>>>             goto out;
>>>>>>         }
>>>>>>
>>>>>> -    /* Check host p2m if no valid entry in alternate */
>>>>>> -    if ( !mfn_valid(mfn) )
>>>>>> -    {
>>>>>> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
>>>>>> -                                    P2M_ALLOC, &page_order, 0);
>>>>>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
>>>>>
>>>>> Is this check correct? Why do you want to get out only when type is
>>>>> non-rw *and* it's copied from the hostp2m? You could have non-rw
>>>>> entries like mmio in the altp2m that were lazily copied and I don't
>>>>> think we want to allow remapping to those either.
>>>>
>>>> I just copied the functionality. If this is needed I will add a || t !=
>>>> p2m_mmio_dm and p2m_mmio_direct.
>>>
>>> My problem is with the && copied_form_hostp2m part. Why is that a criteria?
>>
>> The (t != p2m_ram_rw) check was done only for the get from hostp2m.
>>
>> If you think that I should do the check for all mfns (hostp2 and altp2m)
>> then I can drop the copied_from_hostp2m bool and add mmio check.
> 
> I think you should just drop the && copied_from_hostp2m part of it.
> Remappings should only be allowed for p2m_ram_rw type, no matter which
> p2m its coming from.
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/3] x86/mm: Introduce altp2m_set_entry_by_page_order

[George Dunlap]

On 4/9/19 1:03 PM, Alexandru Stefan ISAILA wrote:
> This patch moves common code from p2m_set_altp2m_mem_access() and
> p2m_change_altp2m_gfn() into one function
> 
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> ---
>  xen/arch/x86/mm/mem_access.c |  2 +-
>  xen/include/asm-x86/p2m.h    | 11 +++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index bf67ddb15a..6a22512555 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -279,7 +279,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>          gfn_t gfn2 = _gfn(gfn_l & mask);
>          mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>  
> -        /* Note: currently it is not safe to remap to a shared entry */
> +	/* Note: currently it is not safe to remap to a shared entry */
>          if ( t != p2m_ram_rw )
>              return -ESRCH;
>  
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 6de1546d76..90a6c135a7 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -467,6 +467,17 @@ static inline mfn_t altp2m_get_gfn_type_access(
>      return mfn;
>  }
>  
> +static inline int altp2m_set_entry_by_page_order(
> +    struct p2m_domain *ap2m, unsigned long gfn,  mfn_t mfn,
> +    unsigned int page_order, p2m_type_t t, p2m_access_t a)

This function doesn't seem to be called anywhere in this series.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/3] x86/mm: Introduce altp2m_set_entry_by_page_order

[George Dunlap]

On 4/9/19 1:03 PM, Alexandru Stefan ISAILA wrote:
> This patch moves common code from p2m_set_altp2m_mem_access() and
> p2m_change_altp2m_gfn() into one function
> 
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> ---
>  xen/arch/x86/mm/mem_access.c |  2 +-
>  xen/include/asm-x86/p2m.h    | 11 +++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index bf67ddb15a..6a22512555 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -279,7 +279,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>          gfn_t gfn2 = _gfn(gfn_l & mask);
>          mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>  
> -        /* Note: currently it is not safe to remap to a shared entry */
> +	/* Note: currently it is not safe to remap to a shared entry */
>          if ( t != p2m_ram_rw )
>              return -ESRCH;
>  
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 6de1546d76..90a6c135a7 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -467,6 +467,17 @@ static inline mfn_t altp2m_get_gfn_type_access(
>      return mfn;
>  }
>  
> +static inline int altp2m_set_entry_by_page_order(
> +    struct p2m_domain *ap2m, unsigned long gfn,  mfn_t mfn,
> +    unsigned int page_order, p2m_type_t t, p2m_access_t a)

This function doesn't seem to be called anywhere in this series.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/3] x86/mm: Introduce altp2m_set_entry_by_page_order

[Alexandru Stefan ISAILA]

On 10.04.2019 17:18, George Dunlap wrote:
> On 4/9/19 1:03 PM, Alexandru Stefan ISAILA wrote:
>> This patch moves common code from p2m_set_altp2m_mem_access() and
>> p2m_change_altp2m_gfn() into one function
>>
>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>> ---
>>   xen/arch/x86/mm/mem_access.c |  2 +-
>>   xen/include/asm-x86/p2m.h    | 11 +++++++++++
>>   2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index bf67ddb15a..6a22512555 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -279,7 +279,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>           gfn_t gfn2 = _gfn(gfn_l & mask);
>>           mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>   
>> -        /* Note: currently it is not safe to remap to a shared entry */
>> +	/* Note: currently it is not safe to remap to a shared entry */
>>           if ( t != p2m_ram_rw )
>>               return -ESRCH;
>>   
>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>> index 6de1546d76..90a6c135a7 100644
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -467,6 +467,17 @@ static inline mfn_t altp2m_get_gfn_type_access(
>>       return mfn;
>>   }
>>   
>> +static inline int altp2m_set_entry_by_page_order(
>> +    struct p2m_domain *ap2m, unsigned long gfn,  mfn_t mfn,
>> +    unsigned int page_order, p2m_type_t t, p2m_access_t a)
> 
> This function doesn't seem to be called anywhere in this series.

Yes I saw that yesterday after sending the patch. The call got lost in 
the  re-base/merge process. Sorry about that, it will go on again in v4

Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/3] x86/mm: Introduce altp2m_set_entry_by_page_order

[Alexandru Stefan ISAILA]

On 10.04.2019 17:18, George Dunlap wrote:
> On 4/9/19 1:03 PM, Alexandru Stefan ISAILA wrote:
>> This patch moves common code from p2m_set_altp2m_mem_access() and
>> p2m_change_altp2m_gfn() into one function
>>
>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>> ---
>>   xen/arch/x86/mm/mem_access.c |  2 +-
>>   xen/include/asm-x86/p2m.h    | 11 +++++++++++
>>   2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index bf67ddb15a..6a22512555 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -279,7 +279,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>           gfn_t gfn2 = _gfn(gfn_l & mask);
>>           mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>   
>> -        /* Note: currently it is not safe to remap to a shared entry */
>> +	/* Note: currently it is not safe to remap to a shared entry */
>>           if ( t != p2m_ram_rw )
>>               return -ESRCH;
>>   
>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>> index 6de1546d76..90a6c135a7 100644
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -467,6 +467,17 @@ static inline mfn_t altp2m_get_gfn_type_access(
>>       return mfn;
>>   }
>>   
>> +static inline int altp2m_set_entry_by_page_order(
>> +    struct p2m_domain *ap2m, unsigned long gfn,  mfn_t mfn,
>> +    unsigned int page_order, p2m_type_t t, p2m_access_t a)
> 
> This function doesn't seem to be called anywhere in this series.

Yes I saw that yesterday after sending the patch. The call got lost in 
the  re-base/merge process. Sorry about that, it will go on again in v4

Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 7401 bytes --]

On 4/9/19 1:03 PM, Alexandru Stefan ISAILA wrote:
> This patch moves common code from p2m_set_altp2m_mem_access() and
> p2m_change_altp2m_gfn() into one function
> 
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

This patch contains a lot of behavioral changes which aren't mentioned
or explained.  For instance...

> ---
> Changes since V2:
> 	- Change var name from found_in_hostp2m to copied_from_hostp2m
> 	- Move the type check from altp2m_get_gfn_type_access() to the
> 	callers.
> ---
>  xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
>  xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
>  xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
>  3 files changed, 49 insertions(+), 43 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 56c06a4fc6..bf67ddb15a 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>      unsigned int page_order;
>      unsigned long gfn_l = gfn_x(gfn);
>      int rc;
> +    bool copied_from_hostp2m;
>  
> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
>  
> -    /* Check host p2m if no valid entry in alternate */
>      if ( !mfn_valid(mfn) )
> +        return -ESRCH;
> +
> +    /* If this is a superpage, copy that first */
> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
>      {
> +        unsigned long mask = ~((1UL << page_order) - 1);
> +        gfn_t gfn2 = _gfn(gfn_l & mask);
> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>  
> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> +        /* Note: currently it is not safe to remap to a shared entry */
> +        if ( t != p2m_ram_rw )
> +            return -ESRCH;
>  
> -        rc = -ESRCH;
> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> +        if ( rc )
>              return rc;
> -
> -        /* If this is a superpage, copy that first */
> -        if ( page_order != PAGE_ORDER_4K )
> -        {
> -            unsigned long mask = ~((1UL << page_order) - 1);
> -            gfn_t gfn2 = _gfn(gfn_l & mask);
> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> -
> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> -            if ( rc )
> -                return rc;
> -        }
>      }
>  
>      /*
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index b9bbb8f485..d38d7c29ca 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>      mfn_t mfn;
>      unsigned int page_order;
>      int rc = -EINVAL;
> +    bool copied_from_hostp2m;
>  
>      if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>          return rc;
> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>      p2m_lock(hp2m);
>      p2m_lock(ap2m);
>  
> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);

Before, if new_gfn was INVALID_GFN, then the host p2m wasn't read at
all.  Now, the hostp2m will have __get_gfn_type_access() called with
P2M_ALLOC | P2M_UNSHARE.  Is that change intentional, and if so, why?

>  
>      if ( gfn_eq(new_gfn, INVALID_GFN) )
>      {
> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>          goto out;
>      }
>  
> -    /* Check host p2m if no valid entry in alternate */
> -    if ( !mfn_valid(mfn) )
> -    {
> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
> -                                    P2M_ALLOC, &page_order, 0);
> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
> +         goto out;
>  
> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> -            goto out;
> -
> -        /* If this is a superpage, copy that first */
> -        if ( page_order != PAGE_ORDER_4K )
> -        {
> -            gfn_t gfn;
> -            unsigned long mask;
> +    /* If this is a superpage, copy that first */
> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
> +    {
> +        gfn_t gfn;
> +        unsigned long mask;
>  
> -            mask = ~((1UL << page_order) - 1);
> -            gfn = _gfn(gfn_x(old_gfn) & mask);
> -            mfn = _mfn(mfn_x(mfn) & mask);
> +        mask = ~((1UL << page_order) - 1);
> +        gfn = _gfn(gfn_x(old_gfn) & mask);
> +        mfn = _mfn(mfn_x(mfn) & mask);
>  
> -            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
> -                goto out;
> -        }
> +        if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
> +            goto out;
>      }
>  
> -    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
> -
> -    if ( !mfn_valid(mfn) )
> -        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &copied_from_hostp2m);

Similiarly here: Before this patch, hp2m->get_entry() is called
directly; after this patch, we go through __get_gfn_type_access(), which
adds some extra code, and will also unshare / allocate.  Is that
intentional, and if so, why?

>  
>      /* Note: currently it is not safe to remap to a shared entry */
> -    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
>          goto out;
>  
>      if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 2801a8ccca..6de1546d76 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -448,6 +448,25 @@ static inline mfn_t __nonnull(3) get_gfn_type(
>      return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL);
>  }
>  
> +static inline mfn_t altp2m_get_gfn_type_access(
> +    struct p2m_domain *ap2m, gfn_t gfn, p2m_type_t *t, p2m_access_t *a,
> +    unsigned int *page_order, bool *copied_from_hostp2m)
> +{
> +    mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
> +
> +    *copied_from_hostp2m = false;
> +
> +    /* Check host p2m if no valid entry in alternate */
> +    if ( !mfn_valid(mfn) )
> +    {
> +        mfn = __get_gfn_type_access(p2m_get_hostp2m(ap2m->domain), gfn_x(gfn), t, a,
> +                                    P2M_ALLOC | P2M_UNSHARE, page_order, false);
> +        *copied_from_hostp2m = mfn_valid(mfn);
> +    }
> +
> +    return mfn;
> +}

Given that the main goal here seems to be to clean up the interface, I'm
not clear why you don't have this function do both the "host
see-through" and the prepopulation.  Would something like the attached
work (not even compile tested)?

(To be clear, this is just meant to be a sketch so you can see what I'm
talking about; if you were to use it you'd need to fix it up
appropriately, including considering whether "seethrough" is an
appropriate name for the function.)

 -George

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-altp2m-Add-a-function-to-automatically-handle-copyin.patch --]
[-- Type: text/x-patch; name="0001-altp2m-Add-a-function-to-automatically-handle-copyin.patch", Size: 5966 bytes --]

From 45c141269c81c46836bd065bf0da15cad2881011 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Wed, 10 Apr 2019 16:43:30 +0100
Subject: [PATCH] altp2m: Add a function to automatically handle copying /
 prepopulating from hostpm

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
 xen/arch/x86/mm/mem_access.c | 65 +++++++++++++++++++++++++-----------
 xen/arch/x86/mm/p2m.c        | 38 ++++-----------------
 2 files changed, 52 insertions(+), 51 deletions(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index a144bb0ce4..253ec94d1b 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -255,43 +255,70 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
     return (p2ma != p2m_access_n2rwx);
 }
 
-int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
-                              struct p2m_domain *ap2m, p2m_access_t a,
-                              gfn_t gfn)
+static int get_altp2m_entry(struct p2m_domain *ap2m,
+                            gfn_t gfn, mfn_t *mfn, p2m_type_t *t,
+                            p2m_access_t *a, bool prepopulate)
 {
-    mfn_t mfn;
-    p2m_type_t t;
-    p2m_access_t old_a;
-    unsigned int page_order;
-    unsigned long gfn_l = gfn_x(gfn);
-    int rc;
-
-    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
+    *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
 
     /* Check host p2m if no valid entry in alternate */
-    if ( !mfn_valid(mfn) )
+    if ( !mfn_valid(*mfn) )
     {
+        struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain);
+        unsigned int page_order;
+        int rc;
 
-        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
-                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
+        *mfn = __get_gfn_type_access(hp2m, gfn, t, old_a,
+                                     P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
 
         rc = -ESRCH;
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
+        if ( !mfn_valid(*mfn) || *t != p2m_ram_rw )
             return rc;
 
         /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
+        if ( prepopulate && page_order != PAGE_ORDER_4K )
         {
             unsigned long mask = ~((1UL << page_order) - 1);
-            gfn_t gfn2 = _gfn(gfn_l & mask);
-            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
+            gfn_t gfn_aligned = _gfn(gfn & mask);
+            mfn_t mfn_aligned = _mfn(mfn_x(mfn) & mask);
 
-            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
+            rc = ap2m->set_entry(ap2m, gfn_aligned, mfn_aligned, page_order, t, old_a, 1);
             if ( rc )
                 return rc;
         }
     }
 
+    return 0;
+}
+
+int altp2m_get_entry_seethrough(struct p2m_domain *ap2m,
+                                        gfn_t gfn, mfn_t *mfn, p2m_type_t *t,
+                                        p2m_access_t *a)
+{
+    return get_altp2m_entry(ap2m, gfn, mfn, t, a, false);
+}
+
+int altp2m_get_entry_prepopulate(struct p2m_domain *ap2m,
+                                        gfn_t gfn, mfn_t *mfn, p2m_type_t *t,
+                                        p2m_access_t *a)
+{
+    return get_altp2m_entry(ap2m, gfn, mfn, t, a, true);
+}
+
+int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
+                              struct p2m_domain *ap2m, p2m_access_t a,
+                              gfn_t gfn)
+{
+    mfn_t mfn;
+    p2m_type_t t;
+    p2m_access_t old_a;
+    unsigned long gfn_l = gfn_x(gfn);
+    int rc;
+
+    rc = alt2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a);
+    if ( rc )
+        return rc;
+
     /*
      * Inherit the old suppress #VE bit value if it is already set, or set it
      * to 1 otherwise
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b9bbb8f485..d9f7135be5 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2636,47 +2636,21 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     p2m_lock(hp2m);
     p2m_lock(ap2m);
 
-    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
-
     if ( gfn_eq(new_gfn, INVALID_GFN) )
     {
+        mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
         if ( mfn_valid(mfn) )
             p2m_remove_page(ap2m, gfn_x(old_gfn), mfn_x(mfn), PAGE_ORDER_4K);
         rc = 0;
         goto out;
     }
 
-    /* Check host p2m if no valid entry in alternate */
-    if ( !mfn_valid(mfn) )
-    {
-        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
-                                    P2M_ALLOC, &page_order, 0);
-
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
-            goto out;
-
-        /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
-        {
-            gfn_t gfn;
-            unsigned long mask;
-
-            mask = ~((1UL << page_order) - 1);
-            gfn = _gfn(gfn_x(old_gfn) & mask);
-            mfn = _mfn(mfn_x(mfn) & mask);
-
-            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
-                goto out;
-        }
-    }
-
-    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
-
-    if ( !mfn_valid(mfn) )
-        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
+    rc = altp2m_get_entry_prepopulate(ap2m, old_gfn, &mfn, &t, &a);
+    if ( rc )
+        goto out;
 
-    /* Note: currently it is not safe to remap to a shared entry */
-    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
+    rc = altp2m_get_entry_seethrough(ap2m, new_gfn, &mfn, &t, &a);
+    if ( rc )
         goto out;
 
     if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
-- 
2.20.1


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 7401 bytes --]

On 4/9/19 1:03 PM, Alexandru Stefan ISAILA wrote:
> This patch moves common code from p2m_set_altp2m_mem_access() and
> p2m_change_altp2m_gfn() into one function
> 
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

This patch contains a lot of behavioral changes which aren't mentioned
or explained.  For instance...

> ---
> Changes since V2:
> 	- Change var name from found_in_hostp2m to copied_from_hostp2m
> 	- Move the type check from altp2m_get_gfn_type_access() to the
> 	callers.
> ---
>  xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
>  xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
>  xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
>  3 files changed, 49 insertions(+), 43 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 56c06a4fc6..bf67ddb15a 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>      unsigned int page_order;
>      unsigned long gfn_l = gfn_x(gfn);
>      int rc;
> +    bool copied_from_hostp2m;
>  
> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
>  
> -    /* Check host p2m if no valid entry in alternate */
>      if ( !mfn_valid(mfn) )
> +        return -ESRCH;
> +
> +    /* If this is a superpage, copy that first */
> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
>      {
> +        unsigned long mask = ~((1UL << page_order) - 1);
> +        gfn_t gfn2 = _gfn(gfn_l & mask);
> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>  
> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> +        /* Note: currently it is not safe to remap to a shared entry */
> +        if ( t != p2m_ram_rw )
> +            return -ESRCH;
>  
> -        rc = -ESRCH;
> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> +        if ( rc )
>              return rc;
> -
> -        /* If this is a superpage, copy that first */
> -        if ( page_order != PAGE_ORDER_4K )
> -        {
> -            unsigned long mask = ~((1UL << page_order) - 1);
> -            gfn_t gfn2 = _gfn(gfn_l & mask);
> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> -
> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> -            if ( rc )
> -                return rc;
> -        }
>      }
>  
>      /*
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index b9bbb8f485..d38d7c29ca 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>      mfn_t mfn;
>      unsigned int page_order;
>      int rc = -EINVAL;
> +    bool copied_from_hostp2m;
>  
>      if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>          return rc;
> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>      p2m_lock(hp2m);
>      p2m_lock(ap2m);
>  
> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);

Before, if new_gfn was INVALID_GFN, then the host p2m wasn't read at
all.  Now, the hostp2m will have __get_gfn_type_access() called with
P2M_ALLOC | P2M_UNSHARE.  Is that change intentional, and if so, why?

>  
>      if ( gfn_eq(new_gfn, INVALID_GFN) )
>      {
> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>          goto out;
>      }
>  
> -    /* Check host p2m if no valid entry in alternate */
> -    if ( !mfn_valid(mfn) )
> -    {
> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
> -                                    P2M_ALLOC, &page_order, 0);
> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
> +         goto out;
>  
> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> -            goto out;
> -
> -        /* If this is a superpage, copy that first */
> -        if ( page_order != PAGE_ORDER_4K )
> -        {
> -            gfn_t gfn;
> -            unsigned long mask;
> +    /* If this is a superpage, copy that first */
> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
> +    {
> +        gfn_t gfn;
> +        unsigned long mask;
>  
> -            mask = ~((1UL << page_order) - 1);
> -            gfn = _gfn(gfn_x(old_gfn) & mask);
> -            mfn = _mfn(mfn_x(mfn) & mask);
> +        mask = ~((1UL << page_order) - 1);
> +        gfn = _gfn(gfn_x(old_gfn) & mask);
> +        mfn = _mfn(mfn_x(mfn) & mask);
>  
> -            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
> -                goto out;
> -        }
> +        if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
> +            goto out;
>      }
>  
> -    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
> -
> -    if ( !mfn_valid(mfn) )
> -        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &copied_from_hostp2m);

Similiarly here: Before this patch, hp2m->get_entry() is called
directly; after this patch, we go through __get_gfn_type_access(), which
adds some extra code, and will also unshare / allocate.  Is that
intentional, and if so, why?

>  
>      /* Note: currently it is not safe to remap to a shared entry */
> -    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
>          goto out;
>  
>      if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 2801a8ccca..6de1546d76 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -448,6 +448,25 @@ static inline mfn_t __nonnull(3) get_gfn_type(
>      return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL);
>  }
>  
> +static inline mfn_t altp2m_get_gfn_type_access(
> +    struct p2m_domain *ap2m, gfn_t gfn, p2m_type_t *t, p2m_access_t *a,
> +    unsigned int *page_order, bool *copied_from_hostp2m)
> +{
> +    mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
> +
> +    *copied_from_hostp2m = false;
> +
> +    /* Check host p2m if no valid entry in alternate */
> +    if ( !mfn_valid(mfn) )
> +    {
> +        mfn = __get_gfn_type_access(p2m_get_hostp2m(ap2m->domain), gfn_x(gfn), t, a,
> +                                    P2M_ALLOC | P2M_UNSHARE, page_order, false);
> +        *copied_from_hostp2m = mfn_valid(mfn);
> +    }
> +
> +    return mfn;
> +}

Given that the main goal here seems to be to clean up the interface, I'm
not clear why you don't have this function do both the "host
see-through" and the prepopulation.  Would something like the attached
work (not even compile tested)?

(To be clear, this is just meant to be a sketch so you can see what I'm
talking about; if you were to use it you'd need to fix it up
appropriately, including considering whether "seethrough" is an
appropriate name for the function.)

 -George

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-altp2m-Add-a-function-to-automatically-handle-copyin.patch --]
[-- Type: text/x-patch; name="0001-altp2m-Add-a-function-to-automatically-handle-copyin.patch", Size: 5966 bytes --]

From 45c141269c81c46836bd065bf0da15cad2881011 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Wed, 10 Apr 2019 16:43:30 +0100
Subject: [PATCH] altp2m: Add a function to automatically handle copying /
 prepopulating from hostpm

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
 xen/arch/x86/mm/mem_access.c | 65 +++++++++++++++++++++++++-----------
 xen/arch/x86/mm/p2m.c        | 38 ++++-----------------
 2 files changed, 52 insertions(+), 51 deletions(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index a144bb0ce4..253ec94d1b 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -255,43 +255,70 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
     return (p2ma != p2m_access_n2rwx);
 }
 
-int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
-                              struct p2m_domain *ap2m, p2m_access_t a,
-                              gfn_t gfn)
+static int get_altp2m_entry(struct p2m_domain *ap2m,
+                            gfn_t gfn, mfn_t *mfn, p2m_type_t *t,
+                            p2m_access_t *a, bool prepopulate)
 {
-    mfn_t mfn;
-    p2m_type_t t;
-    p2m_access_t old_a;
-    unsigned int page_order;
-    unsigned long gfn_l = gfn_x(gfn);
-    int rc;
-
-    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
+    *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
 
     /* Check host p2m if no valid entry in alternate */
-    if ( !mfn_valid(mfn) )
+    if ( !mfn_valid(*mfn) )
     {
+        struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain);
+        unsigned int page_order;
+        int rc;
 
-        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
-                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
+        *mfn = __get_gfn_type_access(hp2m, gfn, t, old_a,
+                                     P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
 
         rc = -ESRCH;
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
+        if ( !mfn_valid(*mfn) || *t != p2m_ram_rw )
             return rc;
 
         /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
+        if ( prepopulate && page_order != PAGE_ORDER_4K )
         {
             unsigned long mask = ~((1UL << page_order) - 1);
-            gfn_t gfn2 = _gfn(gfn_l & mask);
-            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
+            gfn_t gfn_aligned = _gfn(gfn & mask);
+            mfn_t mfn_aligned = _mfn(mfn_x(mfn) & mask);
 
-            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
+            rc = ap2m->set_entry(ap2m, gfn_aligned, mfn_aligned, page_order, t, old_a, 1);
             if ( rc )
                 return rc;
         }
     }
 
+    return 0;
+}
+
+int altp2m_get_entry_seethrough(struct p2m_domain *ap2m,
+                                        gfn_t gfn, mfn_t *mfn, p2m_type_t *t,
+                                        p2m_access_t *a)
+{
+    return get_altp2m_entry(ap2m, gfn, mfn, t, a, false);
+}
+
+int altp2m_get_entry_prepopulate(struct p2m_domain *ap2m,
+                                        gfn_t gfn, mfn_t *mfn, p2m_type_t *t,
+                                        p2m_access_t *a)
+{
+    return get_altp2m_entry(ap2m, gfn, mfn, t, a, true);
+}
+
+int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
+                              struct p2m_domain *ap2m, p2m_access_t a,
+                              gfn_t gfn)
+{
+    mfn_t mfn;
+    p2m_type_t t;
+    p2m_access_t old_a;
+    unsigned long gfn_l = gfn_x(gfn);
+    int rc;
+
+    rc = alt2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a);
+    if ( rc )
+        return rc;
+
     /*
      * Inherit the old suppress #VE bit value if it is already set, or set it
      * to 1 otherwise
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b9bbb8f485..d9f7135be5 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2636,47 +2636,21 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     p2m_lock(hp2m);
     p2m_lock(ap2m);
 
-    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
-
     if ( gfn_eq(new_gfn, INVALID_GFN) )
     {
+        mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
         if ( mfn_valid(mfn) )
             p2m_remove_page(ap2m, gfn_x(old_gfn), mfn_x(mfn), PAGE_ORDER_4K);
         rc = 0;
         goto out;
     }
 
-    /* Check host p2m if no valid entry in alternate */
-    if ( !mfn_valid(mfn) )
-    {
-        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
-                                    P2M_ALLOC, &page_order, 0);
-
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
-            goto out;
-
-        /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
-        {
-            gfn_t gfn;
-            unsigned long mask;
-
-            mask = ~((1UL << page_order) - 1);
-            gfn = _gfn(gfn_x(old_gfn) & mask);
-            mfn = _mfn(mfn_x(mfn) & mask);
-
-            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
-                goto out;
-        }
-    }
-
-    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
-
-    if ( !mfn_valid(mfn) )
-        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
+    rc = altp2m_get_entry_prepopulate(ap2m, old_gfn, &mfn, &t, &a);
+    if ( rc )
+        goto out;
 
-    /* Note: currently it is not safe to remap to a shared entry */
-    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
+    rc = altp2m_get_entry_seethrough(ap2m, new_gfn, &mfn, &t, &a);
+    if ( rc )
         goto out;
 
     if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
-- 
2.20.1


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Alexandru Stefan ISAILA]

[-- Attachment #1: Type: text/plain, Size: 8579 bytes --]

Hi George,

Thanks for the patch, I've did some work on it and I've attached a 
version that suits our needs.

I have a few comments/questions to you and Tamas because in v3 there was 
a discussion about keeping the "if ( !mfn_valid(*mfn) || *t != 
p2m_ram_rw )" check inside the new function or let the caller do this 
check. If we let the caller do it then we need to used a bool to signal 
if the result was found in hostp2m (this is to keep the current 
functionality).

So the main questions are if we need that check? and where should it be?

On 10.04.2019 19:02, George Dunlap wrote:
> On 4/9/19 1:03 PM, Alexandru Stefan ISAILA wrote:
>> This patch moves common code from p2m_set_altp2m_mem_access() and
>> p2m_change_altp2m_gfn() into one function
>>
>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> 
> This patch contains a lot of behavioral changes which aren't mentioned
> or explained.  For instance...
> 
>> ---
>> Changes since V2:
>> 	- Change var name from found_in_hostp2m to copied_from_hostp2m
>> 	- Move the type check from altp2m_get_gfn_type_access() to the
>> 	callers.
>> ---
>>   xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
>>   xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
>>   xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
>>   3 files changed, 49 insertions(+), 43 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index 56c06a4fc6..bf67ddb15a 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>       unsigned int page_order;
>>       unsigned long gfn_l = gfn_x(gfn);
>>       int rc;
>> +    bool copied_from_hostp2m;
>>   
>> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
>>   
>> -    /* Check host p2m if no valid entry in alternate */
>>       if ( !mfn_valid(mfn) )
>> +        return -ESRCH;
>> +
>> +    /* If this is a superpage, copy that first */
>> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
>>       {
>> +        unsigned long mask = ~((1UL << page_order) - 1);
>> +        gfn_t gfn2 = _gfn(gfn_l & mask);
>> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>   
>> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>> +        /* Note: currently it is not safe to remap to a shared entry */
>> +        if ( t != p2m_ram_rw )
>> +            return -ESRCH;
>>   
>> -        rc = -ESRCH;
>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>> +        if ( rc )
>>               return rc;
>> -
>> -        /* If this is a superpage, copy that first */
>> -        if ( page_order != PAGE_ORDER_4K )
>> -        {
>> -            unsigned long mask = ~((1UL << page_order) - 1);
>> -            gfn_t gfn2 = _gfn(gfn_l & mask);
>> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>> -
>> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>> -            if ( rc )
>> -                return rc;
>> -        }
>>       }
>>   
>>       /*
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index b9bbb8f485..d38d7c29ca 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>       mfn_t mfn;
>>       unsigned int page_order;
>>       int rc = -EINVAL;
>> +    bool copied_from_hostp2m;
>>   
>>       if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>           return rc;
>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>       p2m_lock(hp2m);
>>       p2m_lock(ap2m);
>>   
>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
> 
> Before, if new_gfn was INVALID_GFN, then the host p2m wasn't read at
> all.  Now, the hostp2m will have __get_gfn_type_access() called with
> P2M_ALLOC | P2M_UNSHARE.  Is that change intentional, and if so, why?

This has been requested by Tamas in v2.

> 
>>   
>>       if ( gfn_eq(new_gfn, INVALID_GFN) )
>>       {
>> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>           goto out;
>>       }
>>   
>> -    /* Check host p2m if no valid entry in alternate */
>> -    if ( !mfn_valid(mfn) )
>> -    {
>> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
>> -                                    P2M_ALLOC, &page_order, 0);
>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
>> +         goto out;
>>   
>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>> -            goto out;
>> -
>> -        /* If this is a superpage, copy that first */
>> -        if ( page_order != PAGE_ORDER_4K )
>> -        {
>> -            gfn_t gfn;
>> -            unsigned long mask;
>> +    /* If this is a superpage, copy that first */
>> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
>> +    {
>> +        gfn_t gfn;
>> +        unsigned long mask;
>>   
>> -            mask = ~((1UL << page_order) - 1);
>> -            gfn = _gfn(gfn_x(old_gfn) & mask);
>> -            mfn = _mfn(mfn_x(mfn) & mask);
>> +        mask = ~((1UL << page_order) - 1);
>> +        gfn = _gfn(gfn_x(old_gfn) & mask);
>> +        mfn = _mfn(mfn_x(mfn) & mask);
>>   
>> -            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
>> -                goto out;
>> -        }
>> +        if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
>> +            goto out;
>>       }
>>   
>> -    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
>> -
>> -    if ( !mfn_valid(mfn) )
>> -        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &copied_from_hostp2m);
> 
> Similiarly here: Before this patch, hp2m->get_entry() is called
> directly; after this patch, we go through __get_gfn_type_access(), which
> adds some extra code, and will also unshare / allocate.  Is that
> intentional, and if so, why?
> 
>>   
>>       /* Note: currently it is not safe to remap to a shared entry */
>> -    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
>>           goto out;
>>   
>>       if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>> index 2801a8ccca..6de1546d76 100644
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -448,6 +448,25 @@ static inline mfn_t __nonnull(3) get_gfn_type(
>>       return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL);
>>   }
>>   
>> +static inline mfn_t altp2m_get_gfn_type_access(
>> +    struct p2m_domain *ap2m, gfn_t gfn, p2m_type_t *t, p2m_access_t *a,
>> +    unsigned int *page_order, bool *copied_from_hostp2m)
>> +{
>> +    mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
>> +
>> +    *copied_from_hostp2m = false;
>> +
>> +    /* Check host p2m if no valid entry in alternate */
>> +    if ( !mfn_valid(mfn) )
>> +    {
>> +        mfn = __get_gfn_type_access(p2m_get_hostp2m(ap2m->domain), gfn_x(gfn), t, a,
>> +                                    P2M_ALLOC | P2M_UNSHARE, page_order, false);
>> +        *copied_from_hostp2m = mfn_valid(mfn);
>> +    }
>> +
>> +    return mfn;
>> +}
> 
> Given that the main goal here seems to be to clean up the interface, I'm
> not clear why you don't have this function do both the "host
> see-through" and the prepopulation.  Would something like the attached
> work (not even compile tested)?

I was thinking of going with  altp2m_get_entry_direct over see-through

> 
> (To be clear, this is just meant to be a sketch so you can see what I'm
> talking about; if you were to use it you'd need to fix it up
> appropriately, including considering whether "seethrough" is an
> appropriate name for the function.)
> 

Alex

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-altp2m-Add-a-function-to-automatically-handle-copyin.patch --]
[-- Type: text/x-patch; name="0001-altp2m-Add-a-function-to-automatically-handle-copyin.patch", Size: 7513 bytes --]

From c1b1f554ce343763add0aad479edcd7e02fc565f Mon Sep 17 00:00:00 2001
From: Alexandru Isaila <aisaila@bitdefender.com>
Date: Thu, 11 Apr 2019 14:39:31 +0300
Subject: [PATCH] altp2m: Add a function to automatically handle copying

prepopulating from hostpm

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
 xen/arch/x86/mm/mem_access.c | 30 ++--------------
 xen/arch/x86/mm/p2m.c        | 84 ++++++++++++++++++++++++--------------------
 xen/include/asm-x86/p2m.h    | 17 +++++++++
 3 files changed, 66 insertions(+), 65 deletions(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index a144bb0..ddfe016 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
     mfn_t mfn;
     p2m_type_t t;
     p2m_access_t old_a;
-    unsigned int page_order;
-    unsigned long gfn_l = gfn_x(gfn);
     int rc;
 
-    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
-
-    /* Check host p2m if no valid entry in alternate */
-    if ( !mfn_valid(mfn) )
-    {
-
-        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
-                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
-
-        rc = -ESRCH;
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
-            return rc;
-
-        /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
-        {
-            unsigned long mask = ~((1UL << page_order) - 1);
-            gfn_t gfn2 = _gfn(gfn_l & mask);
-            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
-
-            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
-            if ( rc )
-                return rc;
-        }
-    }
+    rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a);
+    if ( rc )
+        return rc;
 
     /*
      * Inherit the old suppress #VE bit value if it is already set, or set it
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 9e81a30..aea200f 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m)
         mm_write_unlock(&p2m->lock);
 }
 
+int get_altp2m_entry(struct p2m_domain *ap2m,
+                            gfn_t gfn, mfn_t *mfn, p2m_type_t *t,
+                            p2m_access_t *a, bool prepopulate)
+{
+    *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
+
+    /* Check host p2m if no valid entry in alternate */
+    if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) )
+    {
+        struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain);
+        unsigned int page_order;
+        int rc;
+
+        *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a,
+                                     P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
+
+        rc = -ESRCH;
+        if ( !mfn_valid(*mfn) || *t != p2m_ram_rw )
+            return rc;
+
+        /* If this is a superpage, copy that first */
+        if ( prepopulate && page_order != PAGE_ORDER_4K )
+        {
+            unsigned long mask = ~((1UL << page_order) - 1);
+            gfn_t gfn_aligned = _gfn(gfn_x(gfn) & mask);
+            mfn_t mfn_aligned = _mfn(mfn_x(*mfn) & mask);
+
+            rc = ap2m->set_entry(ap2m, gfn_aligned, mfn_aligned, page_order, *t, *a, 1);
+            if ( rc )
+                return rc;
+        }
+    }
+
+    return 0;
+}
+
+
 mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn_l,
                     p2m_type_t *t, p2m_access_t *a, p2m_query_t q,
                     unsigned int *page_order, bool_t locked)
@@ -2618,7 +2655,6 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     p2m_access_t a;
     p2m_type_t t;
     mfn_t mfn;
-    unsigned int page_order;
     int rc = -EINVAL;
 
     if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
@@ -2630,47 +2666,21 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     p2m_lock(hp2m);
     p2m_lock(ap2m);
 
-    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
-
     if ( gfn_eq(new_gfn, INVALID_GFN) )
     {
+        mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
         if ( mfn_valid(mfn) )
             p2m_remove_page(ap2m, gfn_x(old_gfn), mfn_x(mfn), PAGE_ORDER_4K);
         rc = 0;
         goto out;
     }
 
-    /* Check host p2m if no valid entry in alternate */
-    if ( !mfn_valid(mfn) )
-    {
-        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
-                                    P2M_ALLOC, &page_order, 0);
-
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
-            goto out;
-
-        /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
-        {
-            gfn_t gfn;
-            unsigned long mask;
-
-            mask = ~((1UL << page_order) - 1);
-            gfn = _gfn(gfn_x(old_gfn) & mask);
-            mfn = _mfn(mfn_x(mfn) & mask);
-
-            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
-                goto out;
-        }
-    }
-
-    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
-
-    if ( !mfn_valid(mfn) )
-        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
+    rc = altp2m_get_entry_prepopulate(ap2m, old_gfn, &mfn, &t, &a);
+    if ( rc )
+        goto out;
 
-    /* Note: currently it is not safe to remap to a shared entry */
-    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
+    rc = altp2m_get_entry_seethrough(ap2m, new_gfn, &mfn, &t, &a);
+    if ( rc )
         goto out;
 
     if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
@@ -3002,12 +3012,10 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
     if ( ap2m )
         p2m_lock(ap2m);
 
-    mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
-    if ( !mfn_valid(mfn) )
-    {
-        rc = -ESRCH;
+    rc = altp2m_get_entry_seethrough(p2m, gfn, &mfn, &t, &a);
+
+    if ( rc )
         goto out;
-    }
 
     rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, t, a, suppress_ve);
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 2801a8c..d3613c0 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -514,6 +514,23 @@ static inline unsigned long mfn_to_gfn(struct domain *d, mfn_t mfn)
         return mfn_x(mfn);
 }
 
+int get_altp2m_entry(struct p2m_domain *ap2m, gfn_t gfn, mfn_t *mfn,
+                     p2m_type_t *t, p2m_access_t *a, bool prepopulate);
+
+static inline int altp2m_get_entry_seethrough(struct p2m_domain *ap2m,
+                                              gfn_t gfn, mfn_t *mfn,
+                                              p2m_type_t *t, p2m_access_t *a)
+{
+    return get_altp2m_entry(ap2m, gfn, mfn, t, a, false);
+}
+
+static inline int altp2m_get_entry_prepopulate(struct p2m_domain *ap2m,
+                                               gfn_t gfn, mfn_t *mfn,
+                                               p2m_type_t *t, p2m_access_t *a)
+{
+    return get_altp2m_entry(ap2m, gfn, mfn, t, a, true);
+}
+
 /* Deadlock-avoidance scheme when calling get_gfn on different gfn's */
 struct two_gfns {
     struct domain *first_domain, *second_domain;
-- 
2.7.4


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Alexandru Stefan ISAILA]

[-- Attachment #1: Type: text/plain, Size: 8579 bytes --]

Hi George,

Thanks for the patch, I've did some work on it and I've attached a 
version that suits our needs.

I have a few comments/questions to you and Tamas because in v3 there was 
a discussion about keeping the "if ( !mfn_valid(*mfn) || *t != 
p2m_ram_rw )" check inside the new function or let the caller do this 
check. If we let the caller do it then we need to used a bool to signal 
if the result was found in hostp2m (this is to keep the current 
functionality).

So the main questions are if we need that check? and where should it be?

On 10.04.2019 19:02, George Dunlap wrote:
> On 4/9/19 1:03 PM, Alexandru Stefan ISAILA wrote:
>> This patch moves common code from p2m_set_altp2m_mem_access() and
>> p2m_change_altp2m_gfn() into one function
>>
>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> 
> This patch contains a lot of behavioral changes which aren't mentioned
> or explained.  For instance...
> 
>> ---
>> Changes since V2:
>> 	- Change var name from found_in_hostp2m to copied_from_hostp2m
>> 	- Move the type check from altp2m_get_gfn_type_access() to the
>> 	callers.
>> ---
>>   xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
>>   xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
>>   xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
>>   3 files changed, 49 insertions(+), 43 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index 56c06a4fc6..bf67ddb15a 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>       unsigned int page_order;
>>       unsigned long gfn_l = gfn_x(gfn);
>>       int rc;
>> +    bool copied_from_hostp2m;
>>   
>> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);
>>   
>> -    /* Check host p2m if no valid entry in alternate */
>>       if ( !mfn_valid(mfn) )
>> +        return -ESRCH;
>> +
>> +    /* If this is a superpage, copy that first */
>> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
>>       {
>> +        unsigned long mask = ~((1UL << page_order) - 1);
>> +        gfn_t gfn2 = _gfn(gfn_l & mask);
>> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>   
>> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>> +        /* Note: currently it is not safe to remap to a shared entry */
>> +        if ( t != p2m_ram_rw )
>> +            return -ESRCH;
>>   
>> -        rc = -ESRCH;
>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>> +        if ( rc )
>>               return rc;
>> -
>> -        /* If this is a superpage, copy that first */
>> -        if ( page_order != PAGE_ORDER_4K )
>> -        {
>> -            unsigned long mask = ~((1UL << page_order) - 1);
>> -            gfn_t gfn2 = _gfn(gfn_l & mask);
>> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>> -
>> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>> -            if ( rc )
>> -                return rc;
>> -        }
>>       }
>>   
>>       /*
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index b9bbb8f485..d38d7c29ca 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>       mfn_t mfn;
>>       unsigned int page_order;
>>       int rc = -EINVAL;
>> +    bool copied_from_hostp2m;
>>   
>>       if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>           return rc;
>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>       p2m_lock(hp2m);
>>       p2m_lock(ap2m);
>>   
>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
> 
> Before, if new_gfn was INVALID_GFN, then the host p2m wasn't read at
> all.  Now, the hostp2m will have __get_gfn_type_access() called with
> P2M_ALLOC | P2M_UNSHARE.  Is that change intentional, and if so, why?

This has been requested by Tamas in v2.

> 
>>   
>>       if ( gfn_eq(new_gfn, INVALID_GFN) )
>>       {
>> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>           goto out;
>>       }
>>   
>> -    /* Check host p2m if no valid entry in alternate */
>> -    if ( !mfn_valid(mfn) )
>> -    {
>> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
>> -                                    P2M_ALLOC, &page_order, 0);
>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
>> +         goto out;
>>   
>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>> -            goto out;
>> -
>> -        /* If this is a superpage, copy that first */
>> -        if ( page_order != PAGE_ORDER_4K )
>> -        {
>> -            gfn_t gfn;
>> -            unsigned long mask;
>> +    /* If this is a superpage, copy that first */
>> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
>> +    {
>> +        gfn_t gfn;
>> +        unsigned long mask;
>>   
>> -            mask = ~((1UL << page_order) - 1);
>> -            gfn = _gfn(gfn_x(old_gfn) & mask);
>> -            mfn = _mfn(mfn_x(mfn) & mask);
>> +        mask = ~((1UL << page_order) - 1);
>> +        gfn = _gfn(gfn_x(old_gfn) & mask);
>> +        mfn = _mfn(mfn_x(mfn) & mask);
>>   
>> -            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
>> -                goto out;
>> -        }
>> +        if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
>> +            goto out;
>>       }
>>   
>> -    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
>> -
>> -    if ( !mfn_valid(mfn) )
>> -        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &copied_from_hostp2m);
> 
> Similiarly here: Before this patch, hp2m->get_entry() is called
> directly; after this patch, we go through __get_gfn_type_access(), which
> adds some extra code, and will also unshare / allocate.  Is that
> intentional, and if so, why?
> 
>>   
>>       /* Note: currently it is not safe to remap to a shared entry */
>> -    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
>>           goto out;
>>   
>>       if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>> index 2801a8ccca..6de1546d76 100644
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -448,6 +448,25 @@ static inline mfn_t __nonnull(3) get_gfn_type(
>>       return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL);
>>   }
>>   
>> +static inline mfn_t altp2m_get_gfn_type_access(
>> +    struct p2m_domain *ap2m, gfn_t gfn, p2m_type_t *t, p2m_access_t *a,
>> +    unsigned int *page_order, bool *copied_from_hostp2m)
>> +{
>> +    mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
>> +
>> +    *copied_from_hostp2m = false;
>> +
>> +    /* Check host p2m if no valid entry in alternate */
>> +    if ( !mfn_valid(mfn) )
>> +    {
>> +        mfn = __get_gfn_type_access(p2m_get_hostp2m(ap2m->domain), gfn_x(gfn), t, a,
>> +                                    P2M_ALLOC | P2M_UNSHARE, page_order, false);
>> +        *copied_from_hostp2m = mfn_valid(mfn);
>> +    }
>> +
>> +    return mfn;
>> +}
> 
> Given that the main goal here seems to be to clean up the interface, I'm
> not clear why you don't have this function do both the "host
> see-through" and the prepopulation.  Would something like the attached
> work (not even compile tested)?

I was thinking of going with  altp2m_get_entry_direct over see-through

> 
> (To be clear, this is just meant to be a sketch so you can see what I'm
> talking about; if you were to use it you'd need to fix it up
> appropriately, including considering whether "seethrough" is an
> appropriate name for the function.)
> 

Alex

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-altp2m-Add-a-function-to-automatically-handle-copyin.patch --]
[-- Type: text/x-patch; name="0001-altp2m-Add-a-function-to-automatically-handle-copyin.patch", Size: 7513 bytes --]

From c1b1f554ce343763add0aad479edcd7e02fc565f Mon Sep 17 00:00:00 2001
From: Alexandru Isaila <aisaila@bitdefender.com>
Date: Thu, 11 Apr 2019 14:39:31 +0300
Subject: [PATCH] altp2m: Add a function to automatically handle copying

prepopulating from hostpm

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
 xen/arch/x86/mm/mem_access.c | 30 ++--------------
 xen/arch/x86/mm/p2m.c        | 84 ++++++++++++++++++++++++--------------------
 xen/include/asm-x86/p2m.h    | 17 +++++++++
 3 files changed, 66 insertions(+), 65 deletions(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index a144bb0..ddfe016 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
     mfn_t mfn;
     p2m_type_t t;
     p2m_access_t old_a;
-    unsigned int page_order;
-    unsigned long gfn_l = gfn_x(gfn);
     int rc;
 
-    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
-
-    /* Check host p2m if no valid entry in alternate */
-    if ( !mfn_valid(mfn) )
-    {
-
-        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
-                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
-
-        rc = -ESRCH;
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
-            return rc;
-
-        /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
-        {
-            unsigned long mask = ~((1UL << page_order) - 1);
-            gfn_t gfn2 = _gfn(gfn_l & mask);
-            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
-
-            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
-            if ( rc )
-                return rc;
-        }
-    }
+    rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a);
+    if ( rc )
+        return rc;
 
     /*
      * Inherit the old suppress #VE bit value if it is already set, or set it
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 9e81a30..aea200f 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m)
         mm_write_unlock(&p2m->lock);
 }
 
+int get_altp2m_entry(struct p2m_domain *ap2m,
+                            gfn_t gfn, mfn_t *mfn, p2m_type_t *t,
+                            p2m_access_t *a, bool prepopulate)
+{
+    *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
+
+    /* Check host p2m if no valid entry in alternate */
+    if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) )
+    {
+        struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain);
+        unsigned int page_order;
+        int rc;
+
+        *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a,
+                                     P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
+
+        rc = -ESRCH;
+        if ( !mfn_valid(*mfn) || *t != p2m_ram_rw )
+            return rc;
+
+        /* If this is a superpage, copy that first */
+        if ( prepopulate && page_order != PAGE_ORDER_4K )
+        {
+            unsigned long mask = ~((1UL << page_order) - 1);
+            gfn_t gfn_aligned = _gfn(gfn_x(gfn) & mask);
+            mfn_t mfn_aligned = _mfn(mfn_x(*mfn) & mask);
+
+            rc = ap2m->set_entry(ap2m, gfn_aligned, mfn_aligned, page_order, *t, *a, 1);
+            if ( rc )
+                return rc;
+        }
+    }
+
+    return 0;
+}
+
+
 mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn_l,
                     p2m_type_t *t, p2m_access_t *a, p2m_query_t q,
                     unsigned int *page_order, bool_t locked)
@@ -2618,7 +2655,6 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     p2m_access_t a;
     p2m_type_t t;
     mfn_t mfn;
-    unsigned int page_order;
     int rc = -EINVAL;
 
     if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
@@ -2630,47 +2666,21 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     p2m_lock(hp2m);
     p2m_lock(ap2m);
 
-    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
-
     if ( gfn_eq(new_gfn, INVALID_GFN) )
     {
+        mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
         if ( mfn_valid(mfn) )
             p2m_remove_page(ap2m, gfn_x(old_gfn), mfn_x(mfn), PAGE_ORDER_4K);
         rc = 0;
         goto out;
     }
 
-    /* Check host p2m if no valid entry in alternate */
-    if ( !mfn_valid(mfn) )
-    {
-        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
-                                    P2M_ALLOC, &page_order, 0);
-
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
-            goto out;
-
-        /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
-        {
-            gfn_t gfn;
-            unsigned long mask;
-
-            mask = ~((1UL << page_order) - 1);
-            gfn = _gfn(gfn_x(old_gfn) & mask);
-            mfn = _mfn(mfn_x(mfn) & mask);
-
-            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
-                goto out;
-        }
-    }
-
-    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
-
-    if ( !mfn_valid(mfn) )
-        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
+    rc = altp2m_get_entry_prepopulate(ap2m, old_gfn, &mfn, &t, &a);
+    if ( rc )
+        goto out;
 
-    /* Note: currently it is not safe to remap to a shared entry */
-    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
+    rc = altp2m_get_entry_seethrough(ap2m, new_gfn, &mfn, &t, &a);
+    if ( rc )
         goto out;
 
     if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
@@ -3002,12 +3012,10 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
     if ( ap2m )
         p2m_lock(ap2m);
 
-    mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
-    if ( !mfn_valid(mfn) )
-    {
-        rc = -ESRCH;
+    rc = altp2m_get_entry_seethrough(p2m, gfn, &mfn, &t, &a);
+
+    if ( rc )
         goto out;
-    }
 
     rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, t, a, suppress_ve);
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 2801a8c..d3613c0 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -514,6 +514,23 @@ static inline unsigned long mfn_to_gfn(struct domain *d, mfn_t mfn)
         return mfn_x(mfn);
 }
 
+int get_altp2m_entry(struct p2m_domain *ap2m, gfn_t gfn, mfn_t *mfn,
+                     p2m_type_t *t, p2m_access_t *a, bool prepopulate);
+
+static inline int altp2m_get_entry_seethrough(struct p2m_domain *ap2m,
+                                              gfn_t gfn, mfn_t *mfn,
+                                              p2m_type_t *t, p2m_access_t *a)
+{
+    return get_altp2m_entry(ap2m, gfn, mfn, t, a, false);
+}
+
+static inline int altp2m_get_entry_prepopulate(struct p2m_domain *ap2m,
+                                               gfn_t gfn, mfn_t *mfn,
+                                               p2m_type_t *t, p2m_access_t *a)
+{
+    return get_altp2m_entry(ap2m, gfn, mfn, t, a, true);
+}
+
 /* Deadlock-avoidance scheme when calling get_gfn on different gfn's */
 struct two_gfns {
     struct domain *first_domain, *second_domain;
-- 
2.7.4


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[George Dunlap]

On 4/11/19 1:17 PM, Alexandru Stefan ISAILA wrote:
>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>> index b9bbb8f485..d38d7c29ca 100644
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>       mfn_t mfn;
>>>       unsigned int page_order;
>>>       int rc = -EINVAL;
>>> +    bool copied_from_hostp2m;
>>>   
>>>       if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>>           return rc;
>>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>       p2m_lock(hp2m);
>>>       p2m_lock(ap2m);
>>>   
>>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
>>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
>>
>> Before, if new_gfn was INVALID_GFN, then the host p2m wasn't read at
>> all.  Now, the hostp2m will have __get_gfn_type_access() called with
>> P2M_ALLOC | P2M_UNSHARE.  Is that change intentional, and if so, why?
> 
> This has been requested by Tamas in v2.

That's nice, but 1) you still haven't answered the question, and 2) it's
not in the changelog.

Literally two weeks ago [1] we had a situation where neither a comment
nor the changelog that introduced a change explained why a decision was
made, and the result was a heated argument between two maintainers about
the safest way to change it, that was only resolved when I went through
the mail archives and read through a fairly long and winding thread from
9 years ago.

That shouldn't happen.  The code + the changelog should give someone
generally familiar with the codebase everything they need to understand
what's going on, and why the change was made.

 -George

[1] marc.info/?i=<689b8f75-20dd-24d9-bd5f-f03a8201b2e2@citrix.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[George Dunlap]

On 4/11/19 1:17 PM, Alexandru Stefan ISAILA wrote:
>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>> index b9bbb8f485..d38d7c29ca 100644
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>       mfn_t mfn;
>>>       unsigned int page_order;
>>>       int rc = -EINVAL;
>>> +    bool copied_from_hostp2m;
>>>   
>>>       if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>>           return rc;
>>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>       p2m_lock(hp2m);
>>>       p2m_lock(ap2m);
>>>   
>>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
>>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
>>
>> Before, if new_gfn was INVALID_GFN, then the host p2m wasn't read at
>> all.  Now, the hostp2m will have __get_gfn_type_access() called with
>> P2M_ALLOC | P2M_UNSHARE.  Is that change intentional, and if so, why?
> 
> This has been requested by Tamas in v2.

That's nice, but 1) you still haven't answered the question, and 2) it's
not in the changelog.

Literally two weeks ago [1] we had a situation where neither a comment
nor the changelog that introduced a change explained why a decision was
made, and the result was a heated argument between two maintainers about
the safest way to change it, that was only resolved when I went through
the mail archives and read through a fairly long and winding thread from
9 years ago.

That shouldn't happen.  The code + the changelog should give someone
generally familiar with the codebase everything they need to understand
what's going on, and why the change was made.

 -George

[1] marc.info/?i=<689b8f75-20dd-24d9-bd5f-f03a8201b2e2@citrix.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Tamas K Lengyel]

On Thu, Apr 11, 2019 at 6:50 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 4/11/19 1:17 PM, Alexandru Stefan ISAILA wrote:
> >>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> >>> index b9bbb8f485..d38d7c29ca 100644
> >>> --- a/xen/arch/x86/mm/p2m.c
> >>> +++ b/xen/arch/x86/mm/p2m.c
> >>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>>       mfn_t mfn;
> >>>       unsigned int page_order;
> >>>       int rc = -EINVAL;
> >>> +    bool copied_from_hostp2m;
> >>>
> >>>       if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
> >>>           return rc;
> >>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>>       p2m_lock(hp2m);
> >>>       p2m_lock(ap2m);
> >>>
> >>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
> >>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
> >>
> >> Before, if new_gfn was INVALID_GFN, then the host p2m wasn't read at
> >> all.  Now, the hostp2m will have __get_gfn_type_access() called with
> >> P2M_ALLOC | P2M_UNSHARE.  Is that change intentional, and if so, why?
> >
> > This has been requested by Tamas in v2.
>
> That's nice, but 1) you still haven't answered the question, and 2) it's
> not in the changelog.

What I requested was that if remapping is being done then both the old
and new gfn's should be unshared in the hostp2m for keeping things
consistent. The page type of old_gfn was already checked whether it's
p2m_ram_rw and bail if it wasn't so functionality-wise this just
simplifies things as a user don't have to request unsharing manually
before remapping. Now, if the new_gfn is invalid it shouldn't query
the hostp2m as that is effectively a request to remove the entry from
the altp2m. But provided that scenario is used only when removing
entries that were previously remapped/copied to the altp2m, those
entries already went through P2M_ALLOC | P2M_UNSHARE before, so it
won't have an affect. But it's also pointless.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Tamas K Lengyel]

On Thu, Apr 11, 2019 at 6:50 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 4/11/19 1:17 PM, Alexandru Stefan ISAILA wrote:
> >>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> >>> index b9bbb8f485..d38d7c29ca 100644
> >>> --- a/xen/arch/x86/mm/p2m.c
> >>> +++ b/xen/arch/x86/mm/p2m.c
> >>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>>       mfn_t mfn;
> >>>       unsigned int page_order;
> >>>       int rc = -EINVAL;
> >>> +    bool copied_from_hostp2m;
> >>>
> >>>       if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
> >>>           return rc;
> >>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>>       p2m_lock(hp2m);
> >>>       p2m_lock(ap2m);
> >>>
> >>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
> >>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
> >>
> >> Before, if new_gfn was INVALID_GFN, then the host p2m wasn't read at
> >> all.  Now, the hostp2m will have __get_gfn_type_access() called with
> >> P2M_ALLOC | P2M_UNSHARE.  Is that change intentional, and if so, why?
> >
> > This has been requested by Tamas in v2.
>
> That's nice, but 1) you still haven't answered the question, and 2) it's
> not in the changelog.

What I requested was that if remapping is being done then both the old
and new gfn's should be unshared in the hostp2m for keeping things
consistent. The page type of old_gfn was already checked whether it's
p2m_ram_rw and bail if it wasn't so functionality-wise this just
simplifies things as a user don't have to request unsharing manually
before remapping. Now, if the new_gfn is invalid it shouldn't query
the hostp2m as that is effectively a request to remove the entry from
the altp2m. But provided that scenario is used only when removing
entries that were previously remapped/copied to the altp2m, those
entries already went through P2M_ALLOC | P2M_UNSHARE before, so it
won't have an affect. But it's also pointless.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Alexandru Stefan ISAILA]

On 11.04.2019 16:28, Tamas K Lengyel wrote:
> On Thu, Apr 11, 2019 at 6:50 AM George Dunlap <george.dunlap@citrix.com> wrote:
>>
>> On 4/11/19 1:17 PM, Alexandru Stefan ISAILA wrote:
>>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>>>> index b9bbb8f485..d38d7c29ca 100644
>>>>> --- a/xen/arch/x86/mm/p2m.c
>>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>>        mfn_t mfn;
>>>>>        unsigned int page_order;
>>>>>        int rc = -EINVAL;
>>>>> +    bool copied_from_hostp2m;
>>>>>
>>>>>        if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>>>>            return rc;
>>>>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>>        p2m_lock(hp2m);
>>>>>        p2m_lock(ap2m);
>>>>>
>>>>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
>>>>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
>>>>
>>>> Before, if new_gfn was INVALID_GFN, then the host p2m wasn't read at
>>>> all.  Now, the hostp2m will have __get_gfn_type_access() called with
>>>> P2M_ALLOC | P2M_UNSHARE.  Is that change intentional, and if so, why?
>>>
>>> This has been requested by Tamas in v2.
>>
>> That's nice, but 1) you still haven't answered the question, and 2) it's
>> not in the changelog.
> 
> What I requested was that if remapping is being done then both the old
> and new gfn's should be unshared in the hostp2m for keeping things
> consistent. The page type of old_gfn was already checked whether it's
> p2m_ram_rw and bail if it wasn't so functionality-wise this just
> simplifies things as a user don't have to request unsharing manually
> before remapping. Now, if the new_gfn is invalid it shouldn't query
> the hostp2m as that is effectively a request to remove the entry from
> the altp2m. But provided that scenario is used only when removing
> entries that were previously remapped/copied to the altp2m, those
> entries already went through P2M_ALLOC | P2M_UNSHARE before, so it
> won't have an affect. But it's also pointless.
> 

If this has been cleared there was one more question, where should the 
"if ( !mfn_valid(*mfn) || *t != p2m_ram_rw )" check end up in the 
attached form of the patch?

Thanks,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Alexandru Stefan ISAILA]

On 11.04.2019 16:28, Tamas K Lengyel wrote:
> On Thu, Apr 11, 2019 at 6:50 AM George Dunlap <george.dunlap@citrix.com> wrote:
>>
>> On 4/11/19 1:17 PM, Alexandru Stefan ISAILA wrote:
>>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>>>> index b9bbb8f485..d38d7c29ca 100644
>>>>> --- a/xen/arch/x86/mm/p2m.c
>>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>>        mfn_t mfn;
>>>>>        unsigned int page_order;
>>>>>        int rc = -EINVAL;
>>>>> +    bool copied_from_hostp2m;
>>>>>
>>>>>        if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>>>>            return rc;
>>>>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>>        p2m_lock(hp2m);
>>>>>        p2m_lock(ap2m);
>>>>>
>>>>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
>>>>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
>>>>
>>>> Before, if new_gfn was INVALID_GFN, then the host p2m wasn't read at
>>>> all.  Now, the hostp2m will have __get_gfn_type_access() called with
>>>> P2M_ALLOC | P2M_UNSHARE.  Is that change intentional, and if so, why?
>>>
>>> This has been requested by Tamas in v2.
>>
>> That's nice, but 1) you still haven't answered the question, and 2) it's
>> not in the changelog.
> 
> What I requested was that if remapping is being done then both the old
> and new gfn's should be unshared in the hostp2m for keeping things
> consistent. The page type of old_gfn was already checked whether it's
> p2m_ram_rw and bail if it wasn't so functionality-wise this just
> simplifies things as a user don't have to request unsharing manually
> before remapping. Now, if the new_gfn is invalid it shouldn't query
> the hostp2m as that is effectively a request to remove the entry from
> the altp2m. But provided that scenario is used only when removing
> entries that were previously remapped/copied to the altp2m, those
> entries already went through P2M_ALLOC | P2M_UNSHARE before, so it
> won't have an affect. But it's also pointless.
> 

If this has been cleared there was one more question, where should the 
"if ( !mfn_valid(*mfn) || *t != p2m_ram_rw )" check end up in the 
attached form of the patch?

Thanks,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Jan Beulich]

>>> On 09.04.19 at 14:03, <aisaila@bitdefender.com> wrote:
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>      unsigned int page_order;
>      unsigned long gfn_l = gfn_x(gfn);
>      int rc;
> +    bool copied_from_hostp2m;
>  
> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);

Long line (also elsewhere).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Jan Beulich]

>>> On 09.04.19 at 14:03, <aisaila@bitdefender.com> wrote:
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>      unsigned int page_order;
>      unsigned long gfn_l = gfn_x(gfn);
>      int rc;
> +    bool copied_from_hostp2m;
>  
> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m);

Long line (also elsewhere).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/3] x86/mm: Introduce altp2m_set_entry_by_page_order

[Jan Beulich]

>>> On 09.04.19 at 14:03, <aisaila@bitdefender.com> wrote:
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -279,7 +279,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>          gfn_t gfn2 = _gfn(gfn_l & mask);
>          mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>  
> -        /* Note: currently it is not safe to remap to a shared entry */
> +	/* Note: currently it is not safe to remap to a shared entry */

Stray and bad (hard tab) change. But you've been meaning
to re-send this anyway.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/3] x86/mm: Introduce altp2m_set_entry_by_page_order

[Jan Beulich]

>>> On 09.04.19 at 14:03, <aisaila@bitdefender.com> wrote:
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -279,7 +279,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>          gfn_t gfn2 = _gfn(gfn_l & mask);
>          mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>  
> -        /* Note: currently it is not safe to remap to a shared entry */
> +	/* Note: currently it is not safe to remap to a shared entry */

Stray and bad (hard tab) change. But you've been meaning
to re-send this anyway.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel