Re: [Qemu-devel] [PATCH] tcg/mips: Bugfix for crash whenrunningprogram with qemu-i386.

Re: [Qemu-devel] [PATCH] tcg/mips: Bugfix for crash whenrunningprogram with qemu-i386.

[jiang.biao2]

> On 07/09/2017 04:04 PM, jiang.biao2@zte.com.cn wrote:
> >  >>       if (TCG_TARGET_REG_BITS > TARGET_LONG_BITS) {
> >  >>           tcg_out_ext32u(s, base, addr_regl)
> >  >> -        addr_regl = base
> >  >> +        tcg_out_mov(s, TCG_TYPE_PTR, addr_regl, base)
> >  >>       }
> >  >>       if (guest_base == 0 && data_regl != addr_regl) {
> >  >>           base = addr_regl
> >  >
> >  > This is wrong, because you're not allowed to modify the input operands.
> >  >
> >  > Try this, just a few lines lower in the function:
> > 
> >  > -        tcg_out_movi(s, TCG_TYPE_PTR, base, guest_base)
> >  > -        tcg_out_opc_reg(s, ALIAS_PADD, base, base, addr_regl)
> >  > +        tcg_out_movi(s, TCG_TYPE_PTR, TCG_TMP0, guest_base)
> >  > +        tcg_out_opc_reg(s, ALIAS_PADD, base, TCG_TMP0, addr_regl)
> > 
> >  >
> > 
> > Got it, but the real problem is for addr_regl instead of guest_base.
> 
> Guest base is a problem simply because we require a temporary for it, and we 
> were trying to put two temporaries into the same register.
> 
> If we retain guest_base in a register all of the time, then (1) we do not have 
> to recompute it for every memory load and (2) we do not need a temporary for it.
> 


> > >  > Better would be to reserve a register for the guest_base, like we do for ppc.
> >  > See all of the uses of TCG_GUEST_BASE_REG in tcg/ppc/tcg-target.inc.c.
> > 
> > It uses base(TCG_REG_A0) for temperary use for guest_base in this case.
> 
> No it doesn't.  It computes guest_base into a register in the prologue:
> 
> > #ifndef CONFIG_SOFTMMU
> >     if (guest_base) {
> >         tcg_out_movi(s, TCG_TYPE_PTR, TCG_GUEST_BASE_REG, guest_base)
> >         tcg_regset_set_reg(s->reserved_regs, TCG_GUEST_BASE_REG)
> >     }
> > #endif
> 
> and then it uses a reg+reg addressing mode during qemu_ld/st:
> 
> >     rbase = guest_base ? TCG_GUEST_BASE_REG : 0
> ....
> >             insn = qemu_ldx_opc[opc & (MO_SIZE | MO_BSWAP)]
> >             tcg_out32(s, insn | TAB(datalo, rbase, addrlo))
> 
> Obviously mips doesn't have a reg+reg addressing mode, so a PADD instruction is 
> required, but otherwise you can use the same scheme.  Using TCG_REG_S1 on mips 
> for TCG_GUEST_BASE_REG would be fine.



Understood. guest_base is a problem indeed, and it would be better to be like that.

However, the bug I mentioned here seems to have no direct connect with the 


guest_base problem. It lies in the following code, 


>     if (TCG_TARGET_REG_BITS > TARGET_LONG_BITS) {
>         tcg_out_ext32u(s, base, addr_regl)
>        addr_regl = base //problem is here. 
>    }

this section of code is to extend the addr_regl to 64bit, and use *base* as temp 


intermedia. The real intention could be to extend addr_regl into base, and then 


move base back to addr_regl for later use, but it wrongly assigning  base to 


addr_regl directly, which will cause crash for every use of tcg_out_qemu_st.

My intention is to fix this bug, and intend to use TCG_TMP0 for temporary use 


for addr_regl.




As for the guest_base problem, I'll try to optimize it later, taking ppc code for 


reference.





thanks for your advise and help.

Re: [Qemu-devel] [PATCH] tcg/mips: Bugfix for crash whenrunningprogram with qemu-i386.

[Richard Henderson]

On 07/09/2017 05:15 PM, jiang.biao2@zte.com.cn wrote:
> It lies in the following code,
> 
>  >     if (TCG_TARGET_REG_BITS > TARGET_LONG_BITS) {
>  >         tcg_out_ext32u(s, base, addr_regl);
>  >        addr_regl = base; //problem is here.
>  >    }
> 
> this section of code is to extend the addr_regl to 64bit, and use *base* as temp
> intermedia. The real intention could be to extend addr_regl into base, and then
> move base back to addr_regl for later use, but it wrongly assigning  base to
> addr_regl directly, which will cause crash for every use of tcg_out_qemu_st.

The intent is to zero-extend addr_regl into a temporary; base = A0, and at this 
point is known to be unused.  Afterward, whenever we talk about the low part of 
the address, we want to use the temporary and not the original input.

For example, if addr_regl = S0, and guest_base = S1, then we want

	dext	a0, s0, 31, 0
	dadd	a0, s1, a0

instead of

	dext	a0, s0, 31, 0
	mov	s0, a0
	dadd	a0, s1, s0

> My intention is to fix this bug, and intend to use TCG_TMP0 for temporary use
> for addr_regl.

That is also fine; there's nothing magic about using base = A0 for the temporary.


r~