Re: [PATCH v3] scripts/gdb: add lx_current support for riscv
From: Deepak Gupta
Date: Tue Nov 15 2022 - 13:44:00 EST
On Tue, Nov 15, 2022 at 06:06:34PM +0000, Conor.Dooley@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx <mailto:conor.dooley@xxxxxxxxxxxxx>> 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@xxxxxxxxxxxx <mailto:debug@xxxxxxxxxxxx>>
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