Re: [PATCH v2 02/14] arm64, hibernate: create_safe_exec_page cleanup

Hi Mark,

Thank you for your review comments. My replies below:

On Mon, Aug 19, 2019 at 11:50 AM Mark Rutland [off-list ref] wrote:

On Fri, Aug 16, 2019 at 10:46:17PM -0400, Pavel Tatashin wrote:

quoted

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.

quoted

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...

quoted

+     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);

quoted

-     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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel