[RFC PATCH] arm: Fix cache inconsistency when using fixmap

[Ard Biesheuvel]

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>