| Message ID | 20160816221714.22041-11-proskurin@sec.in.tum.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Sergej, On 16/08/16 23:16, Sergej Proskurin wrote: > --- > xen/arch/arm/p2m.c | 71 +++++++++++++++++++++++++++++++++++++++++------ > xen/include/asm-arm/p2m.h | 11 ++++++++ > 2 files changed, 73 insertions(+), 9 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index e859fca..9ef19d4 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1245,27 +1245,53 @@ 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 = p2m_get_hostp2m(d); > - struct page_info *pg; > + struct page_info *page, *pg; > + unsigned int i; > + > + if ( p2m->root ) > + { > + page = p2m->root; > + > + /* Clear all concatenated first level pages. */ s/first/root/ > + for ( i = 0; i < P2M_ROOT_PAGES; i++ ) > + clear_and_clean_page(page + i); Clearing live page table like that is not safe. Each entry (64-bit) should be written atomically to avoid breaking coherency (e.g if the MMU is walking the page table at the same time). However you don't know the implementation of clear_and_clean_page. Also, this adds a small overhead when tearing down a p2m because the clear is not necessary. > + } > + > + /* > + * Flush TLBs before releasing remaining intermediate p2m page tables to > + * prevent illegal access to stalled TLB entries. > + */ > + p2m_flush_tlb(p2m); p2m_flush_table is called in 2 places: - p2m_teardown_one - altp2m_reset For p2m_teardown_one, the p2m may not have been allocated because the initialization failed. So try flush the TLBs may lead to a panic in Xen (the vttbr is invalid). For altp2m_reset, this is called while updating the page tables (see altp2m_propagate_change). vCPU may still use the page tables at that time. I am a bit worry that clear_and_clean_page > > + /* Free the rest of the trie pages back to the paging pool. */ > while ( (pg = page_list_remove_head(&p2m->pages)) ) > free_domheap_page(pg); > > + p2m->lowest_mapped_gfn = INVALID_GFN; > + p2m->max_mapped_gfn = _gfn(0); > +} > + > +void p2m_teardown_one(struct p2m_domain *p2m) > +{ > + p2m_flush_table(p2m); > + > if ( p2m->root ) > free_domheap_pages(p2m->root, P2M_ROOT_ORDER); > > p2m->root = NULL; > > - p2m_free_vmid(d); > + p2m_free_vmid(p2m->domain); > + > + p2m->vttbr = INVALID_VTTBR; Why did you add this line? > > 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 = p2m_get_hostp2m(d); > int rc = 0; > > rwlock_init(&p2m->lock); > @@ -1278,11 +1304,14 @@ int p2m_init(struct domain *d) > return rc; > > p2m->max_mapped_gfn = _gfn(0); > - p2m->lowest_mapped_gfn = _gfn(ULONG_MAX); > + p2m->lowest_mapped_gfn = INVALID_GFN; Why this change? > > p2m->domain = d; > + p2m->access_required = false; This is not necessary as the p2m is zeroed during the allocation. > p2m->default_access = p2m_access_rwx; > p2m->mem_access_enabled = false; > + p2m->root = NULL; > + p2m->vttbr = INVALID_VTTBR; Why do you add those 2 lines? > radix_tree_init(&p2m->mem_access_settings); > > /* > @@ -1293,9 +1322,33 @@ int p2m_init(struct domain *d) > p2m->clean_pte = iommu_enabled && > !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK); > > - rc = p2m_alloc_table(d); > + return p2m_alloc_table(d); > +} > > - return rc; > +static void p2m_teardown_hostp2m(struct domain *d) > +{ > + struct p2m_domain *p2m = p2m_get_hostp2m(d); > + > + p2m_teardown_one(p2m); > +} > + > +void p2m_teardown(struct domain *d) > +{ > + p2m_teardown_hostp2m(d); > +} > + > +static int p2m_init_hostp2m(struct domain *d) > +{ > + struct p2m_domain *p2m = p2m_get_hostp2m(d); > + > + p2m->p2m_class = p2m_host; > + > + return p2m_init_one(d, p2m); > +} > + > +int p2m_init(struct domain *d) > +{ > + return p2m_init_hostp2m(d); > } > > /* > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index fa07e19..1a004ed 100644 > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h > @@ -11,6 +11,8 @@ > > #define paddr_bits PADDR_BITS > > +#define INVALID_VTTBR (0UL) Looking at the current use, you only use INVALID_VTTBR to set but not tested. However, the 2 places where it is use are not necessary. > + > /* Holds the bit size of IPAs in p2m tables. */ > extern unsigned int p2m_ipa_bits; > > @@ -226,6 +228,15 @@ void guest_physmap_remove_page(struct domain *d, > > mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn); > > +/* Flushes the page table held by the p2m. */ > +void p2m_flush_table(struct p2m_domain *p2m); > + > +/* Initialize the p2m structure. */ > +int p2m_init_one(struct domain *d, struct p2m_domain *p2m); > + > +/* Release resources held by the p2m structure. */ > +void p2m_teardown_one(struct p2m_domain *p2m); > + > /* > * P2M rwlock helpers. > */ > Regards,
Hi Julien, On 09/01/2016 07:36 PM, Julien Grall wrote: > Hello Sergej, > > On 16/08/16 23:16, Sergej Proskurin wrote: >> --- >> xen/arch/arm/p2m.c | 71 >> +++++++++++++++++++++++++++++++++++++++++------ >> xen/include/asm-arm/p2m.h | 11 ++++++++ >> 2 files changed, 73 insertions(+), 9 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index e859fca..9ef19d4 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -1245,27 +1245,53 @@ 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 = p2m_get_hostp2m(d); >> - struct page_info *pg; >> + struct page_info *page, *pg; >> + unsigned int i; >> + >> + if ( p2m->root ) >> + { >> + page = p2m->root; >> + >> + /* Clear all concatenated first level pages. */ > > s/first/root/ > Ok. >> + for ( i = 0; i < P2M_ROOT_PAGES; i++ ) >> + clear_and_clean_page(page + i); > > Clearing live page table like that is not safe. Each entry (64-bit) > should be written atomically to avoid breaking coherency (e.g if the MMU > is walking the page table at the same time). However you don't know the > implementation of clear_and_clean_page. The function p2m_flush_table gets called by the altp2m subsystem indrectly through the function p2m_teardown_one when the associated view gets destroyed. In this case we should not worry about crashing the domain, as the altp2m views are not active anyway. The function altp2m_reset calls the function p2m_flush_table directly (with active altp2m views), however, locks the p2m before flushing the table. I did not find any locks on page-granularity, so please provide me with further information about the solution you had in mind. > > Also, this adds a small overhead when tearing down a p2m because the > clear is not necessary. > The p2m views are cleared very rarely so the overhead is really minimal as it affects clearing the root tables. Besides the function altp2m_reset calls the function p2m_flush_table and assumes that the root tables are cleared as well. If the root tables would not be cleared at this point, stalled entries in the altp2m views that got wiped out in the hostp2m would make the system unstable. >> + } >> + >> + /* >> + * Flush TLBs before releasing remaining intermediate p2m page >> tables to >> + * prevent illegal access to stalled TLB entries. >> + */ >> + p2m_flush_tlb(p2m); > > p2m_flush_table is called in 2 places: > - p2m_teardown_one > - altp2m_reset > > For p2m_teardown_one, the p2m may not have been allocated because the > initialization failed. So try flush the TLBs may lead to a panic in Xen > (the vttbr is invalid). > > For altp2m_reset, this is called while updating the page tables (see > altp2m_propagate_change). vCPU may still use the page tables at that time. > > I am a bit worry that clear_and_clean_page > >> >> + /* Free the rest of the trie pages back to the paging pool. */ >> while ( (pg = page_list_remove_head(&p2m->pages)) ) >> free_domheap_page(pg); >> >> + p2m->lowest_mapped_gfn = INVALID_GFN; >> + p2m->max_mapped_gfn = _gfn(0); >> +} >> + >> +void p2m_teardown_one(struct p2m_domain *p2m) >> +{ >> + p2m_flush_table(p2m); >> + >> if ( p2m->root ) >> free_domheap_pages(p2m->root, P2M_ROOT_ORDER); >> >> p2m->root = NULL; >> >> - p2m_free_vmid(d); >> + p2m_free_vmid(p2m->domain); >> + >> + p2m->vttbr = INVALID_VTTBR; > > Why did you add this line? > Ok. On every p2m_teardown_one invocation the p2m gets destroyed. That is, I will remove this additional resetting of the VTTBR in the next patch. Thank you. >> >> 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 = p2m_get_hostp2m(d); >> int rc = 0; >> >> rwlock_init(&p2m->lock); >> @@ -1278,11 +1304,14 @@ int p2m_init(struct domain *d) >> return rc; >> >> p2m->max_mapped_gfn = _gfn(0); >> - p2m->lowest_mapped_gfn = _gfn(ULONG_MAX); >> + p2m->lowest_mapped_gfn = INVALID_GFN; > > Why this change? > Since we compare the gfn's with INVALID_GFN throughout the code it makes sense to use the macro instead of a hardcoded value. >> >> p2m->domain = d; >> + p2m->access_required = false; > > This is not necessary as the p2m is zeroed during the allocation. > Since the p2m is zeroed out at this moment, I can savely remove this initialization. >> p2m->default_access = p2m_access_rwx; >> p2m->mem_access_enabled = false; >> + p2m->root = NULL; >> + p2m->vttbr = INVALID_VTTBR; > > Why do you add those 2 lines? > Same here. >> radix_tree_init(&p2m->mem_access_settings); >> >> /* >> @@ -1293,9 +1322,33 @@ int p2m_init(struct domain *d) >> p2m->clean_pte = iommu_enabled && >> !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK); >> >> - rc = p2m_alloc_table(d); >> + return p2m_alloc_table(d); >> +} >> >> - return rc; >> +static void p2m_teardown_hostp2m(struct domain *d) >> +{ >> + struct p2m_domain *p2m = p2m_get_hostp2m(d); >> + >> + p2m_teardown_one(p2m); >> +} >> + >> +void p2m_teardown(struct domain *d) >> +{ >> + p2m_teardown_hostp2m(d); >> +} >> + >> +static int p2m_init_hostp2m(struct domain *d) >> +{ >> + struct p2m_domain *p2m = p2m_get_hostp2m(d); >> + >> + p2m->p2m_class = p2m_host; >> + >> + return p2m_init_one(d, p2m); >> +} >> + >> +int p2m_init(struct domain *d) >> +{ >> + return p2m_init_hostp2m(d); >> } >> >> /* >> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h >> index fa07e19..1a004ed 100644 >> --- a/xen/include/asm-arm/p2m.h >> +++ b/xen/include/asm-arm/p2m.h >> @@ -11,6 +11,8 @@ >> >> #define paddr_bits PADDR_BITS >> >> +#define INVALID_VTTBR (0UL) > > Looking at the current use, you only use INVALID_VTTBR to set but not > tested. However, the 2 places where it is use are not necessary. > I will remove the macro including the both cases, where it is used for initialization. >> + >> /* Holds the bit size of IPAs in p2m tables. */ >> extern unsigned int p2m_ipa_bits; >> >> @@ -226,6 +228,15 @@ void guest_physmap_remove_page(struct domain *d, >> >> mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn); >> >> +/* Flushes the page table held by the p2m. */ >> +void p2m_flush_table(struct p2m_domain *p2m); >> + >> +/* Initialize the p2m structure. */ >> +int p2m_init_one(struct domain *d, struct p2m_domain *p2m); >> + >> +/* Release resources held by the p2m structure. */ >> +void p2m_teardown_one(struct p2m_domain *p2m); >> + >> /* >> * P2M rwlock helpers. >> */ >> > > Regards, > Best regards, ~Sergej
On 02/09/16 10:09, Sergej Proskurin wrote: > Hi Julien, > > On 09/01/2016 07:36 PM, Julien Grall wrote: >> Hello Sergej, >> >> On 16/08/16 23:16, Sergej Proskurin wrote: >>> --- >>> xen/arch/arm/p2m.c | 71 >>> +++++++++++++++++++++++++++++++++++++++++------ >>> xen/include/asm-arm/p2m.h | 11 ++++++++ >>> 2 files changed, 73 insertions(+), 9 deletions(-) >>> >>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>> index e859fca..9ef19d4 100644 >>> --- a/xen/arch/arm/p2m.c >>> +++ b/xen/arch/arm/p2m.c >>> @@ -1245,27 +1245,53 @@ 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 = p2m_get_hostp2m(d); >>> - struct page_info *pg; >>> + struct page_info *page, *pg; >>> + unsigned int i; >>> + >>> + if ( p2m->root ) >>> + { >>> + page = p2m->root; >>> + >>> + /* Clear all concatenated first level pages. */ >> >> s/first/root/ >> > > Ok. > >>> + for ( i = 0; i < P2M_ROOT_PAGES; i++ ) >>> + clear_and_clean_page(page + i); >> >> Clearing live page table like that is not safe. Each entry (64-bit) >> should be written atomically to avoid breaking coherency (e.g if the MMU >> is walking the page table at the same time). However you don't know the >> implementation of clear_and_clean_page. > > The function p2m_flush_table gets called by the altp2m subsystem > indrectly through the function p2m_teardown_one when the associated view > gets destroyed. In this case we should not worry about crashing the > domain, as the altp2m views are not active anyway. > > The function altp2m_reset calls the function p2m_flush_table directly > (with active altp2m views), however, locks the p2m before flushing the > table. I did not find any locks on page-granularity, so please provide > me with further information about the solution you had in mind. I never mentioned any locking problem. As said in my previous mail, the altp2m may still be in use by another vCPU. So the MMU (i.e the hardware) may want do a table walk whilst you modify the entry. The MMU is reading the entry (64-bit) at once so it also expects the entry to be modified atomically. However, you don't know the implementation of clean_and_clean_page. The function might write 32-bit at the time, which means that the MMU will see bogus entry. At best it will lead to a crash, at worst open a security issue. >> >> Also, this adds a small overhead when tearing down a p2m because the >> clear is not necessary. >> > > The p2m views are cleared very rarely so the overhead is really minimal > as it affects clearing the root tables. You seem to forget the p2m teardown is also called during domain destruction. > Besides the function > altp2m_reset calls the function p2m_flush_table and assumes that the > root tables are cleared as well. If the root tables would not be cleared > at this point, stalled entries in the altp2m views that got wiped out in > the hostp2m would make the system unstable. As I already mentioned in the previous version, this code was not present before and based of the description of this commit the patch is supposed to only move code around. This is not the case here. >>> >>> 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 = p2m_get_hostp2m(d); >>> int rc = 0; >>> >>> rwlock_init(&p2m->lock); >>> @@ -1278,11 +1304,14 @@ int p2m_init(struct domain *d) >>> return rc; >>> >>> p2m->max_mapped_gfn = _gfn(0); >>> - p2m->lowest_mapped_gfn = _gfn(ULONG_MAX); >>> + p2m->lowest_mapped_gfn = INVALID_GFN; >> >> Why this change? >> > > Since we compare the gfn's with INVALID_GFN throughout the code it makes > sense to use the macro instead of a hardcoded value. Please don't do unnecessary change. This patch is complex enough to review. Regards,
Hi Julien, On 09/02/2016 12:51 PM, Julien Grall wrote: > > > On 02/09/16 10:09, Sergej Proskurin wrote: >> Hi Julien, >> >> On 09/01/2016 07:36 PM, Julien Grall wrote: >>> Hello Sergej, >>> >>> On 16/08/16 23:16, Sergej Proskurin wrote: >>>> --- >>>> xen/arch/arm/p2m.c | 71 >>>> +++++++++++++++++++++++++++++++++++++++++------ >>>> xen/include/asm-arm/p2m.h | 11 ++++++++ >>>> 2 files changed, 73 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>>> index e859fca..9ef19d4 100644 >>>> --- a/xen/arch/arm/p2m.c >>>> +++ b/xen/arch/arm/p2m.c >>>> @@ -1245,27 +1245,53 @@ 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 = p2m_get_hostp2m(d); >>>> - struct page_info *pg; >>>> + struct page_info *page, *pg; >>>> + unsigned int i; >>>> + >>>> + if ( p2m->root ) >>>> + { >>>> + page = p2m->root; >>>> + >>>> + /* Clear all concatenated first level pages. */ >>> >>> s/first/root/ >>> >> >> Ok. >> >>>> + for ( i = 0; i < P2M_ROOT_PAGES; i++ ) >>>> + clear_and_clean_page(page + i); >>> >>> Clearing live page table like that is not safe. Each entry (64-bit) >>> should be written atomically to avoid breaking coherency (e.g if the >>> MMU >>> is walking the page table at the same time). However you don't know the >>> implementation of clear_and_clean_page. >> >> The function p2m_flush_table gets called by the altp2m subsystem >> indrectly through the function p2m_teardown_one when the associated view >> gets destroyed. In this case we should not worry about crashing the >> domain, as the altp2m views are not active anyway. >> >> The function altp2m_reset calls the function p2m_flush_table directly >> (with active altp2m views), however, locks the p2m before flushing the >> table. I did not find any locks on page-granularity, so please provide >> me with further information about the solution you had in mind. > > I never mentioned any locking problem. As said in my previous mail, > the altp2m may still be in use by another vCPU. So the MMU (i.e the > hardware) may want do a table walk whilst you modify the entry. > > The MMU is reading the entry (64-bit) at once so it also expects the > entry to be modified atomically. However, you don't know the > implementation of clean_and_clean_page. The function might write > 32-bit at the time, which means that the MMU will see bogus entry. At > best it will lead to a crash, at worst open a security issue. > I see your point. Not sure how to fix this, though. I believe that the described issue would remain if we would use free_domheap_pages. Instead, maybe we should manually set the value in the translation tables? Or, what if we flush the TLBs immediately before unmapping the root pages? This would cause the system to load the mappings from memory and delay a MMU table walk so that it would potentially resolve the atomicity issue. Do you have another suggestion? >>> >>> Also, this adds a small overhead when tearing down a p2m because the >>> clear is not necessary. >>> >> >> The p2m views are cleared very rarely so the overhead is really minimal >> as it affects clearing the root tables. > > You seem to forget the p2m teardown is also called during domain > destruction. > >> Besides the function >> altp2m_reset calls the function p2m_flush_table and assumes that the >> root tables are cleared as well. If the root tables would not be cleared >> at this point, stalled entries in the altp2m views that got wiped out in >> the hostp2m would make the system unstable. > > As I already mentioned in the previous version, this code was not > present before and based of the description of this commit the patch > is supposed to only move code around. This is not the case here. > I will move the part with the clear_and_clean_page invocation into a separate patch. > >>>> >>>> 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 = p2m_get_hostp2m(d); >>>> int rc = 0; >>>> >>>> rwlock_init(&p2m->lock); >>>> @@ -1278,11 +1304,14 @@ int p2m_init(struct domain *d) >>>> return rc; >>>> >>>> p2m->max_mapped_gfn = _gfn(0); >>>> - p2m->lowest_mapped_gfn = _gfn(ULONG_MAX); >>>> + p2m->lowest_mapped_gfn = INVALID_GFN; >>> >>> Why this change? >>> >> >> Since we compare the gfn's with INVALID_GFN throughout the code it makes >> sense to use the macro instead of a hardcoded value. > > Please don't do unnecessary change. This patch is complex enough to > review. Ok. Cheers, ~Sergej
Hello Sergej, On 05/09/16 11:23, Sergej Proskurin wrote: > On 09/02/2016 12:51 PM, Julien Grall wrote: >> On 02/09/16 10:09, Sergej Proskurin wrote: >>> On 09/01/2016 07:36 PM, Julien Grall wrote: >>>> On 16/08/16 23:16, Sergej Proskurin wrote: >>>> Clearing live page table like that is not safe. Each entry (64-bit) >>>> should be written atomically to avoid breaking coherency (e.g if the >>>> MMU >>>> is walking the page table at the same time). However you don't know the >>>> implementation of clear_and_clean_page. >>> >>> The function p2m_flush_table gets called by the altp2m subsystem >>> indrectly through the function p2m_teardown_one when the associated view >>> gets destroyed. In this case we should not worry about crashing the >>> domain, as the altp2m views are not active anyway. >>> >>> The function altp2m_reset calls the function p2m_flush_table directly >>> (with active altp2m views), however, locks the p2m before flushing the >>> table. I did not find any locks on page-granularity, so please provide >>> me with further information about the solution you had in mind. >> >> I never mentioned any locking problem. As said in my previous mail, >> the altp2m may still be in use by another vCPU. So the MMU (i.e the >> hardware) may want do a table walk whilst you modify the entry. >> >> The MMU is reading the entry (64-bit) at once so it also expects the >> entry to be modified atomically. However, you don't know the >> implementation of clean_and_clean_page. The function might write >> 32-bit at the time, which means that the MMU will see bogus entry. At >> best it will lead to a crash, at worst open a security issue. >> > > I see your point. Not sure how to fix this, though. I believe that the > described issue would remain if we would use free_domheap_pages. > Instead, maybe we should manually set the value in the translation tables? This is the right way to go. > > Or, what if we flush the TLBs immediately before unmapping the root > pages? This would cause the system to load the mappings from memory and > delay a MMU table walk so that it would potentially resolve the > atomicity issue. I am not sure to fully understand this suggestion. Anyway, the first suggestion (i.e invalidating the entry one by one is the right way to go). >> >>>>> >>>>> 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 = p2m_get_hostp2m(d); >>>>> int rc = 0; >>>>> >>>>> rwlock_init(&p2m->lock); >>>>> @@ -1278,11 +1304,14 @@ int p2m_init(struct domain *d) >>>>> return rc; >>>>> >>>>> p2m->max_mapped_gfn = _gfn(0); >>>>> - p2m->lowest_mapped_gfn = _gfn(ULONG_MAX); >>>>> + p2m->lowest_mapped_gfn = INVALID_GFN; >>>> >>>> Why this change? >>>> >>> >>> Since we compare the gfn's with INVALID_GFN throughout the code it makes >>> sense to use the macro instead of a hardcoded value. >> >> Please don't do unnecessary change. This patch is complex enough to >> review. > > Ok. If you want to do the change, then it should be done separately. Regards,
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index e859fca..9ef19d4 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1245,27 +1245,53 @@ 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 = p2m_get_hostp2m(d); - struct page_info *pg; + struct page_info *page, *pg; + unsigned int i; + + if ( p2m->root ) + { + page = p2m->root; + + /* Clear all concatenated first level pages. */ + for ( i = 0; i < P2M_ROOT_PAGES; i++ ) + clear_and_clean_page(page + i); + } + + /* + * Flush TLBs before releasing remaining intermediate p2m page tables to + * prevent illegal access to stalled TLB entries. + */ + p2m_flush_tlb(p2m); + /* Free the rest of the trie pages back to the paging pool. */ while ( (pg = page_list_remove_head(&p2m->pages)) ) free_domheap_page(pg); + p2m->lowest_mapped_gfn = INVALID_GFN; + p2m->max_mapped_gfn = _gfn(0); +} + +void p2m_teardown_one(struct p2m_domain *p2m) +{ + p2m_flush_table(p2m); + if ( p2m->root ) free_domheap_pages(p2m->root, P2M_ROOT_ORDER); p2m->root = NULL; - p2m_free_vmid(d); + p2m_free_vmid(p2m->domain); + + p2m->vttbr = INVALID_VTTBR; 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 = p2m_get_hostp2m(d); int rc = 0; rwlock_init(&p2m->lock); @@ -1278,11 +1304,14 @@ int p2m_init(struct domain *d) return rc; p2m->max_mapped_gfn = _gfn(0); - p2m->lowest_mapped_gfn = _gfn(ULONG_MAX); + p2m->lowest_mapped_gfn = INVALID_GFN; p2m->domain = d; + p2m->access_required = false; p2m->default_access = p2m_access_rwx; p2m->mem_access_enabled = false; + p2m->root = NULL; + p2m->vttbr = INVALID_VTTBR; radix_tree_init(&p2m->mem_access_settings); /* @@ -1293,9 +1322,33 @@ int p2m_init(struct domain *d) p2m->clean_pte = iommu_enabled && !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK); - rc = p2m_alloc_table(d); + return p2m_alloc_table(d); +} - return rc; +static void p2m_teardown_hostp2m(struct domain *d) +{ + struct p2m_domain *p2m = p2m_get_hostp2m(d); + + p2m_teardown_one(p2m); +} + +void p2m_teardown(struct domain *d) +{ + p2m_teardown_hostp2m(d); +} + +static int p2m_init_hostp2m(struct domain *d) +{ + struct p2m_domain *p2m = p2m_get_hostp2m(d); + + p2m->p2m_class = p2m_host; + + return p2m_init_one(d, p2m); +} + +int p2m_init(struct domain *d) +{ + return p2m_init_hostp2m(d); } /* diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index fa07e19..1a004ed 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -11,6 +11,8 @@ #define paddr_bits PADDR_BITS +#define INVALID_VTTBR (0UL) + /* Holds the bit size of IPAs in p2m tables. */ extern unsigned int p2m_ipa_bits; @@ -226,6 +228,15 @@ void guest_physmap_remove_page(struct domain *d, mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn); +/* Flushes the page table held by the p2m. */ +void p2m_flush_table(struct p2m_domain *p2m); + +/* Initialize the p2m structure. */ +int p2m_init_one(struct domain *d, struct p2m_domain *p2m); + +/* Release resources held by the p2m structure. */ +void p2m_teardown_one(struct p2m_domain *p2m); + /* * P2M rwlock helpers. */
This commit pulls out generic init/teardown functionality out of "p2m_init" and "p2m_teardown" into "p2m_init_one", "p2m_teardown_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. v3: Removed struct vttbr. Moved define INVALID_VTTBR to p2m.h. Exported function prototypes of "p2m_flush_table", "p2m_init_one", and "p2m_teardown_one" in p2m.h. Extended the function "p2m_flush_table" by additionally resetting the fields lowest_mapped_gfn and max_mapped_gfn. Added a "p2m_flush_tlb" call in "p2m_flush_table". On altp2m reset in function "altp2m_reset", it is important to flush the TLBs after clearing the root table pages and before clearing the intermediate altp2m page tables to prevent illegal access to stalled TLB entries on currently active VCPUs. Added a check checking whether p2m->root is NULL in p2m_flush_table. Renamed the function "p2m_free_one" to "p2m_teardown_one". Removed resetting p2m->vttbr in "p2m_teardown_one", as it the p2m will be destroyed afterwards. Moved call to "p2m_alloc_table" back to "p2m_init_one". Moved the introduction of the type p2m_class_t out of this patch. Moved the backpointer to the struct domain out of the struct p2m_domain. --- xen/arch/arm/p2m.c | 71 +++++++++++++++++++++++++++++++++++++++++------ xen/include/asm-arm/p2m.h | 11 ++++++++ 2 files changed, 73 insertions(+), 9 deletions(-)