Re: [RFC v3 4/8] Add kernel module support for linux-kernel target

[Yao Qi <qiyaoltc@gmail.com>]

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 (齐尧)