[PATCH] MIPS: KASLR: Fix sync_icache() trapped in loop when synci_step is zero

[PATCH] MIPS: KASLR: Fix sync_icache() trapped in loop when synci_step is zero

[Jinyang He]

Reading synci_step by using rdhwr instruction may return zero if no cache
need be synchronized. On the one hand, to make sure all load operation and
store operation finished we do __sync() for every platform. On the other
hand, some platform need operate synci one time although step is zero.

Signed-off-by: Jinyang He <hejinyang@loongson.cn>
---
 arch/mips/kernel/relocate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/kernel/relocate.c b/arch/mips/kernel/relocate.c
index 57bdd276..47aeb33 100644
--- a/arch/mips/kernel/relocate.c
+++ b/arch/mips/kernel/relocate.c
@@ -64,7 +64,7 @@ static void __init sync_icache(void *kbase, unsigned long kernel_length)
 			: "r" (kbase));
 
 		kbase += step;
-	} while (kbase < kend);
+	} while (step && kbase < kend);
 
 	/* Completion barrier */
 	__sync();
-- 
2.1.0

Re: [PATCH] MIPS: KASLR: Fix sync_icache() trapped in loop when synci_step is zero

[Thomas Bogendoerfer]

On Wed, Dec 02, 2020 at 11:00:05AM +0800, Jinyang He wrote:
> Reading synci_step by using rdhwr instruction may return zero if no cache
> need be synchronized. On the one hand, to make sure all load operation and
> store operation finished we do __sync() for every platform. On the other
> hand, some platform need operate synci one time although step is zero.

Should this be someting like: Avoid endless loop, if no synci is needed ?

> diff --git a/arch/mips/kernel/relocate.c b/arch/mips/kernel/relocate.c
> index 57bdd276..47aeb33 100644
> --- a/arch/mips/kernel/relocate.c
> +++ b/arch/mips/kernel/relocate.c
> @@ -64,7 +64,7 @@ static void __init sync_icache(void *kbase, unsigned long kernel_length)
>  			: "r" (kbase));
>  
>  		kbase += step;
> -	} while (kbase < kend);
> +	} while (step && kbase < kend);

why not do a

	if (step == 0)
		return;

before entering the loop ? According to MIPS32PRA no synci is needed,
if stepi value is zero.

Thomas.

PS: Does anybody know a reason, why this code doesn't use an old fashioned
dache/icache flushing, which might be slower but would work also on
legecy cores ?

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: KASLR: Fix sync_icache() trapped in loop when synci_step is zero

[Jinyang He]

Hi, Thomas,

On 12/02/2020 06:39 PM, Thomas Bogendoerfer wrote:
> On Wed, Dec 02, 2020 at 11:00:05AM +0800, Jinyang He wrote:
>> Reading synci_step by using rdhwr instruction may return zero if no cache
>> need be synchronized. On the one hand, to make sure all load operation and
>> store operation finished we do __sync() for every platform. On the other
>> hand, some platform need operate synci one time although step is zero.
> Should this be someting like: Avoid endless loop, if no synci is needed ?
>
>> diff --git a/arch/mips/kernel/relocate.c b/arch/mips/kernel/relocate.c
>> index 57bdd276..47aeb33 100644
>> --- a/arch/mips/kernel/relocate.c
>> +++ b/arch/mips/kernel/relocate.c
>> @@ -64,7 +64,7 @@ static void __init sync_icache(void *kbase, unsigned long kernel_length)
>>   			: "r" (kbase));
>>   
>>   		kbase += step;
>> -	} while (kbase < kend);
>> +	} while (step && kbase < kend);
> why not do a
>
> 	if (step == 0)
> 		return;
>
> before entering the loop ? According to MIPS32PRA no synci is needed,
> if stepi value is zero.

Thanks for your reply.

Most platforms do not need to do synci instruction operations
when synci_step is 0. But for example, the synci implementation
on Loongson64 platform has some changes. On the one hand, it
ensures that the memory access instructions have been completed.
On the other hand, it guarantees that all prefetch instructions
need to be fetched again. And its address information is useless.
Thus, only one synci operation is required when synci_step is 0
on Loongson64 platform. I guess that some other platforms have
similar implementations on synci, so add judgment conditions in
`while` to ensure that at least all platforms perform synci
operations once. For those platforms that do not need synci,
they just do one more operation similar to nop.

I will modify the submitted information and send v2.

> Thomas.
> PS: Does anybody know a reason, why this code doesn't use an old fashioned
> dache/icache flushing, which might be slower but would work also on
> legecy cores ?
For this, my thought is that different platforms using the cache
instruction to flush caches is inconsistent. Here is just a more
general way to flush these caches.

Thanks,
Jinyang.

Re: [PATCH] MIPS: KASLR: Fix sync_icache() trapped in loop when synci_step is zero

[Jiaxun Yang]

在 2020/12/2 下午6:39, Thomas Bogendoerfer 写道:
> On Wed, Dec 02, 2020 at 11:00:05AM +0800, Jinyang He wrote:
>> Reading synci_step by using rdhwr instruction may return zero if no cache
>> need be synchronized. On the one hand, to make sure all load operation and
>> store operation finished we do __sync() for every platform. On the other
>> hand, some platform need operate synci one time although step is zero.
> Should this be someting like: Avoid endless loop, if no synci is needed ?
>
>> diff --git a/arch/mips/kernel/relocate.c b/arch/mips/kernel/relocate.c
>> index 57bdd276..47aeb33 100644
>> --- a/arch/mips/kernel/relocate.c
>> +++ b/arch/mips/kernel/relocate.c
>> @@ -64,7 +64,7 @@ static void __init sync_icache(void *kbase, unsigned long kernel_length)
>>   			: "r" (kbase));
>>   
>>   		kbase += step;
>> -	} while (kbase < kend);
>> +	} while (step && kbase < kend);
> why not do a
>
> 	if (step == 0)
> 		return;
>
> before entering the loop ? According to MIPS32PRA no synci is needed,
> if stepi value is zero.
>
> Thomas.
>
> PS: Does anybody know a reason, why this code doesn't use an old fashioned
> dache/icache flushing, which might be slower but would work also on
> legecy cores ?

I thought that's because legacy flush requires much more cares.
You'll have to probe cache ways sets and line size to do so.
However relocation happens very early, even before cache probe.

Thanks.

- Jiaxun

Re: [PATCH] MIPS: KASLR: Fix sync_icache() trapped in loop when synci_step is zero

[Thomas Bogendoerfer]

On Thu, Dec 03, 2020 at 12:02:10PM +0800, Jiaxun Yang wrote:
> 
> 
> 在 2020/12/2 下午6:39, Thomas Bogendoerfer 写道:
> > On Wed, Dec 02, 2020 at 11:00:05AM +0800, Jinyang He wrote:
> > > Reading synci_step by using rdhwr instruction may return zero if no cache
> > > need be synchronized. On the one hand, to make sure all load operation and
> > > store operation finished we do __sync() for every platform. On the other
> > > hand, some platform need operate synci one time although step is zero.
> > Should this be someting like: Avoid endless loop, if no synci is needed ?
> > 
> > > diff --git a/arch/mips/kernel/relocate.c b/arch/mips/kernel/relocate.c
> > > index 57bdd276..47aeb33 100644
> > > --- a/arch/mips/kernel/relocate.c
> > > +++ b/arch/mips/kernel/relocate.c
> > > @@ -64,7 +64,7 @@ static void __init sync_icache(void *kbase, unsigned long kernel_length)
> > >   			: "r" (kbase));
> > >   		kbase += step;
> > > -	} while (kbase < kend);
> > > +	} while (step && kbase < kend);
> > why not do a
> > 
> > 	if (step == 0)
> > 		return;
> > 
> > before entering the loop ? According to MIPS32PRA no synci is needed,
> > if stepi value is zero.
> > 
> > Thomas.
> > 
> > PS: Does anybody know a reason, why this code doesn't use an old fashioned
> > dache/icache flushing, which might be slower but would work also on
> > legecy cores ?
> 
> I thought that's because legacy flush requires much more cares.

that's true. It shouldn't be that hard, but probably has to wait until
someone needs it.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: KASLR: Fix sync_icache() trapped in loop when synci_step is zero

[Maciej W. Rozycki]

On Wed, 2 Dec 2020, Thomas Bogendoerfer wrote:

> PS: Does anybody know a reason, why this code doesn't use an old fashioned
> dache/icache flushing, which might be slower but would work also on
> legecy cores ?

 Hmm, this was contributed by ImgTec in 2016 only, so I guess there was no 
reason as such but merely the lack of care about older systems (mind that 
those people really didn't have any at that point).  The option to enable 
this code is only available for R2 and newer CPUs so at least we are safe.

  Maciej

Re: [PATCH] MIPS: KASLR: Fix sync_icache() trapped in loop when synci_step is zero

[Maciej W. Rozycki]

On Thu, 3 Dec 2020, Jinyang He wrote:

> Thus, only one synci operation is required when synci_step is 0
> on Loongson64 platform. I guess that some other platforms have
> similar implementations on synci, so add judgment conditions in
> `while` to ensure that at least all platforms perform synci
> operations once. For those platforms that do not need synci,
> they just do one more operation similar to nop.

 This is non-compliant and looks to me like a risky choice for what was 
invented to guarantee portability across all MIPS systems.  Compliant 
software will fail with Loongson64 processors unless patched like this 
piece, and you don't really have control over all user software out there 
(I would expect issues with JIT engines and the like).

 If only a single SYNCI operation is ever required why wasn't SYNCI_Step 
set to some large value instead that would in reality prevent looping over 
SYNCI from happening?

  Maciej