| Message ID | 20221115012917.1781185-1-debug@rivosinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] scripts/gdb: add lx_current support for riscv | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/patch_count | success | Link |
| conchuod/cover_letter | success | Single patches do not need cover letters |
| conchuod/tree_selection | success | Guessed tree name to be for-next |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/build_warn_rv64 | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | fail | ERROR: trailing whitespace |
| conchuod/source_inline | success | Was 0 now: 0 |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
On Mon, Nov 14, 2022 at 05:29:17PM -0800, Deepak Gupta wrote: > csr_sscratch CSR holds current task_struct address when hart is in user space. > trap handler on entry spills csr_sscratch into "tp" (x2) register and zeroes out > csr_sscratch CSR. trap handler on exit reloads "tp" with expected user mode value > and place current task_struct address again in csr_scratch CSR. > > This patch assumes "tp" is pointing to task_struct. If value in csr_scratch is numerically > greater than "tp" then it assumes csr_scratch is correct address of current task_struct. > This logic holds when > - hart is in user space, "tp" will be less than csr_scratch. > - hart is in kernel space but not in trap handler, "tp" will be more than csr_scratch. > - hart is executing trap handler > - "tp" is still pointing to user mode but csr_scratch contains ptr to task_struct. > numerically higher. > - "tp" is now pointing to task_struct but csr_scratch now contains either 0 or numerically > smaller value. When is (csr_scratch != 0 && csr_scratch < tp)? IIUC, it should always be zero or >= tp. > > Patch also adds new cached type "ulong" in scripts/gdb/linux/utils.py Please wrap commit message lines at ~74. > > Signed-off-by: Deepak Gupta <debug@rivosinc.com> > --- > scripts/gdb/linux/cpus.py | 15 +++++++++++++++ > scripts/gdb/linux/utils.py | 5 +++++ > 2 files changed, 20 insertions(+) > > diff --git a/scripts/gdb/linux/cpus.py b/scripts/gdb/linux/cpus.py > index 15fc4626d236..fd818d7896ce 100644 > --- a/scripts/gdb/linux/cpus.py > +++ b/scripts/gdb/linux/cpus.py > @@ -173,6 +173,21 @@ def get_current_task(cpu): > else: > raise gdb.GdbError("Sorry, obtaining the current task is not allowed " > "while running in userspace(EL0)") > + elif utils.is_target_arch("riscv"): > + current_tp = gdb.parse_and_eval("$tp") > + scratch_reg = gdb.parse_and_eval("$sscratch") > + > + # by default tp points to current task > + current_task = current_tp.cast(task_ptr_type) > + > + # scratch register is set 0 in trap handler after entering kernel. > + # When hart is in user mode, scratch register is pointing to task_struct. > + # So when scratch register holds higher value (negative address as ulong is bigger value) than tp, Please wrap source lines at 100. > + # then use scratch register > + if (scratch_reg.cast(utils.get_ulong_type()) > current_tp.cast(utils.get_ulong_type())): ^ extra space here > + current_task = scratch_reg.cast(task_ptr_type) > + > + return current_task.dereference() > else: > raise gdb.GdbError("Sorry, obtaining the current task is not yet " > "supported with this arch") > diff --git a/scripts/gdb/linux/utils.py b/scripts/gdb/linux/utils.py > index 1553f68716cc..ddaf3089170d 100644 > --- a/scripts/gdb/linux/utils.py > +++ b/scripts/gdb/linux/utils.py > @@ -35,12 +35,17 @@ class CachedType: > > > long_type = CachedType("long") > +ulong_type = CachedType("ulong") > atomic_long_type = CachedType("atomic_long_t") > > def get_long_type(): > global long_type > return long_type.get_type() > > +def get_ulong_type(): > + global ulong_type > + return ulong_type.get_type() > + > def offset_of(typeobj, field): > element = gdb.Value(0).cast(typeobj) > return int(str(element[field].address).split()[0], 16) > -- > 2.25.1 > Besides the line wrapping and the commit message clarification this looks good to me. Reviewed-by: Andrew Jones <ajones@ventanamicro.com> Thanks, drew
Hey Deepak, On Tue, Nov 15, 2022 at 12:49:23AM -0800, Deepak Gupta wrote: > csr_sscratch CSR holds current task_struct address when hart is in > user space. Trap handler on entry spills csr_sscratch into "tp" (x2) > register and zeroes out csr_sscratch CSR. Trap handler on exit reloads > "tp" with expected user mode value and place current task_struct address > again in csr_scratch CSR. > > This patch assumes "tp" is pointing to task_struct. If value in > csr_scratch is numerically greater than "tp" then it assumes csr_scratch nit: s/scratch/sscratch/ ? > is correct address of current task_struct. This logic holds when > - hart is in user space, "tp" will be less than csr_scratch. > - hart is in kernel space but not in trap handler, "tp" will be more > than csr_scratch (csr_scratch being equal to 0). > - hart is executing trap handler > - "tp" is still pointing to user mode but csr_scratch contains > ptr to task_struct. Thus numerically higher. > - "tp" is pointing to task_struct but csr_scratch now contains > either 0 or numerically smaller value (transiently holds > user mode tp) > > Patch also adds new cached type "ulong" in scripts/gdb/linux/utils.py > > Signed-off-by: Deepak Gupta <debug@rivosinc.com> I noticed when looking into patchwork complaining about checkpatch errors in v2, that b4 had actually downloaded v3 but I could not see this patch on the RISC-V list. I don't see a changelog anywhere here from v2 either, nor did you pick up Drew's Reviewed-by. What's the story there? One really minor thing below. Should be able to fix it up trivially up & submit a v4, CCing the linux-riscv list. > --- > scripts/gdb/linux/cpus.py | 15 +++++++++++++++ > scripts/gdb/linux/utils.py | 5 +++++ > 2 files changed, 20 insertions(+) > > diff --git a/scripts/gdb/linux/cpus.py b/scripts/gdb/linux/cpus.py > index 15fc4626d236..ca5215a660c7 100644 > --- a/scripts/gdb/linux/cpus.py > +++ b/scripts/gdb/linux/cpus.py > @@ -173,6 +173,21 @@ def get_current_task(cpu): > else: > raise gdb.GdbError("Sorry, obtaining the current task is not allowed " > "while running in userspace(EL0)") > + elif utils.is_target_arch("riscv"): > + current_tp = gdb.parse_and_eval("$tp") > + scratch_reg = gdb.parse_and_eval("$sscratch") > + > + # by default tp points to current task > + current_task = current_tp.cast(task_ptr_type) > + > + # scratch register is set 0 in trap handler after entering kernel. > + # When hart is in user mode, scratch register is pointing to task_struct. > + # and tp is used by user mode. So when scratch register holds larger value > + # (negative address as ulong is larger value) than tp, then use scratch register. > + if (scratch_reg.cast(utils.get_ulong_type()) > current_tp.cast(utils.get_ulong_type())): ^^ extra space here? > + current_task = scratch_reg.cast(task_ptr_type) > + > + return current_task.dereference() > else: > raise gdb.GdbError("Sorry, obtaining the current task is not yet " > "supported with this arch") > diff --git a/scripts/gdb/linux/utils.py b/scripts/gdb/linux/utils.py > index 1553f68716cc..ddaf3089170d 100644 > --- a/scripts/gdb/linux/utils.py > +++ b/scripts/gdb/linux/utils.py > @@ -35,12 +35,17 @@ class CachedType: > > > long_type = CachedType("long") > +ulong_type = CachedType("ulong") > atomic_long_type = CachedType("atomic_long_t") > > def get_long_type(): > global long_type > return long_type.get_type() > > +def get_ulong_type(): > + global ulong_type > + return ulong_type.get_type() > + > def offset_of(typeobj, field): > element = gdb.Value(0).cast(typeobj) > return int(str(element[field].address).split()[0], 16) > -- > 2.25.1
Hey Deepak, On 15/11/2022 17:49, Deepak Gupta wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > Since I am new to all this. I've had some oversight and am still learning the flow. > Rest inline. No worries chief. Worth noting is that this mail came in html form, which the mailing lists reject. Noone outside of the direct CC list will see this mail. May be worth asking some of the other Rivos lads how they do their plain text emailing. ik Palmer's got his hand rolled stuff, so maybe he's not the best to ask - but try Bjorn or Atish? > > On Tue, Nov 15, 2022 at 6:38 AM Conor Dooley <conor.dooley@microchip.com <mailto:conor.dooley@microchip.com>> wrote: > > Hey Deepak, > > On Tue, Nov 15, 2022 at 12:49:23AM -0800, Deepak Gupta wrote: > > csr_sscratch CSR holds current task_struct address when hart is in > > user space. Trap handler on entry spills csr_sscratch into "tp" (x2) > > register and zeroes out csr_sscratch CSR. Trap handler on exit reloads > > "tp" with expected user mode value and place current task_struct address > > again in csr_scratch CSR. > > > > This patch assumes "tp" is pointing to task_struct. If value in > > csr_scratch is numerically greater than "tp" then it assumes csr_scratch > > nit: s/scratch/sscratch/ ? > > > Will fix it. > > > > > is correct address of current task_struct. This logic holds when > > - hart is in user space, "tp" will be less than csr_scratch. > > - hart is in kernel space but not in trap handler, "tp" will be more > > than csr_scratch (csr_scratch being equal to 0). > > - hart is executing trap handler > > - "tp" is still pointing to user mode but csr_scratch contains > > ptr to task_struct. Thus numerically higher. > > - "tp" is pointing to task_struct but csr_scratch now contains > > either 0 or numerically smaller value (transiently holds > > user mode tp) > > > > Patch also adds new cached type "ulong" in scripts/gdb/linux/utils.py > > > > Signed-off-by: Deepak Gupta <debug@rivosinc.com <mailto:debug@rivosinc.com>> > > I noticed when looking into patchwork complaining about checkpatch > errors in v2, that b4 had actually downloaded v3 but I could not see > this patch on the RISC-V list. > > > I'll make sure to add the risc-v list on the next spin up. > > > I don't see a changelog anywhere here > from v2 either > > > I had been taking inputs and squashing commits on my end. > You want me to send a changelog of changes between versions of patches. Yeah, it's nice to say something like: v2 -> v3: - reworded commit message - fixed compile error in bar.c if !CONFIG_FOO Makes it easier for reviewers to see what changed between versions. > > > , nor did you pick up Drew's Reviewed-by. > > > I should've done that. My mistake and apologize. > I'll fix it in my next submission. > > > > What's the story there? > > One really minor thing below. Should be able to fix it up trivially up > & submit a v4, CCing the linux-riscv list. > > > --- > > scripts/gdb/linux/cpus.py | 15 +++++++++++++++ > > scripts/gdb/linux/utils.py | 5 +++++ > > 2 files changed, 20 insertions(+) > > > > diff --git a/scripts/gdb/linux/cpus.py b/scripts/gdb/linux/cpus.py > > index 15fc4626d236..ca5215a660c7 100644 > > --- a/scripts/gdb/linux/cpus.py > > +++ b/scripts/gdb/linux/cpus.py > > @@ -173,6 +173,21 @@ def get_current_task(cpu): > > else: > > raise gdb.GdbError("Sorry, obtaining the current task is not allowed " > > "while running in userspace(EL0)") > > + elif utils.is_target_arch("riscv"): > > + current_tp = gdb.parse_and_eval("$tp") > > + scratch_reg = gdb.parse_and_eval("$sscratch") > > + > > + # by default tp points to current task > > + current_task = current_tp.cast(task_ptr_type) > > + > > + # scratch register is set 0 in trap handler after entering kernel. > > + # When hart is in user mode, scratch register is pointing to task_struct. > > + # and tp is used by user mode. So when scratch register holds larger value > > + # (negative address as ulong is larger value) than tp, then use scratch register. > > + if (scratch_reg.cast(utils.get_ulong_type()) > current_tp.cast(utils.get_ulong_type())): > ^^ > extra space here? > > > I don't see the space in the patch. Can you clarify which space you're talking about here? There's a double space between the > and current_tp. I put a ^^ under it, but if you've not got a monospace font, which since you're replying in html you probably don't, it may not align for you. Hope that helps, Conor. > > > > > > + current_task = scratch_reg.cast(task_ptr_type) > > + > > + return current_task.dereference() > > else: > > raise gdb.GdbError("Sorry, obtaining the current task is not yet " > > "supported with this arch") > > diff --git a/scripts/gdb/linux/utils.py b/scripts/gdb/linux/utils.py > > index 1553f68716cc..ddaf3089170d 100644 > > --- a/scripts/gdb/linux/utils.py > > +++ b/scripts/gdb/linux/utils.py > > @@ -35,12 +35,17 @@ class CachedType: > > > > > > long_type = CachedType("long") > > +ulong_type = CachedType("ulong") > > atomic_long_type = CachedType("atomic_long_t") > > > > def get_long_type(): > > global long_type > > return long_type.get_type() > > > > +def get_ulong_type(): > > + global ulong_type > > + return ulong_type.get_type() > > + > > def offset_of(typeobj, field): > > element = gdb.Value(0).cast(typeobj) > > return int(str(element[field].address).split()[0], 16) > > -- > > 2.25.1 >
On Tue, Nov 15, 2022 at 09:49:10AM -0800, Deepak Gupta wrote: ... > On Tue, Nov 15, 2022 at 6:38 AM Conor Dooley <conor.dooley@microchip.com> > wrote: > > > Hey Deepak, > > > > On Tue, Nov 15, 2022 at 12:49:23AM -0800, Deepak Gupta wrote: ... > > > + if (scratch_reg.cast(utils.get_ulong_type()) > > > current_tp.cast(utils.get_ulong_type())): > > ^^ > > extra space here? > > > I don't see the space in the patch. Can you clarify which space you're > talking about here? The same one I pointed out in v2. The one after the greater-than sign. Is your editor not using a monospaced font? Thanks, drew
On Tue, Nov 15, 2022 at 06:06:34PM +0000, Conor.Dooley@microchip.com wrote: >Hey Deepak, > >On 15/11/2022 17:49, Deepak Gupta wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> Since I am new to all this. I've had some oversight and am still learning the flow. >> Rest inline. > >No worries chief. Worth noting is that this mail came in html >form, which the mailing lists reject. Noone outside of the >direct CC list will see this mail. May be worth asking some >of the other Rivos lads how they do their plain text emailing. > >ik Palmer's got his hand rolled stuff, so maybe he's not the >best to ask - but try Bjorn or Atish? Sending this time from mutt. Hopefully no bounces. > >> >> On Tue, Nov 15, 2022 at 6:38 AM Conor Dooley <conor.dooley@microchip.com <mailto:conor.dooley@microchip.com>> wrote: >> >> Hey Deepak, >> >> On Tue, Nov 15, 2022 at 12:49:23AM -0800, Deepak Gupta wrote: >> > csr_sscratch CSR holds current task_struct address when hart is in >> > user space. Trap handler on entry spills csr_sscratch into "tp" (x2) >> > register and zeroes out csr_sscratch CSR. Trap handler on exit reloads >> > "tp" with expected user mode value and place current task_struct address >> > again in csr_scratch CSR. >> > >> > This patch assumes "tp" is pointing to task_struct. If value in >> > csr_scratch is numerically greater than "tp" then it assumes csr_scratch >> >> nit: s/scratch/sscratch/ ? >> >> >> Will fix it. >> >> >> >> > is correct address of current task_struct. This logic holds when >> > - hart is in user space, "tp" will be less than csr_scratch. >> > - hart is in kernel space but not in trap handler, "tp" will be more >> > than csr_scratch (csr_scratch being equal to 0). >> > - hart is executing trap handler >> > - "tp" is still pointing to user mode but csr_scratch contains >> > ptr to task_struct. Thus numerically higher. >> > - "tp" is pointing to task_struct but csr_scratch now contains >> > either 0 or numerically smaller value (transiently holds >> > user mode tp) >> > >> > Patch also adds new cached type "ulong" in scripts/gdb/linux/utils.py >> > >> > Signed-off-by: Deepak Gupta <debug@rivosinc.com <mailto:debug@rivosinc.com>> >> >> I noticed when looking into patchwork complaining about checkpatch >> errors in v2, that b4 had actually downloaded v3 but I could not see >> this patch on the RISC-V list. >> >> >> I'll make sure to add the risc-v list on the next spin up. >> >> >> I don't see a changelog anywhere here >> from v2 either >> >> >> I had been taking inputs and squashing commits on my end. >> You want me to send a changelog of changes between versions of patches. > >Yeah, it's nice to say something like: >v2 -> v3: >- reworded commit message >- fixed compile error in bar.c if !CONFIG_FOO > >Makes it easier for reviewers to see what changed between >versions. > >> >> >> , nor did you pick up Drew's Reviewed-by. >> >> >> I should've done that. My mistake and apologize. >> I'll fix it in my next submission. >> >> >> >> What's the story there? >> >> One really minor thing below. Should be able to fix it up trivially up >> & submit a v4, CCing the linux-riscv list. >> >> > --- >> > scripts/gdb/linux/cpus.py | 15 +++++++++++++++ >> > scripts/gdb/linux/utils.py | 5 +++++ >> > 2 files changed, 20 insertions(+) >> > >> > diff --git a/scripts/gdb/linux/cpus.py b/scripts/gdb/linux/cpus.py >> > index 15fc4626d236..ca5215a660c7 100644 >> > --- a/scripts/gdb/linux/cpus.py >> > +++ b/scripts/gdb/linux/cpus.py >> > @@ -173,6 +173,21 @@ def get_current_task(cpu): >> > else: >> > raise gdb.GdbError("Sorry, obtaining the current task is not allowed " >> > "while running in userspace(EL0)") >> > + elif utils.is_target_arch("riscv"): >> > + current_tp = gdb.parse_and_eval("$tp") >> > + scratch_reg = gdb.parse_and_eval("$sscratch") >> > + >> > + # by default tp points to current task >> > + current_task = current_tp.cast(task_ptr_type) >> > + >> > + # scratch register is set 0 in trap handler after entering kernel. >> > + # When hart is in user mode, scratch register is pointing to task_struct. >> > + # and tp is used by user mode. So when scratch register holds larger value >> > + # (negative address as ulong is larger value) than tp, then use scratch register. >> > + if (scratch_reg.cast(utils.get_ulong_type()) > current_tp.cast(utils.get_ulong_type())): >> ^^ >> extra space here? >> >> >> I don't see the space in the patch. Can you clarify which space you're talking about here? > >There's a double space between the > and current_tp. >I put a ^^ under it, but if you've not got a monospace font, which since >you're replying in html you probably don't, it may not align for you. > >Hope that helps, >Conor. Yes can see it now. > >> >> >> >> >> > + current_task = scratch_reg.cast(task_ptr_type) >> > + >> > + return current_task.dereference() >> > else: >> > raise gdb.GdbError("Sorry, obtaining the current task is not yet " >> > "supported with this arch") >> > diff --git a/scripts/gdb/linux/utils.py b/scripts/gdb/linux/utils.py >> > index 1553f68716cc..ddaf3089170d 100644 >> > --- a/scripts/gdb/linux/utils.py >> > +++ b/scripts/gdb/linux/utils.py >> > @@ -35,12 +35,17 @@ class CachedType: >> > >> > >> > long_type = CachedType("long") >> > +ulong_type = CachedType("ulong") >> > atomic_long_type = CachedType("atomic_long_t") >> > >> > def get_long_type(): >> > global long_type >> > return long_type.get_type() >> > >> > +def get_ulong_type(): >> > + global ulong_type >> > + return ulong_type.get_type() >> > + >> > def offset_of(typeobj, field): >> > element = gdb.Value(0).cast(typeobj) >> > return int(str(element[field].address).split()[0], 16) >> > -- >> > 2.25.1 >> >
diff --git a/scripts/gdb/linux/cpus.py b/scripts/gdb/linux/cpus.py index 15fc4626d236..fd818d7896ce 100644 --- a/scripts/gdb/linux/cpus.py +++ b/scripts/gdb/linux/cpus.py @@ -173,6 +173,21 @@ def get_current_task(cpu): else: raise gdb.GdbError("Sorry, obtaining the current task is not allowed " "while running in userspace(EL0)") + elif utils.is_target_arch("riscv"): + current_tp = gdb.parse_and_eval("$tp") + scratch_reg = gdb.parse_and_eval("$sscratch") + + # by default tp points to current task + current_task = current_tp.cast(task_ptr_type) + + # scratch register is set 0 in trap handler after entering kernel. + # When hart is in user mode, scratch register is pointing to task_struct. + # So when scratch register holds higher value (negative address as ulong is bigger value) than tp, + # then use scratch register + if (scratch_reg.cast(utils.get_ulong_type()) > current_tp.cast(utils.get_ulong_type())): + current_task = scratch_reg.cast(task_ptr_type) + + return current_task.dereference() else: raise gdb.GdbError("Sorry, obtaining the current task is not yet " "supported with this arch") diff --git a/scripts/gdb/linux/utils.py b/scripts/gdb/linux/utils.py index 1553f68716cc..ddaf3089170d 100644 --- a/scripts/gdb/linux/utils.py +++ b/scripts/gdb/linux/utils.py @@ -35,12 +35,17 @@ class CachedType: long_type = CachedType("long") +ulong_type = CachedType("ulong") atomic_long_type = CachedType("atomic_long_t") def get_long_type(): global long_type return long_type.get_type() +def get_ulong_type(): + global ulong_type + return ulong_type.get_type() + def offset_of(typeobj, field): element = gdb.Value(0).cast(typeobj) return int(str(element[field].address).split()[0], 16)
csr_sscratch CSR holds current task_struct address when hart is in user space. trap handler on entry spills csr_sscratch into "tp" (x2) register and zeroes out csr_sscratch CSR. trap handler on exit reloads "tp" with expected user mode value and place current task_struct address again in csr_scratch CSR. This patch assumes "tp" is pointing to task_struct. If value in csr_scratch is numerically greater than "tp" then it assumes csr_scratch is correct address of current task_struct. This logic holds when - hart is in user space, "tp" will be less than csr_scratch. - hart is in kernel space but not in trap handler, "tp" will be more than csr_scratch. - hart is executing trap handler - "tp" is still pointing to user mode but csr_scratch contains ptr to task_struct. numerically higher. - "tp" is now pointing to task_struct but csr_scratch now contains either 0 or numerically smaller value. Patch also adds new cached type "ulong" in scripts/gdb/linux/utils.py Signed-off-by: Deepak Gupta <debug@rivosinc.com> --- scripts/gdb/linux/cpus.py | 15 +++++++++++++++ scripts/gdb/linux/utils.py | 5 +++++ 2 files changed, 20 insertions(+)