Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Date:   Thu, 5 Oct 2023 13:29:32 +0800
From:   Hsieh-Tseng Shen <woodrow.shen@sifive.com>
To:     debug@rivosinc.com
Cc:     palmer@dabbelt.com, ajones@ventanamicro.com, aou@eecs.berkeley.edu,
        conor.dooley@microchip.com, jan.kiszka@siemens.com,
        kbingham@kernel.org, linux-kernel@vger.kernel.org,
        linux-riscv@lists.infradead.org, paul.walmsley@sifive.com
Subject: Re: [PATCH v5] scripts/gdb: add lx_current support for riscv
Message-ID: <ZR5JvCSDZwTQAucC@u-NUC7i5BNH>
Reply-To: 20221115221051.1871569-1-debug@rivosinc.com
References: <d65bddf3-c58a-1420-bff9-8333c10edb56@microchip.com>
 <20221115221051.1871569-1-debug@rivosinc.com>
 <98eb5bc7-dee4-b36e-a219-17b1e08d85f6@siemens.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <98eb5bc7-dee4-b36e-a219-17b1e08d85f6@siemens.com>
Precedence: bulk

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