riscv: uaccess: Allow the last potential unrolled copy

[PATCH] riscv: uaccess: Allow the last potential unrolled copy

Posted by Xiao Wang 1 year, 11 months ago

When the dst buffer pointer points to the last accessible aligned addr, we
could still run another iteration of unrolled copy.

Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
---
 arch/riscv/lib/uaccess.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
index 2e665f8f8fcc..1399d797d81b 100644
--- a/arch/riscv/lib/uaccess.S
+++ b/arch/riscv/lib/uaccess.S
@@ -103,7 +103,7 @@ SYM_FUNC_START(fallback_scalar_usercopy)
 	fixup REG_S   t4,  7*SZREG(a0), 10f
 	addi	a0, a0, 8*SZREG
 	addi	a1, a1, 8*SZREG
-	bltu	a0, t0, 2b
+	bleu	a0, t0, 2b
 
 	addi	t0, t0, 8*SZREG /* revert to original value */
 	j	.Lbyte_copy_tail
-- 
2.25.1

Re: [PATCH] riscv: uaccess: Allow the last potential unrolled copy

Posted by Alexandre Ghiti 1 year, 9 months ago

Hi Xiao,

On 13/03/2024 11:33, Xiao Wang wrote:
> When the dst buffer pointer points to the last accessible aligned addr, we
> could still run another iteration of unrolled copy.
>
> Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
> ---
>   arch/riscv/lib/uaccess.S | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
> index 2e665f8f8fcc..1399d797d81b 100644
> --- a/arch/riscv/lib/uaccess.S
> +++ b/arch/riscv/lib/uaccess.S
> @@ -103,7 +103,7 @@ SYM_FUNC_START(fallback_scalar_usercopy)
>   	fixup REG_S   t4,  7*SZREG(a0), 10f
>   	addi	a0, a0, 8*SZREG
>   	addi	a1, a1, 8*SZREG
> -	bltu	a0, t0, 2b
> +	bleu	a0, t0, 2b
>   
>   	addi	t0, t0, 8*SZREG /* revert to original value */
>   	j	.Lbyte_copy_tail


I agree it is still safe to continue for another word_copy here.

Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>

Thanks,

Alex

Re: [PATCH] riscv: uaccess: Allow the last potential unrolled copy

Posted by Ben Dooks 1 year, 9 months ago

On 03/05/2024 13:16, Alexandre Ghiti wrote:
> Hi Xiao,
> 
> On 13/03/2024 11:33, Xiao Wang wrote:
>> When the dst buffer pointer points to the last accessible aligned 
>> addr, we
>> could still run another iteration of unrolled copy.
>>
>> Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
>> ---
>>   arch/riscv/lib/uaccess.S | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
>> index 2e665f8f8fcc..1399d797d81b 100644
>> --- a/arch/riscv/lib/uaccess.S
>> +++ b/arch/riscv/lib/uaccess.S
>> @@ -103,7 +103,7 @@ SYM_FUNC_START(fallback_scalar_usercopy)
>>       fixup REG_S   t4,  7*SZREG(a0), 10f
>>       addi    a0, a0, 8*SZREG
>>       addi    a1, a1, 8*SZREG
>> -    bltu    a0, t0, 2b
>> +    bleu    a0, t0, 2b
>>       addi    t0, t0, 8*SZREG /* revert to original value */
>>       j    .Lbyte_copy_tail
> 
> 
> I agree it is still safe to continue for another word_copy here.
> 
> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>

Out of interest, has anyone checked if causing a schedule event during
this code breaks like the last time we had issues with the upstream
testing?

I did propose saving the state of the user-access flag in the task
struct but we mostly solved it by making sleeping functions stay
away from the address calculation. This of course may have been done
already or need to be done if three's long areas where the user-access
flags can be disabled (generally only a few drivers did this, so we
may not have come across the problem)

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCH] riscv: uaccess: Allow the last potential unrolled copy

Posted by Alexandre Ghiti 1 year, 9 months ago

Hi Ben,

On 03/05/2024 14:19, Ben Dooks wrote:
> On 03/05/2024 13:16, Alexandre Ghiti wrote:
>> Hi Xiao,
>>
>> On 13/03/2024 11:33, Xiao Wang wrote:
>>> When the dst buffer pointer points to the last accessible aligned 
>>> addr, we
>>> could still run another iteration of unrolled copy.
>>>
>>> Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
>>> ---
>>>   arch/riscv/lib/uaccess.S | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
>>> index 2e665f8f8fcc..1399d797d81b 100644
>>> --- a/arch/riscv/lib/uaccess.S
>>> +++ b/arch/riscv/lib/uaccess.S
>>> @@ -103,7 +103,7 @@ SYM_FUNC_START(fallback_scalar_usercopy)
>>>       fixup REG_S   t4,  7*SZREG(a0), 10f
>>>       addi    a0, a0, 8*SZREG
>>>       addi    a1, a1, 8*SZREG
>>> -    bltu    a0, t0, 2b
>>> +    bleu    a0, t0, 2b
>>>       addi    t0, t0, 8*SZREG /* revert to original value */
>>>       j    .Lbyte_copy_tail
>>
>>
>> I agree it is still safe to continue for another word_copy here.
>>
>> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
>
> Out of interest, has anyone checked if causing a schedule event during
> this code breaks like the last time we had issues with the upstream
> testing?


I vaguely remember something, do you have a link to that discussion by 
chance?


>
> I did propose saving the state of the user-access flag in the task
> struct


Makes sense, I just took a quick look and SR_SUM is cleared as soon as 
we enter handle_exception() and it does not seem to be restored. Weird 
it works, unless I missed something!


> but we mostly solved it by making sleeping functions stay
> away from the address calculation. This of course may have been done
> already or need to be done if three's long areas where the user-access
> flags can be disabled (generally only a few drivers did this, so we
> may not have come across the problem)
>
I don't understand what you mean here, would you mind expanding a bit?

Thanks,

Alex

Re: [PATCH] riscv: uaccess: Allow the last potential unrolled copy

Posted by Ben Dooks 1 year, 9 months ago

On 03/05/2024 14:02, Alexandre Ghiti wrote:
> Hi Ben,
> 
> On 03/05/2024 14:19, Ben Dooks wrote:
>> On 03/05/2024 13:16, Alexandre Ghiti wrote:
>>> Hi Xiao,
>>>
>>> On 13/03/2024 11:33, Xiao Wang wrote:
>>>> When the dst buffer pointer points to the last accessible aligned 
>>>> addr, we
>>>> could still run another iteration of unrolled copy.
>>>>
>>>> Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
>>>> ---
>>>>   arch/riscv/lib/uaccess.S | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
>>>> index 2e665f8f8fcc..1399d797d81b 100644
>>>> --- a/arch/riscv/lib/uaccess.S
>>>> +++ b/arch/riscv/lib/uaccess.S
>>>> @@ -103,7 +103,7 @@ SYM_FUNC_START(fallback_scalar_usercopy)
>>>>       fixup REG_S   t4,  7*SZREG(a0), 10f
>>>>       addi    a0, a0, 8*SZREG
>>>>       addi    a1, a1, 8*SZREG
>>>> -    bltu    a0, t0, 2b
>>>> +    bleu    a0, t0, 2b
>>>>       addi    t0, t0, 8*SZREG /* revert to original value */
>>>>       j    .Lbyte_copy_tail
>>>
>>>
>>> I agree it is still safe to continue for another word_copy here.
>>>
>>> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
>>
>> Out of interest, has anyone checked if causing a schedule event during
>> this code breaks like the last time we had issues with the upstream
>> testing?
> 
> 
> I vaguely remember something, do you have a link to that discussion by 
> chance?
> 
> 
>>
>> I did propose saving the state of the user-access flag in the task
>> struct
> 
> 
> Makes sense, I just took a quick look and SR_SUM is cleared as soon as 
> we enter handle_exception() and it does not seem to be restored. Weird 
> it works, unless I missed something!
> 
> 
>> but we mostly solved it by making sleeping functions stay
>> away from the address calculation. This of course may have been done
>> already or need to be done if three's long areas where the user-access
>> flags can be disabled (generally only a few drivers did this, so we
>> may not have come across the problem)
>>
> I don't understand what you mean here, would you mind expanding a bit?
> 

I think this was all gone through in the original post where
we initially suggested saving SR_SUM and then moved as much out
of the critical SR_SUM area by changing how the macros worked

https://lore.kernel.org/linux-riscv/20210318151010.100966-1-ben.dooks@codethink.co.uk/

https://lore.kernel.org/linux-riscv/20210329095749.998940-1-ben.dooks@codethink.co.uk/
-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

RE: [PATCH] riscv: uaccess: Allow the last potential unrolled copy

Posted by Wang, Xiao W 1 year, 9 months ago

Hi Alex,

> -----Original Message-----
> From: ben.dooks@codethink.co.uk <ben.dooks@codethink.co.uk>
> Sent: Friday, May 3, 2024 10:30 PM
> To: Alexandre Ghiti <alex@ghiti.fr>; Wang, Xiao W <xiao.w.wang@intel.com>;
> paul.walmsley@sifive.com; palmer@dabbelt.com; aou@eecs.berkeley.edu
> Cc: jerry.shih@sifive.com; nick.knight@sifive.com;
> ajones@ventanamicro.com; bjorn@rivosinc.com; andy.chiu@sifive.com;
> viro@zeniv.linux.org.uk; cleger@rivosinc.com; alexghiti@rivosinc.com; Li,
> Haicheng <haicheng.li@intel.com>; akira.tsukamoto@gmail.com; linux-
> riscv@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] riscv: uaccess: Allow the last potential unrolled copy
> 
> On 03/05/2024 14:02, Alexandre Ghiti wrote:
> > Hi Ben,
> >
> > On 03/05/2024 14:19, Ben Dooks wrote:
> >> On 03/05/2024 13:16, Alexandre Ghiti wrote:
> >>> Hi Xiao,
> >>>
> >>> On 13/03/2024 11:33, Xiao Wang wrote:
> >>>> When the dst buffer pointer points to the last accessible aligned
> >>>> addr, we
> >>>> could still run another iteration of unrolled copy.
> >>>>
> >>>> Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
> >>>> ---
> >>>>   arch/riscv/lib/uaccess.S | 2 +-
> >>>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
> >>>> index 2e665f8f8fcc..1399d797d81b 100644
> >>>> --- a/arch/riscv/lib/uaccess.S
> >>>> +++ b/arch/riscv/lib/uaccess.S
> >>>> @@ -103,7 +103,7 @@ SYM_FUNC_START(fallback_scalar_usercopy)
> >>>>       fixup REG_S   t4,  7*SZREG(a0), 10f
> >>>>       addi    a0, a0, 8*SZREG
> >>>>       addi    a1, a1, 8*SZREG
> >>>> -    bltu    a0, t0, 2b
> >>>> +    bleu    a0, t0, 2b
> >>>>       addi    t0, t0, 8*SZREG /* revert to original value */
> >>>>       j    .Lbyte_copy_tail
> >>>
> >>>
> >>> I agree it is still safe to continue for another word_copy here.
> >>>
> >>> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> >>
> >> Out of interest, has anyone checked if causing a schedule event during
> >> this code breaks like the last time we had issues with the upstream
> >> testing?
> >
> >
> > I vaguely remember something, do you have a link to that discussion by
> > chance?
> >
> >
> >>
> >> I did propose saving the state of the user-access flag in the task
> >> struct
> >
> >
> > Makes sense, I just took a quick look and SR_SUM is cleared as soon as
> > we enter handle_exception() and it does not seem to be restored. Weird
> > it works, unless I missed something!

I think it's already saved/restored via pt_regs.status, using the PT_STATUS() macro in entry.S.

BRs,
Xiao

> >
> >
> >> but we mostly solved it by making sleeping functions stay
> >> away from the address calculation. This of course may have been done
> >> already or need to be done if three's long areas where the user-access
> >> flags can be disabled (generally only a few drivers did this, so we
> >> may not have come across the problem)
> >>
> > I don't understand what you mean here, would you mind expanding a bit?
> >
> 
> I think this was all gone through in the original post where
> we initially suggested saving SR_SUM and then moved as much out
> of the critical SR_SUM area by changing how the macros worked
> 
> https://lore.kernel.org/linux-riscv/20210318151010.100966-1-
> ben.dooks@codethink.co.uk/
> 
> https://lore.kernel.org/linux-riscv/20210329095749.998940-1-
> ben.dooks@codethink.co.uk/
> --
> Ben Dooks				http://www.codethink.co.uk/
> Senior Engineer				Codethink - Providing Genius
> 
> https://www.codethink.co.uk/privacy.html