[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/4] S/390 host support for TCG
From: |
Ulrich Hecht |
Subject: |
Re: [Qemu-devel] [PATCH 3/4] S/390 host support for TCG |
Date: |
Mon, 9 Nov 2009 18:54:28 +0200 |
User-agent: |
KMail/1.9.10 |
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.)
> > +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.
> > + 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?)
> > + 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.
> > +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.
> > +#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.
CU
Uli
--
SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg)