| Message ID | 20190821183204.23576-7-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: > trans_pgd_create_copy() and trans_pgd_map_page() are going to be > the basis for public interface of new subsystem that handles page Please don't call this a subsystem. 'sound' and 'mm' are subsystems, this is just some shared code. > tables for cases which are between kernels: kexec, and hibernate. Even though you've baked the get_safe_page() calls into trans_pgd_map_page()? > diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c > index 750ecc7f2cbe..2e29d620b56c 100644 > --- a/arch/arm64/kernel/hibernate.c > +++ b/arch/arm64/kernel/hibernate.c > @@ -182,39 +182,15 @@ int arch_hibernation_header_restore(void *addr) > +int trans_pgd_map_page(pgd_t *trans_pgd, void *page, > + unsigned long dst_addr, > + pgprot_t pgprot) If this thing is going to be exposed, its name should reflect that its creating a set of page tables, to map a single page. A function called 'map_page' with this prototype should 'obviously' map @page at @dst_addr in @trans_pgd using the provided @pgprot... but it doesn't. This is what 'create' was doing in the old name, if that wasn't obvious, its because naming things is hard! | trans_create_single_page_mapping()? (might be too verbose) I think this bites you in patch 8, where you 'generalise' this. > { > - void *page = (void *)get_safe_page(GFP_ATOMIC); > - pgd_t *trans_pgd; > pgd_t *pgdp; > pud_t *pudp; > pmd_t *pmdp; > pte_t *ptep; > > - if (!page) > - return -ENOMEM; > - > - memcpy(page, src_start, length); > - __flush_icache_range((unsigned long)page, (unsigned long)page + length); > - > - trans_pgd = (void *)get_safe_page(GFP_ATOMIC); > - if (!trans_pgd) > - return -ENOMEM; > - > pgdp = pgd_offset_raw(trans_pgd, dst_addr); > if (pgd_none(READ_ONCE(*pgdp))) { > pudp = (void *)get_safe_page(GFP_ATOMIC); > @@ -242,6 +218,44 @@ 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(page), PAGE_KERNEL_EXEC)); > > + return 0; > +} > + > +/* > + * Copies length bytes, starting at src_start into an new page, > + * perform cache maintentance, then maps it at the specified address low Could you fix the spelling of maintenance as git thinks you've moved it? > + * address as executable. > + * > + * This is used by hibernate to copy the code it needs to execute when > + * overwriting the kernel text. This function generates a new set of page > + * tables, which it loads into ttbr0. > + * > + * Length is provided as we probably only want 4K of data, even on a 64K > + * page system. > + */ > +static int create_safe_exec_page(void *src_start, size_t length, > + unsigned long dst_addr, > + phys_addr_t *phys_dst_addr) > +{ Thanks, James
On Fri, Sep 6, 2019 at 11:18 AM James Morse <james.morse@arm.com> wrote: > > Hi Pavel, > > On 21/08/2019 19:31, Pavel Tatashin wrote: > > trans_pgd_create_copy() and trans_pgd_map_page() are going to be > > the basis for public interface of new subsystem that handles page > > Please don't call this a subsystem. 'sound' and 'mm' are subsystems, this is just some > shared code. Sounds good: just could not find a better term to call trans_pgd_*. I won't fix log commits. > > > tables for cases which are between kernels: kexec, and hibernate. > > Even though you've baked the get_safe_page() calls into trans_pgd_map_page()? It is getting removed later. Just for a cleaner migration to new place, get_safe_page() is included for now. > > > > diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c > > index 750ecc7f2cbe..2e29d620b56c 100644 > > --- a/arch/arm64/kernel/hibernate.c > > +++ b/arch/arm64/kernel/hibernate.c > > @@ -182,39 +182,15 @@ int arch_hibernation_header_restore(void *addr) > > > +int trans_pgd_map_page(pgd_t *trans_pgd, void *page, > > + unsigned long dst_addr, > > + pgprot_t pgprot) > > If this thing is going to be exposed, its name should reflect that its creating a set of > page tables, to map a single page. > > A function called 'map_page' with this prototype should 'obviously' map @page at @dst_addr > in @trans_pgd using the provided @pgprot... but it doesn't. Answered below... > > This is what 'create' was doing in the old name, if that wasn't obvious, its because > naming things is hard! > | trans_create_single_page_mapping()? > > (might be too verbose) > > I think this bites you in patch 8, where you 'generalise' this. The new naming makes more sense to me. The old code had function named: create_safe_exec_page() It was doing four things: 1. creating the actual page via provided allocator, 2. copying content from the provided page to new page, 3 creating a new page table. 4 mapping it to a new page table at specified destination address After, I generalize this the function the prototype looks like this: int trans_pgd_map_page(struct trans_pgd_info *info, pgd_t *trans_pgd, void *page, unsigned long dst_addr, pgprot_t pgprot) The function only does the "4" from the old code: map the specified page at dst_addr. The trans_pgd is already created. Of course, and mapping function will have to allocate missing tables in the page tables when necessary. > > + return 0; > > +} > > + > > +/* > > + * Copies length bytes, starting at src_start into an new page, > > + * perform cache maintentance, then maps it at the specified address low > > Could you fix the spelling of maintenance as git thinks you've moved it? I will. Thank you, Pasha
Hi Pavel, On 06/09/2019 17:00, Pavel Tatashin wrote: > On Fri, Sep 6, 2019 at 11:18 AM James Morse <james.morse@arm.com> wrote: >> On 21/08/2019 19:31, Pavel Tatashin wrote: >>> trans_pgd_create_copy() and trans_pgd_map_page() are going to be >>> the basis for public interface of new subsystem that handles page >> >> Please don't call this a subsystem. 'sound' and 'mm' are subsystems, this is just some >> shared code. > Sounds good: just could not find a better term to call trans_pgd_*. I don't like the trans_pgd_ name either, but I can't think of anything better, and its only a name. > I won't fix log commits. Please avoid the word 'subsystem', >>> diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c >>> index 750ecc7f2cbe..2e29d620b56c 100644 >>> --- a/arch/arm64/kernel/hibernate.c >>> +++ b/arch/arm64/kernel/hibernate.c >>> @@ -182,39 +182,15 @@ int arch_hibernation_header_restore(void *addr) >> >>> +int trans_pgd_map_page(pgd_t *trans_pgd, void *page, >>> + unsigned long dst_addr, >>> + pgprot_t pgprot) >> >> If this thing is going to be exposed, its name should reflect that its creating a set of >> page tables, to map a single page. >> >> A function called 'map_page' with this prototype should 'obviously' map @page at @dst_addr >> in @trans_pgd using the provided @pgprot... but it doesn't. > > Answered below... > >> >> This is what 'create' was doing in the old name, if that wasn't obvious, its because >> naming things is hard! >> | trans_create_single_page_mapping()? >> >> (might be too verbose) >> >> I think this bites you in patch 8, where you 'generalise' this. > The new naming makes more sense to me. The old code had function named: > > create_safe_exec_page() > > It was doing four things: 1. creating the actual page via provided > allocator, 2. copying content from the provided page to new page, 3 > creating a new page table. 4 mapping it to a new page table at > specified destination address Yup, all implied in the work of creation. > After, I generalize this the function the prototype looks like this: > > int trans_pgd_map_page(struct trans_pgd_info *info, pgd_t *trans_pgd, > void *page, unsigned long > dst_addr, pgprot_t pgprot) > > The function only does the "4" from the old code: map the specified > page at dst_addr. > The trans_pgd is already created. Which one is this? The existing hibernate code has two PGD. One for the copy of the linear-map, one for this safe page that contains the code doing the copying. > Of course, and > mapping function will have to allocate missing tables in the page > tables when necessary. I think you are over generalising this, to support a case that doesn't exist. Hibernate needs a copy of the linear map to relocate memory, without stepping in page-table, and an executable page it can do that from. To get kexec to relocate the kernel with the MMU on... you need the same. When do you need to add an arbitrary page to either of these sets of tables? Its either a copy of the linear-map, or the single executable page. When would does 'trans_pgd_map_page()' get used outside those two? (looking in your later series, I see you are using it to try and idmap stuff into the low memory. We can't do stuff like this because there may not be any memory in range of the page table helpers. More details in that patch) Thanks, James
diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c index 750ecc7f2cbe..2e29d620b56c 100644 --- a/arch/arm64/kernel/hibernate.c +++ b/arch/arm64/kernel/hibernate.c @@ -182,39 +182,15 @@ int arch_hibernation_header_restore(void *addr) } EXPORT_SYMBOL(arch_hibernation_header_restore); -/* - * Copies length bytes, starting at src_start into an new page, - * perform cache maintentance, then maps it at the specified address low - * address as executable. - * - * This is used by hibernate to copy the code it needs to execute when - * overwriting the kernel text. This function generates a new set of page - * tables, which it loads into ttbr0. - * - * Length is provided as we probably only want 4K of data, even on a 64K - * page system. - */ -static int create_safe_exec_page(void *src_start, size_t length, - unsigned long dst_addr, - phys_addr_t *phys_dst_addr) +int trans_pgd_map_page(pgd_t *trans_pgd, void *page, + unsigned long dst_addr, + pgprot_t pgprot) { - void *page = (void *)get_safe_page(GFP_ATOMIC); - pgd_t *trans_pgd; pgd_t *pgdp; pud_t *pudp; pmd_t *pmdp; pte_t *ptep; - if (!page) - return -ENOMEM; - - memcpy(page, src_start, length); - __flush_icache_range((unsigned long)page, (unsigned long)page + length); - - trans_pgd = (void *)get_safe_page(GFP_ATOMIC); - if (!trans_pgd) - return -ENOMEM; - pgdp = pgd_offset_raw(trans_pgd, dst_addr); if (pgd_none(READ_ONCE(*pgdp))) { pudp = (void *)get_safe_page(GFP_ATOMIC); @@ -242,6 +218,44 @@ 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(page), PAGE_KERNEL_EXEC)); + return 0; +} + +/* + * Copies length bytes, starting at src_start into an new page, + * perform cache maintentance, then maps it at the specified address low + * address as executable. + * + * This is used by hibernate to copy the code it needs to execute when + * overwriting the kernel text. This function generates a new set of page + * tables, which it loads into ttbr0. + * + * Length is provided as we probably only want 4K of data, even on a 64K + * page system. + */ +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 *trans_pgd; + int rc; + + if (!page) + return -ENOMEM; + + memcpy(page, src_start, length); + __flush_icache_range((unsigned long)page, (unsigned long)page + length); + + trans_pgd = (void *)get_safe_page(GFP_ATOMIC); + if (!trans_pgd) + return -ENOMEM; + + rc = trans_pgd_map_page(trans_pgd, page, dst_addr, + PAGE_KERNEL_EXEC); + if (rc) + return rc; + /* * Load our new page tables. A strict BBM approach requires that we * ensure that TLBs are free of any entries that may overlap with the @@ -462,6 +476,24 @@ static int copy_page_tables(pgd_t *dst_pgdp, unsigned long start, return 0; } +int trans_pgd_create_copy(pgd_t **dst_pgdp, unsigned long start, + unsigned long end) +{ + int rc; + pgd_t *trans_pgd = (pgd_t *)get_safe_page(GFP_ATOMIC); + + if (!trans_pgd) { + pr_err("Failed to allocate memory for temporary page tables.\n"); + return -ENOMEM; + } + + rc = copy_page_tables(trans_pgd, start, end); + if (!rc) + *dst_pgdp = trans_pgd; + + return rc; +} + /* * Setup then Resume from the hibernate image using swsusp_arch_suspend_exit(). * @@ -483,13 +515,7 @@ int swsusp_arch_resume(void) * Create a second copy of just the linear map, and use this when * restoring. */ - tmp_pg_dir = (pgd_t *)get_safe_page(GFP_ATOMIC); - if (!tmp_pg_dir) { - pr_err("Failed to allocate memory for temporary page tables.\n"); - rc = -ENOMEM; - goto out; - } - rc = copy_page_tables(tmp_pg_dir, PAGE_OFFSET, 0); + rc = trans_pgd_create_copy(&tmp_pg_dir, PAGE_OFFSET, 0); if (rc) goto out;
trans_pgd_create_copy() and trans_pgd_map_page() are going to be the basis for public interface of new subsystem that handles page tables for cases which are between kernels: kexec, and hibernate. Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com> --- arch/arm64/kernel/hibernate.c | 94 ++++++++++++++++++++++------------- 1 file changed, 60 insertions(+), 34 deletions(-)