Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 15 Nov 2022 10:43:48 -0800
From:   Deepak Gupta <debug@rivosinc.com>
To:     Conor.Dooley@microchip.com
Cc:     aou@eecs.berkeley.edu, jan.kiszka@siemens.com, kbingham@kernel.org,
        linux-kernel@vger.kernel.org, palmer@dabbelt.com,
        paul.walmsley@sifive.com, ajones@ventanamicro.com,
        linux-riscv@lists.infradead.org, bjorn@kernel.org,
        atishp@rivosinc.com
Subject: Re: [PATCH v3] scripts/gdb: add lx_current support for riscv
Message-ID: <20221115184348.GA1854852@debug.ba.rivosinc.com>
References: <20221115012917.1781185-1-debug@rivosinc.com>
 <20221115084923.1822572-1-debug@rivosinc.com>
 <Y3OkY6myfea00BaK@wendy>
 <CAKC1njRi9C0m3JKpu0ebAFCC25161EST=tFFWiAj1yZBbnak6A@mail.gmail.com>
 <4f293c39-6a0e-dd25-9ed2-10088bb971e1@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <4f293c39-6a0e-dd25-9ed2-10088bb971e1@microchip.com>
Precedence: bulk

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