| Message ID | 20200606171534.736365-2-ardb@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for running U-boot under QEMU/KVM | expand |
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? > > 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 " 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 { >
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. > > #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 { > > >
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 { >>> >>
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 */ #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 {
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. Signed-off-by: Ard Biesheuvel <ardb at kernel.org> --- arch/arm/include/asm/system.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)