[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 04/18] arm/altp2m: Add altp2m init/teardown routines.
Hello Sergej,
On 04/07/16 12:45, Sergej Proskurin wrote:
The p2m intialization now invokes intialization routines responsible for
s/intialization/initialization/
the allocation and intitialization of altp2m structures. The same
Ditto
applies to teardown routines. The functionality has been adopted from
the x86 altp2m implementation.
This patch would benefit to be split in 2:
1) Moving p2m init/teardown in a separate function
2) Introducing altp2m init/teardown
It will ease the review.
Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
---
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxx>
---
xen/arch/arm/p2m.c | 166 ++++++++++++++++++++++++++++++++++++++++--
xen/include/asm-arm/domain.h | 6 ++
xen/include/asm-arm/hvm/hvm.h | 12 +++
xen/include/asm-arm/p2m.h | 20 +++++
4 files changed, 198 insertions(+), 6 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index aa4e774..e72ca7a 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1400,19 +1400,103 @@ static void p2m_free_vmid(struct domain *d)
spin_unlock(&vmid_alloc_lock);
}
-void p2m_teardown(struct domain *d)
+static int p2m_initialise(struct domain *d, struct p2m_domain *p2m)
AFAICT, this function is only used by p2m_initialise_one. So I would
prefer if you fold the code in the latter.
{
- struct p2m_domain *p2m = &d->arch.p2m;
+ int ret = 0;
+
+ spin_lock_init(&p2m->lock);
+ INIT_PAGE_LIST_HEAD(&p2m->pages);
+
+ spin_lock(&p2m->lock);
+
+ p2m->domain = d;
+ p2m->access_required = false;
+ p2m->mem_access_enabled = false;
+ p2m->default_access = p2m_access_rwx;
+ p2m->p2m_class = p2m_host;
+ p2m->root = NULL;
+
+ /* Adopt VMID of the associated domain */
+ p2m->vmid = d->arch.p2m.vmid;
It looks like to me that re-using the same VMID will require more TLB
flush (such as when a VCPU is migrated to another physical CPU). So
could you explain why you decided to re-use the same VMID?
+ p2m->vttbr.vttbr = 0;
+ p2m->vttbr.vttbr_vmid = p2m->vmid;
+
+ p2m->max_mapped_gfn = 0;
+ p2m->lowest_mapped_gfn = ULONG_MAX;
+ radix_tree_init(&p2m->mem_access_settings);
+
+ spin_unlock(&p2m->lock);
+
+ return ret;
+}
+
+static void p2m_free_one(struct p2m_domain *p2m)
+{
+ mfn_t mfn;
+ unsigned int i;
struct page_info *pg;
spin_lock(&p2m->lock);
while ( (pg = page_list_remove_head(&p2m->pages)) )
- free_domheap_page(pg);
+ if ( pg != p2m->root )
+ free_domheap_page(pg);
+
+ for ( i = 0; i < P2M_ROOT_PAGES; i++ )
+ {
+ mfn = _mfn(page_to_mfn(p2m->root) + i);
+ clear_domain_page(mfn);
+ }
+ free_domheap_pages(p2m->root, P2M_ROOT_ORDER);
+ p2m->root = NULL;
+
+ radix_tree_destroy(&p2m->mem_access_settings, NULL);
+
+ spin_unlock(&p2m->lock);
+
+ xfree(p2m);
+}
+
+static struct p2m_domain *p2m_init_one(struct domain *d)
+{
+ struct p2m_domain *p2m = xzalloc(struct p2m_domain);
+
+ if ( !p2m )
+ return NULL;
+
+ if ( p2m_initialise(d, p2m) )
+ goto free_p2m;
+
+ return p2m;
+
+free_p2m:
+ xfree(p2m);
+ return NULL;
+}
+
+static void p2m_teardown_hostp2m(struct domain *d)
Why does p2m_teardown_hostp2m not use p2m_teardown_one to teardown the
p2m? Assuming xfree(p2m) is move out of the function.
+{
+ struct p2m_domain *p2m = p2m_get_hostp2m(d);
+ struct page_info *pg = NULL;
+ mfn_t mfn;
+ unsigned int i;
+
+ spin_lock(&p2m->lock);
- if ( p2m->root )
Why did you remove this check? The p2m->root could be NULL if the an
error occurred before create the root page table.
- free_domheap_pages(p2m->root, P2M_ROOT_ORDER);
+ while ( (pg = page_list_remove_head(&p2m->pages)) )
+ if ( pg != p2m->root )
Why this check? p2m->root will never be part of p2m->pages.
+ {
+ mfn = _mfn(page_to_mfn(pg));
+ clear_domain_page(mfn);
+ free_domheap_page(pg);
+ }
+ for ( i = 0; i < P2M_ROOT_PAGES; i++ )
+ {
+ mfn = _mfn(page_to_mfn(p2m->root) + i);
+ clear_domain_page(mfn);
+ }
+ free_domheap_pages(p2m->root, P2M_ROOT_ORDER);
p2m->root = NULL;
p2m_free_vmid(d);
@@ -1422,7 +1506,7 @@ void p2m_teardown(struct domain *d)
spin_unlock(&p2m->lock);
}
-int p2m_init(struct domain *d)
+static int p2m_init_hostp2m(struct domain *d)
Why does p2m_init_hostp2m not use p2m_init_one to initialize the p2m?
{
struct p2m_domain *p2m = &d->arch.p2m;
int rc = 0;
@@ -1437,6 +1521,8 @@ int p2m_init(struct domain *d)
if ( rc != 0 )
goto err;
+ p2m->vttbr.vttbr_vmid = p2m->vmid;
+
d->arch.vttbr = 0;
p2m->root = NULL;
@@ -1454,6 +1540,74 @@ err:
return rc;
}
+static void p2m_teardown_altp2m(struct domain *d)
+{
+ unsigned int i;
+ struct p2m_domain *p2m;
+
+ for ( i = 0; i < MAX_ALTP2M; i++ )
+ {
+ if ( !d->arch.altp2m_p2m[i] )
+ continue;
+
+ p2m = d->arch.altp2m_p2m[i];
+ p2m_free_one(p2m);
+ d->arch.altp2m_vttbr[i] = INVALID_MFN;
+ d->arch.altp2m_p2m[i] = NULL;
These 2 lines are not necessary because the domain is destroyed and the
memory associated will be free very soon.
+ }
+
+ d->arch.altp2m_active = false;
+}
+
+static int p2m_init_altp2m(struct domain *d)
+{
+ unsigned int i;
+ struct p2m_domain *p2m;
+
+ spin_lock_init(&d->arch.altp2m_lock);
+ for ( i = 0; i < MAX_ALTP2M; i++ )
+ {
+ d->arch.altp2m_vttbr[i] = INVALID_MFN;
The VTTBR will be stored in altp2m_p2m[i].vttbr. So why do you need to
store in a different way?
Also, please don't mix value that have different meaning. MFN_INVALID
indicates that a MFN is invalid not the VTTBR.
+ d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
+ if ( p2m == NULL )
+ {
+ p2m_teardown_altp2m(d);
This call is not necessary. p2m_teardown_altp2m will be called by
p2m_teardown as part of arch_domain_destroy.
+ return -ENOMEM;
+ }
+ p2m->p2m_class = p2m_alternate;
+ p2m->access_required = 1;
+ _atomic_set(&p2m->active_vcpus, 0);
+ }
+
+ return 0;
+}
+
+void p2m_teardown(struct domain *d)
+{
+ /*
+ * We must teardown altp2m unconditionally because
+ * we initialise it unconditionally.
Why do we need to initialize altp2m unconditionally? When altp2m is not
used we will allocate memory that is never used.
I would prefer to see the allocation of the memory only if the domain
will make use of altp2m.
+ */
+ p2m_teardown_altp2m(d);
+
+ p2m_teardown_hostp2m(d);
+}
+
+int p2m_init(struct domain *d)
+{
+ int rc = 0;
+
+ rc = p2m_init_hostp2m(d);
+ if ( rc )
+ return rc;
+
+ rc = p2m_init_altp2m(d);
+ if ( rc )
+ p2m_teardown_hostp2m(d);
This call is not necessary.
+
+ return rc;
+}
+
int relinquish_p2m_mapping(struct domain *d)
{
struct p2m_domain *p2m = &d->arch.p2m;
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 2039f16..6b9770f 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -29,6 +29,9 @@ enum domain_type {
#define is_64bit_domain(d) (0)
#endif
+#define MAX_ALTP2M 10 /* arbitrary */
+#define INVALID_ALTP2M 0xffff
IMHO, this should either be part of p2m.h or altp2m.h
+
extern int dom0_11_mapping;
#define is_domain_direct_mapped(d) ((d) == hardware_domain && dom0_11_mapping)
@@ -130,6 +133,9 @@ struct arch_domain
/* altp2m: allow multiple copies of host p2m */
bool_t altp2m_active;
+ struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
+ spinlock_t altp2m_lock;
Please document was the lock is protecting.
+ uint64_t altp2m_vttbr[MAX_ALTP2M];
} __cacheline_aligned;
struct arch_vcpu
diff --git a/xen/include/asm-arm/hvm/hvm.h b/xen/include/asm-arm/hvm/hvm.h
index 96c455c..28d5298 100644
--- a/xen/include/asm-arm/hvm/hvm.h
+++ b/xen/include/asm-arm/hvm/hvm.h
@@ -19,6 +19,18 @@
#ifndef __ASM_ARM_HVM_HVM_H__
#define __ASM_ARM_HVM_HVM_H__
+struct vttbr_data {
This structure should not be part of hvm.h but processor.h. Also, I
would rename it to simply vttbr.
+ union {
+ struct {
+ u64 vttbr_baddr :40, /* variable res0: from 0-(x-1) bit */
Please drop vttbr_. Also, this field is 48 bits for ARMv8 (see ARM
D7.2.102 in DDI 0487A.j).
+ res1 :8,
+ vttbr_vmid :8,
Please drop vttbr_.
+ res2 :8;
+ };
+ u64 vttbr;
+ };
+};
+
struct hvm_function_table {
char *name;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 0d1e61e..a78d547 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -8,6 +8,9 @@
#include <xen/p2m-common.h>
#include <public/memory.h>
+#include <asm/atomic.h>
+#include <asm/hvm/hvm.h>
ARM has not concept of HVM nor PV. So I would prefer to see a very
limited usage of hvm.h
+
#define paddr_bits PADDR_BITS
/* Holds the bit size of IPAs in p2m tables. */
@@ -17,6 +20,11 @@ struct domain;
extern void memory_type_changed(struct domain *);
+typedef enum {
+ p2m_host,
+ p2m_alternate,
+} p2m_class_t;
+
/* Per-p2m-table state */
struct p2m_domain {
/* Lock that protects updates to the p2m */
@@ -66,6 +74,18 @@ struct p2m_domain {
/* Radix tree to store the p2m_access_t settings as the pte's don't have
* enough available bits to store this information. */
struct radix_tree_root mem_access_settings;
+
+ /* Alternate p2m: count of vcpu's currently using this p2m. */
+ atomic_t active_vcpus;
+
+ /* Choose between: host/alternate */
+ p2m_class_t p2m_class;
Is there any reason to have this field? It is set but never used.
+
+ /* Back pointer to domain */
+ struct domain *domain;
AFAICT, the only usage of this field is in p2m_altp2m_lazy_copy where
you have direct access to the domain. So this could be dropped.
+
+ /* VTTBR information */
+ struct vttbr_data vttbr;
};
/* List of possible type for each page in the p2m entry.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
