Re: [Qemu-devel] [PATCH 3/4] S/390 host support for TCG

[Aurelien Jarno]

On Mon, Nov 09, 2009 at 06:54:28PM +0200, Ulrich Hecht wrote:
> On Monday 02 November 2009, Aurelien Jarno wrote:
> > First of all I have a question about s390/s390x. It seems that you
> > plan to support both s390 and s390x in the same file. Is that correct?
> 
> Actually, I didn't think about supporting s390 hosts at all, but it 
> should be possible.
> 
> > Do you know how far the 32-bit support is far from compiling/working?
> 
> It is not entirely unlikely that it's close. You obviously can't use the 
> 64-bit instructions on a 31-bit host, but other than that...
> 
> > Would it be really possible to support both in the same file?
> 
> It would, but we would need different implementations for the 64-bit 
> operations.
> 
> From a practical point of view, I don't see any merit in supporting 
> 31-bit hosts, simply because there aren't many in use anymore. (It's a 
> different story for the target: 31-bit _software_ is still widespread.)

My question was mainly to choose the name for the directory between
s390/s390x. If it is possibly doable, I think we can keep s390.

> > > +SEARCH_DIR("/usr/s390x-suse-linux/lib64");
> > > SEARCH_DIR("/usr/local/lib64"); SEARCH_DIR("/lib64");
> > > SEARCH_DIR("/usr/lib64"); SEARCH_DIR("/usr/s390x-suse-linux/lib");
> > > SEARCH_DIR("/usr/lib64"); SEARCH_DIR("/usr/local/lib");
> > > SEARCH_DIR("/lib"); SEARCH_DIR("/usr/lib");
> >
> > Why adding some search directories manually here? The one you are
> > adding are SuSE specific. They should already be detected by the
> > configure script and added to config-host.ld.
> 
> I merely copied the linker script and made some minor adjustments, I 
> didn't really pay much attention to what it actually contains...
> 
> > > +    tcg_out32(s, 0xa7840000); /* je label1 (offset will be patched
> > > in later) */
> >
> > Would be nice to have the corresponding tcg_out_ function and the
> > corresponding value instead of the magic value. Also I am not sure the
> > comment is correct here, "je" being an x86_64 instruction.
> 
> objdump disassembles it as "je", though the POP doesn't specify any 
> aliases for the different mask values for BRC.

ok

> > > +            tcg_out_sh64(s, SH64_SRAG, data_reg, data_reg,
> > > SH64_REG_NONE, 48); +            break;
> > > +        case 2 | 4:
> > > +            tcg_out_b9(s, B9_LGFR, data_reg, arg0);
> > > +            break;
> > > +        case 0: case 1: case 2: case 3: default:
> > > +            /* unsigned -> just copy */
> > > +            tcg_out_b9(s, B9_LGR, data_reg, arg0);
> >
> > Is it always correct? On x86_64, starting with gcc 4.3, it is not
> > guaranteed anymore that non used bits are set to 0.
> 
> Do I understand you correctly in that you are suggesting to do an 
> explicit zero extension here? (But what does GCC have to do with 
> TCG-generated code?)

Yes, this is my suggestion. The value from here is returned by GCC, so
you have to comply with the ABI for exchanging data. I am not sure it is
needed, I just say it may worth checking that.

For example on ia64 the ABI says that the returned value is filled with
undefined value if the returned value's size is less than the register
size.

> > > +    case INDEX_op_call:
> > > +        //fprintf(stderr,"op 0x%x call 0x%lx 0x%lx
> > > 0x%lx\n",opc,args[0],args[1],args[2]); +        if (const_args[0]) {
> > > +            tcg_target_long off = (args[0] -
> > > (tcg_target_long)s->code_ptr + 4) >> 1; /* FIXME: + 4? Where did
> > > that come from? */ +            if (off > -0x80000000 && off <
> > > 0x7fffffff) { /* relative call */ +                tcg_out_brasl(s,
> > > TCG_REG_R14, off << 1);
> > > +                tcg_abort(); // untested
> >
> > If not tested, it's probably better to remove this part and do not
> > mark call accepting constants.
> 
> I suppose so. It's obviously never used anyway.
> 
> >
> > > +            }
> > > +            else { /* too far for a relative call, load full
> > > address */ +                tcg_out_movi(s, TCG_TYPE_PTR,
> > > TCG_REG_R13, args[0]); +                tcg_out_rr(s, RR_BASR,
> > > TCG_REG_R14, TCG_REG_R13);
> >
> > Not sure it is very useful here. It's probably better to redefine the
> > constraints of the constant, so that the reg allocator use a register
> > to pass the value.
> >
> > > +            }
> > > +        }
> > > +        else {   /* call function in register args[0] */
> > > +            tcg_out_rr(s, RR_BASR, TCG_REG_R14, args[0]);
> > > +        }
> > > +        break;
> > > +    case INDEX_op_jmp:
> > > +        fprintf(stderr,"op 0x%x jmp 0x%lx 0x%lx
> > > 0x%lx\n",opc,args[0],args[1],args[2]); +        tcg_abort();
> > > +        break;
> >
> > What about implementing that?
> 
> Do you know a target that actually uses it? I try to avoid implementing 
> stuff that I can't test.

Currently I can't find one. However it should not be very different than
implementing call.

> > > +static inline void tcg_out_addi(TCGContext *s, int reg,
> > > tcg_target_long val) +{
> > > +    tcg_abort();
> > > +}
> >
> > Why this one is not implemented? This is definitely needed so that
> > arguments can be passed on the stack.
> 
> Generally, if something is not implemented yet it's because it has never 
> been used anywhere.

It is used in tcg/tcg.c to pass values on the stack, so I am pretty sure
it is used by some targets having helpers with high number of arguments.

> > > +#define TCG_TARGET_HAS_div_i32
> > > +#undef TCG_TARGET_HAS_div_i64
> >
> > Is that corrects? It looks strange an architecture has different
> > division in 32- and 64-bits mode.
> 
> The 64-bit instruction set actually has both. Maybe I just forgot to 
> implement it.
> 

This op is definitely needed, for example to boot a linux kernel under
qemu-system-mips64(el). It is however not used in x86/x86_64 targets.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net