| Message ID | 1463035000-28745-1-git-send-email-van.freenix@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <van.freenix@gmail.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > --- > 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,
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 <van.freenix@gmail.com> >>Cc: Stefano Stabellini <sstabellini@kernel.org> >>Cc: Julien Grall <julien.grall@arm.com> >>--- >> 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
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 Fan <van.freenix@gmail.com> >>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>> Cc: Julien Grall <julien.grall@arm.com> >>> --- >>> 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,
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++ )
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 Fan <van.freenix@gmail.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/mm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)