Hi Sergej,
On 16/08/16 23:17, Sergej Proskurin wrote:
The HVMOP_altp2m_set_mem_access allows to set gfn permissions of
(currently one page at a time) of a specific altp2m view. In case the
view does not hold the requested gfn entry, it will be first copied from
the host's p2m table and then modified as requested.
Signed-off-by: Sergej Proskurin <prosku...@sec.in.tum.de>
---
Cc: Stefano Stabellini <sstabell...@kernel.org>
Cc: Julien Grall <julien.gr...@arm.com>
---
v2: Prevent the page reference count from being falsely updated on
altp2m modification. Therefore, we add a check determining whether
the target p2m is a hostp2m before p2m_put_l3_page is called.
v3: Cosmetic fixes.
Added the functionality to set/get the default_access also in/from
the requested altp2m view.
Read-locked hp2m in "altp2m_set_mem_access".
Moved the functions "p2m_is_(hostp2m|altp2m)" out of this commit.
Moved the funtion "modify_altp2m_entry" out of this commit.
Moved the function "p2m_lookup_attr" out of this commit.
Moved guards for "p2m_put_l3_page" out of this commit.
---
xen/arch/arm/altp2m.c | 53 ++++++++++++++++++++++++++++
xen/arch/arm/hvm.c | 7 +++-
xen/arch/arm/p2m.c | 82 ++++++++++++++++++++++++++++++++------------
xen/include/asm-arm/altp2m.h | 12 +++++++
4 files changed, 131 insertions(+), 23 deletions(-)
diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
index ba345b9..03b8ce5 100644
--- a/xen/arch/arm/altp2m.c
+++ b/xen/arch/arm/altp2m.c
@@ -80,6 +80,59 @@ int altp2m_switch_domain_altp2m_by_id(struct domain *d,
unsigned int idx)
return rc;
}
+int altp2m_set_mem_access(struct domain *d,
+ struct p2m_domain *hp2m,
+ struct p2m_domain *ap2m,
+ p2m_access_t a,
+ gfn_t gfn)
+{
+ p2m_type_t p2mt;
+ p2m_access_t old_a;
+ mfn_t mfn;
+ unsigned int page_order;
+ int rc;
+
+ altp2m_lock(d);
Why do you have this lock? This will serialize all the mem access
modification even if the ap2m is different.
I am also surprised that you only lock the altp2m in
modify_altp2m_entry. IHMO, the change in the same a2pm should be serialized.
+ p2m_read_lock(hp2m);
+
+ /* Check if entry is part of the altp2m view. */
+ mfn = p2m_lookup_attr(ap2m, gfn, &p2mt, NULL, &page_order);
+
+ /* Check host p2m if no valid entry in ap2m. */
+ if ( mfn_eq(mfn, INVALID_MFN) )
+ {
+ /* Check if entry is part of the host p2m view. */
+ mfn = p2m_lookup_attr(hp2m, gfn, &p2mt, &old_a, &page_order);
As mentioned in patch #27, p2m_lookup_attr will take a read reference on
the hp2m lock. However you already did it above.
Anyway, I really think that p2m_lookup_attr should not exist and ever
caller be replaced by a direct call to p2m_get_entry.
+ if ( mfn_eq(mfn, INVALID_MFN) ||
+ ((p2mt != p2m_ram_rw) && (p2mt != p2m_ram_ro)) )
Please use p2m_is_ram rather than open-coding the check on p2mt.
However, I don't understand why access restriction on altp2m is limited
to RAM whilst the hostp2m allows restriction on any type of page.
+ {
+ rc = -ESRCH;
+ goto out;
+ }
+
+ /* If this is a superpage, copy that first. */
+ if ( page_order != THIRD_ORDER )
+ {
+ rc = modify_altp2m_entry(ap2m, gfn, mfn, p2mt, old_a, page_order);
+ if ( rc < 0 )
+ {
+ rc = -ESRCH;
+ goto out;
+ }
+ }
+ }
+
+ /* Set mem access attributes - currently supporting only one (4K) page. */
+ page_order = THIRD_ORDER;
This looks pointless, please use THIRD_ORDER directly as argument.
+ rc = modify_altp2m_entry(ap2m, gfn, mfn, p2mt, a, page_order);
+
+out:
+ p2m_read_unlock(hp2m);
+ altp2m_unlock(d);
+
+ return rc;
+}
+
static void altp2m_vcpu_reset(struct vcpu *v)
{
struct altp2mvcpu *av = &altp2m_vcpu(v);
diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 9ac3422..df78893 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -136,7 +136,12 @@ static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg)
break;
case HVMOP_altp2m_set_mem_access:
- rc = -EOPNOTSUPP;
+ if ( a.u.set_mem_access.pad )
+ rc = -EINVAL;
+ else
+ rc = p2m_set_mem_access(d, _gfn(a.u.set_mem_access.gfn), 1, 0, 0,
+ a.u.set_mem_access.hvmmem_access,
+ a.u.set_mem_access.view);
break;
case HVMOP_altp2m_change_gfn:
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index df2b85b..8dee02187 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1913,7 +1913,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn,
uint32_t nr,
uint32_t start, uint32_t mask, xenmem_access_t access,
unsigned int altp2m_idx)
{
- struct p2m_domain *p2m = p2m_get_hostp2m(d);
+ struct p2m_domain *hp2m = p2m_get_hostp2m(d), *ap2m = NULL;
p2m_access_t a;
unsigned int order;
long rc = 0;
@@ -1933,13 +1933,26 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn,
uint32_t nr,
#undef ACCESS
};
+ /* altp2m view 0 is treated as the hostp2m */
+ if ( altp2m_idx )
+ {
+ if ( altp2m_idx >= MAX_ALTP2M ||
+ d->arch.altp2m_p2m[altp2m_idx] == NULL )
+ return -EINVAL;
+
+ ap2m = d->arch.altp2m_p2m[altp2m_idx];
+ }
+
switch ( access )
{
case 0 ... ARRAY_SIZE(memaccess) - 1:
a = memaccess[access];
break;
case XENMEM_access_default:
- a = p2m->default_access;
+ if ( ap2m )
+ a = ap2m->default_access;
+ else
+ a = hp2m->default_access;
On the previous version, I requested to have this change documented in
the code [1]. I don't see it here.
break;
default:
return -EINVAL;
@@ -1949,39 +1962,66 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn,
uint32_t nr,
* Flip mem_access_enabled to true when a permission is set, as to prevent
* allocating or inserting super-pages.
*/
- p2m->mem_access_enabled = true;
+ if ( ap2m )
+ ap2m->mem_access_enabled = true;
+ else
+ hp2m->mem_access_enabled = true;
/* If request to set default access. */
if ( gfn_eq(gfn, INVALID_GFN) )
{
- p2m->default_access = a;
+ if ( ap2m )
+ ap2m->default_access = a;
+ else
+ hp2m->default_access = a;
+
return 0;
}
- p2m_write_lock(p2m);
-
for ( gfn = gfn_add(gfn, start); nr > start; gfn = gfn_add(gfn, 1UL <<
order) )
{
- p2m_type_t t;
- mfn_t mfn = p2m_get_entry(p2m, gfn, &t, NULL, &order);
-
- /* Skip hole */
- if ( mfn_eq(mfn, INVALID_MFN) )
+ if ( ap2m )
{
+ order = THIRD_ORDER;
+
/*
- * the order corresponds to the order of the mapping in the
- * page table. so we need to align the gfn before
- * incrementing.
+ * ARM altp2m currently supports only setting of memory access
rights
+ * of only one (4K) page at a time.
This looks like a TODO to me. So please add either : "XXX:" or "TODO:"
in front.
*/
- gfn = _gfn(gfn_x(gfn) & ~((1UL << order) - 1));
- continue;
+ rc = altp2m_set_mem_access(d, hp2m, ap2m, a, gfn);
+ if ( rc && rc != -ESRCH )
+ break;
}
else
{
- order = 0;
- rc = __p2m_set_entry(p2m, gfn, 0, mfn, t, a);
- if ( rc )
- break;
+ p2m_type_t t;
+ mfn_t mfn;
+
+ p2m_write_lock(hp2m);
+
+ mfn = p2m_get_entry(hp2m, gfn, &t, NULL, &order);
+
+ /* Skip hole */
+ if ( mfn_eq(mfn, INVALID_MFN) )
+ {
+ /*
+ * the order corresponds to the order of the mapping in the
+ * page table. so we need to align the gfn before
+ * incrementing.
+ */
+ gfn = _gfn(gfn_x(gfn) & ~((1UL << order) - 1));
+ continue;
I just noticed that my series is buggy. The "continue" should be
dropped. I will do it in the next version.
+ }
+ else
+ {
+ order = 0;
+
+ rc = __p2m_set_entry(hp2m, gfn, 0, mfn, t, a);
+ if ( rc )
+ break;
+ }
+
+ p2m_write_unlock(hp2m);
By moving the lock within the loop, you will impose a TLB flush per-4K
which is currently not the case.
Looking at the function, I think the implementation the support of
altp2m could really be simplified. The main difference is if the entry
is not present in the altp2m then you lookup the host p2m.
}
start += (1UL << order);
@@ -1993,8 +2033,6 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn,
uint32_t nr,
}
}
- p2m_write_unlock(p2m);
-
if ( rc < 0 )
return rc;
else if ( rc > 0 )
diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
index c2e44ab..3e4c36d 100644
--- a/xen/include/asm-arm/altp2m.h
+++ b/xen/include/asm-arm/altp2m.h
@@ -71,4 +71,16 @@ void altp2m_flush(struct domain *d);
int altp2m_destroy_by_id(struct domain *d,
unsigned int idx);
+/*
+ * Set memory access attributes of the gfn in the altp2m view. If the altp2m
+ * view does not contain the particular entry, copy it first from the hostp2m.
+ *
+ * Currently supports memory attribute adoptions of only one (4K) page.
+ */
+int altp2m_set_mem_access(struct domain *d,
+ struct p2m_domain *hp2m,
+ struct p2m_domain *ap2m,
+ p2m_access_t a,
+ gfn_t gfn);
+
#endif /* __ASM_ARM_ALTP2M_H */
Regards,
[1]
https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg01007.html
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
