[U-Boot] [PATCH] efi_loader: Always flush in cache line size granularity

[U-Boot] [PATCH] efi_loader: Always flush in cache line size granularity

[Alexander Graf]

The cache line flush helpers only work properly when they get aligned
start and end addresses. Round our flush range to cache line size. It's
safe because we're guaranteed to flush within a single page which has the
same cache attributes.

Reported-by: Marek Vasut <marex@denx.de>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 lib/efi_loader/efi_runtime.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 22bcd08..40acec0 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -194,7 +194,8 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map)
 #endif
 
 		*p = newaddr;
-		flush_dcache_range((ulong)p, (ulong)&p[1]);
+		flush_dcache_range((ulong)p & ~(CONFIG_SYS_CACHELINE_SIZE - 1),
+			ALIGN((ulong)&p[1], CONFIG_SYS_CACHELINE_SIZE));
 	}
 
 #ifndef IS_RELA
-- 
1.8.5.6

[U-Boot] [PATCH] efi_loader: Always flush in cache line size granularity

[Marek Vasut]

On 04/04/2016 09:32 AM, Alexander Graf wrote:
> The cache line flush helpers only work properly when they get aligned
> start and end addresses. Round our flush range to cache line size. It's
> safe because we're guaranteed to flush within a single page which has the
> same cache attributes.
> 
> Reported-by: Marek Vasut <marex@denx.de>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Thanks!

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] efi_loader: Always flush in cache line size granularity

[Andreas Färber]

Am 04.04.2016 um 09:32 schrieb Alexander Graf:
> The cache line flush helpers only work properly when they get aligned
> start and end addresses. Round our flush range to cache line size. It's
> safe because we're guaranteed to flush within a single page which has the
> same cache attributes.
> 
> Reported-by: Marek Vasut <marex@denx.de>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  lib/efi_loader/efi_runtime.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> index 22bcd08..40acec0 100644
> --- a/lib/efi_loader/efi_runtime.c
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -194,7 +194,8 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map)
>  #endif
>  
>  		*p = newaddr;
> -		flush_dcache_range((ulong)p, (ulong)&p[1]);
> +		flush_dcache_range((ulong)p & ~(CONFIG_SYS_CACHELINE_SIZE - 1),
> +			ALIGN((ulong)&p[1], CONFIG_SYS_CACHELINE_SIZE));

dragonboard410c_defconfig fails to build with this due to undefined
CONFIG_SYS_CACHELINE_SIZE. Do we need to #ifdef here or is the
dragonboard410c at fault for not defining it?

jetson-tk1_defconfig for instance builds fine with this patch.

Regards,
Andreas

>  	}
>  
>  #ifndef IS_RELA

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

[U-Boot] [PATCH] efi_loader: Always flush in cache line size granularity

[Alexander Graf]

On 11.04.16 00:24, Andreas F?rber wrote:
> Am 04.04.2016 um 09:32 schrieb Alexander Graf:
>> The cache line flush helpers only work properly when they get aligned
>> start and end addresses. Round our flush range to cache line size. It's
>> safe because we're guaranteed to flush within a single page which has the
>> same cache attributes.
>>
>> Reported-by: Marek Vasut <marex@denx.de>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>  lib/efi_loader/efi_runtime.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
>> index 22bcd08..40acec0 100644
>> --- a/lib/efi_loader/efi_runtime.c
>> +++ b/lib/efi_loader/efi_runtime.c
>> @@ -194,7 +194,8 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map)
>>  #endif
>>  
>>  		*p = newaddr;
>> -		flush_dcache_range((ulong)p, (ulong)&p[1]);
>> +		flush_dcache_range((ulong)p & ~(CONFIG_SYS_CACHELINE_SIZE - 1),
>> +			ALIGN((ulong)&p[1], CONFIG_SYS_CACHELINE_SIZE));
> 
> dragonboard410c_defconfig fails to build with this due to undefined
> CONFIG_SYS_CACHELINE_SIZE. Do we need to #ifdef here or is the
> dragonboard410c at fault for not defining it?

Some USB drivers use it unconditionally, but I guess the dragonboard401c
doesn't enable USB.

Let me come up with a more compatible version.


Alex

[U-Boot] [PATCH] efi_loader: Always flush in cache line size granularity

[Marek Vasut]

On 04/11/2016 12:31 AM, Alexander Graf wrote:
> 
> 
> On 11.04.16 00:24, Andreas F?rber wrote:
>> Am 04.04.2016 um 09:32 schrieb Alexander Graf:
>>> The cache line flush helpers only work properly when they get aligned
>>> start and end addresses. Round our flush range to cache line size. It's
>>> safe because we're guaranteed to flush within a single page which has the
>>> same cache attributes.
>>>
>>> Reported-by: Marek Vasut <marex@denx.de>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>>  lib/efi_loader/efi_runtime.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
>>> index 22bcd08..40acec0 100644
>>> --- a/lib/efi_loader/efi_runtime.c
>>> +++ b/lib/efi_loader/efi_runtime.c
>>> @@ -194,7 +194,8 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map)
>>>  #endif
>>>  
>>>  		*p = newaddr;
>>> -		flush_dcache_range((ulong)p, (ulong)&p[1]);
>>> +		flush_dcache_range((ulong)p & ~(CONFIG_SYS_CACHELINE_SIZE - 1),
>>> +			ALIGN((ulong)&p[1], CONFIG_SYS_CACHELINE_SIZE));
>>
>> dragonboard410c_defconfig fails to build with this due to undefined
>> CONFIG_SYS_CACHELINE_SIZE. Do we need to #ifdef here or is the
>> dragonboard410c at fault for not defining it?
> 
> Some USB drivers use it unconditionally, but I guess the dragonboard401c
> doesn't enable USB.
> 
> Let me come up with a more compatible version.

This version is fine, dragonboard needs fixing. Every single board must
define CONFIG_SYS_CACHELINE_SIZE .

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] efi_loader: Always flush in cache line size granularity

[Alexander Graf]

The cache line flush helpers only work properly when they get aligned
start and end addresses. Round our flush range to cache line size. It's
safe because we're guaranteed to flush within a single page which has the
same cache attributes.

Reported-by: Marek Vasut <marex@denx.de>
Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - Fix compilation on systems without CONFIG_SYS_CACHELINE_SIZE

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 22bcd08..3ee27ca 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -20,6 +20,13 @@ static efi_status_t EFI_RUNTIME_TEXT EFIAPI efi_unimplemented(void);
 static efi_status_t EFI_RUNTIME_TEXT EFIAPI efi_device_error(void);
 static efi_status_t EFI_RUNTIME_TEXT EFIAPI efi_invalid_parameter(void);
 
+#ifdef CONFIG_SYS_CACHELINE_SIZE
+#define EFI_CACHELINE_SIZE CONFIG_SYS_CACHELINE_SIZE
+#else
+/* Just use the greatest cache flush alignment requirement I'm aware of */
+#define EFI_CACHELINE_SIZE 128
+#endif
+
 #if defined(CONFIG_ARM64)
 #define R_RELATIVE	1027
 #define R_MASK		0xffffffffULL
@@ -194,7 +201,8 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map)
 #endif
 
 		*p = newaddr;
-		flush_dcache_range((ulong)p, (ulong)&p[1]);
+		flush_dcache_range((ulong)p & ~(EFI_CACHELINE_SIZE - 1),
+			ALIGN((ulong)&p[1], EFI_CACHELINE_SIZE));
 	}
 
 #ifndef IS_RELA

[U-Boot] [PATCH] efi_loader: Always flush in cache line size granularity

[Tom Rini]

On Mon, Apr 11, 2016 at 12:36:33AM +0200, Marek Vasut wrote:

> On 04/11/2016 12:31 AM, Alexander Graf wrote:
> > 
> > 
> > On 11.04.16 00:24, Andreas F?rber wrote:
> >> Am 04.04.2016 um 09:32 schrieb Alexander Graf:
> >>> The cache line flush helpers only work properly when they get aligned
> >>> start and end addresses. Round our flush range to cache line size. It's
> >>> safe because we're guaranteed to flush within a single page which has the
> >>> same cache attributes.
> >>>
> >>> Reported-by: Marek Vasut <marex@denx.de>
> >>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>> ---
> >>>  lib/efi_loader/efi_runtime.c | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> >>> index 22bcd08..40acec0 100644
> >>> --- a/lib/efi_loader/efi_runtime.c
> >>> +++ b/lib/efi_loader/efi_runtime.c
> >>> @@ -194,7 +194,8 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map)
> >>>  #endif
> >>>  
> >>>  		*p = newaddr;
> >>> -		flush_dcache_range((ulong)p, (ulong)&p[1]);
> >>> +		flush_dcache_range((ulong)p & ~(CONFIG_SYS_CACHELINE_SIZE - 1),
> >>> +			ALIGN((ulong)&p[1], CONFIG_SYS_CACHELINE_SIZE));
> >>
> >> dragonboard410c_defconfig fails to build with this due to undefined
> >> CONFIG_SYS_CACHELINE_SIZE. Do we need to #ifdef here or is the
> >> dragonboard410c at fault for not defining it?
> > 
> > Some USB drivers use it unconditionally, but I guess the dragonboard401c
> > doesn't enable USB.
> > 
> > Let me come up with a more compatible version.
> 
> This version is fine, dragonboard needs fixing. Every single board must
> define CONFIG_SYS_CACHELINE_SIZE .

Yeah... but this isn't quite the right time to break all of:
       arm:  +   mx28evk integratorap_cm720t at91sam9261ek_dataflash_cs3
       at91sam9261ek_dataflash_cs0 integratorap_cm946es mx28evk_nand
       integratorcp_cm1136 at91sam9x5ek_nandflash
       at91sam9xeek_dataflash_cs1 at91sam9260ek_dataflash_cs0
       at91sam9260ek_dataflash_cs1 at91sam9g20ek_nandflash
       meesc_dataflash at91sam9260ek_nandflash mx35pdk xfi3
       at91sam9263ek_dataflash_cs0 at91sam9xeek_dataflash_cs0
       at91sam9xeek_nandflash bg0900 highbank sansa_fuze_plus mx25pdk
       mx23evk at91sam9x5ek_mmc igep0030_nand m28evk
       at91sam9g20ek_dataflash_cs1 at91sam9g20ek_dataflash_cs0
       integratorcp_cm946es at91sam9n12ek_nandflash
       at91sam9n12ek_spiflash da850_am18xxevm devkit3250
       at91rm9200ek_ram picosam9g45 at91sam9n12ek_mmc
       integratorcp_cm926ejs integratorap_cm920t
       at91sam9m10g45ek_nandflash sc_sps_1 omapl138_lcdk mx28evk_spi
       integratorap_cm926ejs at91sam9rlek_mmc apx4devkit mx23_olinuxino
       at91sam9x5ek_spiflash at91sam9g10ek_nandflash
       at91sam9g10ek_dataflash_cs3 at91sam9g10ek_dataflash_cs0
       at91sam9g20ek_2mmc_nandflash apf27 stv0991
       at91sam9261ek_nandflash at91rm9200ek at91sam9g20ek_2mmc
       da850evm_direct_nor at91sam9263ek_norflash meesc
       at91sam9263ek_dataflash at91sam9m10g45ek_mmc armadillo-800eva
       at91sam9x5ek_dataflash da850evm usb_a9263_dataflash
       at91sam9rlek_nandflash mx28evk_auart_console
       at91sam9263ek_norflash_boot integratorcp_cm920t
       at91sam9263ek_nandflash at91sam9rlek_dataflash  
(and maybe a few more, toolchain here craps out due to libgcc problems),
and we've talked about this before too I think.  The right long term
answer is to borrow the logic of arch/arm/mm/Kconfig and asm/cache.h
from the kernel to get the right size, always.  For the short term,
we'll just work-around it.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160411/fdcc7b78/attachment.sig>

[U-Boot] [PATCH] efi_loader: Always flush in cache line size granularity

[Marek Vasut]

On 04/12/2016 02:39 AM, Tom Rini wrote:
> On Mon, Apr 11, 2016 at 12:36:33AM +0200, Marek Vasut wrote:
> 
>> On 04/11/2016 12:31 AM, Alexander Graf wrote:
>>>
>>>
>>> On 11.04.16 00:24, Andreas F?rber wrote:
>>>> Am 04.04.2016 um 09:32 schrieb Alexander Graf:
>>>>> The cache line flush helpers only work properly when they get aligned
>>>>> start and end addresses. Round our flush range to cache line size. It's
>>>>> safe because we're guaranteed to flush within a single page which has the
>>>>> same cache attributes.
>>>>>
>>>>> Reported-by: Marek Vasut <marex@denx.de>
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>> ---
>>>>>  lib/efi_loader/efi_runtime.c | 3 ++-
>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
>>>>> index 22bcd08..40acec0 100644
>>>>> --- a/lib/efi_loader/efi_runtime.c
>>>>> +++ b/lib/efi_loader/efi_runtime.c
>>>>> @@ -194,7 +194,8 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map)
>>>>>  #endif
>>>>>  
>>>>>  		*p = newaddr;
>>>>> -		flush_dcache_range((ulong)p, (ulong)&p[1]);
>>>>> +		flush_dcache_range((ulong)p & ~(CONFIG_SYS_CACHELINE_SIZE - 1),
>>>>> +			ALIGN((ulong)&p[1], CONFIG_SYS_CACHELINE_SIZE));
>>>>
>>>> dragonboard410c_defconfig fails to build with this due to undefined
>>>> CONFIG_SYS_CACHELINE_SIZE. Do we need to #ifdef here or is the
>>>> dragonboard410c at fault for not defining it?
>>>
>>> Some USB drivers use it unconditionally, but I guess the dragonboard401c
>>> doesn't enable USB.
>>>
>>> Let me come up with a more compatible version.
>>
>> This version is fine, dragonboard needs fixing. Every single board must
>> define CONFIG_SYS_CACHELINE_SIZE .
> 
> Yeah... but this isn't quite the right time to break all of:
>        arm:  +   mx28evk integratorap_cm720t at91sam9261ek_dataflash_cs3
>        at91sam9261ek_dataflash_cs0 integratorap_cm946es mx28evk_nand
>        integratorcp_cm1136 at91sam9x5ek_nandflash
>        at91sam9xeek_dataflash_cs1 at91sam9260ek_dataflash_cs0
>        at91sam9260ek_dataflash_cs1 at91sam9g20ek_nandflash
>        meesc_dataflash at91sam9260ek_nandflash mx35pdk xfi3
>        at91sam9263ek_dataflash_cs0 at91sam9xeek_dataflash_cs0
>        at91sam9xeek_nandflash bg0900 highbank sansa_fuze_plus mx25pdk
>        mx23evk at91sam9x5ek_mmc igep0030_nand m28evk
>        at91sam9g20ek_dataflash_cs1 at91sam9g20ek_dataflash_cs0
>        integratorcp_cm946es at91sam9n12ek_nandflash
>        at91sam9n12ek_spiflash da850_am18xxevm devkit3250
>        at91rm9200ek_ram picosam9g45 at91sam9n12ek_mmc
>        integratorcp_cm926ejs integratorap_cm920t
>        at91sam9m10g45ek_nandflash sc_sps_1 omapl138_lcdk mx28evk_spi
>        integratorap_cm926ejs at91sam9rlek_mmc apx4devkit mx23_olinuxino
>        at91sam9x5ek_spiflash at91sam9g10ek_nandflash
>        at91sam9g10ek_dataflash_cs3 at91sam9g10ek_dataflash_cs0
>        at91sam9g20ek_2mmc_nandflash apf27 stv0991
>        at91sam9261ek_nandflash at91rm9200ek at91sam9g20ek_2mmc
>        da850evm_direct_nor at91sam9263ek_norflash meesc
>        at91sam9263ek_dataflash at91sam9m10g45ek_mmc armadillo-800eva
>        at91sam9x5ek_dataflash da850evm usb_a9263_dataflash
>        at91sam9rlek_nandflash mx28evk_auart_console
>        at91sam9263ek_norflash_boot integratorcp_cm920t
>        at91sam9263ek_nandflash at91sam9rlek_dataflash  
> (and maybe a few more, toolchain here craps out due to libgcc problems),

Ha, libgcc ... :-(

> and we've talked about this before too I think.  The right long term
> answer is to borrow the logic of arch/arm/mm/Kconfig and asm/cache.h
> from the kernel to get the right size, always.  For the short term,
> we'll just work-around it.

I thought we fixed these cache issues already, but that was on v7 only,
dang. Now it is v4/v5 which broke.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] efi_loader: Always flush in cache line size granularity

[Andreas Färber]

Am 11.04.2016 um 23:20 schrieb Alexander Graf:
> The cache line flush helpers only work properly when they get aligned
> start and end addresses. Round our flush range to cache line size. It's
> safe because we're guaranteed to flush within a single page which has the
> same cache attributes.
> 
> Reported-by: Marek Vasut <marex@denx.de>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Reviewed-by: Andreas F?rber <afaerber@suse.de>
Tested-by: Andreas F?rber <afaerber@suse.de>

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)