| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1380902861.17503.57.camel@deneb.redhat.com>
Date: Fri, 04 Oct 2013 12:07:41 -0400
From: Mark Salter <msalter@...hat.com>
To: Catalin Marinas <catalin.marinas@....com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Will Deacon <Will.Deacon@....com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] arm64: add early_ioremap support
On Fri, 2013-10-04 at 16:04 +0100, Catalin Marinas wrote:
> 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.
Right, there are a quite a few using similar fixmap code. I actually
looked at consolidating them, but I gave up so that I could get
something working for EFI work in progress. But yeah, I think I
will step back and have another go at it. I think I can get at a
few of the architectures using common code and make it so others
can be converted as desired.
>
> > --- 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?
No, early_ioremap_debug is not initialized which makes the debug
prints in early_ioremap_init() unreachable. I'll fix that. On x86,
the init comes first, but there is other code between that and
parse_early_param() which uses early_ioremap(). Not the case for
arm64.
>
> > --- 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];
ok
>
> > +
> > +static inline pmd_t * __init early_ioremap_pmd(unsigned long addr)
> > +{
> > + pgd_t *pgd = &swapper_pg_dir[pgd_index(addr)];
>
> pgd_offset_k(addr);
ok
>
> > + 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())?
Ah, yes. Nice!
>
> > +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.
No we don't. If .bss isn't cleared here, we have bigger problems.
>
> > + 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.
Right. Copy and paste from x86 where is isn't static, so the trick to
trigger a BUILD_BUG_ON isn't needed here.
>
> > +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.
A valid pte shouldn't be updated except for the unmap case. Nothing
prevents it though. We could do the tlb flush only in the else block
and add a BUG_ON to catch attempts to modify an already used pte.
>
> 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?
>
Yes, that's where I started. I will look at consolidating the common
bits.
Thanks.
--Mark
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists