[RFC PATCH] arm: Fix cache inconsistency when using fixmap
Ard Biesheuvel
ard.biesheuvel at linaro.org
Mon Feb 27 11:47:48 PST 2017
On 27 February 2017 at 18:56, Jon Medhurst (Tixy) <tixy at linaro.org> wrote:
> On Mon, 2017-02-27 at 18:43 +0000, Ard Biesheuvel wrote:
>> On 27 February 2017 at 18:40, Stefan Agner <stefan at agner.ch> wrote:
>> > On 2017-02-27 06:35, Jon Medhurst (Tixy) wrote:
>> > > On Sun, 2017-02-26 at 14:35 +0000, Ard Biesheuvel wrote:
>> > >
>> > > > However, we could make this a bit more private to the fixmap code by
>> > > > using something like
>> > > >
>> > > >
>> > > > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
>> > > > index 5c17d2dec777..6c375aea8f22 100644
>> > > > --- a/arch/arm/include/asm/fixmap.h
>> > > > +++ b/arch/arm/include/asm/fixmap.h
>> > > > @@ -41,11 +41,14 @@ static const enum fixed_addresses __end_of_fixed_addresses =
>> > > >
>> > > > #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT |
>> > > > L_PTE_XN | L_PTE_DIRTY)
>> > > >
>> > > > -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK)
>> > > > -#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY)
>> > > > +#define __FIXMAP_MT_NORMAL ((pgprot_val(pgprot_kernel) ?:
>> > >
>> > > The code above misses out setting L_PTE_XN when using pgprot_kernel
>> > >
>> > > > FIXMAP_PAGE_COMMON) | \
>> > > > + L_PTE_MT_WRITEBACK)
>> > > > +#define FIXMAP_PAGE_NORMAL __pgprot(__FIXMAP_MT_NORMAL)
>> > > > +#define FIXMAP_PAGE_RO __pgprot(__FIXMAP_MT_NORMAL | L_PTE_RDONLY)
>> > > >
>> > > > /* Used by set_fixmap_(io|nocache), both meant for mapping a device */
>> > > > -#define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON |
>> > > > L_PTE_MT_DEV_SHARED | L_PTE_SHARED)
>> > > > +#define FIXMAP_PAGE_IO __pgprot(FIXMAP_PAGE_COMMON |
>> > > > L_PTE_MT_DEV_SHARED | \
>> > > > + L_PTE_SHARED)
>> > > > #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO
>> > > >
>> > > > #define __early_set_fixmap __set_fixmap
>> > > >
>> > > > (and drop the change to mm/mmu.c), which boils down to the same for
>> > > > fixmap but does not affect other users of pgprot_kernel. Also, it
>> > > > appears these definitions are broken under STRICT_MM_TYPECHECKS so
>> > > > this is a good opportunity to get that fixed as well.
>> > >
>> > > I like this method better because as you say it keeps things private to
>> > > fixmap, and it doesn't risk affecting other things.
>> >
>> > Yes, I agree this is nicer than my proposal.
>> >
>> > >
>> > > As for getting STRICT_MM_TYPECHECKS working that looks good too, but
>> > > should be a separate cleanup patch, especially as a fix for the cache
>> > > problem possibly should go to stable kernels.
>> > >
>> > > I also think FIXMAP_PAGE_COMMON should be renamed (prefixed with '__')
>> > > as it's an implementation detail an not a memory type used with fixmap.
>> > >
>> > > So I was thinking, one patch as a bugfix:
>> > >
>> > > ----------------------------------------------------------------------
>> > > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
>> > > index 5c17d2dec777..4e6784dc5668 100644
>> > > --- a/arch/arm/include/asm/fixmap.h
>> > > +++ b/arch/arm/include/asm/fixmap.h
>> > > @@ -41,7 +41,8 @@ static const enum fixed_addresses __end_of_fixed_addresses =
>> > >
>> > > #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN |
>> > > L_PTE_DIRTY)
>> > >
>> > > -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK)
>> > > +#define FIXMAP_PAGE_NORMAL (pgprot_kernel ? pgprot_kernel | L_PTE_XN : \
>> > > + FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK)
>> > > #define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY)
>> > >
>> > > /* Used by set_fixmap_(io|nocache), both meant for mapping a device */
>> > > ----------------------------------------------------------------------
>> > >
>> > > Second patch as a cleanup. Note this is different to Ard's version as
>> > > it uses PAGE_KERNEL rather than open coding the same code from
>> > > pgtable.h to add XN permisions (Also, the default generic definition of
>> > > FIXMAP_PAGE_NORMAL is PAGE_KERNEL, and the only reason we want to change
>> > > it is to select an alternate value if that is not yet setup)
>> > >
>> > > ----------------------------------------------------------------------
>> > > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
>> > > index 4e6784dc5668..ec689c6aa644 100644
>> > > --- a/arch/arm/include/asm/fixmap.h
>> > > +++ b/arch/arm/include/asm/fixmap.h
>> > > @@ -39,14 +39,15 @@ static const enum fixed_addresses __end_of_fixed_addresses =
>> > > __end_of_fixmap_region > __end_of_early_ioremap_region ?
>> > > __end_of_fixmap_region : __end_of_early_ioremap_region;
>> > >
>> > > -#define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN |
>> > > L_PTE_DIRTY)
>> > > +#define __FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN
>> > > > L_PTE_DIRTY)
>> > >
>> > > -#define FIXMAP_PAGE_NORMAL (pgprot_kernel ? pgprot_kernel | L_PTE_XN : \
>> > > - FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK)
>> > > -#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY)
>> > > +#define FIXMAP_PAGE_NORMAL (pgprot_val(pgprot_kernel) ? PAGE_KERNEL : \
>> > > + __pgprot(__FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK))
>> > > +#define FIXMAP_PAGE_RO _MOD_PROT(FIXMAP_PAGE_NORMAL, L_PTE_RDONLY)
>> > >
>> > > /* Used by set_fixmap_(io|nocache), both meant for mapping a device */
>> > > -#define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | L_PTE_MT_DEV_SHARED |
>> > > L_PTE_SHARED)
>> > > +#define FIXMAP_PAGE_IO __pgprot(__FIXMAP_PAGE_COMMON |
>> > > L_PTE_MT_DEV_SHARED | \
>> > > + L_PTE_SHARED)
>> > > #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO
>> > >
>> > > #define __early_set_fixmap __set_fixmap
>> > > ----------------------------------------------------------------------
>> >
>> > Fix and cleanup looks good to me,
>> >
>> > Reviewed-by: Stefan Agner <stefan at agner.ch>
>> >
>>
>> Likewise,
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>
> Thanks both. I'll do some more testing then post proper patches with
> commit messages, and add something like
>
> Based on changes
> Suggested-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>
If you insist, but between Stefan, you and me, I am hardly the person
to have given the most substantial input into this discussion
More information about the linux-arm-kernel
mailing list