Updating python gdb command lx_current for riscv

Updating python gdb command lx_current for riscv

[debug]

scripts/gdb has a good deal of useful commands to locate critical kernel
data structures from gdb. lx_current misses support for locating current
task on this cpu. This patch fixes that.
It's a small patch. Please merge.

[PATCH] gdb-script: updated lx_current for riscv

[debug]

From: Deepak Gupta <debug@rivosinc.com>

lx_current python gdb command defined in scripts/gdb/cpus.py updated
to support riscv architecture.

Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
 scripts/gdb/linux/cpus.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/scripts/gdb/linux/cpus.py b/scripts/gdb/linux/cpus.py
index 15fc4626d236..ce6703f1e35a 100644
--- a/scripts/gdb/linux/cpus.py
+++ b/scripts/gdb/linux/cpus.py
@@ -173,6 +173,14 @@ 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_task_addr = gdb.parse_and_eval("$tp")
+         if((current_task_addr.cast(utils.get_long_type()) >> 63) != 0):
+             current_task = current_task_addr.cast(task_ptr_type)
+             return current_task.dereference()
+         else:
+             raise gdb.GdbError("Sorry, obtaining the current task is not allowed "
+                                "while running in userspace")
     else:
         raise gdb.GdbError("Sorry, obtaining the current task is not yet "
                            "supported with this arch")
-- 
2.25.1

Re: [PATCH] gdb-script: updated lx_current for riscv

[Conor Dooley]

On Fri, Nov 11, 2022 at 11:59:38AM -0800, debug@rivosinc.com wrote:
> From: Deepak Gupta <debug@rivosinc.com>
> 
> lx_current python gdb command defined in scripts/gdb/cpus.py updated
> to support riscv architecture.

The commit which added support for arm64 gave an explanation of why
SP_EL0 needed to be checked. Would be nice if you could do the same here
for RISC-V. See 526940e39626 ("scripts/gdb: add lx_current support for
arm64") for what I mean.

While you're at it, "scripts/gdb: add support for RISC-V" would appear
to be a more standard $subject for this file.

Thanks,
Conor.

> 
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
>  scripts/gdb/linux/cpus.py | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/scripts/gdb/linux/cpus.py b/scripts/gdb/linux/cpus.py
> index 15fc4626d236..ce6703f1e35a 100644
> --- a/scripts/gdb/linux/cpus.py
> +++ b/scripts/gdb/linux/cpus.py
> @@ -173,6 +173,14 @@ 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_task_addr = gdb.parse_and_eval("$tp")
> +         if((current_task_addr.cast(utils.get_long_type()) >> 63) != 0):
> +             current_task = current_task_addr.cast(task_ptr_type)
> +             return current_task.dereference()
> +         else:
> +             raise gdb.GdbError("Sorry, obtaining the current task is not allowed "
> +                                "while running in userspace")
>      else:
>          raise gdb.GdbError("Sorry, obtaining the current task is not yet "
>                             "supported with this arch")
> -- 
> 2.25.1
> 

Re: [PATCH] gdb-script: updated lx_current for riscv

[Andrew Jones]

On Fri, Nov 11, 2022 at 11:59:38AM -0800, debug@rivosinc.com wrote:
> From: Deepak Gupta <debug@rivosinc.com>
> 
> lx_current python gdb command defined in scripts/gdb/cpus.py updated
> to support riscv architecture.
> 
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
>  scripts/gdb/linux/cpus.py | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/scripts/gdb/linux/cpus.py b/scripts/gdb/linux/cpus.py
> index 15fc4626d236..ce6703f1e35a 100644
> --- a/scripts/gdb/linux/cpus.py
> +++ b/scripts/gdb/linux/cpus.py
> @@ -173,6 +173,14 @@ 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_task_addr = gdb.parse_and_eval("$tp")
> +         if((current_task_addr.cast(utils.get_long_type()) >> 63) != 0):

Shouldn't there be a get_long_type().sizeof == 8 check somewhere before
shifting by 63? Or are 32-bit targets not supported at all for some
reason?

Thanks,
drew

> +             current_task = current_task_addr.cast(task_ptr_type)
> +             return current_task.dereference()
> +         else:
> +             raise gdb.GdbError("Sorry, obtaining the current task is not allowed "
> +                                "while running in userspace")
>      else:
>          raise gdb.GdbError("Sorry, obtaining the current task is not yet "
>                             "supported with this arch")
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] gdb-script: updated lx_current for riscv

[Conor Dooley]

On Sun, Nov 13, 2022 at 12:13:40AM +0000, Conor Dooley wrote:
> On Fri, Nov 11, 2022 at 11:59:38AM -0800, debug@rivosinc.com wrote:
> > From: Deepak Gupta <debug@rivosinc.com>
> > 
> > lx_current python gdb command defined in scripts/gdb/cpus.py updated
> > to support riscv architecture.
> 
> The commit which added support for arm64 gave an explanation of why
> SP_EL0 needed to be checked. Would be nice if you could do the same here
> for RISC-V. See 526940e39626 ("scripts/gdb: add lx_current support for
> arm64") for what I mean.
> 
> While you're at it, "scripts/gdb: add support for RISC-V" would appear

Realised I omitted a word there, sorry: s/add support/add lx_current support/

> to be a more standard $subject for this file.
> 
> Thanks,
> Conor.
> 
> > 
> > Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> > ---
> >  scripts/gdb/linux/cpus.py | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/scripts/gdb/linux/cpus.py b/scripts/gdb/linux/cpus.py
> > index 15fc4626d236..ce6703f1e35a 100644
> > --- a/scripts/gdb/linux/cpus.py
> > +++ b/scripts/gdb/linux/cpus.py
> > @@ -173,6 +173,14 @@ 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_task_addr = gdb.parse_and_eval("$tp")
> > +         if((current_task_addr.cast(utils.get_long_type()) >> 63) != 0):
> > +             current_task = current_task_addr.cast(task_ptr_type)
> > +             return current_task.dereference()
> > +         else:
> > +             raise gdb.GdbError("Sorry, obtaining the current task is not allowed "
> > +                                "while running in userspace")
> >      else:
> >          raise gdb.GdbError("Sorry, obtaining the current task is not yet "
> >                             "supported with this arch")
> > -- 
> > 2.25.1
> > 

[PATCH v2] scripts/gdb: add lx_current support for riscv

[Deepak Gupta]

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(+)

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)
-- 
2.25.1

Re: [PATCH v2] scripts/gdb: add lx_current support for riscv

[Andrew Jones]

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

[PATCH v3] scripts/gdb: add lx_current support for riscv

[Deepak Gupta]

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 (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>
---
 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())):
+             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

Re: [PATCH v3] scripts/gdb: add lx_current support for riscv

[Conor Dooley]

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

Re: [PATCH v3] scripts/gdb: add lx_current support for riscv

[Conor.Dooley]

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
> 

Re: [PATCH v3] scripts/gdb: add lx_current support for riscv

[Andrew Jones]

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

Re: [PATCH v3] scripts/gdb: add lx_current support for riscv

[Deepak Gupta]

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
>>
>

[PATCH v4] scripts/gdb: add lx_current support for riscv

[Deepak Gupta]

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_sscratch CSR.

This patch assumes "tp" is pointing to task_struct. If value in
csr_sscratch is numerically greater than "tp" then it assumes csr_sscratch
is correct address of current task_struct. This logic holds when
   - hart is in user space, "tp" will be less than csr_sscratch.
   - hart is in kernel space but not in trap handler, "tp" will be more
     than csr_sscratch (csr_sscratch being equal to 0).
   - hart is executing trap handler
       - "tp" is still pointing to user mode but csr_sscratch contains
          ptr to task_struct. Thus numerically higher.
       - "tp" is  pointing to task_struct but csr_sscratch 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

Since patch has changed a little bit from v1 and I didn't include
changelog earlier, here it is.

---
v1 --> v2:
 - added logic to locate task_struct irrespective of priv
 - made locating task_struct agnostic to bitness(32 vs 64).
 - added caching of ulong type in scripts/gdb/linux/utils.py
 - added more descriptive commit message

v2 --> v3:
 - amended commit message and source line to fit column width

v3 --> v4:
 - amended commit message and remove whitespace in source
 - added Reviewed-by for reviewers
---

Signed-off-by: Deepak Gupta <debug@rivosinc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.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..14c22f82449b 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())):
+             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

Re: [PATCH v4] scripts/gdb: add lx_current support for riscv

[Conor.Dooley]

Hey Deepak,

On 15/11/2022 20:40, Deepak Gupta wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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_sscratch CSR.
> 
> This patch assumes "tp" is pointing to task_struct. If value in
> csr_sscratch is numerically greater than "tp" then it assumes csr_sscratch
> is correct address of current task_struct. This logic holds when
>    - hart is in user space, "tp" will be less than csr_sscratch.
>    - hart is in kernel space but not in trap handler, "tp" will be more
>      than csr_sscratch (csr_sscratch being equal to 0).
>    - hart is executing trap handler
>        - "tp" is still pointing to user mode but csr_sscratch contains
>           ptr to task_struct. Thus numerically higher.
>        - "tp" is  pointing to task_struct but csr_sscratch 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
> 

> Since patch has changed a little bit from v1 and I didn't include
> changelog earlier, here it is.

This bit here needs to go below the --- line as it should not end up
in the commit logs (as everything below the --- is ignored.

> 
> ---
> v1 --> v2:
>  - added logic to locate task_struct irrespective of priv
>  - made locating task_struct agnostic to bitness(32 vs 64).
>  - added caching of ulong type in scripts/gdb/linux/utils.py
>  - added more descriptive commit message
> 
> v2 --> v3:
>  - amended commit message and source line to fit column width
> 
> v3 --> v4:
>  - amended commit message and remove whitespace in source
>  - added Reviewed-by for reviewers
> ---
> 
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

And this bit needs to go above the first --- line as it does need
to go into the commit logs.

The order is:
$subject

body

tags
---
changelog & comments

diffs

For example, this is what I see when I apply the patch:

$ b4 shazam 20221115204003.1866421-1-debug@rivosinc.com
Grabbing thread from lore.kernel.org/all/20221115204003.1866421-1-debug%40rivosinc.com/t.mbox.gz
Checking for newer revisions on https://lore.kernel.org/all/
Analyzing 13 messages in the thread
Will use the latest revision: v4
You can pick other revisions using the -vN flag
Checking attestation on all messages, may take a moment...
---
  ✓ [PATCH v4] scripts/gdb: add lx_current support for riscv
  ---
  ✓ Signed: DKIM/rivosinc-com.20210112.gappssmtp.com (From: debug@rivosinc.com)
---
Total patches: 1
---
Applying: scripts/gdb: add lx_current support for riscv

$ git show
commit 5c93617c68980f767e312fc51849d78093f56e72 (HEAD)
Author: Deepak Gupta <debug@rivosinc.com>
Date:   Tue Nov 15 12:40:03 2022 -0800

    scripts/gdb: add lx_current support for riscv
    
    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_sscratch CSR.
    
    This patch assumes "tp" is pointing to task_struct. If value in
    csr_sscratch is numerically greater than "tp" then it assumes csr_sscratch
    is correct address of current task_struct. This logic holds when
       - hart is in user space, "tp" will be less than csr_sscratch.
       - hart is in kernel space but not in trap handler, "tp" will be more
         than csr_sscratch (csr_sscratch being equal to 0).
       - hart is executing trap handler
           - "tp" is still pointing to user mode but csr_sscratch contains
              ptr to task_struct. Thus numerically higher.
           - "tp" is  pointing to task_struct but csr_sscratch 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
    
    Since patch has changed a little bit from v1 and I didn't include
    changelog earlier, here it is.

Taken a wee bit of the back and forth, Looks like you're nearly
there though!

Conor.

[PATCH v5] scripts/gdb: add lx_current support for riscv

[Deepak Gupta]

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_sscratch CSR.

This patch assumes "tp" is pointing to task_struct. If value in
csr_sscratch is numerically greater than "tp" then it assumes csr_sscratch
is correct address of current task_struct. This logic holds when
   - hart is in user space, "tp" will be less than csr_sscratch.
   - hart is in kernel space but not in trap handler, "tp" will be more
     than csr_sscratch (csr_sscratch being equal to 0).
   - hart is executing trap handler
       - "tp" is still pointing to user mode but csr_sscratch contains
          ptr to task_struct. Thus numerically higher.
       - "tp" is  pointing to task_struct but csr_sscratch 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>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

---
Since patch has changed a little bit from v1 and I didn't include
changelog earlier, here it is.

v1 --> v2:
 - added logic to locate task_struct irrespective of priv
 - made locating task_struct agnostic to bitness(32 vs 64).
 - added caching of ulong type in scripts/gdb/linux/utils.py
 - added more descriptive commit message

v2 --> v3:
 - amended commit message and source line to fit column width

v3 --> v4:
 - amended commit message and remove whitespace in source
 - added Reviewed-by for reviewers

v4 --> v5:
 - changing the order of changelog and sign off/review tags in commit
---
---
 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..14c22f82449b 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())):
+             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

Re: [PATCH v5] scripts/gdb: add lx_current support for riscv

[Andrew Jones]

On Tue, Nov 15, 2022 at 02:10:51PM -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_sscratch CSR.
> 
> This patch assumes "tp" is pointing to task_struct. If value in
> csr_sscratch is numerically greater than "tp" then it assumes csr_sscratch
> is correct address of current task_struct. This logic holds when
>    - hart is in user space, "tp" will be less than csr_sscratch.
>    - hart is in kernel space but not in trap handler, "tp" will be more
>      than csr_sscratch (csr_sscratch being equal to 0).
>    - hart is executing trap handler
>        - "tp" is still pointing to user mode but csr_sscratch contains
>           ptr to task_struct. Thus numerically higher.
>        - "tp" is  pointing to task_struct but csr_sscratch 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>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> 
> ---
> Since patch has changed a little bit from v1 and I didn't include
> changelog earlier, here it is.
> 
> v1 --> v2:
>  - added logic to locate task_struct irrespective of priv
>  - made locating task_struct agnostic to bitness(32 vs 64).
>  - added caching of ulong type in scripts/gdb/linux/utils.py
>  - added more descriptive commit message
> 
> v2 --> v3:
>  - amended commit message and source line to fit column width
> 
> v3 --> v4:
>  - amended commit message and remove whitespace in source
>  - added Reviewed-by for reviewers
> 
> v4 --> v5:
>  - changing the order of changelog and sign off/review tags in commit
> ---
> ---

Everything looks good, but you've got extra ---'s here. They don't hurt,
but if you're still ironing out your workflow you may want to keep in
mind that you don't need them. You only need one, which goes above the
changelog.

Thanks,
drew

>  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..14c22f82449b 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())):
> +             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
> 

Re: [PATCH v5] scripts/gdb: add lx_current support for riscv

[Deepak Gupta]

On Wed, Nov 16, 2022 at 09:16:49AM +0100, Andrew Jones wrote:
>On Tue, Nov 15, 2022 at 02:10:51PM -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_sscratch CSR.
>>
>> This patch assumes "tp" is pointing to task_struct. If value in
>> csr_sscratch is numerically greater than "tp" then it assumes csr_sscratch
>> is correct address of current task_struct. This logic holds when
>>    - hart is in user space, "tp" will be less than csr_sscratch.
>>    - hart is in kernel space but not in trap handler, "tp" will be more
>>      than csr_sscratch (csr_sscratch being equal to 0).
>>    - hart is executing trap handler
>>        - "tp" is still pointing to user mode but csr_sscratch contains
>>           ptr to task_struct. Thus numerically higher.
>>        - "tp" is  pointing to task_struct but csr_sscratch 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>
>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>>
>> ---
>> Since patch has changed a little bit from v1 and I didn't include
>> changelog earlier, here it is.
>>
>> v1 --> v2:
>>  - added logic to locate task_struct irrespective of priv
>>  - made locating task_struct agnostic to bitness(32 vs 64).
>>  - added caching of ulong type in scripts/gdb/linux/utils.py
>>  - added more descriptive commit message
>>
>> v2 --> v3:
>>  - amended commit message and source line to fit column width
>>
>> v3 --> v4:
>>  - amended commit message and remove whitespace in source
>>  - added Reviewed-by for reviewers
>>
>> v4 --> v5:
>>  - changing the order of changelog and sign off/review tags in commit
>> ---
>> ---
>
>Everything looks good, but you've got extra ---'s here. They don't hurt,
>but if you're still ironing out your workflow you may want to keep in
>mind that you don't need them. You only need one, which goes above the
>changelog.
>
>Thanks,
>drew
>

Noted. Thanks.

>>  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..14c22f82449b 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())):
>> +             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
>>

Re: [PATCH v5] scripts/gdb: add lx_current support for riscv

[Palmer Dabbelt]

On Tue, 15 Nov 2022 14:10:51 PST (-0800), debug@rivosinc.com 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_sscratch CSR.
>
> This patch assumes "tp" is pointing to task_struct. If value in
> csr_sscratch is numerically greater than "tp" then it assumes csr_sscratch
> is correct address of current task_struct. This logic holds when
>    - hart is in user space, "tp" will be less than csr_sscratch.
>    - hart is in kernel space but not in trap handler, "tp" will be more
>      than csr_sscratch (csr_sscratch being equal to 0).
>    - hart is executing trap handler
>        - "tp" is still pointing to user mode but csr_sscratch contains
>           ptr to task_struct. Thus numerically higher.
>        - "tp" is  pointing to task_struct but csr_sscratch 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>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>
> ---
> Since patch has changed a little bit from v1 and I didn't include
> changelog earlier, here it is.
>
> v1 --> v2:
>  - added logic to locate task_struct irrespective of priv
>  - made locating task_struct agnostic to bitness(32 vs 64).
>  - added caching of ulong type in scripts/gdb/linux/utils.py
>  - added more descriptive commit message
>
> v2 --> v3:
>  - amended commit message and source line to fit column width
>
> v3 --> v4:
>  - amended commit message and remove whitespace in source
>  - added Reviewed-by for reviewers
>
> v4 --> v5:
>  - changing the order of changelog and sign off/review tags in commit
> ---
> ---
>  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..14c22f82449b 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())):
> +             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)

Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
Acked-by: Palmer Dabbelt <palmer@rivosinc.com>

Thanks!

Re: [PATCH v5] scripts/gdb: add lx_current support for riscv

[Jan Kiszka]

On 15.11.22 23:10, 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_sscratch CSR.
> 
> This patch assumes "tp" is pointing to task_struct. If value in
> csr_sscratch is numerically greater than "tp" then it assumes csr_sscratch
> is correct address of current task_struct. This logic holds when
>    - hart is in user space, "tp" will be less than csr_sscratch.
>    - hart is in kernel space but not in trap handler, "tp" will be more
>      than csr_sscratch (csr_sscratch being equal to 0).
>    - hart is executing trap handler
>        - "tp" is still pointing to user mode but csr_sscratch contains
>           ptr to task_struct. Thus numerically higher.
>        - "tp" is  pointing to task_struct but csr_sscratch 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>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> 
> ---
> Since patch has changed a little bit from v1 and I didn't include
> changelog earlier, here it is.
> 
> v1 --> v2:
>  - added logic to locate task_struct irrespective of priv
>  - made locating task_struct agnostic to bitness(32 vs 64).
>  - added caching of ulong type in scripts/gdb/linux/utils.py
>  - added more descriptive commit message
> 
> v2 --> v3:
>  - amended commit message and source line to fit column width
> 
> v3 --> v4:
>  - amended commit message and remove whitespace in source
>  - added Reviewed-by for reviewers
> 
> v4 --> v5:
>  - changing the order of changelog and sign off/review tags in commit
> ---
> ---
>  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..14c22f82449b 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())):
> +             current_task = scratch_reg.cast(task_ptr_type)

Why not if-else for the assignment here?

> +
> +         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)

Looks good to me otherwise.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH v5] scripts/gdb: add lx_current support for riscv

[Hsieh-Tseng Shen]

On Mon, Jan 02, 2023 at 10:09:01AM +0100, Jan Kiszka wrote:
> On 15.11.22 23:10, 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_sscratch CSR.
> > 
> > This patch assumes "tp" is pointing to task_struct. If value in
> > csr_sscratch is numerically greater than "tp" then it assumes csr_sscratch
> > is correct address of current task_struct. This logic holds when
> >    - hart is in user space, "tp" will be less than csr_sscratch.
> >    - hart is in kernel space but not in trap handler, "tp" will be more
> >      than csr_sscratch (csr_sscratch being equal to 0).
> >    - hart is executing trap handler
> >        - "tp" is still pointing to user mode but csr_sscratch contains
> >           ptr to task_struct. Thus numerically higher.
> >        - "tp" is  pointing to task_struct but csr_sscratch 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>
> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> > 
> > ---
> > Since patch has changed a little bit from v1 and I didn't include
> > changelog earlier, here it is.
> > 
> > v1 --> v2:
> >  - added logic to locate task_struct irrespective of priv
> >  - made locating task_struct agnostic to bitness(32 vs 64).
> >  - added caching of ulong type in scripts/gdb/linux/utils.py
> >  - added more descriptive commit message
> > 
> > v2 --> v3:
> >  - amended commit message and source line to fit column width
> > 
> > v3 --> v4:
> >  - amended commit message and remove whitespace in source
> >  - added Reviewed-by for reviewers
> > 
> > v4 --> v5:
> >  - changing the order of changelog and sign off/review tags in commit
> > ---
> > ---
> >  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..14c22f82449b 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())):
> > +             current_task = scratch_reg.cast(task_ptr_type)
> 
> Why not if-else for the assignment here?
> 
> > +
> > +         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)
> 
> Looks good to me otherwise.
> 
> Jan
> 
> -- 
> Siemens AG, Technology
> Competence Center Embedded Linux

This patch had been pending for quite a while, and not sure if it's
still acceptable to be merged, but from my testing it's working fine
for me. Nevertheless, the v5 patch now has conflict with the current
master, so I've slightly modified for reference. It would be helpful
if Deepak can send v6 later on.

Tested-by: Hsieh-Tseng Shen <woodrow.shen@sifive.com>

v5 --> v6:
 - dropped cache type "ulong" in scripts/gdb/linux/utils.py as it
 already exists in the current upstream
---
 scripts/gdb/linux/cpus.py | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/scripts/gdb/linux/cpus.py b/scripts/gdb/linux/cpus.py
index 255dc18cb9da..f8325cab5f1b 100644
--- a/scripts/gdb/linux/cpus.py
+++ b/scripts/gdb/linux/cpus.py
@@ -179,6 +179,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())):
+            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")
--
2.31.1

Thank you

Re: [PATCH v5] scripts/gdb: add lx_current support for riscv

[Deepak Gupta]

On Wed, Oct 4, 2023 at 10:29 PM Hsieh-Tseng Shen
<woodrow.shen@sifive.com> wrote:
>
> On Mon, Jan 02, 2023 at 10:09:01AM +0100, Jan Kiszka wrote:
> > On 15.11.22 23:10, 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_sscratch CSR.
> > >
> > > This patch assumes "tp" is pointing to task_struct. If value in
> > > csr_sscratch is numerically greater than "tp" then it assumes csr_sscratch
> > > is correct address of current task_struct. This logic holds when
> > >    - hart is in user space, "tp" will be less than csr_sscratch.
> > >    - hart is in kernel space but not in trap handler, "tp" will be more
> > >      than csr_sscratch (csr_sscratch being equal to 0).
> > >    - hart is executing trap handler
> > >        - "tp" is still pointing to user mode but csr_sscratch contains
> > >           ptr to task_struct. Thus numerically higher.
> > >        - "tp" is  pointing to task_struct but csr_sscratch 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>
> > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> > >
> > > ---
> > > Since patch has changed a little bit from v1 and I didn't include
> > > changelog earlier, here it is.
> > >
> > > v1 --> v2:
> > >  - added logic to locate task_struct irrespective of priv
> > >  - made locating task_struct agnostic to bitness(32 vs 64).
> > >  - added caching of ulong type in scripts/gdb/linux/utils.py
> > >  - added more descriptive commit message
> > >
> > > v2 --> v3:
> > >  - amended commit message and source line to fit column width
> > >
> > > v3 --> v4:
> > >  - amended commit message and remove whitespace in source
> > >  - added Reviewed-by for reviewers
> > >
> > > v4 --> v5:
> > >  - changing the order of changelog and sign off/review tags in commit
> > > ---
> > > ---
> > >  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..14c22f82449b 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())):
> > > +             current_task = scratch_reg.cast(task_ptr_type)
> >
> > Why not if-else for the assignment here?
> >
> > > +
> > > +         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)
> >
> > Looks good to me otherwise.
> >
> > Jan
> >
> > --
> > Siemens AG, Technology
> > Competence Center Embedded Linux
>
> This patch had been pending for quite a while, and not sure if it's
> still acceptable to be merged, but from my testing it's working fine
> for me. Nevertheless, the v5 patch now has conflict with the current
> master, so I've slightly modified for reference. It would be helpful
> if Deepak can send v6 later on.

Thanks for testing it out.
I just sent out a v6 based on v6.6-rc5.

-Deepak

>
> Tested-by: Hsieh-Tseng Shen <woodrow.shen@sifive.com>
>
> v5 --> v6:
>  - dropped cache type "ulong" in scripts/gdb/linux/utils.py as it
>  already exists in the current upstream
> ---
>  scripts/gdb/linux/cpus.py | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/scripts/gdb/linux/cpus.py b/scripts/gdb/linux/cpus.py
> index 255dc18cb9da..f8325cab5f1b 100644
> --- a/scripts/gdb/linux/cpus.py
> +++ b/scripts/gdb/linux/cpus.py
> @@ -179,6 +179,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())):
> +            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")
> --
> 2.31.1
>
> Thank you