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

[Deepak Gupta <debug@rivosinc.com>]

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