Re: [Qemu-devel] [PATCH 2/9] S/390 CPU emulation

[Aurelien Jarno]

On Mon, Oct 19, 2009 at 07:17:18PM +0200, Ulrich Hecht wrote:
> On Saturday 17 October 2009, Aurelien Jarno wrote:
> > On Fri, Oct 16, 2009 at 02:38:48PM +0200, Ulrich Hecht wrote:
> > First of all a few general comments. Note that I know very few things
> > about S390/S390X, so I may have dumb comments/questions. Also as the
> > patch is very long, I probably have missed things, we should probably
> > iterate with new versions of the patches.
> >
> > Is it possible given the current implementation to emulate S390 in
> > addition to S390X?
> 
> Not as it stands. The S/390 instruction set is a subset of zArch 
> (64-bit), but currently only 64-bit addressing is implemented (because 
> that is the only mode used by 64-bit Linux binaries).
> 
> > If yes how different would be a S390 only target? 
> 
> It would need support for 31-bit addressing. 24-bit, too, if you want to 
> run the really old stuff.
> 
> > If they won't be too different, it probably worth using _tl type
> > registers and call it target-s390. A bit the way it is done with
> > i386/x86_64, ppc/ppc64, mips/mips64 or sparc/sparc64.
> 
> If it's going to be implemented, it will certainly share most code with 
> the 64-bit target, so having a common directory would most likely be a 
> good idea.
> 
> There are some cases where _tl types might be appropriate, but generally 
> everything that is shared between S/390 and zArch is 32-bit, and 
> instructions present in both architectures also have the same operand 
> size: L, for instance, is a 32-bit load to a 32-bit register on both 
> architectures.

Ok, so I think we should go for putting the code is the target-s390 
directory. As no code is shared for now, we can just ignore the _tl
stuff.

> > Secondly there seems to be a lot of mix between 32-bit and 64-bit
> > TCG registers. They should not be mixed. _i32 ops apply to TCGv_i32
> > variables, _i64 ops apply to TCGv_i64 variables, and _tl ops to TGCv
> > variables. TCGv/_tl types can map either to _i32 or _i64 depending on
> > your target. You should try to build the target with
> > --enable-debug-tcg
> 
> Er, reading the code and observing the behavior of the (AMD64) code 
> generator, it seemed to me that neither frontends nor backends care one 
> bit about that. I'll look into it, though. (I won't comment on those 
> below, except in cases where the solution or, indeed, the problem is not 
> clear to me.)

It is not strictly needed on AMD64, because a move between 32- and 64-
bit registers also zero extends the value. It is definitely needs on
other hosts (and most probably s390x as I understand).

> > It would be nice to split all the disassembling part in a separate
> > combined with the related makefile changes. This way it can be applied
> > separately.
> 
> Can do.
> 
> > >  //  switch (info->mach)
> > >  //    {
> > >  //    case bfd_mach_s390_31:
> > > -      current_arch_mask = 1 << S390_OPCODE_ESA;
> > > +//      current_arch_mask = 1 << S390_OPCODE_ESA;
> > >  //      break;
> > >  //    case bfd_mach_s390_64:
> > > -//      current_arch_mask = 1 << S390_OPCODE_ZARCH;
> > > +      current_arch_mask = 1 << S390_OPCODE_ZARCH;
> > >  //      break;
> > >  //    default:
> > >  //      abort ();
> >
> > While I understand the second part, Why is it necessary to comment the
> > bfd_mach_s390_31 case?
> 
> It's not necessary, but current_arch_mask can only hold one value...

I'll answer in the new version of the patch.

> 
> > > +    /* FIXME: reset vector? */
> >
> > Yes, reset vector, MMU mode, default register values, ...
> 
> Works perfectly fine without. ;)
> 
> Also, the question remains what kind of reset cpu_reset() is supposed to 
> be. For instance, a CPU Reset or Initial CPU Reset on S/390 leaves the 
> general-purpose registers unchanged, a Clear Reset or Power-On Reset 
> sets them to zero. See the table on p. 4-50 in the zArchitecture 
> Principles of Operation (POP), 
> http://publibfp.boulder.ibm.com/cgi-bin/bookmgr/download/A2278324.pdf, 
> for the gory details.

I don't know what is the best, probably a CPU Reset is enough. Just
consider that as the code that will be run if someone press the reset
button.

> > > +    for (i = 0; i <= l; i++) {
> > > +        x = ldub(dest + i) & ldub(src + i);
> > > +        if (x) cc = 1;
> >
> > coding style
> 
> Maybe you are referring to "Every indented statement is braced; even if 
> the block contains just one statement.", but I figured that that does 
> not apply here because there is no indentation...

You are basically trying to workaround the coding style. The idea behind
it is to write: 
if (x) {
    cc = 1;
}

> > All the branch part would really gain to be coded in TCG, as it will
> > allow TB chaining.
> 
> I vaguely remember trying that as well, but I don't know if I gave up on 
> it because it was slower, or because I couldn't get it to work...

Probably the second. Changing the instruction pointer in the helper
instead of using the proper goto_tb TCG op prevents TB chaining, and
therefore as a huge impact on performance.

It's something not difficult to implement, and that I would definitely
want to see in the patch before getting it merged.

> > It should probably be 
> > rewritten as load_reg(TCGv t, int reg).
> 
> Later. This is the kind of change that you never get right on the first 
> try, so I'd like to do that after everything went upstream.

It also means huge patch difficult to review when it is done after the
initial merge, as they need to be checked within the context.

 
> > > +        tmp = tcg_const_i32(r1);
> >
> > Wrong TCGv type.
> >
> > > +        gen_helper_mlg(tmp, tmp2);
> 
> Huh? It's a register number. There are only 16 GP registers. Fits a 
> 32-bit value, last time I checked. The helper also expects a 32-bit 
> value.

Then a TCGv_i32 variable should be used. TCGv is 64-bit, as the whole
target is 64-bit.

> > > +    if (tmp2) tcg_temp_free(tmp2);
> > > +    if (tmp3) tcg_temp_free(tmp3);
> >
> > Comparison on TCGv type is not allowed.
> 
> Not even to TCGV_UNUSED?

TCGV_UNUSED() is a macro to set the value, not to retrieve it.

> > > +    LOG_DISAS("disas_c0: op 0x%x r1 %d i2 %d\n", op, r1, i2);
> > > +    uint64_t target = s->pc + i2 * 2;
> > > +    /* FIXME: huh? */ target &= 0xffffffff;
> >
> > That should be fixed before a merge.
> 
> Tricky, because I have not yet understood why it is necessary. According 
> to the POP, the address calculation in 64-bit addressing mode should be 
> 64 bits. In practice, on a real machine running a real Linux kernel, it 
> wraps around at 32 bits, and userland code actually relies on that. No 
> clue what I could be missing here...

Does it means the userspace is actually limited to 32-bit addressing?

> > > +#if 1
> > > +    cc = tcg_temp_local_new_i32();
> > > +    tcg_gen_mov_i32(cc, global_cc);
> > > +#else
> > > +    cc = global_cc;
> > > +#endif
> >
> > Why this?
> 
> Because it didn't work otherwise. I don't claim to understand the 
> mysterious ways of the TCG to its full extent...

What do you mean by didn't work? TCG error? Wrong emulation? Something
else?

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