Re: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg

From: David Gibson <david@gibson.dropbear.id.au>
To: Fabiano Rosas <farosas@linux.ibm.com>
Cc: Thomas Huth <thuth@redhat.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"lagarcia@br.ibm.com" <lagarcia@br.ibm.com>,
	Lucas Mateus Martins Araujo e Castro
	<lucas.araujo@eldorado.org.br>,
	Fernando Eckhardt Valle <fernando.valle@eldorado.org.br>,
	"qemu-ppc@nongnu.org" <qemu-ppc@nongnu.org>,
	Andre Fernando da Silva <andre.silva@eldorado.org.br>,
	Bruno Piazera Larsen <bruno.larsen@eldorado.org.br>,
	Matheus Kowalczuk Ferst <matheus.ferst@eldorado.org.br>,
	Luis Fernando Fujita Pires <luis.pires@eldorado.org.br>
Subject: Re: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg
Date: Fri, 23 Apr 2021 10:08:05 +1000	[thread overview]
Message-ID: <YIIP5RVQvS+Xd+WF@yekko.fritz.box> (raw)
In-Reply-To: <87mttq15a1.fsf@linux.ibm.com>

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

On Thu, Apr 22, 2021 at 04:35:34PM -0300, Fabiano Rosas wrote:
> Bruno Piazera Larsen <bruno.larsen@eldorado.org.br> writes:
> 
> >> > You are correct! I've just tweaked the code that defines spr_register and
> >> > it should be working now. I'm still working in splitting the SPR functions
> >> > from translate_init, since I think it would make it easier to prepare the
> >> > !TCG case and for adding new architectures in the future, and I found a
> >> > few more problems:
> >>
> >> Actually looking at the stuff below, I suspect that separating our
> >> "spr" logic specifically might be a bad idea.  At least some of the
> >> SPRs control pretty fundamental things about how the processor
> >> operates, and I suspect separating it from the main translation logic
> >> may be more trouble than it's worth.
> 
> I disagree with the code proximity argument. Having TCG code clearly
> separate from common code seems more important to me than having the SPR
> callbacks close to the init_proc functions.

Hmm.. I may be misinterpreting what you're intending here.  I
certainly agree that separating TCG only code from common code is a
good idea.  My point, though, is that the vast majority of the SPR
code *is* TCG specific - there are just a relatively few cases where
SPRs have a common path.  That basically only happens when a) the SPR
can be affected by means other than the guest executing instructions
specifically to do that (i.e. usually by hypercalls) and b) accessing
the SPR has some side effects that need to be handled in both TCG and
KVM cases

From the descriptions it sounded like you were trying to separate
*all* SPR code, not just these specific cases from the translation
core, and that's what I'm saying is a bad idea.

> But maybe we should take a look at this RFC before we start discussing
> personal preference too much.
> 
> > Well, all the errors that I got were related to to read/write functions, which
> > I was already separating into a spr_tcg file. The solutions I can see are to
> > include this file in translate.c, and either have the read/write functions not be
> > static, or include the spr_common.c in translate as well, but only for TCG
> > builds. Both solutions sound pretty bad imo, but the first sounds less bad,
> > because it's a bit less complexity in the build process.
> 
> It would be helpful if we could apply these patches and do some
> experimentation before recommending a solution. So I would pick the less
> bad for now. Mention it in the cover letter and then we can discuss
> looking at something more concrete.
> 

-- 
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 --]