| Message ID | 20190821183204.23576-5-pasha.tatashin@soleen.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: MMU enabled kexec relocation | expand |
Hi Pavel, On 21/08/2019 19:31, Pavel Tatashin wrote: > create_safe_exec_page() allocates a safe page and maps it at a > specific location, also this function returns the physical address > of newly allocated page. > > The destination VA, and PA are specified in arguments: dst_addr, > phys_dst_addr > > However, within the function it uses "dst" which has unsigned long > type, but is actually a pointers in the current virtual space. This > is confusing to read. The type? There are plenty of places in the kernel that an unsigned-long is actually a pointer. This isn't unusual. > Rename dst to more appropriate page (page that is created), and also > change its time to "void *" If you think its clearer, Reviewed-by: James Morse <james.morse@arm.com> Thanks, James
On Fri, Sep 6, 2019 at 11:17 AM James Morse <james.morse@arm.com> wrote: > > Hi Pavel, > > On 21/08/2019 19:31, Pavel Tatashin wrote: > > create_safe_exec_page() allocates a safe page and maps it at a > > specific location, also this function returns the physical address > > of newly allocated page. > > > > The destination VA, and PA are specified in arguments: dst_addr, > > phys_dst_addr > > > > However, within the function it uses "dst" which has unsigned long > > type, but is actually a pointers in the current virtual space. This > > is confusing to read. > > The type? There are plenty of places in the kernel that an unsigned-long is actually a > pointer. This isn't unusual. > > > > Rename dst to more appropriate page (page that is created), and also > > change its time to "void *" > > If you think its clearer, > Reviewed-by: James Morse <james.morse@arm.com> Thank you for your review. Pasha
diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c index c8211108ec11..ee34a06d8a35 100644 --- a/arch/arm64/kernel/hibernate.c +++ b/arch/arm64/kernel/hibernate.c @@ -198,17 +198,17 @@ static int create_safe_exec_page(void *src_start, size_t length, unsigned long dst_addr, phys_addr_t *phys_dst_addr) { + void *page = (void *)get_safe_page(GFP_ATOMIC); pgd_t *pgdp; pud_t *pudp; pmd_t *pmdp; pte_t *ptep; - unsigned long dst = get_safe_page(GFP_ATOMIC); - if (!dst) + if (!page) return -ENOMEM; - memcpy((void *)dst, src_start, length); - __flush_icache_range(dst, dst + length); + memcpy(page, src_start, length); + __flush_icache_range((unsigned long)page, (unsigned long)page + length); pgdp = pgd_offset_raw((void *)get_safe_page(GFP_ATOMIC), dst_addr); if (pgd_none(READ_ONCE(*pgdp))) { @@ -235,7 +235,7 @@ static int create_safe_exec_page(void *src_start, size_t length, } ptep = pte_offset_kernel(pmdp, dst_addr); - set_pte(ptep, pfn_pte(virt_to_pfn(dst), PAGE_KERNEL_EXEC)); + set_pte(ptep, pfn_pte(virt_to_pfn(page), PAGE_KERNEL_EXEC)); /* * Load our new page tables. A strict BBM approach requires that we @@ -254,7 +254,7 @@ static int create_safe_exec_page(void *src_start, size_t length, write_sysreg(phys_to_ttbr(virt_to_phys(pgdp)), ttbr0_el1); isb(); - *phys_dst_addr = virt_to_phys((void *)dst); + *phys_dst_addr = virt_to_phys(page); return 0; }
create_safe_exec_page() allocates a safe page and maps it at a specific location, also this function returns the physical address of newly allocated page. The destination VA, and PA are specified in arguments: dst_addr, phys_dst_addr However, within the function it uses "dst" which has unsigned long type, but is actually a pointers in the current virtual space. This is confusing to read. Rename dst to more appropriate page (page that is created), and also change its time to "void *" Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com> --- arch/arm64/kernel/hibernate.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)