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: Philipp Rudo <prudo at linux dot vnet dot ibm dot com>
- To: Yao Qi <qiyaoltc at gmail 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: Mon, 8 May 2017 11:18:37 +0200
- 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> <86d1bnouo0.fsf@gmail.com>
Hi
On Fri, 05 May 2017 22:33:51 +0100
Yao Qi <qiyaoltc@gmail.com> wrote:
> 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?
What about lk_virt_to_phys and lk_kvirt_to_phys? For the general virtual to
physical translation and the special case when the kernel page tables are used.
>
> >>
> >> > +{
> >> > + 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.
Yes, I understand. It's just that you replace one mechanism to restore the
global variables with an other instead of removing the root cause, global
variables...
> >> > +
> >> > + 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.
Well switching the target is 'just' a backup. Only a few lines below in
lk_xfer_partial I do this
ret_val = ops->beneath->to_xfer_partial (ops->beneath, object, annex,
readbuf, writebuf, offset, len,
xfered_len);
Switching the target assures that, if ops->beneath->to_xfer_partial calls other
target methods via the global current_target those methods also belong to
ops->beneath. Moving this to lk_kvtop or lk_read_* wouldn't prevent the
problem but would lead to code duplication.
Thanks
Philipp