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

[Ulrich Hecht]

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)