| Message ID | 20250103141814.508865-1-cleger@rivosinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: use local label names instead of global ones in assembly | expand |
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
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 >
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 > > >
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
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(-)