qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: David Gibson <david@gibson.dropbear.id.au>
To: Fabiano Rosas <farosas@linux.ibm.com>
Cc: lucas.araujo@eldorado.org.br, qemu-devel@nongnu.org,
	lagarcia@br.ibm.com, luis.pires@eldorado.org.br,
	fernando.valle@eldorado.org.br, qemu-ppc@nongnu.org,
	andre.silva@eldorado.org.br,
	"Bruno Larsen \(billionai\)" <bruno.larsen@eldorado.org.br>,
	matheus.ferst@eldorado.org.br
Subject: Re: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg
Date: Mon, 12 Apr 2021 14:34:10 +1000	[thread overview]
Message-ID: <YHPNwtCDtq7XGP/A@yekko.fritz.box> (raw)
In-Reply-To: <87h7kf5jdy.fsf@linux.ibm.com>

[-- Attachment #1: Type: text/plain, Size: 1801 bytes --]

On Fri, Apr 09, 2021 at 04:48:41PM -0300, Fabiano Rosas wrote:
> "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> writes:
> 
> A general advice for this whole series is: make sure you add in some
> words explaining why you decided to make a particular change. It will be
> much easier to review if we know what were the logical steps leading to
> the change.
> 
> > This commit does the necessary code motion from translate_init.c.inc
> 
> For instance, I don't immediately see why these changes are necessary. I
> see that translate_init.c.inc already has some `#ifdef CONFIG_TCG`, so
> why do we need to move a bunch of code into cpu.c instead of just adding
> more code under ifdef CONFIG_TCG? (I'm not saying it's wrong, just trying to
> understand the reasoning).
> 
> Is translate_init.c.inc intended to be TCG only? But then I see you
> moved TCG-only functions out of it (ppc_fixup_cpu) and left not TCG-only
> functions (gen_spr_generic).
> 
> > This moves all functions that start with gdb_* into target/ppc/gdbstub.c
> > and creates a new function that calls those and is called by ppc_cpu_realize
> 
> This looks like it makes sense regardless of disable-tcg, could we have
> it in a standalone patch?
> 
> > All functions related to realizing the cpu have been moved to cpu.c, which
> > may call functions from gdbstub or translate_init
> 
> Again, I don't disagree with this, but at first sight it doesn't seem
> entirely related to disabling TCG.

Fabioano's points seconded.  This isn't necessarily a bad idea, but a
rationale would really help.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2021-04-12  5:27 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-09 15:19 [RFC PATCH 0/4] target/ppc: add disable-tcg option Bruno Larsen (billionai)
2021-04-09 15:19 ` [PATCH 1/4] target/ppc: Code motion required to build disabling tcg Bruno Larsen (billionai)
2021-04-09 19:48   ` Fabiano Rosas
2021-04-12  4:34     ` David Gibson [this message]
2021-04-09 15:19 ` [PATCH 2/4] target/ppc: added solutions for building with disable-tcg Bruno Larsen (billionai)
2021-04-09 20:40   ` Fabiano Rosas
2021-04-12  5:08   ` David Gibson
2021-04-12 15:40     ` Richard Henderson
2021-04-13  6:42       ` David Gibson
2021-04-09 15:19 ` [PATCH 3/4] target/ppc: Add stubs for tcg functions, so it builds Bruno Larsen (billionai)
2021-04-19  5:28   ` David Gibson
2021-04-09 15:19 ` [PATCH 4/4] target/ppc: updated build rules for disable-tcg option Bruno Larsen (billionai)
2021-04-09 15:57 ` [RFC PATCH 0/4] target/ppc: add " no-reply
2021-04-12  5:13   ` David Gibson
2021-04-12  4:32 ` David Gibson
2021-04-12 12:05 [PATCH 1/4] target/ppc: Code motion required to build disabling tcg Bruno Piazera Larsen
2021-04-12 13:56 ` Fabiano Rosas
2021-04-13  6:40 ` David Gibson
2021-04-13 17:43 Bruno Piazera Larsen
2021-04-13 21:38 ` Fabiano Rosas
2021-04-14 12:04   ` Bruno Piazera Larsen
2021-04-14 20:05     ` Fabiano Rosas
2021-04-19  5:23   ` David Gibson
2021-04-14 19:37 ` Richard Henderson
2021-04-14 20:07   ` Bruno Piazera Larsen
2021-04-14 20:32     ` Richard Henderson
2021-04-19  5:21 ` David Gibson
2021-04-19 14:40 Bruno Piazera Larsen
2021-04-20  1:20 ` David Gibson
2021-04-20 19:02 Bruno Piazera Larsen
2021-04-21  5:13 ` David Gibson
2021-04-22 12:34 Bruno Piazera Larsen
2021-04-22 19:35 ` Fabiano Rosas
2021-04-23  0:08   ` David Gibson
2021-04-23 13:28     ` Fabiano Rosas
2021-04-27  1:29       ` David Gibson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YHPNwtCDtq7XGP/A@yekko.fritz.box \
    --to=david@gibson.dropbear.id.au \
    --cc=andre.silva@eldorado.org.br \
    --cc=bruno.larsen@eldorado.org.br \
    --cc=farosas@linux.ibm.com \
    --cc=fernando.valle@eldorado.org.br \
    --cc=lagarcia@br.ibm.com \
    --cc=lucas.araujo@eldorado.org.br \
    --cc=luis.pires@eldorado.org.br \
    --cc=matheus.ferst@eldorado.org.br \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    --subject='Re: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).