riscv: use local label names instead of global ones in assembly

[PATCH] riscv: use local label names instead of global ones in assembly

Posted by Clément Léger 1 year, 1 month ago

Local labels should be prefix by '.L' or they'll be exported in the
symbol table. Additionally, this messes up the backtrace by displaying
an incorrect symbol:

  ...
  [   12.751810] [<ffffffff80441628>] _copy_from_user+0x28/0xc2
  [   12.752035] [<ffffffff800152ca>] handle_misaligned_load+0x1ca/0x2fc
  [   12.752310] [<ffffffff80a033e8>] do_trap_load_misaligned+0x24/0xee
  [   12.752596] [<ffffffff80a0dcae>] _new_vmalloc_restore_context_a0+0xc2/0xce

After:
  ...
  [   10.243916] [<ffffffff804415e4>] _copy_from_user+0x28/0xc2
  [   10.244026] [<ffffffff800152ca>] handle_misaligned_load+0x1ca/0x2fc
  [   10.244150] [<ffffffff80a033a0>] do_trap_load_misaligned+0x24/0xee
  [   10.244268] [<ffffffff80a0dc66>] handle_exception+0x146/0x152

Signed-off-by: Clément Léger <cleger@rivosinc.com>

---
 arch/riscv/kernel/entry.S | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index c200d329d4bd..216581835eb0 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -23,21 +23,21 @@
 	REG_S 	a0, TASK_TI_A0(tp)
 	csrr 	a0, CSR_CAUSE
 	/* Exclude IRQs */
-	blt  	a0, zero, _new_vmalloc_restore_context_a0
+	blt  	a0, zero, .Lnew_vmalloc_restore_context_a0
 
 	REG_S 	a1, TASK_TI_A1(tp)
 	/* Only check new_vmalloc if we are in page/protection fault */
 	li   	a1, EXC_LOAD_PAGE_FAULT
-	beq  	a0, a1, _new_vmalloc_kernel_address
+	beq  	a0, a1, .Lnew_vmalloc_kernel_address
 	li   	a1, EXC_STORE_PAGE_FAULT
-	beq  	a0, a1, _new_vmalloc_kernel_address
+	beq  	a0, a1, .Lnew_vmalloc_kernel_address
 	li   	a1, EXC_INST_PAGE_FAULT
-	bne  	a0, a1, _new_vmalloc_restore_context_a1
+	bne  	a0, a1, .Lnew_vmalloc_restore_context_a1
 
-_new_vmalloc_kernel_address:
+.Lnew_vmalloc_kernel_address:
 	/* Is it a kernel address? */
 	csrr 	a0, CSR_TVAL
-	bge 	a0, zero, _new_vmalloc_restore_context_a1
+	bge 	a0, zero, .Lnew_vmalloc_restore_context_a1
 
 	/* Check if a new vmalloc mapping appeared that could explain the trap */
 	REG_S	a2, TASK_TI_A2(tp)
@@ -69,7 +69,7 @@ _new_vmalloc_kernel_address:
 	/* Check the value of new_vmalloc for this cpu */
 	REG_L	a2, 0(a0)
 	and	a2, a2, a1
-	beq	a2, zero, _new_vmalloc_restore_context
+	beq	a2, zero, .Lnew_vmalloc_restore_context
 
 	/* Atomically reset the current cpu bit in new_vmalloc */
 	amoxor.d	a0, a1, (a0)
@@ -83,11 +83,11 @@ _new_vmalloc_kernel_address:
 	csrw	CSR_SCRATCH, x0
 	sret
 
-_new_vmalloc_restore_context:
+.Lnew_vmalloc_restore_context:
 	REG_L 	a2, TASK_TI_A2(tp)
-_new_vmalloc_restore_context_a1:
+.Lnew_vmalloc_restore_context_a1:
 	REG_L 	a1, TASK_TI_A1(tp)
-_new_vmalloc_restore_context_a0:
+.Lnew_vmalloc_restore_context_a0:
 	REG_L	a0, TASK_TI_A0(tp)
 .endm
 
-- 
2.45.2

Re: [PATCH] riscv: use local label names instead of global ones in assembly

Posted by Alexandre Ghiti 1 year, 1 month ago

Hi Clément,

On 03/01/2025 15:17, Clément Léger wrote:
> Local labels should be prefix by '.L' or they'll be exported in the
> symbol table. Additionally, this messes up the backtrace by displaying
> an incorrect symbol:
>
>    ...
>    [   12.751810] [<ffffffff80441628>] _copy_from_user+0x28/0xc2
>    [   12.752035] [<ffffffff800152ca>] handle_misaligned_load+0x1ca/0x2fc
>    [   12.752310] [<ffffffff80a033e8>] do_trap_load_misaligned+0x24/0xee
>    [   12.752596] [<ffffffff80a0dcae>] _new_vmalloc_restore_context_a0+0xc2/0xce


Yes, I noticed this last week, thanks for looking into it. I would add a 
Fixes tag and merge that in the next -rc if possible as the above 
backtrace is "disturbing".


>
> After:
>    ...
>    [   10.243916] [<ffffffff804415e4>] _copy_from_user+0x28/0xc2
>    [   10.244026] [<ffffffff800152ca>] handle_misaligned_load+0x1ca/0x2fc
>    [   10.244150] [<ffffffff80a033a0>] do_trap_load_misaligned+0x24/0xee
>    [   10.244268] [<ffffffff80a0dc66>] handle_exception+0x146/0x152
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>
> ---
>   arch/riscv/kernel/entry.S | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index c200d329d4bd..216581835eb0 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -23,21 +23,21 @@
>   	REG_S 	a0, TASK_TI_A0(tp)
>   	csrr 	a0, CSR_CAUSE
>   	/* Exclude IRQs */
> -	blt  	a0, zero, _new_vmalloc_restore_context_a0
> +	blt  	a0, zero, .Lnew_vmalloc_restore_context_a0
>   
>   	REG_S 	a1, TASK_TI_A1(tp)
>   	/* Only check new_vmalloc if we are in page/protection fault */
>   	li   	a1, EXC_LOAD_PAGE_FAULT
> -	beq  	a0, a1, _new_vmalloc_kernel_address
> +	beq  	a0, a1, .Lnew_vmalloc_kernel_address
>   	li   	a1, EXC_STORE_PAGE_FAULT
> -	beq  	a0, a1, _new_vmalloc_kernel_address
> +	beq  	a0, a1, .Lnew_vmalloc_kernel_address
>   	li   	a1, EXC_INST_PAGE_FAULT
> -	bne  	a0, a1, _new_vmalloc_restore_context_a1
> +	bne  	a0, a1, .Lnew_vmalloc_restore_context_a1
>   
> -_new_vmalloc_kernel_address:
> +.Lnew_vmalloc_kernel_address:
>   	/* Is it a kernel address? */
>   	csrr 	a0, CSR_TVAL
> -	bge 	a0, zero, _new_vmalloc_restore_context_a1
> +	bge 	a0, zero, .Lnew_vmalloc_restore_context_a1
>   
>   	/* Check if a new vmalloc mapping appeared that could explain the trap */
>   	REG_S	a2, TASK_TI_A2(tp)
> @@ -69,7 +69,7 @@ _new_vmalloc_kernel_address:
>   	/* Check the value of new_vmalloc for this cpu */
>   	REG_L	a2, 0(a0)
>   	and	a2, a2, a1
> -	beq	a2, zero, _new_vmalloc_restore_context
> +	beq	a2, zero, .Lnew_vmalloc_restore_context
>   
>   	/* Atomically reset the current cpu bit in new_vmalloc */
>   	amoxor.d	a0, a1, (a0)
> @@ -83,11 +83,11 @@ _new_vmalloc_kernel_address:
>   	csrw	CSR_SCRATCH, x0
>   	sret
>   
> -_new_vmalloc_restore_context:
> +.Lnew_vmalloc_restore_context:
>   	REG_L 	a2, TASK_TI_A2(tp)
> -_new_vmalloc_restore_context_a1:
> +.Lnew_vmalloc_restore_context_a1:
>   	REG_L 	a1, TASK_TI_A1(tp)
> -_new_vmalloc_restore_context_a0:
> +.Lnew_vmalloc_restore_context_a0:
>   	REG_L	a0, TASK_TI_A0(tp)
>   .endm
>   


You can add:

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

Thanks,

Alex

Re: [PATCH] riscv: use local label names instead of global ones in assembly

Posted by Clément Léger 1 year, 1 month ago


On 06/01/2025 09:52, Alexandre Ghiti wrote:
> Hi Clément,
> 
> On 03/01/2025 15:17, Clément Léger wrote:
>> Local labels should be prefix by '.L' or they'll be exported in the
>> symbol table. Additionally, this messes up the backtrace by displaying
>> an incorrect symbol:
>>
>>    ...
>>    [   12.751810] [<ffffffff80441628>] _copy_from_user+0x28/0xc2
>>    [   12.752035] [<ffffffff800152ca>] handle_misaligned_load+0x1ca/0x2fc
>>    [   12.752310] [<ffffffff80a033e8>] do_trap_load_misaligned+0x24/0xee
>>    [   12.752596] [<ffffffff80a0dcae>]
>> _new_vmalloc_restore_context_a0+0xc2/0xce
> 
> 
> Yes, I noticed this last week, thanks for looking into it. I would add a
> Fixes tag and merge that in the next -rc if possible as the above
> backtrace is "disturbing".

Hi Alex,

Yeah, I thought the Fixes tag, but since it wasn't really "breaking"
anything, I left it out. I'll add it and sent a V2.

Thanks

Clément

> 
> 
>>
>> After:
>>    ...
>>    [   10.243916] [<ffffffff804415e4>] _copy_from_user+0x28/0xc2
>>    [   10.244026] [<ffffffff800152ca>] handle_misaligned_load+0x1ca/0x2fc
>>    [   10.244150] [<ffffffff80a033a0>] do_trap_load_misaligned+0x24/0xee
>>    [   10.244268] [<ffffffff80a0dc66>] handle_exception+0x146/0x152
>>
>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>>
>> ---
>>   arch/riscv/kernel/entry.S | 20 ++++++++++----------
>>   1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
>> index c200d329d4bd..216581835eb0 100644
>> --- a/arch/riscv/kernel/entry.S
>> +++ b/arch/riscv/kernel/entry.S
>> @@ -23,21 +23,21 @@
>>       REG_S     a0, TASK_TI_A0(tp)
>>       csrr     a0, CSR_CAUSE
>>       /* Exclude IRQs */
>> -    blt      a0, zero, _new_vmalloc_restore_context_a0
>> +    blt      a0, zero, .Lnew_vmalloc_restore_context_a0
>>         REG_S     a1, TASK_TI_A1(tp)
>>       /* Only check new_vmalloc if we are in page/protection fault */
>>       li       a1, EXC_LOAD_PAGE_FAULT
>> -    beq      a0, a1, _new_vmalloc_kernel_address
>> +    beq      a0, a1, .Lnew_vmalloc_kernel_address
>>       li       a1, EXC_STORE_PAGE_FAULT
>> -    beq      a0, a1, _new_vmalloc_kernel_address
>> +    beq      a0, a1, .Lnew_vmalloc_kernel_address
>>       li       a1, EXC_INST_PAGE_FAULT
>> -    bne      a0, a1, _new_vmalloc_restore_context_a1
>> +    bne      a0, a1, .Lnew_vmalloc_restore_context_a1
>>   -_new_vmalloc_kernel_address:
>> +.Lnew_vmalloc_kernel_address:
>>       /* Is it a kernel address? */
>>       csrr     a0, CSR_TVAL
>> -    bge     a0, zero, _new_vmalloc_restore_context_a1
>> +    bge     a0, zero, .Lnew_vmalloc_restore_context_a1
>>         /* Check if a new vmalloc mapping appeared that could explain
>> the trap */
>>       REG_S    a2, TASK_TI_A2(tp)
>> @@ -69,7 +69,7 @@ _new_vmalloc_kernel_address:
>>       /* Check the value of new_vmalloc for this cpu */
>>       REG_L    a2, 0(a0)
>>       and    a2, a2, a1
>> -    beq    a2, zero, _new_vmalloc_restore_context
>> +    beq    a2, zero, .Lnew_vmalloc_restore_context
>>         /* Atomically reset the current cpu bit in new_vmalloc */
>>       amoxor.d    a0, a1, (a0)
>> @@ -83,11 +83,11 @@ _new_vmalloc_kernel_address:
>>       csrw    CSR_SCRATCH, x0
>>       sret
>>   -_new_vmalloc_restore_context:
>> +.Lnew_vmalloc_restore_context:
>>       REG_L     a2, TASK_TI_A2(tp)
>> -_new_vmalloc_restore_context_a1:
>> +.Lnew_vmalloc_restore_context_a1:
>>       REG_L     a1, TASK_TI_A1(tp)
>> -_new_vmalloc_restore_context_a0:
>> +.Lnew_vmalloc_restore_context_a0:
>>       REG_L    a0, TASK_TI_A0(tp)
>>   .endm
>>   
> 
> 
> You can add:
> 
> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> 
> Thanks,
> 
> Alex
>

Re: [PATCH] riscv: use local label names instead of global ones in assembly

Posted by Alexandre Ghiti 1 year, 1 month ago

On Mon, Jan 6, 2025 at 10:36 AM Clément Léger <cleger@rivosinc.com> wrote:
>
>
>
> On 06/01/2025 09:52, Alexandre Ghiti wrote:
> > Hi Clément,
> >
> > On 03/01/2025 15:17, Clément Léger wrote:
> >> Local labels should be prefix by '.L' or they'll be exported in the
> >> symbol table. Additionally, this messes up the backtrace by displaying
> >> an incorrect symbol:
> >>
> >>    ...
> >>    [   12.751810] [<ffffffff80441628>] _copy_from_user+0x28/0xc2
> >>    [   12.752035] [<ffffffff800152ca>] handle_misaligned_load+0x1ca/0x2fc
> >>    [   12.752310] [<ffffffff80a033e8>] do_trap_load_misaligned+0x24/0xee
> >>    [   12.752596] [<ffffffff80a0dcae>]
> >> _new_vmalloc_restore_context_a0+0xc2/0xce
> >
> >
> > Yes, I noticed this last week, thanks for looking into it. I would add a
> > Fixes tag and merge that in the next -rc if possible as the above
> > backtrace is "disturbing".
>
> Hi Alex,
>
> Yeah, I thought the Fixes tag, but since it wasn't really "breaking"
> anything, I left it out. I'll add it and sent a V2.

No need to send a v2, b4 will pick the Fixes tag:

Fixes: 503638e0babf3 ("riscv: Stop emitting preventive sfence.vma for
new vmalloc mappings")

Thanks,

Alex

>
> Thanks
>
> Clément
>
> >
> >
> >>
> >> After:
> >>    ...
> >>    [   10.243916] [<ffffffff804415e4>] _copy_from_user+0x28/0xc2
> >>    [   10.244026] [<ffffffff800152ca>] handle_misaligned_load+0x1ca/0x2fc
> >>    [   10.244150] [<ffffffff80a033a0>] do_trap_load_misaligned+0x24/0xee
> >>    [   10.244268] [<ffffffff80a0dc66>] handle_exception+0x146/0x152
> >>
> >> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> >>
> >> ---
> >>   arch/riscv/kernel/entry.S | 20 ++++++++++----------
> >>   1 file changed, 10 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> >> index c200d329d4bd..216581835eb0 100644
> >> --- a/arch/riscv/kernel/entry.S
> >> +++ b/arch/riscv/kernel/entry.S
> >> @@ -23,21 +23,21 @@
> >>       REG_S     a0, TASK_TI_A0(tp)
> >>       csrr     a0, CSR_CAUSE
> >>       /* Exclude IRQs */
> >> -    blt      a0, zero, _new_vmalloc_restore_context_a0
> >> +    blt      a0, zero, .Lnew_vmalloc_restore_context_a0
> >>         REG_S     a1, TASK_TI_A1(tp)
> >>       /* Only check new_vmalloc if we are in page/protection fault */
> >>       li       a1, EXC_LOAD_PAGE_FAULT
> >> -    beq      a0, a1, _new_vmalloc_kernel_address
> >> +    beq      a0, a1, .Lnew_vmalloc_kernel_address
> >>       li       a1, EXC_STORE_PAGE_FAULT
> >> -    beq      a0, a1, _new_vmalloc_kernel_address
> >> +    beq      a0, a1, .Lnew_vmalloc_kernel_address
> >>       li       a1, EXC_INST_PAGE_FAULT
> >> -    bne      a0, a1, _new_vmalloc_restore_context_a1
> >> +    bne      a0, a1, .Lnew_vmalloc_restore_context_a1
> >>   -_new_vmalloc_kernel_address:
> >> +.Lnew_vmalloc_kernel_address:
> >>       /* Is it a kernel address? */
> >>       csrr     a0, CSR_TVAL
> >> -    bge     a0, zero, _new_vmalloc_restore_context_a1
> >> +    bge     a0, zero, .Lnew_vmalloc_restore_context_a1
> >>         /* Check if a new vmalloc mapping appeared that could explain
> >> the trap */
> >>       REG_S    a2, TASK_TI_A2(tp)
> >> @@ -69,7 +69,7 @@ _new_vmalloc_kernel_address:
> >>       /* Check the value of new_vmalloc for this cpu */
> >>       REG_L    a2, 0(a0)
> >>       and    a2, a2, a1
> >> -    beq    a2, zero, _new_vmalloc_restore_context
> >> +    beq    a2, zero, .Lnew_vmalloc_restore_context
> >>         /* Atomically reset the current cpu bit in new_vmalloc */
> >>       amoxor.d    a0, a1, (a0)
> >> @@ -83,11 +83,11 @@ _new_vmalloc_kernel_address:
> >>       csrw    CSR_SCRATCH, x0
> >>       sret
> >>   -_new_vmalloc_restore_context:
> >> +.Lnew_vmalloc_restore_context:
> >>       REG_L     a2, TASK_TI_A2(tp)
> >> -_new_vmalloc_restore_context_a1:
> >> +.Lnew_vmalloc_restore_context_a1:
> >>       REG_L     a1, TASK_TI_A1(tp)
> >> -_new_vmalloc_restore_context_a0:
> >> +.Lnew_vmalloc_restore_context_a0:
> >>       REG_L    a0, TASK_TI_A0(tp)
> >>   .endm
> >>
> >
> >
> > You can add:
> >
> > Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> >
> > Thanks,
> >
> > Alex
> >
>