Re: [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access

From: Palmer Dabbelt <palmer@dabbelt.com>
To: viro@zeniv.linux.org.uk
Cc: macro@wdc.com, linux-riscv@lists.infradead.org,
	Paul Walmsley <paul.walmsley@sifive.com>,
	aou@eecs.berkeley.edu, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access
Date: Tue, 04 Aug 2020 20:15:20 -0700 (PDT)	[thread overview]
Message-ID: <mhng-2d0483dd-411b-4e2b-aae1-b8bb52417d29@palmerdabbelt-glaptop1> (raw)
In-Reply-To: <20200805024807.GM1236603@ZenIV.linux.org.uk>

On Tue, 04 Aug 2020 19:48:07 PDT (-0700), viro@zeniv.linux.org.uk wrote:
> On Tue, Aug 04, 2020 at 07:20:05PM -0700, Palmer Dabbelt wrote:
>> On Tue, 04 Aug 2020 19:07:45 PDT (-0700), viro@zeniv.linux.org.uk wrote:
>> > On Tue, Aug 04, 2020 at 07:01:01PM -0700, Palmer Dabbelt wrote:
>> >
>> > > > We currently have @start_pos fixed at 0 across all calls, which works as
>> > > > a result of the implementation, in particular because we have no padding
>> > > > between the FP general registers and the FP control and status register,
>> > > > but appears not to have been the intent of the API and is not what other
>> > > > ports do, requiring one to study the copy handlers to understand what is
>> > > > going on here.
>> >
>> > start_pos *is* fixed at 0 and it's going to go away, along with the
>> > sodding user_regset_copyout() very shortly.  ->get() is simply a bad API.
>> > See vfs.git#work.regset for replacement.  And ->put() is also going to be
>> > taken out and shot (next cycle, most likely).
>>
>> I'm not sure I understand what you're saying, but given that branch replaces
>> all of this I guess it's best to just do nothing on our end here?
>
> It doesn't replace ->put() (for now); it _does_ replace ->get() and AFAICS the
> replacement is much saner:
>
> static int riscv_fpr_get(struct task_struct *target,
>                          const struct user_regset *regset,
>                          struct membuf to)
> {
> 	struct __riscv_d_ext_state *fstate = &target->thread.fstate;
>
> 	membuf_write(&to, fstate, offsetof(struct __riscv_d_ext_state, fcsr));
> 	membuf_store(&to, fstate->fcsr);
> 	return membuf_zero(&to, 4);     // explicitly pad
> }
>
> user_regset_copyout() calling conventions are atrocious and so are those of
> regset ->get().  The best thing to do with both is to take them out of their
> misery and be done with that.  Do you see any problems with riscv gdbserver
> on current linux-next?  If not, I'd rather see that "API" simply go away...
> If there are problems, I would very much prefer fixes on top of what's done
> in that branch.

I guess my confusion was about "start_pos *is* fixed at 0": it certainly is
zero in the code right now, but when poking around while review the patch I
didn't see any reason that must be so.  Admittedly all I did was read the
prototype and function, so maybe I'm just missing something.  That said, if
it's all going away anyway then I don't really care either way.

As far as I can tell the patch set in question (the RISC-V one) doesn't change
any functionality.  I don't actually use GDB, but I haven't seen any issues
reported in a few years so if there is one I've missed it.

I did this ptrace stuff many years ago (IIRC it was actually my first RISC-V
Linux patch), and all I really remember is that it seemed way more complicated
than it needed to be.  I'm happy to just drop our patch set, as yours looks way
cleaner to me and if you're already planning on fixing put() then it doesn't
seem worth the churn.