Significant performance regression in qemu-system-mips.

On Friday 26 March 2010 14:00:00 Aurelien Jarno wrote:
> > I'm not asking anyone to care about me personally, I'm asking them to
> > care about specific technical issues.  If those issues don't interest
> > you, they don't interest you.
> >
> > Speaking of ppc, last month I sent this patch:
> >
> >   http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg00917.html
> >
> > And I was under the impression people agreed with it:
> >
> >   http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg01044.html
> >   http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg01714.html
> >
> > But today's -git is still having that same issue, and the same patch
> > still applies cleanly and fixes it for me.
>
> Re-read the last link you quoted, and especially this part:
> | The
> | same way using CONFIG_BSD in linux-user/elfload.c doesn't make sense,
> | as this code will never been compiled.

I didn't know the BSD comments werer addressed at me.  (I haven't got a BSD
test system.)

> While your patch goes in the good direction, it doesn't mean it is
> correct. Conditionally compiling code on CONFIG_BSD in a Linux specific
> file doesn't make sense.

Ok.

> I am pretty fine applying a correct patch if you send a new one.

By which you mean rip out the whole #ifdef block?

Here you go:


Re-tested and it works fine for me.

> I have no problem with you having no interest in sh4, a lot of people
> are in you case. I don't think it gives you the right to describe the
> sh4 kernel maintainer as "sh4 linux-kernel maintainer officially doesn't
> care about anybody who isn't employed by his company", or later "It's
> not real hardware, it's a one-company toy". This is not something
> reflected in link you quoted. Paul Mundt has been nicely answering your
> question on this thread.

Ok, I agree I was a bit harsh.  (He's the one who introduced his employer into
the discussion, but I suspect I read more into that than he meant by it.)

But this doesn't revive my interest in the platform.  He did confirm that he
doesn't care if users of even six-month-old tools could still build the thing,
and that he was willing to break toolchains that had built existing usable
kernels for years in the name of cleanup patches.  Add in the fact that
I've never encountered anyone actually using this hardware since the
Dreamcast, and that the first, third, and fourth hits googling for "sh4" are
the video games Silent Hunter 4 and Silent Hill 4.  (The first four hits for
"arm" are the processor despite everybody having two of that word attached to
their shoulders...)

Rob

On Fri, Mar 26, 2010 at 04:47:51PM -0500, Rob Landley wrote:
> On Friday 26 March 2010 14:00:00 Aurelien Jarno wrote:
> > > I'm not asking anyone to care about me personally, I'm asking them to
> > > care about specific technical issues.  If those issues don't interest
> > > you, they don't interest you.
> > >
> > > Speaking of ppc, last month I sent this patch:
> > >
> > >   http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg00917.html
> > >
> > > And I was under the impression people agreed with it:
> > >
> > >   http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg01044.html
> > >   http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg01714.html
> > >
> > > But today's -git is still having that same issue, and the same patch
> > > still applies cleanly and fixes it for me.
> >
> > Re-read the last link you quoted, and especially this part:
> > | The
> > | same way using CONFIG_BSD in linux-user/elfload.c doesn't make sense,
> > | as this code will never been compiled.
> 
> I didn't know the BSD comments werer addressed at me.  (I haven't got a BSD
> test system.)
> 
> > While your patch goes in the good direction, it doesn't mean it is
> > correct. Conditionally compiling code on CONFIG_BSD in a Linux specific
> > file doesn't make sense.
> 
> Ok.
> 
> > I am pretty fine applying a correct patch if you send a new one.
> 
> By which you mean rip out the whole #ifdef block?

Yes.

> Here you go:

This looks much better. Can you please resend it with the changes below
and a Signed-off-by: ?

> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 682a813..44405dd 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -518,12 +518,11 @@ do {                                                                    \
>  static inline void init_thread(struct target_pt_regs *_regs, struct image_info *infop)
>  {
>      abi_ulong pos = infop->start_stack;

This line should probably be removed...

> -    abi_ulong tmp;
>  #if defined(TARGET_PPC64) && !defined(TARGET_ABI32)
>      abi_ulong entry, toc;
>  #endif
>  
> -    _regs->gpr[1] = infop->start_stack;
> +    _regs->gpr[1] = pos;

...instead of doing the change here.

>  #if defined(TARGET_PPC64) && !defined(TARGET_ABI32)
>      entry = ldq_raw(infop->entry) + infop->load_addr;
>      toc = ldq_raw(infop->entry + 8) + infop->load_addr;
> @@ -531,17 +530,6 @@ static inline void init_thread(struct target_pt_regs *_regs, struct image_info *
>      infop->entry = entry;
>  #endif
>      _regs->nip = infop->entry;
> -    /* Note that isn't exactly what regular kernel does
> -     * but this is what the ABI wants and is needed to allow
> -     * execution of PPC BSD programs.
> -     */
> -    /* FIXME - what to for failure of get_user()? */
> -    get_user_ual(_regs->gpr[3], pos);
> -    pos += sizeof(abi_ulong);
> -    _regs->gpr[4] = pos;
> -    for (tmp = 1; tmp != 0; pos += sizeof(abi_ulong))
> -        tmp = ldl(pos);
> -    _regs->gpr[5] = pos;
>  }
>  
>  #define ELF_EXEC_PAGESIZE	4096
> 
> Re-tested and it works fine for me.
> 
> > I have no problem with you having no interest in sh4, a lot of people
> > are in you case. I don't think it gives you the right to describe the
> > sh4 kernel maintainer as "sh4 linux-kernel maintainer officially doesn't
> > care about anybody who isn't employed by his company", or later "It's
> > not real hardware, it's a one-company toy". This is not something
> > reflected in link you quoted. Paul Mundt has been nicely answering your
> > question on this thread.
> 
> Ok, I agree I was a bit harsh.  (He's the one who introduced his employer into
> the discussion, but I suspect I read more into that than he meant by it.)
> 

I think you misunderstood him. You were talking about Super-Hitachi
which is a train [1] from Hitachi (not his employer), while he was talking
about Super-H which is a CPU [2] from Renesas (his employer).

He has the right to not care about trains ;-)

[1] http://en.wikipedia.org/wiki/Hitachi_(Japanese_train)
[2] http://en.wikipedia.org/wiki/SuperH