Re: [Xen-devel] [PATCH] xen/arm: mm: fix nr_second calculation in setup_frametable_mappings
On 12/05/16 12:30, Peng Fan wrote: Hi Julien, On Thu, May 12, 2016 at 11:48:30AM +0100, Julien Grall wrote: Hi Peng, On 12/05/16 07:36, Peng Fan wrote: To ARM64, "frametable_size >> SECOND_SHIFT" means the number of second level entries, not the number of second level pages. "DIV_ROUND_UP(frametable_size >> SECOND_SHIFT, LPAE_ENTRIES)" is the correct way to calculate the second level pages needed for frametable mapping. Good catch! Signed-off-by: Peng FanCc: Stefano Stabellini Cc: Julien Grall --- xen/arch/arm/mm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 0a4f845..7c7f8e9 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -767,7 +767,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe) base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12)); #ifdef CONFIG_ARM_64 -nr_second = frametable_size >> SECOND_SHIFT; +nr_second = DIV_ROUND_UP(frametable_size >> SECOND_SHIFT, LPAE_ENTRIES); I think the following would be clearer: ROUNDUP(frametable_size, FIRST_SIZE) >> FIRST_SHIFT; Agree. This would also benefit a comment to explain that second level of page table are populating. Do you mean I need to add a comment for the upper code? Yes. For the commit log, I would like to change to this: " To ARM64, "frametable_size >> SECOND_SHIFT" means the number On ARM64, s/means/computes/ of second level entries, not the number of second level pages. "ROUNDUP(frametable_size, FIRST_SIZE) >> FIRST_SHIFT" which means s/means/computes/ the number of the first level entries(the number of second level pages), missing space after "entries" is the correct one that should be used. " 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: fix nr_second calculation in setup_frametable_mappings
Hi Julien, On Thu, May 12, 2016 at 11:48:30AM +0100, Julien Grall wrote: >Hi Peng, > >On 12/05/16 07:36, Peng Fan wrote: >>To ARM64, "frametable_size >> SECOND_SHIFT" means the number >>of second level entries, not the number of second level pages. >> >>"DIV_ROUND_UP(frametable_size >> SECOND_SHIFT, LPAE_ENTRIES)" >>is the correct way to calculate the second level pages needed >>for frametable mapping. > >Good catch! > >>Signed-off-by: Peng Fan>>Cc: Stefano Stabellini >>Cc: Julien Grall >>--- >> xen/arch/arm/mm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >>diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >>index 0a4f845..7c7f8e9 100644 >>--- a/xen/arch/arm/mm.c >>+++ b/xen/arch/arm/mm.c >>@@ -767,7 +767,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t >>pe) >> base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12)); >> >> #ifdef CONFIG_ARM_64 >>-nr_second = frametable_size >> SECOND_SHIFT; >>+nr_second = DIV_ROUND_UP(frametable_size >> SECOND_SHIFT, LPAE_ENTRIES); > > >I think the following would be clearer: > >ROUNDUP(frametable_size, FIRST_SIZE) >> FIRST_SHIFT; Agree. > >This would also benefit a comment to explain that second level of page table >are populating. Do you mean I need to add a comment for the upper code? For the commit log, I would like to change to this: " To ARM64, "frametable_size >> SECOND_SHIFT" means the number of second level entries, not the number of second level pages. "ROUNDUP(frametable_size, FIRST_SIZE) >> FIRST_SHIFT" which means the number of the first level entries(the number of second level pages), is the correct one that should be used. " How do you think? Thanks, Peng. > >> second_base = alloc_boot_pages(nr_second, 1); >> second = mfn_to_virt(second_base); >> for ( i = 0; i < nr_second; i++ ) >> > >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: fix nr_second calculation in setup_frametable_mappings
Hi Peng, On 12/05/16 07:36, Peng Fan wrote: To ARM64, "frametable_size >> SECOND_SHIFT" means the number of second level entries, not the number of second level pages. "DIV_ROUND_UP(frametable_size >> SECOND_SHIFT, LPAE_ENTRIES)" is the correct way to calculate the second level pages needed for frametable mapping. Good catch! Signed-off-by: Peng FanCc: Stefano Stabellini Cc: Julien Grall --- xen/arch/arm/mm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 0a4f845..7c7f8e9 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -767,7 +767,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe) base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12)); #ifdef CONFIG_ARM_64 -nr_second = frametable_size >> SECOND_SHIFT; +nr_second = DIV_ROUND_UP(frametable_size >> SECOND_SHIFT, LPAE_ENTRIES); I think the following would be clearer: ROUNDUP(frametable_size, FIRST_SIZE) >> FIRST_SHIFT; This would also benefit a comment to explain that second level of page table are populating. second_base = alloc_boot_pages(nr_second, 1); second = mfn_to_virt(second_base); for ( i = 0; i < nr_second; i++ ) Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] xen/arm: mm: fix nr_second calculation in setup_frametable_mappings
To ARM64, "frametable_size >> SECOND_SHIFT" means the number of second level entries, not the number of second level pages. "DIV_ROUND_UP(frametable_size >> SECOND_SHIFT, LPAE_ENTRIES)" is the correct way to calculate the second level pages needed for frametable mapping. Signed-off-by: Peng FanCc: Stefano Stabellini Cc: Julien Grall --- xen/arch/arm/mm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 0a4f845..7c7f8e9 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -767,7 +767,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe) base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12)); #ifdef CONFIG_ARM_64 -nr_second = frametable_size >> SECOND_SHIFT; +nr_second = DIV_ROUND_UP(frametable_size >> SECOND_SHIFT, LPAE_ENTRIES); second_base = alloc_boot_pages(nr_second, 1); second = mfn_to_virt(second_base); for ( i = 0; i < nr_second; i++ ) -- 2.6.2 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel