Re: [PATCH] arm64: add early_ioremap support

[Catalin Marinas]

Hi Mark,

> --- /dev/null
> +++ b/arch/arm64/include/asm/fixmap.h
> @@ -0,0 +1,117 @@
> +/*
> + * fixmap.h: compile-time virtual memory allocation
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Copyright (C) 1998 Ingo Molnar
> + *
> + */

I can see several architectures having very similar macros/functions in
fixmap.h. It would make sense to create a generic fixmap.h holding at
least the fix_to_virt and related macros, FIXADDR_START etc. with enum
fixed_addresses in arch code.

> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -42,6 +42,7 @@
>  #include <linux/of_fdt.h>
>  #include <linux/of_platform.h>
> 
> +#include <asm/fixmap.h>
>  #include <asm/cputype.h>
>  #include <asm/elf.h>
>  #include <asm/cputable.h>
> @@ -252,6 +253,8 @@ void __init setup_arch(char **cmdline_p)
> 
>         *cmdline_p = boot_command_line;
> 
> +       early_ioremap_init();
> +
>         parse_early_param();

Should the early_ioremap_init() call happen after parse_early_param()?
Is early_ioremap_debug initialised already?

> --- a/arch/arm64/mm/ioremap.c
> +++ b/arch/arm64/mm/ioremap.c
> @@ -25,6 +25,10 @@
>  #include <linux/vmalloc.h>
>  #include <linux/io.h>
> 
> +#include <asm/fixmap.h>
> +#include <asm/tlbflush.h>
> +#include <asm/pgalloc.h>
> +
>  static void __iomem *__ioremap_caller(phys_addr_t phys_addr, size_t size,
>                                       pgprot_t prot, void *caller)
>  {
> @@ -82,3 +86,284 @@ void __iounmap(volatile void __iomem *io_addr)
>         vunmap(addr);
>  }
>  EXPORT_SYMBOL(__iounmap);
> +
> +static int early_ioremap_debug __initdata;
> +
> +static int __init early_ioremap_debug_setup(char *str)
> +{
> +       early_ioremap_debug = 1;
> +
> +       return 0;
> +}
> +early_param("early_ioremap_debug", early_ioremap_debug_setup);
> +
> +static int after_paging_init __initdata;
> +#ifndef CONFIG_ARM64_64K_PAGES
> +static pte_t bm_pte[PAGE_SIZE/sizeof(pte_t)] __page_aligned_bss;
> +#endif

bm_pte[PTRS_PER_PTE];

> +
> +static inline pmd_t * __init early_ioremap_pmd(unsigned long addr)
> +{
> +       pgd_t *pgd = &swapper_pg_dir[pgd_index(addr)];

pgd_offset_k(addr);

> +       pud_t *pud = pud_offset(pgd, addr);
> +       pmd_t *pmd = pmd_offset(pud, addr);
> +
> +       return pmd;
> +}
> +
> +static inline pte_t * __init early_ioremap_pte(unsigned long addr)
> +{
> +#ifdef CONFIG_ARM64_64K_PAGES
> +       pmd_t *pmd = early_ioremap_pmd(addr);
> +       return pte_offset_kernel(pmd, addr);
> +#else
> +       return &bm_pte[pte_index(addr)];
> +#endif

If we populate the pmd correctly with 4K pages (and I think we do in
early_ioremap_init()), can we not just use this function without the
#ifdef-else part (always pte_offset_kernel())?

> +static unsigned long slot_virt[FIX_BTMAPS_SLOTS] __initdata;
> +
> +void __init early_ioremap_init(void)
> +{
> +       pmd_t *pmd;
> +       int i;
> +
> +       if (early_ioremap_debug)
> +               pr_info("early_ioremap_init()\n");
> +
> +       for (i = 0; i < FIX_BTMAPS_SLOTS; i++)
> +               slot_virt[i] = __fix_to_virt(FIX_BTMAP_BEGIN - NR_FIX_BTMAPS*i);
> +
> +       pmd = early_ioremap_pmd(fix_to_virt(FIX_BTMAP_BEGIN));
> +#ifndef CONFIG_ARM64_64K_PAGES
> +       /* need to populate pmd for 4k pagesize only */
> +       memset(bm_pte, 0, sizeof(bm_pte));

Do we need memset() here? bm_pte[] is placed in the .bss section.

> +       pmd_populate_kernel(&init_mm, pmd, bm_pte);
> +#endif
> +
> +       /*
> +        * The boot-ioremap range spans multiple pmds, for which
> +        * we are not prepared:
> +        */
> +#define __FIXADDR_TOP (-PAGE_SIZE)
> +       BUILD_BUG_ON((__fix_to_virt(FIX_BTMAP_BEGIN) >> PMD_SHIFT)
> +                    != (__fix_to_virt(FIX_BTMAP_END) >> PMD_SHIFT));
> +#undef __FIXADDR_TOP

Why this #define/#undef? FIXADDR_TOP is statically defined.

> +void __init __set_fixmap(enum fixed_addresses idx,
> +                        phys_addr_t phys, pgprot_t flags)
> +{
> +       unsigned long addr = __fix_to_virt(idx);
> +       pte_t *pte;
> +
> +       if (idx >= __end_of_fixed_addresses) {
> +               BUG();
> +               return;
> +       }
> +       if (after_paging_init) {
> +               WARN_ON(1);
> +               return;
> +       }
> +
> +       pte = early_ioremap_pte(addr);
> +
> +       if (pgprot_val(flags))
> +               set_pte(pte, pfn_pte(phys >> PAGE_SHIFT, flags));
> +       else
> +               pte_clear(&init_mm, addr, pte);
> +       flush_tlb_kernel_range(addr, addr+PAGE_SIZE);

Would __set_fixmap be used to change valid ptes? If not, we could keep
the flush_tlb_kernel_range() call only under the 'else' block.

As I was going through the patch, I realised that the early_ioremap()
looks really to the x86 implementation. Can we move it into a library to
be shared between the two?

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/