[v3] 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_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(+)
  

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