| Message ID | 20160801171028.11615-5-proskurin@sec.in.tum.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Sergej, Title: s/altp2m/p2m/ and please drop the full stop. On 01/08/16 18:10, Sergej Proskurin wrote: > This commit pulls out generic init/teardown functionality out of > p2m_init and p2m_teardown into p2m_init_one, p2m_free_one, and > p2m_flush_table functions. This allows our future implementation to > reuse existing code for the initialization/teardown of altp2m views. Please avoid to mix-up code movement and new additions. This makes the code more difficult to review. Also, you don't mention the new changes in the commit message. After reading the patch, it should really be divided and explain why you split like that. > > Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > --- > v2: Added the function p2m_flush_table to the previous version. > --- > xen/arch/arm/p2m.c | 74 +++++++++++++++++++++++++++++++++++++---------- > xen/include/asm-arm/p2m.h | 11 +++++++ > 2 files changed, 70 insertions(+), 15 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index cbc64a1..17f3299 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1360,50 +1360,94 @@ static void p2m_free_vmid(struct domain *d) > spin_unlock(&vmid_alloc_lock); > } > > -void p2m_teardown(struct domain *d) > +/* Reset this p2m table to be empty */ > +void p2m_flush_table(struct p2m_domain *p2m) Any function exported should have its prototype in an header within the same patch. > { > - struct p2m_domain *p2m = &d->arch.p2m; > - struct page_info *pg; > + struct page_info *page, *pg; > + unsigned int i; > + > + page = p2m->root; This function can be called with p2m->root equal to NULL. (see the check in p2m_free_one. > + > + /* Clear all concatenated first level pages */ > + for ( i = 0; i < P2M_ROOT_PAGES; i++ ) > + clear_and_clean_page(page + i); > > + /* Free the rest of the trie pages back to the paging pool */ > while ( (pg = page_list_remove_head(&p2m->pages)) ) > free_domheap_page(pg); > +} > + > +static inline void p2m_free_one(struct p2m_domain *p2m) Why inline here? Also, it seems that you export the function later. Why don't you do it here? Finally, I think this function should be rename p2m_teardown_one to match the callers' name. > +{ > + p2m_flush_table(p2m); > + > + /* Free VMID and reset VTTBR */ > + p2m_free_vmid(p2m->domain); Why do you move the call to p2m_free_vmid? > + p2m->vttbr.vttbr = INVALID_VTTBR; Why do you reset vttbr, the p2m will never be used afterwards. > > if ( p2m->root ) > free_domheap_pages(p2m->root, P2M_ROOT_ORDER); > > p2m->root = NULL; > > - p2m_free_vmid(d); > - > radix_tree_destroy(&p2m->mem_access_settings, NULL); > } > > -int p2m_init(struct domain *d) > +int p2m_init_one(struct domain *d, struct p2m_domain *p2m) Any function exported should have its prototype in an header within the same patch. > { > - struct p2m_domain *p2m = &d->arch.p2m; > int rc = 0; > > rwlock_init(&p2m->lock); > INIT_PAGE_LIST_HEAD(&p2m->pages); > > - p2m->vmid = INVALID_VMID; > - Why this is dropped? > rc = p2m_alloc_vmid(d); > if ( rc != 0 ) > return rc; > > - p2m->max_mapped_gfn = _gfn(0); > - p2m->lowest_mapped_gfn = _gfn(ULONG_MAX); > - > - p2m->default_access = p2m_access_rwx; > + p2m->domain = d; > + p2m->access_required = false; > p2m->mem_access_enabled = false; > + p2m->default_access = p2m_access_rwx; > + p2m->root = NULL; > + p2m->max_mapped_gfn = _gfn(0); > + p2m->lowest_mapped_gfn = INVALID_GFN; Please don't move code when it is not necessary. This make the code review more difficult to read. > + p2m->vttbr.vttbr = INVALID_VTTBR; > radix_tree_init(&p2m->mem_access_settings); > > - rc = p2m_alloc_table(d); > - The function p2m_init_one should fully initialize the p2m (i.e allocate the table). Why altp2m_destroy_by_id don't free the p2m entirely? This would simply a lot this series and avoid to spread p2m initialization everywhere. > return rc; > } > > +static void p2m_teardown_hostp2m(struct domain *d) > +{ > + struct p2m_domain *p2m = p2m_get_hostp2m(d); > + > + p2m_free_one(p2m); > +} > + > +void p2m_teardown(struct domain *d) > +{ > + p2m_teardown_hostp2m(d); > +} > + > +static int p2m_init_hostp2m(struct domain *d) > +{ > + int rc; > + struct p2m_domain *p2m = p2m_get_hostp2m(d); > + > + p2m->p2m_class = p2m_host; > + > + rc = p2m_init_one(d, p2m); > + if ( rc ) > + return rc; > + > + return p2m_alloc_table(d); > +} > + > +int p2m_init(struct domain *d) > +{ > + return p2m_init_hostp2m(d); > +} > + > int relinquish_p2m_mapping(struct domain *d) > { > struct p2m_domain *p2m = &d->arch.p2m; > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index 5c7cd1a..1f9c370 100644 > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h > @@ -18,6 +18,11 @@ struct domain; > > extern void memory_type_changed(struct domain *); > > +typedef enum { > + p2m_host, > + p2m_alternate, > +} p2m_class_t; > + This addition should really be in a separate patch. > /* Per-p2m-table state */ > struct p2m_domain { > /* Lock that protects updates to the p2m */ > @@ -78,6 +83,12 @@ struct p2m_domain { > * enough available bits to store this information. > */ > struct radix_tree_root mem_access_settings; > + > + /* Choose between: host/alternate */ > + p2m_class_t p2m_class; > + > + /* Back pointer to domain */ > + struct domain *domain; Same here. With justification why we want it. > }; > > /* > Regards,
Hi Julien, On 08/03/2016 07:40 PM, Julien Grall wrote: > Hello Sergej, > > Title: s/altp2m/p2m/ and please drop the full stop. Ok. > > On 01/08/16 18:10, Sergej Proskurin wrote: >> This commit pulls out generic init/teardown functionality out of >> p2m_init and p2m_teardown into p2m_init_one, p2m_free_one, and >> p2m_flush_table functions. This allows our future implementation to >> reuse existing code for the initialization/teardown of altp2m views. > > Please avoid to mix-up code movement and new additions. This makes the > code more difficult to review. I will remove unnecessary code movements. > > Also, you don't mention the new changes in the commit message. Since this patch is new to the patch series (the patch that got split is #07, where I have commented the changes), I did not add any but rather described the patch without being specific to the patch version. > > After reading the patch, it should really be divided and explain why > you split like that. > Ok. >> >> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> >> --- >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Julien Grall <julien.grall@arm.com> >> --- >> v2: Added the function p2m_flush_table to the previous version. >> --- >> xen/arch/arm/p2m.c | 74 >> +++++++++++++++++++++++++++++++++++++---------- >> xen/include/asm-arm/p2m.h | 11 +++++++ >> 2 files changed, 70 insertions(+), 15 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index cbc64a1..17f3299 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -1360,50 +1360,94 @@ static void p2m_free_vmid(struct domain *d) >> spin_unlock(&vmid_alloc_lock); >> } >> >> -void p2m_teardown(struct domain *d) >> +/* Reset this p2m table to be empty */ >> +void p2m_flush_table(struct p2m_domain *p2m) > > Any function exported should have its prototype in an header within > the same patch. > I will change that. >> { >> - struct p2m_domain *p2m = &d->arch.p2m; >> - struct page_info *pg; >> + struct page_info *page, *pg; >> + unsigned int i; >> + >> + page = p2m->root; > > > This function can be called with p2m->root equal to NULL. (see the > check in p2m_free_one. > I will add the check, thank you. >> + >> + /* Clear all concatenated first level pages */ >> + for ( i = 0; i < P2M_ROOT_PAGES; i++ ) >> + clear_and_clean_page(page + i); >> >> + /* Free the rest of the trie pages back to the paging pool */ >> while ( (pg = page_list_remove_head(&p2m->pages)) ) >> free_domheap_page(pg); >> +} >> + >> +static inline void p2m_free_one(struct p2m_domain *p2m) > > Why inline here? Also, it seems that you export the function later. > Why don't you do it here? > I will do that. Thank you. > Finally, I think this function should be rename p2m_teardown_one to > match the callers' name. > Ok. >> +{ >> + p2m_flush_table(p2m); >> + >> + /* Free VMID and reset VTTBR */ >> + p2m_free_vmid(p2m->domain); > > Why do you move the call to p2m_free_vmid? > When flushing a table, I did not want to free the associated VMID, as it would need to be allocated right afterwards (see altp2m_propagate_change and altp2m_reset). Since this would need to be done also in functions like p2m_altp2m_reset, I moved this call to p2m_free_one. I believe there is no need to free the VMIDs if the associated p2m is not freed as well. >> + p2m->vttbr.vttbr = INVALID_VTTBR; > > Why do you reset vttbr, the p2m will never be used afterwards. > Fair. I did that just for the sake of completeness. >> >> if ( p2m->root ) >> free_domheap_pages(p2m->root, P2M_ROOT_ORDER); >> >> p2m->root = NULL; >> >> - p2m_free_vmid(d); >> - >> radix_tree_destroy(&p2m->mem_access_settings, NULL); >> } >> >> -int p2m_init(struct domain *d) >> +int p2m_init_one(struct domain *d, struct p2m_domain *p2m) > > Any function exported should have its prototype in an header within > the same patch. > I will change that, thank you. >> { >> - struct p2m_domain *p2m = &d->arch.p2m; >> int rc = 0; >> >> rwlock_init(&p2m->lock); >> INIT_PAGE_LIST_HEAD(&p2m->pages); >> >> - p2m->vmid = INVALID_VMID; >> - > > Why this is dropped? > This will be shown in patch #07. We reuse altp2m views and check whether a p2m was flushed by checking for a valid VMID. >> rc = p2m_alloc_vmid(d); >> if ( rc != 0 ) >> return rc; >> >> - p2m->max_mapped_gfn = _gfn(0); >> - p2m->lowest_mapped_gfn = _gfn(ULONG_MAX); >> - >> - p2m->default_access = p2m_access_rwx; >> + p2m->domain = d; >> + p2m->access_required = false; >> p2m->mem_access_enabled = false; >> + p2m->default_access = p2m_access_rwx; >> + p2m->root = NULL; >> + p2m->max_mapped_gfn = _gfn(0); >> + p2m->lowest_mapped_gfn = INVALID_GFN; > > Please don't move code when it is not necessary. This make the code > review more difficult to read. > Ok. >> + p2m->vttbr.vttbr = INVALID_VTTBR; >> radix_tree_init(&p2m->mem_access_settings); >> >> - rc = p2m_alloc_table(d); >> - > > The function p2m_init_one should fully initialize the p2m (i.e > allocate the table). > The function p2m_init_one is currently called also for initializing the hostp2m, which is not dynamically allocated. Since we are sharing code between the hostp2m and altp2m initialization, I solved it that way. We could always allocate the hostp2m dynamically, that would solve it quite easily without additional checks. The other solution can simply check, whether the p2m is NULL and perform additional p2m allocation. What do you think? > Why altp2m_destroy_by_id don't free the p2m entirely? > This would simply a lot this series and avoid to spread p2m > initialization everywhere. > Same reason. I will change that accordingly, thank you. >> return rc; >> } >> >> +static void p2m_teardown_hostp2m(struct domain *d) >> +{ >> + struct p2m_domain *p2m = p2m_get_hostp2m(d); >> + >> + p2m_free_one(p2m); >> +} >> + >> +void p2m_teardown(struct domain *d) >> +{ >> + p2m_teardown_hostp2m(d); >> +} >> + >> +static int p2m_init_hostp2m(struct domain *d) >> +{ >> + int rc; >> + struct p2m_domain *p2m = p2m_get_hostp2m(d); >> + >> + p2m->p2m_class = p2m_host; >> + >> + rc = p2m_init_one(d, p2m); >> + if ( rc ) >> + return rc; >> + >> + return p2m_alloc_table(d); >> +} >> + >> +int p2m_init(struct domain *d) >> +{ >> + return p2m_init_hostp2m(d); >> +} >> + >> int relinquish_p2m_mapping(struct domain *d) >> { >> struct p2m_domain *p2m = &d->arch.p2m; >> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h >> index 5c7cd1a..1f9c370 100644 >> --- a/xen/include/asm-arm/p2m.h >> +++ b/xen/include/asm-arm/p2m.h >> @@ -18,6 +18,11 @@ struct domain; >> >> extern void memory_type_changed(struct domain *); >> >> +typedef enum { >> + p2m_host, >> + p2m_alternate, >> +} p2m_class_t; >> + > > This addition should really be in a separate patch. > The function p2m_init_hostp2m uses p2m_host for initialization. I can introduce a patch before this one, however to make it easier for the reviewer. >> /* Per-p2m-table state */ >> struct p2m_domain { >> /* Lock that protects updates to the p2m */ >> @@ -78,6 +83,12 @@ struct p2m_domain { >> * enough available bits to store this information. >> */ >> struct radix_tree_root mem_access_settings; >> + >> + /* Choose between: host/alternate */ >> + p2m_class_t p2m_class; >> + >> + /* Back pointer to domain */ >> + struct domain *domain; > > Same here. With justification why we want it. > Ok. >> }; >> >> /* >> Thank you very much. Best regards, ~Sergej
On 05/08/16 08:26, Sergej Proskurin wrote: > Hi Julien, Hello Sergej, > > On 08/03/2016 07:40 PM, Julien Grall wrote: [...] >>> +{ >>> + p2m_flush_table(p2m); >>> + >>> + /* Free VMID and reset VTTBR */ >>> + p2m_free_vmid(p2m->domain); >> >> Why do you move the call to p2m_free_vmid? >> > > When flushing a table, I did not want to free the associated VMID, as it > would need to be allocated right afterwards (see altp2m_propagate_change > and altp2m_reset). Since this would need to be done also in functions > like p2m_altp2m_reset, I moved this call to p2m_free_one. I believe > there is no need to free the VMIDs if the associated p2m is not freed as > well. That does not answer my question. You moved p2m_free_vmid within p2m_free_one. It used to be at the end of the function and now it is at the beginning. See... >>> + p2m->vttbr.vttbr = INVALID_VTTBR; [...] >>> >>> if ( p2m->root ) >>> free_domheap_pages(p2m->root, P2M_ROOT_ORDER); >>> >>> p2m->root = NULL; >>> >>> - p2m_free_vmid(d); >>> - here. So please don't move code unless there is a good reason. This series is already quite difficult to read. >>> radix_tree_destroy(&p2m->mem_access_settings, NULL); >>> } >>> >>> -int p2m_init(struct domain *d) >>> +int p2m_init_one(struct domain *d, struct p2m_domain *p2m) >> >> Any function exported should have its prototype in an header within >> the same patch. >> > > I will change that, thank you. > >>> { >>> - struct p2m_domain *p2m = &d->arch.p2m; >>> int rc = 0; >>> >>> rwlock_init(&p2m->lock); >>> INIT_PAGE_LIST_HEAD(&p2m->pages); >>> >>> - p2m->vmid = INVALID_VMID; >>> - >> >> Why this is dropped? >> > > This will be shown in patch #07. We reuse altp2m views and check whether > a p2m was flushed by checking for a valid VMID. Which is wrong. A flushed altp2m should not need to be reinitialized (i.e reseting the lock...). A flushed altp2m only need to have max_mapped_gfn and lowest_mapped_gfn reset. [...] >>> + p2m->vttbr.vttbr = INVALID_VTTBR; >>> radix_tree_init(&p2m->mem_access_settings); >>> >>> - rc = p2m_alloc_table(d); >>> - >> >> The function p2m_init_one should fully initialize the p2m (i.e >> allocate the table). >> > > The function p2m_init_one is currently called also for initializing the > hostp2m, which is not dynamically allocated. Since we are sharing code > between the hostp2m and altp2m initialization, I solved it that way. We > could always allocate the hostp2m dynamically, that would solve it quite > easily without additional checks. The other solution can simply check, > whether the p2m is NULL and perform additional p2m allocation. What do > you think? I don't understand your point here. It does not matter whether the hostp2m is allocated dynamically or not. p2m_init_one should fully initialize a p2m and this does not depends on dynamic allocation. [...] >>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h >>> index 5c7cd1a..1f9c370 100644 >>> --- a/xen/include/asm-arm/p2m.h >>> +++ b/xen/include/asm-arm/p2m.h >>> @@ -18,6 +18,11 @@ struct domain; >>> >>> extern void memory_type_changed(struct domain *); >>> >>> +typedef enum { >>> + p2m_host, >>> + p2m_alternate, >>> +} p2m_class_t; >>> + >> >> This addition should really be in a separate patch. >> > > The function p2m_init_hostp2m uses p2m_host for initialization. I can > introduce a patch before this one, however to make it easier for the > reviewer. You did not get my point here. A patch should do one thing: moving code or adding new code. Not both at the same. Adding p2m_class_t and setting p2m_host should really be done in a separate patch. This will ease the review this patch series. Regards,
Hi Julien, On 08/05/2016 11:16 AM, Julien Grall wrote: > > > On 05/08/16 08:26, Sergej Proskurin wrote: >> Hi Julien, > > Hello Sergej, > >> >> On 08/03/2016 07:40 PM, Julien Grall wrote: > > [...] > >>>> +{ >>>> + p2m_flush_table(p2m); >>>> + >>>> + /* Free VMID and reset VTTBR */ >>>> + p2m_free_vmid(p2m->domain); >>> >>> Why do you move the call to p2m_free_vmid? >>> >> >> When flushing a table, I did not want to free the associated VMID, as it >> would need to be allocated right afterwards (see altp2m_propagate_change >> and altp2m_reset). Since this would need to be done also in functions >> like p2m_altp2m_reset, I moved this call to p2m_free_one. I believe >> there is no need to free the VMIDs if the associated p2m is not freed as >> well. > > That does not answer my question. You moved p2m_free_vmid within > p2m_free_one. It used to be at the end of the function and now it is > at the beginning. See... > >>>> + p2m->vttbr.vttbr = INVALID_VTTBR; > > [...] > >>>> >>>> if ( p2m->root ) >>>> free_domheap_pages(p2m->root, P2M_ROOT_ORDER); >>>> >>>> p2m->root = NULL; >>>> >>>> - p2m_free_vmid(d); >>>> - > > here. So please don't move code unless there is a good reason. This > series is already quite difficult to read. > This move did not have any particular reason except that for me, it appeared to be more logical and cleaner to read this way. Apart from that: This patch creates two functions out of one (in the case of the former p2m_teardown). Because of this, I did not even think of preserving a certain function call order as the former function does not exit in a way it used to anymore. >>>> radix_tree_destroy(&p2m->mem_access_settings, NULL); >>>> } >>>> >>>> -int p2m_init(struct domain *d) >>>> +int p2m_init_one(struct domain *d, struct p2m_domain *p2m) >>> >>> Any function exported should have its prototype in an header within >>> the same patch. >>> >> >> I will change that, thank you. >> >>>> { >>>> - struct p2m_domain *p2m = &d->arch.p2m; >>>> int rc = 0; >>>> >>>> rwlock_init(&p2m->lock); >>>> INIT_PAGE_LIST_HEAD(&p2m->pages); >>>> >>>> - p2m->vmid = INVALID_VMID; >>>> - >>> >>> Why this is dropped? >>> >> >> This will be shown in patch #07. We reuse altp2m views and check whether >> a p2m was flushed by checking for a valid VMID. > > Which is wrong. A flushed altp2m should not need to be reinitialized > (i.e reseting the lock...). > > A flushed altp2m only need to have max_mapped_gfn and > lowest_mapped_gfn reset. > > [...] > I will try to adjust this function considering your suggestion in the patch #07. >>>> + p2m->vttbr.vttbr = INVALID_VTTBR; >>>> radix_tree_init(&p2m->mem_access_settings); >>>> >>>> - rc = p2m_alloc_table(d); >>>> - >>> >>> The function p2m_init_one should fully initialize the p2m (i.e >>> allocate the table). >>> >> >> The function p2m_init_one is currently called also for initializing the >> hostp2m, which is not dynamically allocated. Since we are sharing code >> between the hostp2m and altp2m initialization, I solved it that way. We >> could always allocate the hostp2m dynamically, that would solve it quite >> easily without additional checks. The other solution can simply check, >> whether the p2m is NULL and perform additional p2m allocation. What do >> you think? > > I don't understand your point here. It does not matter whether the > hostp2m is allocated dynamically or not. > > p2m_init_one should fully initialize a p2m and this does not depends > on dynamic allocation. > > [...] > Yes, you are correct. I will move try to prevent spreading of the p2m allocation across multiple functions. Thank you. >>>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h >>>> index 5c7cd1a..1f9c370 100644 >>>> --- a/xen/include/asm-arm/p2m.h >>>> +++ b/xen/include/asm-arm/p2m.h >>>> @@ -18,6 +18,11 @@ struct domain; >>>> >>>> extern void memory_type_changed(struct domain *); >>>> >>>> +typedef enum { >>>> + p2m_host, >>>> + p2m_alternate, >>>> +} p2m_class_t; >>>> + >>> >>> This addition should really be in a separate patch. >>> >> >> The function p2m_init_hostp2m uses p2m_host for initialization. I can >> introduce a patch before this one, however to make it easier for the >> reviewer. > > You did not get my point here. A patch should do one thing: moving > code or adding new code. Not both at the same. > > Adding p2m_class_t and setting p2m_host should really be done in a > separate patch. This will ease the review this patch series. Fair enough. Thank you. Best regards, ~Sergej
On 06/08/2016 09:43, Sergej Proskurin wrote: > Hi Julien, Hello Sergej, > On 08/05/2016 11:16 AM, Julien Grall wrote: >> On 05/08/16 08:26, Sergej Proskurin wrote: >>> On 08/03/2016 07:40 PM, Julien Grall wrote: [...] >>>>> + p2m->vttbr.vttbr = INVALID_VTTBR; >> >> [...] >> >>>>> >>>>> if ( p2m->root ) >>>>> free_domheap_pages(p2m->root, P2M_ROOT_ORDER); >>>>> >>>>> p2m->root = NULL; >>>>> >>>>> - p2m_free_vmid(d); >>>>> - >> >> here. So please don't move code unless there is a good reason. This >> series is already quite difficult to read. >> > > This move did not have any particular reason except that for me, it > appeared to be more logical and cleaner to read this way. Apart from > that: This patch creates two functions out of one (in the case of the > former p2m_teardown). Because of this, I did not even think of > preserving a certain function call order as the former function does not > exit in a way it used to anymore. In this specific case, the call p2m_free_vmid is at the end of function to match the reverse order of p2m_init. Regardless that I cannot see why moving p2m_free_vmid ealier will be more logical. Anyway, you don't move code within a function unless there is a reason. And this should really be outside of a patch doing bigger. A series like altp2m takes me about a full working day to review. So please don't make it more difficult to review. Regards,
On 08/06/2016 03:26 PM, Julien Grall wrote: > On 06/08/2016 09:43, Sergej Proskurin wrote: >> Hi Julien, > > Hello Sergej, > >> On 08/05/2016 11:16 AM, Julien Grall wrote: >>> On 05/08/16 08:26, Sergej Proskurin wrote: >>>> On 08/03/2016 07:40 PM, Julien Grall wrote: > > [...] > >>>>>> + p2m->vttbr.vttbr = INVALID_VTTBR; >>> >>> [...] >>> >>>>>> >>>>>> if ( p2m->root ) >>>>>> free_domheap_pages(p2m->root, P2M_ROOT_ORDER); >>>>>> >>>>>> p2m->root = NULL; >>>>>> >>>>>> - p2m_free_vmid(d); >>>>>> - >>> >>> here. So please don't move code unless there is a good reason. This >>> series is already quite difficult to read. >>> >> >> This move did not have any particular reason except that for me, it >> appeared to be more logical and cleaner to read this way. Apart from >> that: This patch creates two functions out of one (in the case of the >> former p2m_teardown). Because of this, I did not even think of >> preserving a certain function call order as the former function does not >> exit in a way it used to anymore. > > In this specific case, the call p2m_free_vmid is at the end of > function to match the reverse order of p2m_init. > > Regardless that I cannot see why moving p2m_free_vmid ealier will be > more logical. > > Anyway, you don't move code within a function unless there is a > reason. And this should really be outside of a patch doing bigger. > > A series like altp2m takes me about a full working day to review. So > please don't make it more difficult to review. > I will try to avoid such code movements. Thank you. Best regards, ~Sergej
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index cbc64a1..17f3299 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1360,50 +1360,94 @@ static void p2m_free_vmid(struct domain *d) spin_unlock(&vmid_alloc_lock); } -void p2m_teardown(struct domain *d) +/* Reset this p2m table to be empty */ +void p2m_flush_table(struct p2m_domain *p2m) { - struct p2m_domain *p2m = &d->arch.p2m; - struct page_info *pg; + struct page_info *page, *pg; + unsigned int i; + + page = p2m->root; + + /* Clear all concatenated first level pages */ + for ( i = 0; i < P2M_ROOT_PAGES; i++ ) + clear_and_clean_page(page + i); + /* Free the rest of the trie pages back to the paging pool */ while ( (pg = page_list_remove_head(&p2m->pages)) ) free_domheap_page(pg); +} + +static inline void p2m_free_one(struct p2m_domain *p2m) +{ + p2m_flush_table(p2m); + + /* Free VMID and reset VTTBR */ + p2m_free_vmid(p2m->domain); + p2m->vttbr.vttbr = INVALID_VTTBR; if ( p2m->root ) free_domheap_pages(p2m->root, P2M_ROOT_ORDER); p2m->root = NULL; - p2m_free_vmid(d); - radix_tree_destroy(&p2m->mem_access_settings, NULL); } -int p2m_init(struct domain *d) +int p2m_init_one(struct domain *d, struct p2m_domain *p2m) { - struct p2m_domain *p2m = &d->arch.p2m; int rc = 0; rwlock_init(&p2m->lock); INIT_PAGE_LIST_HEAD(&p2m->pages); - p2m->vmid = INVALID_VMID; - rc = p2m_alloc_vmid(d); if ( rc != 0 ) return rc; - p2m->max_mapped_gfn = _gfn(0); - p2m->lowest_mapped_gfn = _gfn(ULONG_MAX); - - p2m->default_access = p2m_access_rwx; + p2m->domain = d; + p2m->access_required = false; p2m->mem_access_enabled = false; + p2m->default_access = p2m_access_rwx; + p2m->root = NULL; + p2m->max_mapped_gfn = _gfn(0); + p2m->lowest_mapped_gfn = INVALID_GFN; + p2m->vttbr.vttbr = INVALID_VTTBR; radix_tree_init(&p2m->mem_access_settings); - rc = p2m_alloc_table(d); - return rc; } +static void p2m_teardown_hostp2m(struct domain *d) +{ + struct p2m_domain *p2m = p2m_get_hostp2m(d); + + p2m_free_one(p2m); +} + +void p2m_teardown(struct domain *d) +{ + p2m_teardown_hostp2m(d); +} + +static int p2m_init_hostp2m(struct domain *d) +{ + int rc; + struct p2m_domain *p2m = p2m_get_hostp2m(d); + + p2m->p2m_class = p2m_host; + + rc = p2m_init_one(d, p2m); + if ( rc ) + return rc; + + return p2m_alloc_table(d); +} + +int p2m_init(struct domain *d) +{ + return p2m_init_hostp2m(d); +} + int relinquish_p2m_mapping(struct domain *d) { struct p2m_domain *p2m = &d->arch.p2m; diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 5c7cd1a..1f9c370 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -18,6 +18,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 */ @@ -78,6 +83,12 @@ struct p2m_domain { * enough available bits to store this information. */ struct radix_tree_root mem_access_settings; + + /* Choose between: host/alternate */ + p2m_class_t p2m_class; + + /* Back pointer to domain */ + struct domain *domain; }; /*
This commit pulls out generic init/teardown functionality out of p2m_init and p2m_teardown into p2m_init_one, p2m_free_one, and p2m_flush_table functions. This allows our future implementation to reuse existing code for the initialization/teardown of altp2m views. Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> --- v2: Added the function p2m_flush_table to the previous version. --- xen/arch/arm/p2m.c | 74 +++++++++++++++++++++++++++++++++++++---------- xen/include/asm-arm/p2m.h | 11 +++++++ 2 files changed, 70 insertions(+), 15 deletions(-)