| Message ID | 20210425201318.15447-11-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: mm: Remove open-coding mappings | expand |
On Sun, 25 Apr 2021, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > xen_{un,}map_table() already uses the helper to map/unmap pages > on-demand (note this is currently a NOP on arm64). So switching to > domheap don't have any disavantage. > > But this as the benefit: > - to keep the page tables unmapped if an arch decided to do so > - reduce xenheap use on arm32 which can be pretty small > > Signed-off-by: Julien Grall <jgrall@amazon.com> Thanks for the patch. It looks OK but let me ask a couple of questions to clarify my doubts. This change should have no impact to arm64, right? For arm32, I wonder why we were using map_domain_page before in xen_map_table: it wasn't necessary, was it? In fact, one could even say that it was wrong? > --- > Changes in v2: > - New patch > --- > xen/arch/arm/mm.c | 36 +++++++++++++++++++++--------------- > 1 file changed, 21 insertions(+), 15 deletions(-) > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index 19ecf73542ce..ae5a07ea956b 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -969,21 +969,6 @@ void *ioremap(paddr_t pa, size_t len) > return ioremap_attr(pa, len, PAGE_HYPERVISOR_NOCACHE); > } > > -static int create_xen_table(lpae_t *entry) > -{ > - void *p; > - lpae_t pte; > - > - p = alloc_xenheap_page(); > - if ( p == NULL ) > - return -ENOMEM; > - clear_page(p); > - pte = mfn_to_xen_entry(virt_to_mfn(p), MT_NORMAL); > - pte.pt.table = 1; > - write_pte(entry, pte); > - return 0; > -} > - > static lpae_t *xen_map_table(mfn_t mfn) > { > /* > @@ -1024,6 +1009,27 @@ static void xen_unmap_table(const lpae_t *table) > unmap_domain_page(table); > } > > +static int create_xen_table(lpae_t *entry) > +{ > + struct page_info *pg; > + void *p; > + lpae_t pte; > + > + pg = alloc_domheap_page(NULL, 0); > + if ( pg == NULL ) > + return -ENOMEM; > + > + p = xen_map_table(page_to_mfn(pg)); > + clear_page(p); > + xen_unmap_table(p); > + > + pte = mfn_to_xen_entry(page_to_mfn(pg), MT_NORMAL); > + pte.pt.table = 1; > + write_pte(entry, pte); > + > + return 0; > +} > + > #define XEN_TABLE_MAP_FAILED 0 > #define XEN_TABLE_SUPER_PAGE 1 > #define XEN_TABLE_NORMAL_PAGE 2 > -- > 2.17.1 >
Hi Stefano, On 12/05/2021 23:44, Stefano Stabellini wrote: > On Sun, 25 Apr 2021, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> xen_{un,}map_table() already uses the helper to map/unmap pages >> on-demand (note this is currently a NOP on arm64). So switching to >> domheap don't have any disavantage. >> >> But this as the benefit: >> - to keep the page tables unmapped if an arch decided to do so >> - reduce xenheap use on arm32 which can be pretty small >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> > > Thanks for the patch. It looks OK but let me ask a couple of questions > to clarify my doubts. > > This change should have no impact to arm64, right? > > For arm32, I wonder why we were using map_domain_page before in > xen_map_table: it wasn't necessary, was it? In fact, one could even say > that it was wrong? In xen_map_table() we need to be able to map pages from Xen binary, xenheap... On arm64, we would be able to use mfn_to_virt() because everything is mapped in Xen. But that's not the case on arm32. So we need a way to map anything easily. The only difference between xenheap and domheap are the former is always mapped while the latter may not be. So one can also view a xenheap page as a glorified domheap. I also don't really want to create yet another interface to map pages (we have vmap(), map_domain_page(), map_domain_global_page()...). So, when I implemented xen_map_table() a couple of years ago, I came to the conclusion that this is a convenient (ab)use of the interface. Cheers,
On Thu, 13 May 2021, Julien Grall wrote: > Hi Stefano, > > On 12/05/2021 23:44, Stefano Stabellini wrote: > > On Sun, 25 Apr 2021, Julien Grall wrote: > > > From: Julien Grall <jgrall@amazon.com> > > > > > > xen_{un,}map_table() already uses the helper to map/unmap pages > > > on-demand (note this is currently a NOP on arm64). So switching to > > > domheap don't have any disavantage. > > > > > > But this as the benefit: > > > - to keep the page tables unmapped if an arch decided to do so > > > - reduce xenheap use on arm32 which can be pretty small > > > > > > Signed-off-by: Julien Grall <jgrall@amazon.com> > > > > Thanks for the patch. It looks OK but let me ask a couple of questions > > to clarify my doubts. > > > > This change should have no impact to arm64, right? > > > > For arm32, I wonder why we were using map_domain_page before in > > xen_map_table: it wasn't necessary, was it? In fact, one could even say > > that it was wrong? > In xen_map_table() we need to be able to map pages from Xen binary, xenheap... > On arm64, we would be able to use mfn_to_virt() because everything is mapped > in Xen. But that's not the case on arm32. So we need a way to map anything > easily. > > The only difference between xenheap and domheap are the former is always > mapped while the latter may not be. So one can also view a xenheap page as a > glorified domheap. > > I also don't really want to create yet another interface to map pages (we have > vmap(), map_domain_page(), map_domain_global_page()...). So, when I > implemented xen_map_table() a couple of years ago, I came to the conclusion > that this is a convenient (ab)use of the interface. Got it. Repeating to check if I see the full picture. On ARM64 there are no changes. On ARM32, at runtime there are no changes mapping/unmapping pages because xen_map_table is already mapping all pages using domheap, even xenheap pages are mapped as domheap; so this patch makes no difference in mapping/unmapping, correct? The only difference is that on arm32 we are using domheap to allocate the pages, which is a different (larger) pool.
Hi Stefano, On 13/05/2021 23:27, Stefano Stabellini wrote: > On Thu, 13 May 2021, Julien Grall wrote: >> Hi Stefano, >> >> On 12/05/2021 23:44, Stefano Stabellini wrote: >>> On Sun, 25 Apr 2021, Julien Grall wrote: >>>> From: Julien Grall <jgrall@amazon.com> >>>> >>>> xen_{un,}map_table() already uses the helper to map/unmap pages >>>> on-demand (note this is currently a NOP on arm64). So switching to >>>> domheap don't have any disavantage. >>>> >>>> But this as the benefit: >>>> - to keep the page tables unmapped if an arch decided to do so >>>> - reduce xenheap use on arm32 which can be pretty small >>>> >>>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>> >>> Thanks for the patch. It looks OK but let me ask a couple of questions >>> to clarify my doubts. >>> >>> This change should have no impact to arm64, right? >>> >>> For arm32, I wonder why we were using map_domain_page before in >>> xen_map_table: it wasn't necessary, was it? In fact, one could even say >>> that it was wrong? >> In xen_map_table() we need to be able to map pages from Xen binary, xenheap... >> On arm64, we would be able to use mfn_to_virt() because everything is mapped >> in Xen. But that's not the case on arm32. So we need a way to map anything >> easily. >> >> The only difference between xenheap and domheap are the former is always >> mapped while the latter may not be. So one can also view a xenheap page as a >> glorified domheap. >> >> I also don't really want to create yet another interface to map pages (we have >> vmap(), map_domain_page(), map_domain_global_page()...). So, when I >> implemented xen_map_table() a couple of years ago, I came to the conclusion >> that this is a convenient (ab)use of the interface. > > Got it. Repeating to check if I see the full picture. On ARM64 there are > no changes. On ARM32, at runtime there are no changes mapping/unmapping > pages because xen_map_table is already mapping all pages using domheap, > even xenheap pages are mapped as domheap; so this patch makes no > difference in mapping/unmapping, correct? For arm32, it makes a slight difference when allocating a new page table (we didn't call map/unmap before) but this is not called often. The main "drop" in performance happened when xen_{,map}_table() was introduced. > > The only difference is that on arm32 we are using domheap to allocate > the pages, which is a different (larger) pool. Yes. Would you be happy to give you acked-by/reviewed-by on this basis? Cheers,
On Sat, 15 May 2021, Julien Grall wrote: > Hi Stefano, > > On 13/05/2021 23:27, Stefano Stabellini wrote: > > On Thu, 13 May 2021, Julien Grall wrote: > > > Hi Stefano, > > > > > > On 12/05/2021 23:44, Stefano Stabellini wrote: > > > > On Sun, 25 Apr 2021, Julien Grall wrote: > > > > > From: Julien Grall <jgrall@amazon.com> > > > > > > > > > > xen_{un,}map_table() already uses the helper to map/unmap pages > > > > > on-demand (note this is currently a NOP on arm64). So switching to > > > > > domheap don't have any disavantage. > > > > > > > > > > But this as the benefit: > > > > > - to keep the page tables unmapped if an arch decided to do so > > > > > - reduce xenheap use on arm32 which can be pretty small > > > > > > > > > > Signed-off-by: Julien Grall <jgrall@amazon.com> > > > > > > > > Thanks for the patch. It looks OK but let me ask a couple of questions > > > > to clarify my doubts. > > > > > > > > This change should have no impact to arm64, right? > > > > > > > > For arm32, I wonder why we were using map_domain_page before in > > > > xen_map_table: it wasn't necessary, was it? In fact, one could even say > > > > that it was wrong? > > > In xen_map_table() we need to be able to map pages from Xen binary, > > > xenheap... > > > On arm64, we would be able to use mfn_to_virt() because everything is > > > mapped > > > in Xen. But that's not the case on arm32. So we need a way to map anything > > > easily. > > > > > > The only difference between xenheap and domheap are the former is always > > > mapped while the latter may not be. So one can also view a xenheap page as > > > a > > > glorified domheap. > > > > > > I also don't really want to create yet another interface to map pages (we > > > have > > > vmap(), map_domain_page(), map_domain_global_page()...). So, when I > > > implemented xen_map_table() a couple of years ago, I came to the > > > conclusion > > > that this is a convenient (ab)use of the interface. > > > > Got it. Repeating to check if I see the full picture. On ARM64 there are > > no changes. On ARM32, at runtime there are no changes mapping/unmapping > > pages because xen_map_table is already mapping all pages using domheap, > > even xenheap pages are mapped as domheap; so this patch makes no > > difference in mapping/unmapping, correct? > > For arm32, it makes a slight difference when allocating a new page table (we > didn't call map/unmap before) but this is not called often. > > The main "drop" in performance happened when xen_{,map}_table() was > introduced. > > > > > The only difference is that on arm32 we are using domheap to allocate > > the pages, which is a different (larger) pool. > > Yes. > > Would you be happy to give you acked-by/reviewed-by on this basis? Yes Acked-by: Stefano Stabellini <sstabellini@kernel.org>
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 19ecf73542ce..ae5a07ea956b 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -969,21 +969,6 @@ void *ioremap(paddr_t pa, size_t len) return ioremap_attr(pa, len, PAGE_HYPERVISOR_NOCACHE); } -static int create_xen_table(lpae_t *entry) -{ - void *p; - lpae_t pte; - - p = alloc_xenheap_page(); - if ( p == NULL ) - return -ENOMEM; - clear_page(p); - pte = mfn_to_xen_entry(virt_to_mfn(p), MT_NORMAL); - pte.pt.table = 1; - write_pte(entry, pte); - return 0; -} - static lpae_t *xen_map_table(mfn_t mfn) { /* @@ -1024,6 +1009,27 @@ static void xen_unmap_table(const lpae_t *table) unmap_domain_page(table); } +static int create_xen_table(lpae_t *entry) +{ + struct page_info *pg; + void *p; + lpae_t pte; + + pg = alloc_domheap_page(NULL, 0); + if ( pg == NULL ) + return -ENOMEM; + + p = xen_map_table(page_to_mfn(pg)); + clear_page(p); + xen_unmap_table(p); + + pte = mfn_to_xen_entry(page_to_mfn(pg), MT_NORMAL); + pte.pt.table = 1; + write_pte(entry, pte); + + return 0; +} + #define XEN_TABLE_MAP_FAILED 0 #define XEN_TABLE_SUPER_PAGE 1 #define XEN_TABLE_NORMAL_PAGE 2