Re: [Xen-devel] [PATCH] xen/arm: mm: optimize setup_pagetables
On Wed, May 11, 2016 at 11:03:06AM +0100, Julien Grall wrote: > > >On 11/05/2016 10:57, Peng Fan wrote: >>Hi Julien, > >Hi Peng, > >>On Wed, May 11, 2016 at 10:31:49AM +0100, Julien Grall wrote: >>> >>>[...] >>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 94ea054..bebd82f 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -443,12 +443,6 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) lpae_t pte, *p; int i; -/* Map the destination in the boot misc area. */ -dest_va = BOOT_RELOC_VIRT_START; -pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC); -write_pte(xen_second + second_table_offset(dest_va), pte); -flush_xen_data_tlb_range_va_local(dest_va, SECOND_SIZE); - /* Calculate virt-to-phys offset for the new location */ phys_offset = xen_paddr - (unsigned long) _start; @@ -498,6 +492,11 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) /* Map the destination in the boot misc area. */ dest_va = BOOT_RELOC_VIRT_START; pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC); +xen_second[second_table_offset(dest_va)] = pte; + +/* Map the destination in the boot misc area. */ +dest_va = BOOT_RELOC_VIRT_START; +pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC); >>> >>>Why do you need to recompute "pte" and "dest_va"? They will be the same for >>>"boot_second" and "xen_second". >> >>I just change "write_pte(xen_second + second_table_offset(dest_va), pte);" to >>"xen_second[second_table_offset(dest_va)] = pte;". >> >>The pte and dest_va are not changed. > >So you could do some like: > >/* Comment */ >dest_va = BOOT... >pte = ... >/* Comment */ >xen_second[...] = pte >/* Comment */ >write_pte(boot_second...); This looks better. Will use this in V2. Thanks, Peng. > >Regards, > >-- >Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: mm: optimize setup_pagetables
On 11/05/2016 10:57, Peng Fan wrote: Hi Julien, Hi Peng, On Wed, May 11, 2016 at 10:31:49AM +0100, Julien Grall wrote: [...] diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 94ea054..bebd82f 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -443,12 +443,6 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) lpae_t pte, *p; int i; -/* Map the destination in the boot misc area. */ -dest_va = BOOT_RELOC_VIRT_START; -pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC); -write_pte(xen_second + second_table_offset(dest_va), pte); -flush_xen_data_tlb_range_va_local(dest_va, SECOND_SIZE); - /* Calculate virt-to-phys offset for the new location */ phys_offset = xen_paddr - (unsigned long) _start; @@ -498,6 +492,11 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) /* Map the destination in the boot misc area. */ dest_va = BOOT_RELOC_VIRT_START; pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC); +xen_second[second_table_offset(dest_va)] = pte; + +/* Map the destination in the boot misc area. */ +dest_va = BOOT_RELOC_VIRT_START; +pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC); Why do you need to recompute "pte" and "dest_va"? They will be the same for "boot_second" and "xen_second". I just change "write_pte(xen_second + second_table_offset(dest_va), pte);" to "xen_second[second_table_offset(dest_va)] = pte;". The pte and dest_va are not changed. So you could do some like: /* Comment */ dest_va = BOOT... pte = ... /* Comment */ xen_second[...] = pte /* Comment */ write_pte(boot_second...); Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: mm: optimize setup_pagetables
Hi Julien, On Wed, May 11, 2016 at 10:31:49AM +0100, Julien Grall wrote: >Hi Peng, > >I would rename the title: "xen/arm: mm: remove unnecessary tlb flush in >setup_pagetables". Thanks. Will fix in V2. > >On 11/05/2016 08:59, Peng Fan wrote: >>Before relocating xen to high address, need to build >>the mapping BOOT_RELOC_VIRT_START to xen_paddr into >>boot_pgtable and xen_pgtable. >> >>In setup_pagetables, relocate_xen will switch the root >>page table from boot_pgtable to xen_pgtable/cpu0_pgtable. >> >>Before relocate_xen: >>If touching xen_pgtable, no need to flush_xen_data_tlb_range_va_local. >>We only need to flush tlb when touch boot_pgtable. >>And no need to use write_pte for xen_pgtable, we can simply use >>"xen_second[second_table_offset(dest_va)] = pte;" >> >>Also move the piece code near DTB/Fixmap/XEN_VIRT_START to >>be more easier to understand. > >The commit message is rather hard to understand. Can you please rework it >say: > - CPU0 is using the boot pages table and xen_second is not part of them => >No need to flush the TLB > - Clean up the code (i.e the code movement) > Sorry for my bad english. Will refine the commit log in V2. >However, I would prefer to see 2 patches, one for removing the tlb flush, the >other to move the code. Will fix in V2. > >[...] > >>diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >>index 94ea054..bebd82f 100644 >>--- a/xen/arch/arm/mm.c >>+++ b/xen/arch/arm/mm.c >>@@ -443,12 +443,6 @@ void __init setup_pagetables(unsigned long >>boot_phys_offset, paddr_t xen_paddr) >> lpae_t pte, *p; >> int i; >> >>-/* Map the destination in the boot misc area. */ >>-dest_va = BOOT_RELOC_VIRT_START; >>-pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC); >>-write_pte(xen_second + second_table_offset(dest_va), pte); >>-flush_xen_data_tlb_range_va_local(dest_va, SECOND_SIZE); >>- >> /* Calculate virt-to-phys offset for the new location */ >> phys_offset = xen_paddr - (unsigned long) _start; >> >>@@ -498,6 +492,11 @@ void __init setup_pagetables(unsigned long >>boot_phys_offset, paddr_t xen_paddr) >> /* Map the destination in the boot misc area. */ >> dest_va = BOOT_RELOC_VIRT_START; >> pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC); >>+xen_second[second_table_offset(dest_va)] = pte; >>+ >>+/* Map the destination in the boot misc area. */ >>+dest_va = BOOT_RELOC_VIRT_START; >>+pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC); > >Why do you need to recompute "pte" and "dest_va"? They will be the same for >"boot_second" and "xen_second". I just change "write_pte(xen_second + second_table_offset(dest_va), pte);" to "xen_second[second_table_offset(dest_va)] = pte;". The pte and dest_va are not changed. > >Also, I think the code is still confusing in the current state because of the >comments "Map the destination in the boot misc area.". They need to be >updated to differentiate xen_second vs boot_second. Yeah. This confused me when I first read this piece code. I can update the comments in V2. Thanks, Peng. > >> write_pte(boot_second + second_table_offset(dest_va), pte); >> flush_xen_data_tlb_range_va_local(dest_va, SECOND_SIZE); >> #ifdef CONFIG_ARM_64 >> > >Regards, > >-- >Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: mm: optimize setup_pagetables
Hi Peng, I would rename the title: "xen/arm: mm: remove unnecessary tlb flush in setup_pagetables". On 11/05/2016 08:59, Peng Fan wrote: Before relocating xen to high address, need to build the mapping BOOT_RELOC_VIRT_START to xen_paddr into boot_pgtable and xen_pgtable. In setup_pagetables, relocate_xen will switch the root page table from boot_pgtable to xen_pgtable/cpu0_pgtable. Before relocate_xen: If touching xen_pgtable, no need to flush_xen_data_tlb_range_va_local. We only need to flush tlb when touch boot_pgtable. And no need to use write_pte for xen_pgtable, we can simply use "xen_second[second_table_offset(dest_va)] = pte;" Also move the piece code near DTB/Fixmap/XEN_VIRT_START to be more easier to understand. The commit message is rather hard to understand. Can you please rework it say: - CPU0 is using the boot pages table and xen_second is not part of them => No need to flush the TLB - Clean up the code (i.e the code movement) However, I would prefer to see 2 patches, one for removing the tlb flush, the other to move the code. [...] diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 94ea054..bebd82f 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -443,12 +443,6 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) lpae_t pte, *p; int i; -/* Map the destination in the boot misc area. */ -dest_va = BOOT_RELOC_VIRT_START; -pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC); -write_pte(xen_second + second_table_offset(dest_va), pte); -flush_xen_data_tlb_range_va_local(dest_va, SECOND_SIZE); - /* Calculate virt-to-phys offset for the new location */ phys_offset = xen_paddr - (unsigned long) _start; @@ -498,6 +492,11 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) /* Map the destination in the boot misc area. */ dest_va = BOOT_RELOC_VIRT_START; pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC); +xen_second[second_table_offset(dest_va)] = pte; + +/* Map the destination in the boot misc area. */ +dest_va = BOOT_RELOC_VIRT_START; +pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC); Why do you need to recompute "pte" and "dest_va"? They will be the same for "boot_second" and "xen_second". Also, I think the code is still confusing in the current state because of the comments "Map the destination in the boot misc area.". They need to be updated to differentiate xen_second vs boot_second. write_pte(boot_second + second_table_offset(dest_va), pte); flush_xen_data_tlb_range_va_local(dest_va, SECOND_SIZE); #ifdef CONFIG_ARM_64 Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] xen/arm: mm: optimize setup_pagetables
Before relocating xen to high address, need to build the mapping BOOT_RELOC_VIRT_START to xen_paddr into boot_pgtable and xen_pgtable. In setup_pagetables, relocate_xen will switch the root page table from boot_pgtable to xen_pgtable/cpu0_pgtable. Before relocate_xen: If touching xen_pgtable, no need to flush_xen_data_tlb_range_va_local. We only need to flush tlb when touch boot_pgtable. And no need to use write_pte for xen_pgtable, we can simply use "xen_second[second_table_offset(dest_va)] = pte;" Also move the piece code near DTB/Fixmap/XEN_VIRT_START to be more easier to understand. Signed-off-by: Peng FanCc: Stefano Stabellini Cc: Julien Grall --- There is no function change in this patch, I just think there is no need to use flush_xen_data_tlb_range_va_local and write_pte at that point. And I tested this patch on AArch64. xen/arch/arm/mm.c | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 94ea054..bebd82f 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -443,12 +443,6 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) lpae_t pte, *p; int i; -/* Map the destination in the boot misc area. */ -dest_va = BOOT_RELOC_VIRT_START; -pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC); -write_pte(xen_second + second_table_offset(dest_va), pte); -flush_xen_data_tlb_range_va_local(dest_va, SECOND_SIZE); - /* Calculate virt-to-phys offset for the new location */ phys_offset = xen_paddr - (unsigned long) _start; @@ -498,6 +492,11 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) /* Map the destination in the boot misc area. */ dest_va = BOOT_RELOC_VIRT_START; pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC); +xen_second[second_table_offset(dest_va)] = pte; + +/* Map the destination in the boot misc area. */ +dest_va = BOOT_RELOC_VIRT_START; +pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC); write_pte(boot_second + second_table_offset(dest_va), pte); flush_xen_data_tlb_range_va_local(dest_va, SECOND_SIZE); #ifdef CONFIG_ARM_64 -- 2.6.2 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel