[PATCH 1/5] arm: enable allocate-on-read for LPAE's DCACHE_WRITEBACK

[Heinrich Schuchardt]

On 6/7/20 1:17 AM, Ard Biesheuvel wrote:
> On Sat, 6 Jun 2020 at 22:14, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>> On 6/6/20 7:15 PM, Ard Biesheuvel wrote:
>>> The LPAE version of DCACHE_WRITEBACK is currently defined as no-allocate
>>> for both reads and writes, which deviates from the non-LPAE definition,
>>> and mostly defeats the purpose of enabling the caches in the first place.
>>>
>>> So align LPAE with !LPAE, and enable allocate-on-read.
>>
>> Hello Ard,
>>
>> thanks for analyzing why booting Linux on QEMU fails in some scenarios.
>>
>> Do you know where in U-Boot is the value for !LPAE is defined?
>>
>
> Non-LPAE ARMV7A has (in arch/arm/include/asm/system.h)
>
> DCACHE_WRITETHROUGH = DCACHE_OFF | TTB_SECT_C_MASK,
> DCACHE_WRITEBACK = DCACHE_WRITETHROUGH | TTB_SECT_B_MASK,
> DCACHE_WRITEALLOC = DCACHE_WRITEBACK | TTB_SECT_TEX(1),
>
> and so DCACHE_WRITEBACK has the C and B bits set in the block
> descriptor, and the TEX field set to 0x0
>
> G5.7.2 in the ARM ARM (DDI0487E.a) describes this as
>
> Outer and Inner Write-Back, Read-Allocate
> No Write-Allocate
>
> DCACHE_WRITEALLOC has the C and B bits set in the block descriptor,
> and the TEX field set to 0x1, which is described as
>
> Outer and Inner Write-Back, Read-Allocate Write-Allocate
>
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
>>> ---
>>>  arch/arm/include/asm/system.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
>>> index 7a40b56acdca..21b26557d28b 100644
>>> --- a/arch/arm/include/asm/system.h
>>> +++ b/arch/arm/include/asm/system.h
>>> @@ -445,7 +445,7 @@ static inline void set_dacr(unsigned int val)
>>>   * Memory types
>>>   */
>>
>> To me the lines below look like black magic.
>>
>> In the comment above the definition, please, add a reference explaining
>> where the values are defined and a comment explaining why the actual
>> values are chosen.
>>
>> Maybe this could be a starting point for the description:
>>
>> "This constant is used define memory attribute encodings in a
>> Long-descriptor format translation table entry for stage 1 translations.
>> It is used to set the Memory Attribute Indirection Registers MAIR and
>> HMAIR. For details see [1,2].
>>
>> [1] MAIR0, Memory Attribute Indirection Register 0
>>
>> https://developer.arm.com/docs/ddi0595/b/aarch32-system-registers/mair0/a/DDI0568A_b_armv8_r_supplement.pdf
>> [2] HMAIR0, Hyp Memory Attribute Indirection Register 0
>>
>> https://developer.arm.com/docs/ddi0595/b/aarch32-system-registers/hmair0 "
>>
>
> Better refer to the ARM ARM for the A profile here (not R). [DDI0487E.a]
>
> So the memory types are indexed: four fields of MAIR are populated
> with the four chosen memory types:
>
> [0] Device-nGnrnE
> [1] Outer and Inner Write-Through, Read-Allocate No Write-Allocate
> [2] Outer and Inner Write-Back, Read-Allocate No Write-Allocate
> [3] Outer and Inner Write-Back, Read-Allocate Write-Allocate
>
> and the enum just selects one of these fields:
>
> DCACHE_OFF = TTB_SECT | TTB_SECT_MAIR(0) | TTB_SECT_XN_MASK,
> DCACHE_WRITETHROUGH = TTB_SECT | TTB_SECT_MAIR(1),
> DCACHE_WRITEBACK = TTB_SECT | TTB_SECT_MAIR(2),
> DCACHE_WRITEALLOC = TTB_SECT | TTB_SECT_MAIR(3),
>
> BTW it seems DCACHE_WRITETHROUGH is also incorrect: this should be
> 0xaa for read-allocate as well.

This description would give a good starting point for other developers
when supplied as a comment for MEMORY_ATTRIBUTES.

Best regards

Heinrich

>
>
>>>  #define MEMORY_ATTRIBUTES    ((0x00 << (0 * 8)) | (0x88 << (1 * 8)) | \
>>> -                              (0xcc << (2 * 8)) | (0xff << (3 * 8)))
>>> +                              (0xee << (2 * 8)) | (0xff << (3 * 8)))
>>>
>>>  /* options available for data cache on each page */
>>>  enum dcache_option {
>>>
>>