* [PATCH v2 1/3] x86/mm: Introduce altp2m_get_gfn_type_access
@ 2019-04-05 13:25 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 12+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-04-05 13:25 UTC (permalink / raw)
To: xen-devel
Cc: tamas, wei.liu2, rcojocaru, george.dunlap, andrew.cooper3,
jbeulich, Alexandru Stefan ISAILA, roger.pau
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 | 30 +++++++++++------------------
xen/arch/x86/mm/p2m.c | 37 ++++++++++++++----------------------
xen/include/asm-x86/p2m.h | 23 ++++++++++++++++++++++
3 files changed, 48 insertions(+), 42 deletions(-)
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 56c06a4fc6..608f748a57 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -265,31 +265,23 @@ 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 found_in_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, &found_in_hostp2m);
- /* Check host p2m if no valid entry in alternate */
if ( !mfn_valid(mfn) )
- {
+ return -ESRCH;
- mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
- P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
+ /* If this is a superpage, copy that first */
+ if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
+ {
+ unsigned long mask = ~((1UL << page_order) - 1);
+ gfn_t gfn2 = _gfn(gfn_l & mask);
+ mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
- 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..b2a5c0c42e 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 found_in_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, &found_in_hostp2m);
if ( gfn_eq(new_gfn, INVALID_GFN) )
{
@@ -2648,35 +2649,25 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
/* 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;
+ 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 && found_in_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);
+ mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &found_in_hostp2m);
if ( !mfn_valid(mfn) )
- mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
-
- /* Note: currently it is not safe to remap to a shared entry */
- if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
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..42068b4aed 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -448,6 +448,29 @@ 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 *found_in_hostp2m)
+{
+ mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
+
+ *found_in_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);
+ *found_in_hostp2m = true;
+
+ /* Note: currently it is not safe to remap to a shared entry */
+ if ( !mfn_valid(mfn) || *t != p2m_ram_rw )
+ return INVALID_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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Xen-devel] [PATCH v2 1/3] x86/mm: Introduce altp2m_get_gfn_type_access
@ 2019-04-05 13:25 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 12+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-04-05 13:25 UTC (permalink / raw)
To: xen-devel
Cc: tamas, wei.liu2, rcojocaru, george.dunlap, andrew.cooper3,
jbeulich, Alexandru Stefan ISAILA, roger.pau
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 | 30 +++++++++++------------------
xen/arch/x86/mm/p2m.c | 37 ++++++++++++++----------------------
xen/include/asm-x86/p2m.h | 23 ++++++++++++++++++++++
3 files changed, 48 insertions(+), 42 deletions(-)
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 56c06a4fc6..608f748a57 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -265,31 +265,23 @@ 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 found_in_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, &found_in_hostp2m);
- /* Check host p2m if no valid entry in alternate */
if ( !mfn_valid(mfn) )
- {
+ return -ESRCH;
- mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
- P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
+ /* If this is a superpage, copy that first */
+ if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
+ {
+ unsigned long mask = ~((1UL << page_order) - 1);
+ gfn_t gfn2 = _gfn(gfn_l & mask);
+ mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
- 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..b2a5c0c42e 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 found_in_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, &found_in_hostp2m);
if ( gfn_eq(new_gfn, INVALID_GFN) )
{
@@ -2648,35 +2649,25 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
/* 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;
+ 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 && found_in_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);
+ mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &found_in_hostp2m);
if ( !mfn_valid(mfn) )
- mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
-
- /* Note: currently it is not safe to remap to a shared entry */
- if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
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..42068b4aed 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -448,6 +448,29 @@ 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 *found_in_hostp2m)
+{
+ mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
+
+ *found_in_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);
+ *found_in_hostp2m = true;
+
+ /* Note: currently it is not safe to remap to a shared entry */
+ if ( !mfn_valid(mfn) || *t != p2m_ram_rw )
+ return INVALID_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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/3] x86/mm: Introduce altp2m_set_entry_by_page_order
@ 2019-04-05 13:25 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 12+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-04-05 13:25 UTC (permalink / raw)
To: xen-devel
Cc: tamas, wei.liu2, rcojocaru, george.dunlap, andrew.cooper3,
jbeulich, Alexandru Stefan ISAILA, roger.pau
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 | 7 ++-----
xen/arch/x86/mm/p2m.c | 12 ++----------
xen/include/asm-x86/p2m.h | 11 +++++++++++
3 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 608f748a57..a26727dfee 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -275,11 +275,8 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
/* If this is a superpage, copy that first */
if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
{
- 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);
+ rc = altp2m_set_entry_by_page_order(ap2m, gfn_l, mfn, page_order,
+ t, old_a);
if ( rc )
return rc;
}
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b2a5c0c42e..1f852694ff 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2653,17 +2653,9 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
/* If this is a superpage, copy that first */
if ( page_order != PAGE_ORDER_4K && found_in_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);
-
- if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
+ if ( altp2m_set_entry_by_page_order(ap2m, gfn_x(old_gfn), mfn,
+ page_order, t, a) )
goto out;
- }
mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &found_in_hostp2m);
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 42068b4aed..8ac9427743 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -471,6 +471,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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Xen-devel] [PATCH v2 2/3] x86/mm: Introduce altp2m_set_entry_by_page_order
@ 2019-04-05 13:25 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 12+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-04-05 13:25 UTC (permalink / raw)
To: xen-devel
Cc: tamas, wei.liu2, rcojocaru, george.dunlap, andrew.cooper3,
jbeulich, Alexandru Stefan ISAILA, roger.pau
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 | 7 ++-----
xen/arch/x86/mm/p2m.c | 12 ++----------
xen/include/asm-x86/p2m.h | 11 +++++++++++
3 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 608f748a57..a26727dfee 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -275,11 +275,8 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
/* If this is a superpage, copy that first */
if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
{
- 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);
+ rc = altp2m_set_entry_by_page_order(ap2m, gfn_l, mfn, page_order,
+ t, old_a);
if ( rc )
return rc;
}
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b2a5c0c42e..1f852694ff 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2653,17 +2653,9 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
/* If this is a superpage, copy that first */
if ( page_order != PAGE_ORDER_4K && found_in_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);
-
- if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
+ if ( altp2m_set_entry_by_page_order(ap2m, gfn_x(old_gfn), mfn,
+ page_order, t, a) )
goto out;
- }
mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &found_in_hostp2m);
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 42068b4aed..8ac9427743 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -471,6 +471,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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] x86/mm: Fix p2m_set_suppress_ve
@ 2019-04-05 13:25 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 12+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-04-05 13:25 UTC (permalink / raw)
To: xen-devel
Cc: tamas, wei.liu2, rcojocaru, george.dunlap, andrew.cooper3,
jbeulich, Alexandru Stefan ISAILA, roger.pau
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 1f852694ff..6dee0052f8 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2974,6 +2974,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 )
{
@@ -2991,7 +2992,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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Xen-devel] [PATCH v2 3/3] x86/mm: Fix p2m_set_suppress_ve
@ 2019-04-05 13:25 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 12+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-04-05 13:25 UTC (permalink / raw)
To: xen-devel
Cc: tamas, wei.liu2, rcojocaru, george.dunlap, andrew.cooper3,
jbeulich, Alexandru Stefan ISAILA, roger.pau
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 1f852694ff..6dee0052f8 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2974,6 +2974,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 )
{
@@ -2991,7 +2992,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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] x86/mm: Introduce altp2m_get_gfn_type_access
@ 2019-04-05 15:04 ` Tamas K Lengyel
0 siblings, 0 replies; 12+ messages in thread
From: Tamas K Lengyel @ 2019-04-05 15:04 UTC (permalink / raw)
To: Alexandru Stefan ISAILA
Cc: wei.liu2, rcojocaru, george.dunlap, andrew.cooper3, jbeulich,
xen-devel, roger.pau
On Fri, Apr 5, 2019 at 7:25 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 | 30 +++++++++++------------------
> xen/arch/x86/mm/p2m.c | 37 ++++++++++++++----------------------
> xen/include/asm-x86/p2m.h | 23 ++++++++++++++++++++++
> 3 files changed, 48 insertions(+), 42 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 56c06a4fc6..608f748a57 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -265,31 +265,23 @@ 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 found_in_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, &found_in_hostp2m);
>
> - /* Check host p2m if no valid entry in alternate */
> if ( !mfn_valid(mfn) )
> - {
> + return -ESRCH;
>
> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> + /* If this is a superpage, copy that first */
> + if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
> + {
> + unsigned long mask = ~((1UL << page_order) - 1);
> + gfn_t gfn2 = _gfn(gfn_l & mask);
> + mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>
> - 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..b2a5c0c42e 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 found_in_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, &found_in_hostp2m);
>
> if ( gfn_eq(new_gfn, INVALID_GFN) )
> {
> @@ -2648,35 +2649,25 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>
> /* 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;
> + 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 && found_in_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);
> + mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &found_in_hostp2m);
>
> if ( !mfn_valid(mfn) )
> - mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
> -
> - /* Note: currently it is not safe to remap to a shared entry */
> - if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
> 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..42068b4aed 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -448,6 +448,29 @@ 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 *found_in_hostp2m)
I don't like this. The way it's implemented is very convoluted and not
consistent.
> +{
> + mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
> +
> + *found_in_hostp2m = false;
Just because an entry is in the altp2m now means it's not found in the
hostp2m? Doesn't make sense.
> +
> + /* 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);
> + *found_in_hostp2m = true;
Now it's found in the hostp2m, even if the mfn is invalid? Again,
doesn't make sense.
> +
> + /* Note: currently it is not safe to remap to a shared entry */
> + if ( !mfn_valid(mfn) || *t != p2m_ram_rw )
> + return INVALID_MFN;
This function is supposed to just get the type and access.. but
instead it does sanity checks on the results that the caller is
probably in a better position to judge. Again, inconsistent with the
function's name and seemingly intended use.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Xen-devel] [PATCH v2 1/3] x86/mm: Introduce altp2m_get_gfn_type_access
@ 2019-04-05 15:04 ` Tamas K Lengyel
0 siblings, 0 replies; 12+ messages in thread
From: Tamas K Lengyel @ 2019-04-05 15:04 UTC (permalink / raw)
To: Alexandru Stefan ISAILA
Cc: wei.liu2, rcojocaru, george.dunlap, andrew.cooper3, jbeulich,
xen-devel, roger.pau
On Fri, Apr 5, 2019 at 7:25 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 | 30 +++++++++++------------------
> xen/arch/x86/mm/p2m.c | 37 ++++++++++++++----------------------
> xen/include/asm-x86/p2m.h | 23 ++++++++++++++++++++++
> 3 files changed, 48 insertions(+), 42 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 56c06a4fc6..608f748a57 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -265,31 +265,23 @@ 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 found_in_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, &found_in_hostp2m);
>
> - /* Check host p2m if no valid entry in alternate */
> if ( !mfn_valid(mfn) )
> - {
> + return -ESRCH;
>
> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> + /* If this is a superpage, copy that first */
> + if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
> + {
> + unsigned long mask = ~((1UL << page_order) - 1);
> + gfn_t gfn2 = _gfn(gfn_l & mask);
> + mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>
> - 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..b2a5c0c42e 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 found_in_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, &found_in_hostp2m);
>
> if ( gfn_eq(new_gfn, INVALID_GFN) )
> {
> @@ -2648,35 +2649,25 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>
> /* 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;
> + 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 && found_in_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);
> + mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &found_in_hostp2m);
>
> if ( !mfn_valid(mfn) )
> - mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
> -
> - /* Note: currently it is not safe to remap to a shared entry */
> - if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
> 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..42068b4aed 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -448,6 +448,29 @@ 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 *found_in_hostp2m)
I don't like this. The way it's implemented is very convoluted and not
consistent.
> +{
> + mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
> +
> + *found_in_hostp2m = false;
Just because an entry is in the altp2m now means it's not found in the
hostp2m? Doesn't make sense.
> +
> + /* 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);
> + *found_in_hostp2m = true;
Now it's found in the hostp2m, even if the mfn is invalid? Again,
doesn't make sense.
> +
> + /* Note: currently it is not safe to remap to a shared entry */
> + if ( !mfn_valid(mfn) || *t != p2m_ram_rw )
> + return INVALID_MFN;
This function is supposed to just get the type and access.. but
instead it does sanity checks on the results that the caller is
probably in a better position to judge. Again, inconsistent with the
function's name and seemingly intended use.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] x86/mm: Introduce altp2m_get_gfn_type_access
@ 2019-04-08 8:13 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 12+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-04-08 8:13 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, rcojocaru, george.dunlap, andrew.cooper3, jbeulich,
xen-devel, roger.pau
On 05.04.2019 18:04, Tamas K Lengyel wrote:
> On Fri, Apr 5, 2019 at 7:25 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 | 30 +++++++++++------------------
>> xen/arch/x86/mm/p2m.c | 37 ++++++++++++++----------------------
>> xen/include/asm-x86/p2m.h | 23 ++++++++++++++++++++++
>> 3 files changed, 48 insertions(+), 42 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index 56c06a4fc6..608f748a57 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -265,31 +265,23 @@ 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 found_in_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, &found_in_hostp2m);
>>
>> - /* Check host p2m if no valid entry in alternate */
>> if ( !mfn_valid(mfn) )
>> - {
>> + return -ESRCH;
>>
>> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>> + /* If this is a superpage, copy that first */
>> + if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
>> + {
>> + unsigned long mask = ~((1UL << page_order) - 1);
>> + gfn_t gfn2 = _gfn(gfn_l & mask);
>> + mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>
>> - 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..b2a5c0c42e 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 found_in_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, &found_in_hostp2m);
>>
>> if ( gfn_eq(new_gfn, INVALID_GFN) )
>> {
>> @@ -2648,35 +2649,25 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>
>> /* 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;
>> + 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 && found_in_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);
>> + mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &found_in_hostp2m);
>>
>> if ( !mfn_valid(mfn) )
>> - mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
>> -
>> - /* Note: currently it is not safe to remap to a shared entry */
>> - if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
>> 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..42068b4aed 100644
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -448,6 +448,29 @@ 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 *found_in_hostp2m)
>
> I don't like this. The way it's implemented is very convoluted and not
> consistent.
>
>> +{
>> + mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
>> +
>> + *found_in_hostp2m = false;
>
> Just because an entry is in the altp2m now means it's not found in the
> hostp2m? Doesn't make sense.
The name of the var is not a good one I must agree. I tried to
incorporate as much as the common code in one function.
The found_in_hostp2m var is used by the caller in the page_order !=
PAGE_ORDER_4K case.
>
>> +
>> + /* 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);
>> + *found_in_hostp2m = true;
>
> Now it's found in the hostp2m, even if the mfn is invalid? Again,
> doesn't make sense.
I agree that the true comes here early and it can get true only if mfn
is valid.
>
>> +
>> + /* Note: currently it is not safe to remap to a shared entry */
>> + if ( !mfn_valid(mfn) || *t != p2m_ram_rw )
>> + return INVALID_MFN;
>
> This function is supposed to just get the type and access.. but
> instead it does sanity checks on the results that the caller is
> probably in a better position to judge. Again, inconsistent with the
> function's name and seemingly intended use.
>
Ok I will get the check out of the function and let the caller.
About the found_in_hostp2m var, should it get a different name?
Regards,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Xen-devel] [PATCH v2 1/3] x86/mm: Introduce altp2m_get_gfn_type_access
@ 2019-04-08 8:13 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 12+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-04-08 8:13 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, rcojocaru, george.dunlap, andrew.cooper3, jbeulich,
xen-devel, roger.pau
On 05.04.2019 18:04, Tamas K Lengyel wrote:
> On Fri, Apr 5, 2019 at 7:25 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 | 30 +++++++++++------------------
>> xen/arch/x86/mm/p2m.c | 37 ++++++++++++++----------------------
>> xen/include/asm-x86/p2m.h | 23 ++++++++++++++++++++++
>> 3 files changed, 48 insertions(+), 42 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index 56c06a4fc6..608f748a57 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -265,31 +265,23 @@ 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 found_in_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, &found_in_hostp2m);
>>
>> - /* Check host p2m if no valid entry in alternate */
>> if ( !mfn_valid(mfn) )
>> - {
>> + return -ESRCH;
>>
>> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>> + /* If this is a superpage, copy that first */
>> + if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
>> + {
>> + unsigned long mask = ~((1UL << page_order) - 1);
>> + gfn_t gfn2 = _gfn(gfn_l & mask);
>> + mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>
>> - 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..b2a5c0c42e 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 found_in_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, &found_in_hostp2m);
>>
>> if ( gfn_eq(new_gfn, INVALID_GFN) )
>> {
>> @@ -2648,35 +2649,25 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>
>> /* 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;
>> + 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 && found_in_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);
>> + mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &found_in_hostp2m);
>>
>> if ( !mfn_valid(mfn) )
>> - mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
>> -
>> - /* Note: currently it is not safe to remap to a shared entry */
>> - if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
>> 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..42068b4aed 100644
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -448,6 +448,29 @@ 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 *found_in_hostp2m)
>
> I don't like this. The way it's implemented is very convoluted and not
> consistent.
>
>> +{
>> + mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
>> +
>> + *found_in_hostp2m = false;
>
> Just because an entry is in the altp2m now means it's not found in the
> hostp2m? Doesn't make sense.
The name of the var is not a good one I must agree. I tried to
incorporate as much as the common code in one function.
The found_in_hostp2m var is used by the caller in the page_order !=
PAGE_ORDER_4K case.
>
>> +
>> + /* 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);
>> + *found_in_hostp2m = true;
>
> Now it's found in the hostp2m, even if the mfn is invalid? Again,
> doesn't make sense.
I agree that the true comes here early and it can get true only if mfn
is valid.
>
>> +
>> + /* Note: currently it is not safe to remap to a shared entry */
>> + if ( !mfn_valid(mfn) || *t != p2m_ram_rw )
>> + return INVALID_MFN;
>
> This function is supposed to just get the type and access.. but
> instead it does sanity checks on the results that the caller is
> probably in a better position to judge. Again, inconsistent with the
> function's name and seemingly intended use.
>
Ok I will get the check out of the function and let the caller.
About the found_in_hostp2m var, should it get a different name?
Regards,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] x86/mm: Introduce altp2m_get_gfn_type_access
@ 2019-04-08 15:40 ` Tamas K Lengyel
0 siblings, 0 replies; 12+ messages in thread
From: Tamas K Lengyel @ 2019-04-08 15:40 UTC (permalink / raw)
To: Alexandru Stefan ISAILA
Cc: wei.liu2, rcojocaru, george.dunlap, andrew.cooper3, jbeulich,
xen-devel, roger.pau
On Mon, Apr 8, 2019 at 2:13 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
>
> On 05.04.2019 18:04, Tamas K Lengyel wrote:
> > On Fri, Apr 5, 2019 at 7:25 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 | 30 +++++++++++------------------
> >> xen/arch/x86/mm/p2m.c | 37 ++++++++++++++----------------------
> >> xen/include/asm-x86/p2m.h | 23 ++++++++++++++++++++++
> >> 3 files changed, 48 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> >> index 56c06a4fc6..608f748a57 100644
> >> --- a/xen/arch/x86/mm/mem_access.c
> >> +++ b/xen/arch/x86/mm/mem_access.c
> >> @@ -265,31 +265,23 @@ 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 found_in_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, &found_in_hostp2m);
> >>
> >> - /* Check host p2m if no valid entry in alternate */
> >> if ( !mfn_valid(mfn) )
> >> - {
> >> + return -ESRCH;
> >>
> >> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> >> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> >> + /* If this is a superpage, copy that first */
> >> + if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
> >> + {
> >> + unsigned long mask = ~((1UL << page_order) - 1);
> >> + gfn_t gfn2 = _gfn(gfn_l & mask);
> >> + mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> >>
> >> - 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..b2a5c0c42e 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 found_in_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, &found_in_hostp2m);
> >>
> >> if ( gfn_eq(new_gfn, INVALID_GFN) )
> >> {
> >> @@ -2648,35 +2649,25 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>
> >> /* 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;
> >> + 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 && found_in_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);
> >> + mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &found_in_hostp2m);
> >>
> >> if ( !mfn_valid(mfn) )
> >> - mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
> >> -
> >> - /* Note: currently it is not safe to remap to a shared entry */
> >> - if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
> >> 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..42068b4aed 100644
> >> --- a/xen/include/asm-x86/p2m.h
> >> +++ b/xen/include/asm-x86/p2m.h
> >> @@ -448,6 +448,29 @@ 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 *found_in_hostp2m)
> >
> > I don't like this. The way it's implemented is very convoluted and not
> > consistent.
> >
> >> +{
> >> + mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
> >> +
> >> + *found_in_hostp2m = false;
> >
> > Just because an entry is in the altp2m now means it's not found in the
> > hostp2m? Doesn't make sense.
>
> The name of the var is not a good one I must agree. I tried to
> incorporate as much as the common code in one function.
>
> The found_in_hostp2m var is used by the caller in the page_order !=
> PAGE_ORDER_4K case.
>
> >
> >> +
> >> + /* 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);
> >> + *found_in_hostp2m = true;
> >
> > Now it's found in the hostp2m, even if the mfn is invalid? Again,
> > doesn't make sense.
>
> I agree that the true comes here early and it can get true only if mfn
> is valid.
>
> >
> >> +
> >> + /* Note: currently it is not safe to remap to a shared entry */
> >> + if ( !mfn_valid(mfn) || *t != p2m_ram_rw )
> >> + return INVALID_MFN;
> >
> > This function is supposed to just get the type and access.. but
> > instead it does sanity checks on the results that the caller is
> > probably in a better position to judge. Again, inconsistent with the
> > function's name and seemingly intended use.
> >
>
> Ok I will get the check out of the function and let the caller.
>
> About the found_in_hostp2m var, should it get a different name?
It should reflect what it actually contains, which right now looks
like more like "copied_from_hostp2m".
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Xen-devel] [PATCH v2 1/3] x86/mm: Introduce altp2m_get_gfn_type_access
@ 2019-04-08 15:40 ` Tamas K Lengyel
0 siblings, 0 replies; 12+ messages in thread
From: Tamas K Lengyel @ 2019-04-08 15:40 UTC (permalink / raw)
To: Alexandru Stefan ISAILA
Cc: wei.liu2, rcojocaru, george.dunlap, andrew.cooper3, jbeulich,
xen-devel, roger.pau
On Mon, Apr 8, 2019 at 2:13 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
>
> On 05.04.2019 18:04, Tamas K Lengyel wrote:
> > On Fri, Apr 5, 2019 at 7:25 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 | 30 +++++++++++------------------
> >> xen/arch/x86/mm/p2m.c | 37 ++++++++++++++----------------------
> >> xen/include/asm-x86/p2m.h | 23 ++++++++++++++++++++++
> >> 3 files changed, 48 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> >> index 56c06a4fc6..608f748a57 100644
> >> --- a/xen/arch/x86/mm/mem_access.c
> >> +++ b/xen/arch/x86/mm/mem_access.c
> >> @@ -265,31 +265,23 @@ 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 found_in_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, &found_in_hostp2m);
> >>
> >> - /* Check host p2m if no valid entry in alternate */
> >> if ( !mfn_valid(mfn) )
> >> - {
> >> + return -ESRCH;
> >>
> >> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> >> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> >> + /* If this is a superpage, copy that first */
> >> + if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
> >> + {
> >> + unsigned long mask = ~((1UL << page_order) - 1);
> >> + gfn_t gfn2 = _gfn(gfn_l & mask);
> >> + mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> >>
> >> - 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..b2a5c0c42e 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 found_in_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, &found_in_hostp2m);
> >>
> >> if ( gfn_eq(new_gfn, INVALID_GFN) )
> >> {
> >> @@ -2648,35 +2649,25 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>
> >> /* 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;
> >> + 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 && found_in_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);
> >> + mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &found_in_hostp2m);
> >>
> >> if ( !mfn_valid(mfn) )
> >> - mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
> >> -
> >> - /* Note: currently it is not safe to remap to a shared entry */
> >> - if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
> >> 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..42068b4aed 100644
> >> --- a/xen/include/asm-x86/p2m.h
> >> +++ b/xen/include/asm-x86/p2m.h
> >> @@ -448,6 +448,29 @@ 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 *found_in_hostp2m)
> >
> > I don't like this. The way it's implemented is very convoluted and not
> > consistent.
> >
> >> +{
> >> + mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
> >> +
> >> + *found_in_hostp2m = false;
> >
> > Just because an entry is in the altp2m now means it's not found in the
> > hostp2m? Doesn't make sense.
>
> The name of the var is not a good one I must agree. I tried to
> incorporate as much as the common code in one function.
>
> The found_in_hostp2m var is used by the caller in the page_order !=
> PAGE_ORDER_4K case.
>
> >
> >> +
> >> + /* 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);
> >> + *found_in_hostp2m = true;
> >
> > Now it's found in the hostp2m, even if the mfn is invalid? Again,
> > doesn't make sense.
>
> I agree that the true comes here early and it can get true only if mfn
> is valid.
>
> >
> >> +
> >> + /* Note: currently it is not safe to remap to a shared entry */
> >> + if ( !mfn_valid(mfn) || *t != p2m_ram_rw )
> >> + return INVALID_MFN;
> >
> > This function is supposed to just get the type and access.. but
> > instead it does sanity checks on the results that the caller is
> > probably in a better position to judge. Again, inconsistent with the
> > function's name and seemingly intended use.
> >
>
> Ok I will get the check out of the function and let the caller.
>
> About the found_in_hostp2m var, should it get a different name?
It should reflect what it actually contains, which right now looks
like more like "copied_from_hostp2m".
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-04-08 15:41 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-05 13:25 [PATCH v2 1/3] x86/mm: Introduce altp2m_get_gfn_type_access Alexandru Stefan ISAILA
2019-04-05 13:25 ` [Xen-devel] " Alexandru Stefan ISAILA
2019-04-05 13:25 ` [PATCH v2 2/3] x86/mm: Introduce altp2m_set_entry_by_page_order Alexandru Stefan ISAILA
2019-04-05 13:25 ` [Xen-devel] " Alexandru Stefan ISAILA
2019-04-05 13:25 ` [PATCH v2 3/3] x86/mm: Fix p2m_set_suppress_ve Alexandru Stefan ISAILA
2019-04-05 13:25 ` [Xen-devel] " Alexandru Stefan ISAILA
2019-04-05 15:04 ` [PATCH v2 1/3] x86/mm: Introduce altp2m_get_gfn_type_access Tamas K Lengyel
2019-04-05 15:04 ` [Xen-devel] " Tamas K Lengyel
2019-04-08 8:13 ` Alexandru Stefan ISAILA
2019-04-08 8:13 ` [Xen-devel] " Alexandru Stefan ISAILA
2019-04-08 15:40 ` Tamas K Lengyel
2019-04-08 15:40 ` [Xen-devel] " Tamas K Lengyel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.