| Message ID | 1469031064-23344-16-git-send-email-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
On 22/07/16 09:32, Sergej Proskurin wrote: > Hi Julien, Hello Sergej, >> -int p2m_alloc_table(struct domain *d) >> +static int p2m_alloc_table(struct domain *d) > > While moving parts of the altp2m code out of ./xen/arch/arm/p2m.c, the > function p2m_alloc_table needs to be called from ./xen/arch/arm/altp2m.c > to allocate the individual altp2m views. Hence it should not be static. No, this function should not be called outside p2m.c as it will not fully initialize the p2m. You need to need to provide a function to initialize a p2m (such as p2m_init). Regards.
On 22/07/16 11:16, Sergej Proskurin wrote: > Hi Julien, Hello, > On 07/22/2016 11:18 AM, Julien Grall wrote: >> >> >> On 22/07/16 09:32, Sergej Proskurin wrote: >>> Hi Julien, >> >> Hello Sergej, >> >>>> -int p2m_alloc_table(struct domain *d) >>>> +static int p2m_alloc_table(struct domain *d) >>> >>> While moving parts of the altp2m code out of ./xen/arch/arm/p2m.c, the >>> function p2m_alloc_table needs to be called from ./xen/arch/arm/altp2m.c >>> to allocate the individual altp2m views. Hence it should not be static. >> >> No, this function should not be called outside p2m.c as it will not >> fully initialize the p2m. You need to need to provide a function to >> initialize a p2m (such as p2m_init). >> > > The last time we have discussed reusing existing code, among others, for > individual struct p2m_domain initialization routines. Also, we have > agreed to move altp2m-related parts out of p2m.c into altp2m.c, which > makes it hard not to access parts required for initialization/teardown > (that are equal for both p2m and altp2m). I remember this discussion. However, the p2m initialize/teardown should exactly be the same for the hostp2m and altp2m (except for the type of the p2m). So, a function should be provided to initialize a full p2m to avoid code duplication. Regards,
On 22/07/16 11:39, Sergej Proskurin wrote: > > > On 07/22/2016 12:26 PM, Julien Grall wrote: >> >> >> On 22/07/16 11:16, Sergej Proskurin wrote: >>> Hi Julien, >> >> Hello, >> >>> On 07/22/2016 11:18 AM, Julien Grall wrote: >>>> >>>> >>>> On 22/07/16 09:32, Sergej Proskurin wrote: >>>>> Hi Julien, >>>> >>>> Hello Sergej, >>>> >>>>>> -int p2m_alloc_table(struct domain *d) >>>>>> +static int p2m_alloc_table(struct domain *d) >>>>> >>>>> While moving parts of the altp2m code out of ./xen/arch/arm/p2m.c, the >>>>> function p2m_alloc_table needs to be called from >>>>> ./xen/arch/arm/altp2m.c >>>>> to allocate the individual altp2m views. Hence it should not be static. >>>> >>>> No, this function should not be called outside p2m.c as it will not >>>> fully initialize the p2m. You need to need to provide a function to >>>> initialize a p2m (such as p2m_init). >>>> >>> >>> The last time we have discussed reusing existing code, among others, for >>> individual struct p2m_domain initialization routines. Also, we have >>> agreed to move altp2m-related parts out of p2m.c into altp2m.c, which >>> makes it hard not to access parts required for initialization/teardown >>> (that are equal for both p2m and altp2m). >> >> I remember this discussion. However, the p2m initialize/teardown should >> exactly be the same for the hostp2m and altp2m (except for the type of >> the p2m). So, a function should be provided to initialize a full p2m to >> avoid code duplication. >> > > This is exactly what has been done. Nevertheless, altp2m views are > somewhat more dynamic than hostp2m and hence require frequent > initialization/teardown of individual views. That is, by moving altp2m > parts out of p2m.c we simply need to access this shared code multiple > times at runtime from different altp2m-related functions. This applies > to more functions that just to p2m_alloc_table. I am not convinced that you need to reallocate the root page every time rather than clearing them. Anyway, I will need to see the code to understand what is done. Regards,
On 22/07/16 12:05, Sergej Proskurin wrote: > > > On 07/22/2016 12:38 PM, Julien Grall wrote: >> >> >> On 22/07/16 11:39, Sergej Proskurin wrote: >>> >>> >>> On 07/22/2016 12:26 PM, Julien Grall wrote: >>>> >>>> >>>> On 22/07/16 11:16, Sergej Proskurin wrote: >>>>> Hi Julien, >>>> >>>> Hello, >>>> >>>>> On 07/22/2016 11:18 AM, Julien Grall wrote: >>>>>> >>>>>> >>>>>> On 22/07/16 09:32, Sergej Proskurin wrote: >>>>>>> Hi Julien, >>>>>> >>>>>> Hello Sergej, >>>>>> >>>>>>>> -int p2m_alloc_table(struct domain *d) >>>>>>>> +static int p2m_alloc_table(struct domain *d) >>>>>>> >>>>>>> While moving parts of the altp2m code out of ./xen/arch/arm/p2m.c, >>>>>>> the >>>>>>> function p2m_alloc_table needs to be called from >>>>>>> ./xen/arch/arm/altp2m.c >>>>>>> to allocate the individual altp2m views. Hence it should not be >>>>>>> static. >>>>>> >>>>>> No, this function should not be called outside p2m.c as it will not >>>>>> fully initialize the p2m. You need to need to provide a function to >>>>>> initialize a p2m (such as p2m_init). >>>>>> >>>>> >>>>> The last time we have discussed reusing existing code, among others, >>>>> for >>>>> individual struct p2m_domain initialization routines. Also, we have >>>>> agreed to move altp2m-related parts out of p2m.c into altp2m.c, which >>>>> makes it hard not to access parts required for initialization/teardown >>>>> (that are equal for both p2m and altp2m). >>>> >>>> I remember this discussion. However, the p2m initialize/teardown should >>>> exactly be the same for the hostp2m and altp2m (except for the type of >>>> the p2m). So, a function should be provided to initialize a full p2m to >>>> avoid code duplication. >>>> >>> >>> This is exactly what has been done. Nevertheless, altp2m views are >>> somewhat more dynamic than hostp2m and hence require frequent >>> initialization/teardown of individual views. That is, by moving altp2m >>> parts out of p2m.c we simply need to access this shared code multiple >>> times at runtime from different altp2m-related functions. This applies >>> to more functions that just to p2m_alloc_table. >> >> I am not convinced that you need to reallocate the root page every time >> rather than clearing them. Anyway, I will need to see the code to >> understand what is done. >> >> Regards, >> > > In the following, you will find an excerpt of both the p2m.c and > altp2m.c concerning the (alt)p2m initialization. Please note that > altp2m_init_helper is called from different initialization routines in > altp2m.c. Also, please consider that this code has not yet been ported > to your recent patches. Please let me know if you need more information. > > static int altp2m_init_helper(struct domain *d, unsigned int idx) > { > int rc; > struct p2m_domain *p2m = d->arch.altp2m_p2m[idx]; > > if ( p2m == NULL ) > { > /* Allocate a new altp2m view. */ > p2m = xzalloc(struct p2m_domain); > if ( p2m == NULL) > { > rc = -ENOMEM; > goto err; > } > memset(p2m, 0, sizeof(struct p2m_domain)); > } > > /* Initialize the new altp2m view. */ > rc = p2m_init_one(d, p2m); > if ( rc ) > goto err; > > /* Allocate a root table for the altp2m view. */ > rc = p2m_alloc_table(p2m); This patch moved p2m_alloc_table call into p2m_init (i.e your p2m_init_one). You complained that the function was not exported anymore, but you did not look how it was called in this patch. > if ( rc ) > goto err; > > p2m->p2m_class = p2m_alternate; > p2m->access_required = 1; > _atomic_set(&p2m->active_vcpus, 0); > > d->arch.altp2m_p2m[idx] = p2m; > d->arch.altp2m_vttbr[idx] = p2m->vttbr.vttbr; Regards,
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 61fc08e..688adec 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -572,9 +572,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, if ( (rc = domain_io_init(d)) != 0 ) goto fail; - if ( (rc = p2m_alloc_table(d)) != 0 ) - goto fail; - switch ( config->gic_version ) { case XEN_DOMCTL_CONFIG_GIC_NATIVE: diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 6136767..c407e6a 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1282,7 +1282,7 @@ void guest_physmap_remove_page(struct domain *d, p2m_remove_mapping(d, gfn, (1 << page_order), mfn); } -int p2m_alloc_table(struct domain *d) +static int p2m_alloc_table(struct domain *d) { struct p2m_domain *p2m = &d->arch.p2m; struct page_info *page; @@ -1398,10 +1398,6 @@ int p2m_init(struct domain *d) if ( rc != 0 ) return rc; - d->arch.vttbr = 0; - - p2m->root = NULL; - p2m->max_mapped_gfn = _gfn(0); p2m->lowest_mapped_gfn = _gfn(ULONG_MAX); @@ -1409,6 +1405,8 @@ int p2m_init(struct domain *d) p2m->mem_access_enabled = false; radix_tree_init(&p2m->mem_access_settings); + rc = p2m_alloc_table(d); + return rc; } diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index abda70c..ce28e8a 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -149,13 +149,6 @@ void p2m_teardown(struct domain *d); */ int relinquish_p2m_mapping(struct domain *d); -/* - * Allocate a new p2m table for a domain. - * - * Returns 0 for success or -errno. - */ -int p2m_alloc_table(struct domain *d); - /* Context switch */ void p2m_save_state(struct vcpu *p); void p2m_restore_state(struct vcpu *n);
The p2m root table does not need to be allocate separately. Also remove unnecessary fields initialization as the structure is already memset to 0 and the fields will be override by p2m_alloc_table. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/domain.c | 3 --- xen/arch/arm/p2m.c | 8 +++----- xen/include/asm-arm/p2m.h | 7 ------- 3 files changed, 3 insertions(+), 15 deletions(-)