Re: [PATCH v3 10/17] arm64, trans_pgd: adjust trans_pgd_create_copy interface

quoted

-int trans_pgd_create_copy(pgd_t **dst_pgdp, unsigned long start,
+/*
+ * Create trans_pgd and copy entries from from_table to trans_pgd in range
+ * [start, end)
+ */
+int trans_pgd_create_copy(struct trans_pgd_info *info, pgd_t **trans_pgd,
+                       pgd_t *from_table, unsigned long start,
                        unsigned long end);

This creates a copy of the linear-map. Why does it need to be told from_table?

This what done as a generic page table entries copy, but I agree, will
remove the from_table.

quoted

diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
index 8c2641a9bb09..8bb602e91065 100644
--- a/arch/arm64/kernel/hibernate.c
+++ b/arch/arm64/kernel/hibernate.c

@@ -323,15 +323,42 @@ int swsusp_arch_resume(void)
      phys_addr_t phys_hibernate_exit;
      void __noreturn (*hibernate_exit)(phys_addr_t, phys_addr_t, void *,
                                        void *, phys_addr_t, phys_addr_t);
+     struct trans_pgd_info trans_info = {
+             .trans_alloc_page       = hibernate_page_alloc,
+             .trans_alloc_arg        = (void *)GFP_ATOMIC,
+             /*
+              * Resume will overwrite areas that may be marked read only
+              * (code, rodata). Clear the RDONLY bit from the temporary
+              * mappings we use during restore.
+              */
+             .trans_flags            = TRANS_MKWRITE,
+     };

quoted

+     /*
+      * debug_pagealloc will removed the PTE_VALID bit if the page isn't in
+      * use by the resume kernel. It may have been in use by the original
+      * kernel, in which case we need to put it back in our copy to do the
+      * restore.
+      *
+      * Before marking this entry valid, check the pfn should be mapped.
+      */
+     if (debug_pagealloc_enabled())
+             trans_info.trans_flags |= (TRANS_MKVALID | TRANS_CHECKPFN);

The debug_pagealloc_enabled() check should be with the code that generates a different
entry. Whether the different entry is correct needs to be considered with
debug_pagealloc_enabled() in mind. You are making this tricky logic less clear.

There is no way the existing code invents an entry for a !pfn_valid() page. With your
'checkpfn' flag, this thing can. You don't need to generalise this for hypothetical users.

Ok

If kexec needs to create mappings for bogus pages, I'd like to know why.

It does not.

quoted

      /*
       * Restoring the memory image will overwrite the ttbr1 page tables.
       * Create a second copy of just the linear map, and use this when
       * restoring.
       */
-     rc = trans_pgd_create_copy(&tmp_pg_dir, PAGE_OFFSET, 0);
-     if (rc)
+     rc = trans_pgd_create_copy(&trans_info, &tmp_pg_dir, init_mm.pgd,
+                                PAGE_OFFSET, 0);

quoted

+     if (rc) {
+             if (rc == -ENOMEM)
+                     pr_err("Failed to allocate memory for temporary page tables.\n");
+             else if (rc == -ENXIO)
+                     pr_err("Tried to set PTE for PFN that does not exist\n");
              goto out;
+     }

If you think the distinction for this error message is useful, it would be clearer to
change it in the current hibernate code before you move it. (_copy_pte() to return an
error, instead of silently failing). Done here, this is unrelated noise.

Ok, will do that.

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