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

[Philipp Rudo <prudo at linux dot vnet dot ibm dot 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