| Message ID | 20190817024629.26611-3-pasha.tatashin@soleen.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: MMU enabled kexec relocation | expand |
On Fri, Aug 16, 2019 at 10:46:17PM -0400, Pavel Tatashin wrote: > create_safe_exec_page() is going to be split into two parts in preparation > of moving page table handling code out of hibernate.c > > Remove allocator parameter, and rename dst to page. Also, remove the > goto's, as we can return directly without cleanups. It would be nice if you could do the goto/allocator/rename changes as separate patches, since it's vastly easier to verify each change in isolation that way. What's the point of the rename? It's inconsistent with the phys_dst_addr that you leave as-is, so I'm not sure that's worthwhile. > > Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com> > --- > arch/arm64/kernel/hibernate.c | 60 +++++++++++++++-------------------- > 1 file changed, 26 insertions(+), 34 deletions(-) > > diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c > index 9341fcc6e809..96b6f8da7e49 100644 > --- a/arch/arm64/kernel/hibernate.c > +++ b/arch/arm64/kernel/hibernate.c > @@ -196,57 +196,51 @@ EXPORT_SYMBOL(arch_hibernation_header_restore); > */ > static int create_safe_exec_page(void *src_start, size_t length, > unsigned long dst_addr, > - phys_addr_t *phys_dst_addr, > - void *(*allocator)(gfp_t mask), > - gfp_t mask) > + phys_addr_t *phys_dst_addr) > { > - int rc = 0; > + void *page = (void *)get_safe_page(GFP_ATOMIC); > + pgd_t *trans_table; The addition of this trans_table variable wasn't mentioned in the commit message... > + trans_table = (void *)get_safe_page(GFP_ATOMIC); > + if (!trans_table) > + return -ENOMEM; > > - pgdp = pgd_offset_raw(allocator(mask), dst_addr); > + pgdp = pgd_offset_raw(trans_table, dst_addr); > - write_sysreg(phys_to_ttbr(virt_to_phys(pgdp)), ttbr0_el1); > + write_sysreg(phys_to_ttbr(virt_to_phys(trans_table)), ttbr0_el1); ... and I guess you're trying to ensure that we program the TTBR with the correct base address, without the offset of whatever pgd entry we happen to have plumbed in? I think that's a fix, and should come before any other cleanup or rework. If you can respin that specific change with s/trans_table/pgdir/, that would make sense to me. Thanks, Mark.
Hi Mark, Thank you for your review comments. My replies below: On Mon, Aug 19, 2019 at 11:50 AM Mark Rutland <mark.rutland@arm.com> wrote: > > On Fri, Aug 16, 2019 at 10:46:17PM -0400, Pavel Tatashin wrote: > > create_safe_exec_page() is going to be split into two parts in preparation > > of moving page table handling code out of hibernate.c > > > > Remove allocator parameter, and rename dst to page. Also, remove the > > goto's, as we can return directly without cleanups. > > It would be nice if you could do the goto/allocator/rename changes as > separate patches, since it's vastly easier to verify each change in > isolation that way. Sure, I will split these changes into separate patches in the next version of this patch series. > > What's the point of the rename? It's inconsistent with the phys_dst_addr > that you leave as-is, so I'm not sure that's worthwhile. dst_addr, phys_dst_addr VA/PA destination addresses. But, page is a buffer in the current VA space (hence changed to void *), dst looked confusing as it seemed as it was part of the destination addresses. > > > > > Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com> > > --- > > arch/arm64/kernel/hibernate.c | 60 +++++++++++++++-------------------- > > 1 file changed, 26 insertions(+), 34 deletions(-) > > > > diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c > > index 9341fcc6e809..96b6f8da7e49 100644 > > --- a/arch/arm64/kernel/hibernate.c > > +++ b/arch/arm64/kernel/hibernate.c > > @@ -196,57 +196,51 @@ EXPORT_SYMBOL(arch_hibernation_header_restore); > > */ > > static int create_safe_exec_page(void *src_start, size_t length, > > unsigned long dst_addr, > > - phys_addr_t *phys_dst_addr, > > - void *(*allocator)(gfp_t mask), > > - gfp_t mask) > > + phys_addr_t *phys_dst_addr) > > { > > - int rc = 0; > > + void *page = (void *)get_safe_page(GFP_ATOMIC); > > + pgd_t *trans_table; > > The addition of this trans_table variable wasn't mentioned in the commit > message... > > > + trans_table = (void *)get_safe_page(GFP_ATOMIC); > > + if (!trans_table) > > + return -ENOMEM; > > > > - pgdp = pgd_offset_raw(allocator(mask), dst_addr); > > + pgdp = pgd_offset_raw(trans_table, dst_addr); > > > - write_sysreg(phys_to_ttbr(virt_to_phys(pgdp)), ttbr0_el1); > > + write_sysreg(phys_to_ttbr(virt_to_phys(trans_table)), ttbr0_el1); > > > ... and I guess you're trying to ensure that we program the TTBR with > the correct base address, without the offset of whatever pgd entry we > happen to have plumbed in? > > I think that's a fix, and should come before any other cleanup or > rework. Yes. > > If you can respin that specific change with s/trans_table/pgdir/, that > would make sense to me. I will split this patch into several changes. I will describe trans_table rational in different e-mail. There we will decide what namespace to use. Thank you, Pasha
diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c index 9341fcc6e809..96b6f8da7e49 100644 --- a/arch/arm64/kernel/hibernate.c +++ b/arch/arm64/kernel/hibernate.c @@ -196,57 +196,51 @@ EXPORT_SYMBOL(arch_hibernation_header_restore); */ static int create_safe_exec_page(void *src_start, size_t length, unsigned long dst_addr, - phys_addr_t *phys_dst_addr, - void *(*allocator)(gfp_t mask), - gfp_t mask) + phys_addr_t *phys_dst_addr) { - int rc = 0; + void *page = (void *)get_safe_page(GFP_ATOMIC); + pgd_t *trans_table; pgd_t *pgdp; pud_t *pudp; pmd_t *pmdp; pte_t *ptep; - unsigned long dst = (unsigned long)allocator(mask); - if (!dst) { - rc = -ENOMEM; - goto out; - } + if (!page) + return -ENOMEM; + + memcpy((void *)page, src_start, length); + __flush_icache_range((unsigned long)page, (unsigned long)page + length); - memcpy((void *)dst, src_start, length); - __flush_icache_range(dst, dst + length); + trans_table = (void *)get_safe_page(GFP_ATOMIC); + if (!trans_table) + return -ENOMEM; - pgdp = pgd_offset_raw(allocator(mask), dst_addr); + pgdp = pgd_offset_raw(trans_table, dst_addr); if (pgd_none(READ_ONCE(*pgdp))) { - pudp = allocator(mask); - if (!pudp) { - rc = -ENOMEM; - goto out; - } + pudp = (void *)get_safe_page(GFP_ATOMIC); + if (!pudp) + return -ENOMEM; pgd_populate(&init_mm, pgdp, pudp); } pudp = pud_offset(pgdp, dst_addr); if (pud_none(READ_ONCE(*pudp))) { - pmdp = allocator(mask); - if (!pmdp) { - rc = -ENOMEM; - goto out; - } + pmdp = (void *)get_safe_page(GFP_ATOMIC); + if (!pmdp) + return -ENOMEM; pud_populate(&init_mm, pudp, pmdp); } pmdp = pmd_offset(pudp, dst_addr); if (pmd_none(READ_ONCE(*pmdp))) { - ptep = allocator(mask); - if (!ptep) { - rc = -ENOMEM; - goto out; - } + ptep = (void *)get_safe_page(GFP_ATOMIC); + if (!ptep) + return -ENOMEM; pmd_populate_kernel(&init_mm, pmdp, ptep); } 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 @@ -262,13 +256,12 @@ static int create_safe_exec_page(void *src_start, size_t length, */ cpu_set_reserved_ttbr0(); local_flush_tlb_all(); - write_sysreg(phys_to_ttbr(virt_to_phys(pgdp)), ttbr0_el1); + write_sysreg(phys_to_ttbr(virt_to_phys(trans_table)), ttbr0_el1); isb(); - *phys_dst_addr = virt_to_phys((void *)dst); + *phys_dst_addr = virt_to_phys((void *)page); -out: - return rc; + return 0; } #define dcache_clean_range(start, end) __flush_dcache_area(start, (end - start)) @@ -523,8 +516,7 @@ int swsusp_arch_resume(void) */ rc = create_safe_exec_page(__hibernate_exit_text_start, exit_size, (unsigned long)hibernate_exit, - &phys_hibernate_exit, - (void *)get_safe_page, GFP_ATOMIC); + &phys_hibernate_exit); if (rc) { pr_err("Failed to create safe executable page for hibernate_exit code.\n"); goto out;
create_safe_exec_page() is going to be split into two parts in preparation of moving page table handling code out of hibernate.c Remove allocator parameter, and rename dst to page. Also, remove the goto's, as we can return directly without cleanups. Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com> --- arch/arm64/kernel/hibernate.c | 60 +++++++++++++++-------------------- 1 file changed, 26 insertions(+), 34 deletions(-)