This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC v3 4/8] Add kernel module support for linux-kernel target
- From: Yao Qi <qiyaoltc at gmail dot com>
- To: Philipp Rudo <prudo at linux dot vnet dot ibm dot com>
- Cc: gdb-patches at sourceware dot org, Yao Qi <yao dot qi at linaro dot org>, Peter Griffin <peter dot griffin at linaro dot org>, Omair Javaid <omair dot javaid at linaro dot org>, Andreas Arnez <arnez at linux dot vnet dot ibm dot com>
- Date: Fri, 05 May 2017 22:33:51 +0100
- Subject: Re: [RFC v3 4/8] Add kernel module support for linux-kernel target
- Authentication-results: sourceware.org; auth=none
- References: <20170316165739.88524-1-prudo@linux.vnet.ibm.com> <20170316165739.88524-5-prudo@linux.vnet.ibm.com> <868tmf8kn4.fsf@gmail.com> <20170503181620.53d0bd3f@ThinkPad>
Philipp Rudo <prudo@linux.vnet.ibm.com> writes:
>> > +/* Translate a kernel virtual address ADDR to a physical address. */
>> > +
>> > +CORE_ADDR
>> > +lk_kvtop (CORE_ADDR addr)
>>
>> How about lk_kernel_vir_to_phy_addr?
>
> I prefer kvtop. It's much shorter and (for my taste) is just as readable. But
> I don't insist on keeping the name. Are there other opinions?
>
or maybe lk_vir_to_phy?
>>
>> > +{
>> > + CORE_ADDR pgd = lk_read_addr (LK_ADDR (init_mm)
>> > + + LK_OFFSET (mm_struct, pgd));
>> > + return LK_HOOK->vtop (pgd, addr);
>> > +}
>> > +
>> > +/* Restore current_target to TARGET. */
>> > +static void
>> > +restore_current_target (void *target)
>> > +{
>> > + current_target.beneath = (struct target_ops *) target;
>> > +}
>> > +
>> > +/* Function for targets to_xfer_partial hook. */
>> > +
>> > +enum target_xfer_status
>> > +lk_xfer_partial (struct target_ops *ops, enum target_object object,
>> > + const char *annex, gdb_byte *readbuf,
>> > + const gdb_byte *writebuf, ULONGEST offset, ULONGEST len,
>> > + ULONGEST *xfered_len)
>> > +{
>> > + enum target_xfer_status ret_val;
>> > + struct cleanup *old_chain = make_cleanup (restore_current_target,
>> > ops);
>>
>> Use make_scoped_restore instead of make_cleanup?
>
> Using a scoped_restore probably makes sense. Although I don't see the
> advantage over old style cleanups other than having marginally shorter code ...
>
We want to reduce the usages of cleanup, and even completely remove it
ultimately, so we should avoid using it in new code.
>> > +
>> > + current_target.beneath = ops->beneath;
>> > +
>>
>> Any reasons you switch current_target.beneath temporarily?
>
> Yes. lk_kvtop (at least for s390) reads memory if the address is not
> physical. Thus reading a virtual address calls xfer_partial twice. Once for
> the actual address and a second time for the data lk_kvtop needs. This can
> lead to an endless recursion if there is a bug or memory corruption. Switching
> to the target beneath prevents this.
>
Does it work if you pass ops->beneath to lk_kvtop and all lk_read_XXX
apis, so that we can use ops->beneath there instead of current_target.
--
Yao (齐尧)