| Message ID | 20160704114605.10086-5-proskurin@sec.in.tum.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@sec.in.tum.de> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > --- > 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,
On 04/07/16 12:45, Sergej Proskurin wrote: > +static void p2m_teardown_hostp2m(struct domain *d) > +{ > + 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 ) > - free_domheap_pages(p2m->root, P2M_ROOT_ORDER); > + while ( (pg = page_list_remove_head(&p2m->pages)) ) > + if ( pg != p2m->root ) > + { > + mfn = _mfn(page_to_mfn(pg)); > + clear_domain_page(mfn); Can you explain why you are cleaning the page here? It was not part of p2m_teardown before this series. > + 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); > } Regards,
Hello Julien, On 07/04/2016 05:17 PM, Julien Grall wrote: > 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 > Thanks. >> 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. > I will split this patch up in two parts, thank you. >> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> >> --- >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Julien Grall <julien.grall@arm.com> >> --- >> 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. > I will do that, thanks. >> { >> - 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? > Please correct me if I am wrong, but I associate a VMID with an entire domain. Since, the altp2m view still belongs to the same domain (p2m_init_one is called only from p2m_init_altp2m), the code re-uses the old 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. > I believe you mean p2m_free_one: The p2m_teardown_hostp2m might use the same function but would require the call p2m_free_vmid(d) to be called outside of p2m_free_one as well. This would require another acquisition of the p2m->lock. Just to be sure, I did not want to split the teardown process into two atomic executions. If you believe that it is safe to do, I will gladly change the code and re-use the code from p2m_free_one in p2m_teardown_hostp2m. >> +{ >> + 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. > That was a mistake. Thank you very much. >> - 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. > I was not sure that p2m->root will never be part of p2m->pages. This also answers my question in patch #06. Thank you very much for this comment. >> + { >> + 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? > We dynamically allocate altp2ms. Also, the initialization of both the altp2ms and hostp2m slightly differs (see VMID allocation). I could rewrite the initialization function to be used for both, the hostp2m and altp2m structs. Especially, if you say that we do not associate domains with VMIDs (see your upper question). >> { >> 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. > I will remove them. >> + } >> + >> + 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? You are definitely right. I was already thinking of that. Thank you. > > Also, please don't mix value that have different meaning. MFN_INVALID > indicates that a MFN is invalid not the VTTBR. > Ok. I will include a new type for invalid VTTBRs. >> + 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. > Thanks for the hint. >> + 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. > This is true. At this point, I could check whether opt_altp2m_enabled is set and initialize altp2m accordingly. Thanks. >> + */ >> + 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. > Thank you. >> + >> + 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 > I will place the defines in one of the header files, thank you. >> + >> 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. > Ok. >> + 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. > Ok, I will move it. The struct was named this way to be the counterpart to struct ept_data. Do you still think, we should introduce naming differences for basically the same register at this point? >> + 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_. > Ok, thank you. >> + 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 > Makes sense. I will rename the header file. >> + >> #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. > Actually it is used by p2m_is_altp2m and p2m_is_hostp2m (e.g. see assert in p2m_flush_table). >> + >> + /* 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. > Ok. Thank you >> + >> + /* VTTBR information */ >> + struct vttbr_data vttbr; >> }; >> >> /* List of possible type for each page in the p2m entry. >> > > Regards, > Thank you very much for your comments. Best regards, Sergej
On 04/07/16 17:40, Sergej Proskurin wrote: > >>> { >>> - 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? >> > Please correct me if I am wrong, but I associate a VMID with an entire > domain. Since, the altp2m view still belongs to the same domain > (p2m_init_one is called only from p2m_init_altp2m), the code re-uses the > old VMID. (I am not an ARM expert but) looking into VMIDs from the last time, they are the TLB tag for the address space in use. Does ARM have shared TLBs between multiple cores? If so, you must a separate VMID, otherwise an ALT2PM used by one vcpu could cause a separate vcpu with a different ALTP2M to reuse the wrong translation. ~Andrew
Hello Julien, On 07/04/2016 06:15 PM, Julien Grall wrote: > > > On 04/07/16 12:45, Sergej Proskurin wrote: >> +static void p2m_teardown_hostp2m(struct domain *d) >> +{ >> + 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 ) >> - free_domheap_pages(p2m->root, P2M_ROOT_ORDER); >> + while ( (pg = page_list_remove_head(&p2m->pages)) ) >> + if ( pg != p2m->root ) >> + { >> + mfn = _mfn(page_to_mfn(pg)); >> + clear_domain_page(mfn); > > Can you explain why you are cleaning the page here? It was not part of > p2m_teardown before this series. > With the x86-based altp2m implementation, we experienced the problem that altp2m-teardowns did not clean the pages. As a result, later re-initialization reused the pages, which subsequently led to faults or crashes due to reused mappings. We additionally clean the altp2m pages and for the sake of completeness we clean the hostp2m tables as well. >> + 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); >> } > > Regards, > Thank you very much. Best regards, Sergej
Hi Andrew, On 07/04/2016 06:43 PM, Andrew Cooper wrote: > On 04/07/16 17:40, Sergej Proskurin wrote: >> >>>> { >>>> - 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? >>> >> Please correct me if I am wrong, but I associate a VMID with an entire >> domain. Since, the altp2m view still belongs to the same domain >> (p2m_init_one is called only from p2m_init_altp2m), the code re-uses the >> old VMID. > > (I am not an ARM expert but) looking into VMIDs from the last time, they > are the TLB tag for the address space in use. > > Does ARM have shared TLBs between multiple cores? If so, you must a > separate VMID, otherwise an ALT2PM used by one vcpu could cause a > separate vcpu with a different ALTP2M to reuse the wrong translation. > > ~Andrew > You're absolutely correct. However, on every VMENTRY Xen explicitly flushes the TLBs of the currently active domain (and with it, of the currently active (a)p2m table) and hence it should not result in an issue. Thank you very much. Best regards, Sergej
On 04/07/16 17:56, Sergej Proskurin wrote: > Hi Andrew, > > On 07/04/2016 06:43 PM, Andrew Cooper wrote: >> On 04/07/16 17:40, Sergej Proskurin wrote: >>> >>>>> { >>>>> - 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? >>>> >>> Please correct me if I am wrong, but I associate a VMID with an entire >>> domain. Since, the altp2m view still belongs to the same domain >>> (p2m_init_one is called only from p2m_init_altp2m), the code re-uses the >>> old VMID. >> >> (I am not an ARM expert but) looking into VMIDs from the last time, they >> are the TLB tag for the address space in use. >> >> Does ARM have shared TLBs between multiple cores? If so, you must a >> separate VMID, otherwise an ALT2PM used by one vcpu could cause a >> separate vcpu with a different ALTP2M to reuse the wrong translation. >> >> ~Andrew >> > > You're absolutely correct. However, on every VMENTRY Xen explicitly > flushes the TLBs of the currently active domain (and with it, of the > currently active (a)p2m table) and hence it should not result in an issue. VMENTRY is x86 not ARM. So are you sure you looked at the correct code? Regards,
On 04/07/16 17:43, Andrew Cooper wrote: > On 04/07/16 17:40, Sergej Proskurin wrote: >> >>>> { >>>> - 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? >>> >> Please correct me if I am wrong, but I associate a VMID with an entire >> domain. Since, the altp2m view still belongs to the same domain >> (p2m_init_one is called only from p2m_init_altp2m), the code re-uses the >> old VMID. > > (I am not an ARM expert but) looking into VMIDs from the last time, they > are the TLB tag for the address space in use. Correct. > > Does ARM have shared TLBs between multiple cores? If so, you must a > separate VMID, otherwise an ALT2PM used by one vcpu could cause a > separate vcpu with a different ALTP2M to reuse the wrong translation. From the ARM ARM, I cannot rule out that TLBs cannot be shared from multiple cores (CC a couple of ARM folks to confirm). Nevertheless, using a different VMID per P2M would avoid to care about flushing when moving the vCPU around. Regards,
On 04/07/16 17:40, Sergej Proskurin wrote: > On 07/04/2016 05:17 PM, Julien Grall wrote: >> On 04/07/16 12:45, Sergej Proskurin wrote: [...] >>> +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. >> > > I believe you mean p2m_free_one: The p2m_teardown_hostp2m might use the > same function but would require the call p2m_free_vmid(d) to be called > outside of p2m_free_one as well. This would require another acquisition > of the p2m->lock. Just to be sure, I did not want to split the teardown > process into two atomic executions. If you believe that it is safe to > do, I will gladly change the code and re-use the code from p2m_free_one > in p2m_teardown_hostp2m. Looking at the caller of p2m_teardown, I don't think the lock is necessary because nobody can use the P2M anymore when this function is called. [...] >>> + { >>> + 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? >> > > We dynamically allocate altp2ms. Also, the initialization of both the > altp2ms and hostp2m slightly differs (see VMID allocation). I could > rewrite the initialization function to be used for both, the hostp2m and > altp2m structs. Especially, if you say that we do not associate domains > with VMIDs (see your upper question). I always prefer to see the code rewritten rather than duplicating code. The latter makes harder to fix bug when it is spread in multiple place. [...] >>> + 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. >> > > Ok, I will move it. The struct was named this way to be the counterpart > to struct ept_data. Do you still think, we should introduce naming > differences for basically the same register at this point? Yes, we are talking about two distinct architectures. If you look at ept_data, it stores more than the page table register. Hence the name. [...] >>> + >>> #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. >> > > Actually it is used by p2m_is_altp2m and p2m_is_hostp2m (e.g. see assert > in p2m_flush_table). Right. Sorry, I didn't spot this call. Regards,
On 04/07/16 17:51, Sergej Proskurin wrote: > On 07/04/2016 06:15 PM, Julien Grall wrote: >> >> >> On 04/07/16 12:45, Sergej Proskurin wrote: >>> +static void p2m_teardown_hostp2m(struct domain *d) >>> +{ >>> + 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 ) >>> - free_domheap_pages(p2m->root, P2M_ROOT_ORDER); >>> + while ( (pg = page_list_remove_head(&p2m->pages)) ) >>> + if ( pg != p2m->root ) >>> + { >>> + mfn = _mfn(page_to_mfn(pg)); >>> + clear_domain_page(mfn); >> >> Can you explain why you are cleaning the page here? It was not part of >> p2m_teardown before this series. >> > > With the x86-based altp2m implementation, we experienced the problem > that altp2m-teardowns did not clean the pages. As a result, later > re-initialization reused the pages, which subsequently led to faults or > crashes due to reused mappings. We additionally clean the altp2m pages > and for the sake of completeness we clean the hostp2m tables as well. All the pages allocated for the p2m are cleared before any usage (see p2m_create_table and p2m_allocate_table). So there is no point to zero the page here. Also, unlike x86 we don't have a pool of page and directly use alloc_domheap_page to allocate a new page. So this would not prevent to get a non-zeroed page. In any case, a such change should be in a separate patch and not hidden among big changes. Regards,
On 07/04/2016 07:44 PM, Julien Grall wrote: > > > On 04/07/16 17:56, Sergej Proskurin wrote: >> Hi Andrew, >> >> On 07/04/2016 06:43 PM, Andrew Cooper wrote: >>> On 04/07/16 17:40, Sergej Proskurin wrote: >>>> >>>>>> { >>>>>> - 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? >>>>> >>>> Please correct me if I am wrong, but I associate a VMID with an entire >>>> domain. Since, the altp2m view still belongs to the same domain >>>> (p2m_init_one is called only from p2m_init_altp2m), the code re-uses >>>> the >>>> old VMID. >>> >>> (I am not an ARM expert but) looking into VMIDs from the last time, they >>> are the TLB tag for the address space in use. >>> >>> Does ARM have shared TLBs between multiple cores? If so, you must a >>> separate VMID, otherwise an ALT2PM used by one vcpu could cause a >>> separate vcpu with a different ALTP2M to reuse the wrong translation. >>> >>> ~Andrew >>> >> >> You're absolutely correct. However, on every VMENTRY Xen explicitly >> flushes the TLBs of the currently active domain (and with it, of the >> currently active (a)p2m table) and hence it should not result in an >> issue. > > VMENTRY is x86 not ARM. So are you sure you looked at the correct code? > > Regards, > This is true. I just use the term VMENTER for describing transitions to guests on both, x86 and ARM. In ./xen/arch/arm/domain.c the function ctxt_switch_to calls p2m_restore_state on every context switch, wich in turn loads the VTTBR associated to the domain and flushes the TLBs. Best regards, Sergej
Hello Sergej, On 04/07/2016 22:19, Sergej Proskurin wrote: > On 07/04/2016 07:44 PM, Julien Grall wrote: >> On 04/07/16 17:56, Sergej Proskurin wrote: >>> On 07/04/2016 06:43 PM, Andrew Cooper wrote: >>>> On 04/07/16 17:40, Sergej Proskurin wrote: >>>>> >>>>>>> { >>>>>>> - 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? >>>>>> >>>>> Please correct me if I am wrong, but I associate a VMID with an entire >>>>> domain. Since, the altp2m view still belongs to the same domain >>>>> (p2m_init_one is called only from p2m_init_altp2m), the code re-uses >>>>> the >>>>> old VMID. >>>> >>>> (I am not an ARM expert but) looking into VMIDs from the last time, they >>>> are the TLB tag for the address space in use. >>>> >>>> Does ARM have shared TLBs between multiple cores? If so, you must a >>>> separate VMID, otherwise an ALT2PM used by one vcpu could cause a >>>> separate vcpu with a different ALTP2M to reuse the wrong translation. >>>> >>>> ~Andrew >>>> >>> >>> You're absolutely correct. However, on every VMENTRY Xen explicitly >>> flushes the TLBs of the currently active domain (and with it, of the >>> currently active (a)p2m table) and hence it should not result in an >>> issue. >> >> VMENTRY is x86 not ARM. So are you sure you looked at the correct code? >> >> Regards, >> > > This is true. I just use the term VMENTER for describing transitions to > guests on both, x86 and ARM. In ./xen/arch/arm/domain.c the function > ctxt_switch_to calls p2m_restore_state on every context switch, wich in > turn loads the VTTBR associated to the domain and flushes the TLBs. Really? I have this patch series applied on top of staging and there is no TLB flush instruction in p2m_restore_state nor p2m_load*. Regards,
On 07/04/2016 08:18 PM, Julien Grall wrote: > > > On 04/07/16 17:43, Andrew Cooper wrote: >> On 04/07/16 17:40, Sergej Proskurin wrote: >>> >>>>> { >>>>> - 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? >>>> >>> Please correct me if I am wrong, but I associate a VMID with an entire >>> domain. Since, the altp2m view still belongs to the same domain >>> (p2m_init_one is called only from p2m_init_altp2m), the code re-uses the >>> old VMID. >> >> (I am not an ARM expert but) looking into VMIDs from the last time, they >> are the TLB tag for the address space in use. > > Correct. > >> >> Does ARM have shared TLBs between multiple cores? If so, you must a >> separate VMID, otherwise an ALT2PM used by one vcpu could cause a >> separate vcpu with a different ALTP2M to reuse the wrong translation. > > From the ARM ARM, I cannot rule out that TLBs cannot be shared from > multiple cores (CC a couple of ARM folks to confirm). > > Nevertheless, using a different VMID per P2M would avoid to care about > flushing when moving the vCPU around. > I see what you mean. This would definitely improve the context switch performance. That is, instead of adopting the hostp2m's vmid during altp2m table initialization, I will allocate a new VMID for every altp2m table --as you said before. This would simply eliminate the need for explicit TLB flushing. Thank you very much. > Regards, > Best regards, Sergej
On 07/04/2016 11:19 PM, Sergej Proskurin wrote: > > On 07/04/2016 07:44 PM, Julien Grall wrote: >> >> >> On 04/07/16 17:56, Sergej Proskurin wrote: >>> Hi Andrew, >>> >>> On 07/04/2016 06:43 PM, Andrew Cooper wrote: >>>> On 04/07/16 17:40, Sergej Proskurin wrote: >>>>> >>>>>>> { >>>>>>> - 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? >>>>>> >>>>> Please correct me if I am wrong, but I associate a VMID with an entire >>>>> domain. Since, the altp2m view still belongs to the same domain >>>>> (p2m_init_one is called only from p2m_init_altp2m), the code re-uses >>>>> the >>>>> old VMID. >>>> >>>> (I am not an ARM expert but) looking into VMIDs from the last time, they >>>> are the TLB tag for the address space in use. >>>> >>>> Does ARM have shared TLBs between multiple cores? If so, you must a >>>> separate VMID, otherwise an ALT2PM used by one vcpu could cause a >>>> separate vcpu with a different ALTP2M to reuse the wrong translation. >>>> >>>> ~Andrew >>>> >>> >>> You're absolutely correct. However, on every VMENTRY Xen explicitly >>> flushes the TLBs of the currently active domain (and with it, of the >>> currently active (a)p2m table) and hence it should not result in an >>> issue. >> >> VMENTRY is x86 not ARM. So are you sure you looked at the correct code? >> >> Regards, >> > > This is true. I just use the term VMENTER for describing transitions to > guests on both, x86 and ARM. In ./xen/arch/arm/domain.c the function > ctxt_switch_to calls p2m_restore_state on every context switch, wich in > turn loads the VTTBR associated to the domain and flushes the TLBs. > My bad: after loading the VTTBR, the TLBs are not explicitly flushed, as I claimed before. That is, using a dedicated VMID per altp2m view is definitely required. I will fix this. Thank you very much. Best regards, Sergej
On 07/04/2016 08:30 PM, Julien Grall wrote: > > > On 04/07/16 17:40, Sergej Proskurin wrote: >> On 07/04/2016 05:17 PM, Julien Grall wrote: >>> On 04/07/16 12:45, Sergej Proskurin wrote: > > [...] > >>>> +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. >>> >> >> I believe you mean p2m_free_one: The p2m_teardown_hostp2m might use the >> same function but would require the call p2m_free_vmid(d) to be called >> outside of p2m_free_one as well. This would require another acquisition >> of the p2m->lock. Just to be sure, I did not want to split the teardown >> process into two atomic executions. If you believe that it is safe to >> do, I will gladly change the code and re-use the code from p2m_free_one >> in p2m_teardown_hostp2m. > > Looking at the caller of p2m_teardown, I don't think the lock is > necessary because nobody can use the P2M anymore when this function is > called. > > [...] > Ok. I will adapt the code as discussed. Thank you. >>>> + { >>>> + 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? >>> >> >> We dynamically allocate altp2ms. Also, the initialization of both the >> altp2ms and hostp2m slightly differs (see VMID allocation). I could >> rewrite the initialization function to be used for both, the hostp2m and >> altp2m structs. Especially, if you say that we do not associate domains >> with VMIDs (see your upper question). > > I always prefer to see the code rewritten rather than duplicating code. > The latter makes harder to fix bug when it is spread in multiple place. > > [...] > Makes sense. Especially that we have now discussed the fact that we really need to allocate a new VMID per altp2m view. I will rewrite the functionality and remove the duplicated code. Thank you. >>>> + 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. >>> >> >> Ok, I will move it. The struct was named this way to be the counterpart >> to struct ept_data. Do you still think, we should introduce naming >> differences for basically the same register at this point? > > Yes, we are talking about two distinct architectures. If you look at > ept_data, it stores more than the page table register. Hence the name. > > [...] > Ok. >>>> + >>>> #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. >>> >> >> Actually it is used by p2m_is_altp2m and p2m_is_hostp2m (e.g. see assert >> in p2m_flush_table). > > Right. Sorry, I didn't spot this call. > It's all good: Thank you for the great review :) > Regards, > Best regards, Sergej
Hi Julien, On 07/04/2016 08:34 PM, Julien Grall wrote: > On 04/07/16 17:51, Sergej Proskurin wrote: >> On 07/04/2016 06:15 PM, Julien Grall wrote: >>> >>> >>> On 04/07/16 12:45, Sergej Proskurin wrote: >>>> +static void p2m_teardown_hostp2m(struct domain *d) >>>> +{ >>>> + 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 ) >>>> - free_domheap_pages(p2m->root, P2M_ROOT_ORDER); >>>> + while ( (pg = page_list_remove_head(&p2m->pages)) ) >>>> + if ( pg != p2m->root ) >>>> + { >>>> + mfn = _mfn(page_to_mfn(pg)); >>>> + clear_domain_page(mfn); >>> >>> Can you explain why you are cleaning the page here? It was not part of >>> p2m_teardown before this series. >>> >> >> With the x86-based altp2m implementation, we experienced the problem >> that altp2m-teardowns did not clean the pages. As a result, later >> re-initialization reused the pages, which subsequently led to faults or >> crashes due to reused mappings. We additionally clean the altp2m pages >> and for the sake of completeness we clean the hostp2m tables as well. > > All the pages allocated for the p2m are cleared before any usage (see > p2m_create_table and p2m_allocate_table). So there is no point to zero > the page here. > > Also, unlike x86 we don't have a pool of page and directly use > alloc_domheap_page to allocate a new page. So this would not prevent > to get a non-zeroed page. > > In any case, a such change should be in a separate patch and not > hidden among big changes. > > Regards, > Thank you Julien for clearing that up, I will remove the part that clears the page to be freed. Best regards, Sergej
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) { - 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; + 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) +{ + 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 ) - free_domheap_pages(p2m->root, P2M_ROOT_ORDER); + while ( (pg = page_list_remove_head(&p2m->pages)) ) + if ( pg != p2m->root ) + { + 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) { 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; + } + + 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; + d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d); + if ( p2m == NULL ) + { + p2m_teardown_altp2m(d); + 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. + */ + 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); + + 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 + 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; + 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 { + union { + struct { + u64 vttbr_baddr :40, /* variable res0: from 0-(x-1) bit */ + res1 :8, + vttbr_vmid :8, + 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> + #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; + + /* Back pointer to domain */ + struct domain *domain; + + /* VTTBR information */ + struct vttbr_data vttbr; }; /* List of possible type for each page in the p2m entry.
The p2m intialization now invokes intialization routines responsible for the allocation and intitialization of altp2m structures. The same applies to teardown routines. The functionality has been adopted from the x86 altp2m implementation. Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> --- 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(-)