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

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

[Bruno Piazera Larsen]

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

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

Fair point, I should've thought about that.

> > 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).

There are 3 main reasons for this decision. The first is kind of silly, but when I read translate.c my mind jumped to translating machine code to TCG, and the amount of TCGv variables at the start reinforced this notion.
The second was that both s390x and i386 removed it (translate.c) from compilation, so I had no good reason to doubt it.
The last (and arguably most important) is that translate.c is many thousands of lines long (translate_init.c.inc alone was almost 11k). The whole point of disabling TCG is to speed up compilation and reduce the final file size, so I think it makes sense to remove that big file.
And the final nail in the coffin is that at no point did it cross my mind to keep the init part of translation, but remove the rest

Also, I'm not a fan of big ifdefs, because it's kinda hard to follow them when viewing code in vim. Adding that to already having a cpu.c file, where it makes sense (to me, at least) to add all CPU related functions, just sounded like a good idea.

> 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 is me misjudging what is TCG and what is not, mostly. I think that actually moving everything to cpu.c and adding ifdefs, or creating a cpu_tcg.c.inc or similar, would be the most sensible code motion, but every function I removed from translate gave me 3 new errors, at some point I felt like I should leave something behind otherwise we're compiling everything from TCG again, just in different files, so I tried to guess what was TCG and what was not (also, I really wanted the RFC out by the weekend, I _may_ have rushed a few choices).

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

Sure, I'll try and get it ready ASAP, and make sure I didn't miss one function before sending. Just a noob question... do I edit the patch manually to have it only contain the gdb code motion, or is there a way to move back to before I actually made the commit without needing to re-do the changes?

Thomas, I'm adding you to this discussion since it sort of answers your message on the other one, about the functions used even in a KVM-only build.

> IIRC you don't only have to exclude translate.c from the build, you also
> have to separate translate_init.c.inc from it, i.e. turn
> translate_init.c.inc into a proper .c file and get rid of the #include
> "translate_init.c.inc" statement in translate.c, since many functions in the
> translate_init.c.inc file are still needed for the KVM-only builds, too. So
> maybe that's a good place to start as a first mini series.

Now that I know that translate doesn't mean "translating to TCG", this idea makes more sense. My question is, is it a better option than the code motion I'm suggesting? From my quick check on the bits that I haven't touched it might, but at this point it's clear I'm very lost with what makes sense hahaha.


Bruno Piazera Larsen

Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>

Departamento Computação Embarcada

Analista de Software Trainee

Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>

[-- Attachment #2: Type: text/html, Size: 10238 bytes --]

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

[Fabiano Rosas]

Bruno Piazera Larsen <bruno.larsen@eldorado.org.br> writes:

>> > 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).
>
> There are 3 main reasons for this decision. The first is kind of
> silly, but when I read translate.c my mind jumped to translating
> machine code to TCG, and the amount of TCGv variables at the start
> reinforced this notion.  The second was that both s390x and i386
> removed it (translate.c) from compilation, so I had no good reason to
> doubt it.  The last (and arguably most important) is that translate.c
> is many thousands of lines long (translate_init.c.inc alone was almost
> 11k). The whole point of disabling TCG is to speed up compilation and
> reduce the final file size, so I think it makes sense to remove that
> big file.  And the final nail in the coffin is that at no point did it
> cross my mind to keep the init part of translation, but remove the
> rest

Ok, so whatever you decide to do, make sure you state: "this is where
TCG functions go", "this file is built on KVM|TCG only", and so on. And
it's ok in principle to do a partial move for the RFC, but please
mention that somewhere so we're all in the same page.

>
> Also, I'm not a fan of big ifdefs, because it's kinda hard to follow
> them when viewing code in vim. Adding that to already having a cpu.c
> file, where it makes sense (to me, at least) to add all CPU related
> functions, just sounded like a good idea.

My point about ifdefs is that it would be easier to understand if you
either:

a) wrapped code in ifdefs, got the RCF going and then later moved all
ifdef'ed code into a new tcg-only file;

or

b) define which is the tcg-only file right away and just move code in
there.

When you moved code into cpu.c *along with* the ifdefs, you kind of did
both at the same time, which got confusing; to me at least.

About moving CPU related functions into cpu.c, that's fine. But it is
not strictly related to disable-tcg, so maybe send that in a separate
patch that does it explicitly at the start of the series.

>
>> 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 is me misjudging what is TCG and what is not, mostly. I think
> that actually moving everything to cpu.c and adding ifdefs, or
> creating a cpu_tcg.c.inc or similar, would be the most sensible code
> motion, but every function I removed from translate gave me 3 new
> errors, at some point I felt like I should leave something behind
> otherwise we're compiling everything from TCG again, just in different
> files, so I tried to guess what was TCG and what was not (also, I
> really wanted the RFC out by the weekend, I _may_ have rushed a few
> choices).

Ok, so you left out some things on purpose to reduce complexity for the
first RFC. That's fine, we just need to state these kinds of thing more
clearly.

>
>> > 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?
>
> Sure, I'll try and get it ready ASAP, and make sure I didn't miss one
> function before sending. Just a noob question... do I edit the patch
> manually to have it only contain the gdb code motion, or is there a
> way to move back to before I actually made the commit without needing
> to re-do the changes?

You could git rebase -i back into your first patch and then split it in
two (git reset HEAD~1 and stage + commit each one), one for moving CPU
code into cpu.c and other for moving GDB code into gdbstub.c.

Or just checkout a new branch, apply the patch on top of it and commit
just the GDB change.

Feel free to ping me on IRC if you need help.

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

[David Gibson]

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

On Mon, Apr 12, 2021 at 12:05:31PM +0000, Bruno Piazera Larsen wrote:
> > 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.
> 
> Fair point, I should've thought about that.
> 
> > > 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).
> 
> There are 3 main reasons for this decision. The first is kind of silly, but when I read translate.c my mind jumped to translating machine code to TCG, and the amount of TCGv variables at the start reinforced this notion.
> The second was that both s390x and i386 removed it (translate.c) from compilation, so I had no good reason to doubt it.
> The last (and arguably most important) is that translate.c is many thousands of lines long (translate_init.c.inc alone was almost 11k). The whole point of disabling TCG is to speed up compilation and reduce the final file size, so I think it makes sense to remove that big file.
> And the final nail in the coffin is that at no point did it cross my mind to keep the init part of translation, but remove the rest
> 
> Also, I'm not a fan of big ifdefs, because it's kinda hard to follow them when viewing code in vim. Adding that to already having a cpu.c file, where it makes sense (to me, at least) to add all CPU related functions, just sounded like a good idea.

I think those are all sound reasons; I think not compiling translate.c
for !tcg builds is the right choice.  We might, however, need to
"rescue" certain functions from there by moving them to another file
so they can be used by KVM code as well.

> > 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 is me misjudging what is TCG and what is not, mostly. I think that actually moving everything to cpu.c and adding ifdefs, or creating a cpu_tcg.c.inc or similar, would be the most sensible code motion, but every function I removed from translate gave me 3 new errors, at some point I felt like I should leave something behind otherwise we're compiling everything from TCG again, just in different files, so I tried to guess what was TCG and what was not (also, I really wanted the RFC out by the weekend, I _may_ have rushed a few choices).

I'm actually not sure if we'll want translate_init.c for !tcg builds.
It's *primarily* for TCG, but we still need at least some of the cpu
state structure for KVM, and some of that is initialized in
translate_init.

I think it will probably make more sense to leave it in for a first
cut.  Later refinement might end up removing it.

The whole #include translate_init.c.inc thing might make for some
awkward fiddling in this, of course.

> > > 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?
> 
> Sure, I'll try and get it ready ASAP, and make sure I didn't miss one function before sending. Just a noob question... do I edit the patch manually to have it only contain the gdb code motion, or is there a way to move back to before I actually made the commit without needing to re-do the changes?
> 
> Thomas, I'm adding you to this discussion since it sort of answers your message on the other one, about the functions used even in a KVM-only build.
> 
> > IIRC you don't only have to exclude translate.c from the build, you also
> > have to separate translate_init.c.inc from it, i.e. turn
> > translate_init.c.inc into a proper .c file and get rid of the #include
> > "translate_init.c.inc" statement in translate.c, since many functions in the
> > translate_init.c.inc file are still needed for the KVM-only builds, too. So
> > maybe that's a good place to start as a first mini series.
> 
> Now that I know that translate doesn't mean "translating to TCG", this idea makes more sense. My question is, is it a better option than the code motion I'm suggesting? From my quick check on the bits that I haven't touched it might, but at this point it's clear I'm very lost with what makes sense hahaha.
> 
> 
> Bruno Piazera Larsen
> 
> Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>
> 
> Departamento Computação Embarcada
> 
> Analista de Software Trainee
> 
> Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>

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

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

[Bruno Piazera Larsen]

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

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

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.

Other than that, I don't really see how we can support a !TCG build without
separating SPR, exactly because they are very fundamental to the processor.
Am I missing something? I fully expect to be, at this point, things are
turning out even more complicated than I expected



Bruno Piazera Larsen

Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>

Departamento Computação Embarcada

Analista de Software Trainee

Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>

________________________________
De: David Gibson
Enviadas: Quarta-feira, 21 de Abril de 2021 02:13
Para: Bruno Piazera Larsen
Cc: Fabiano Rosas; Thomas Huth; qemu-devel@nongnu.org; lagarcia@br.ibm.com; Lucas Mateus Martins Araujo e Castro; Fernando Eckhardt Valle; qemu-ppc@nongnu.org; Andre Fernando da Silva; Matheus Kowalczuk Ferst; Luis Fernando Fujita Pires
Assunto: Re: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg

On Tue, Apr 20, 2021 at 07:02:36PM +0000, Bruno Piazera Larsen wrote:
> > > What I was doing was to only register the spr once, and use the
> > > accel-specific functions to set the relevant attributes, so spr_common
> > > wouldn't need to where (and if) spr_read_* exists or not.
> > > Would this work?
> > >
> > > Just ignoring the read and write functions means we still need
> > > to compile them, or at least stub them, otherwise we'd get linker
> > > problems.
> >
> > Not if you use a macro which will simply elide the references in the
> > !TCG case.  Actually I think even an inline wrapper will do it, I'm
> > pretty sure the compiler is smart enough to optimize the references
> > out in that case.
>
> 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.

> 1. Global variables being defined in translate.c and used both there and
> in spr functions. The variables in question are: cpu_gpr, cpu_so, cpu_ov,
> cpu_ca, cpu_ov32, cpu_ca32, cpu_xer, cpu_lr and cpu_ctr. The easy way
> out is to have global "getters" and "setters" for those, but I'm not sure
> it's a good solution.

Yeah, that seems really messy, plus those are special variables used
by TCG generated code whose operation I don't really understand.

> 2. Static functions defined in translate.c, used there and TCG-related
> spr functions. They are: gen_load_spr, gen_store_spr, gen_stop_exception,
> gen_inval_exception. The easy solution is adding a prototype to internal.h
> and removing the static, but again, not sure it's a good solution

I think gen_*() functions should stay in translate.c, since they are,
explicitly, about translation ("gen" for "generating code").

> 3. gen_read_xer (currently in spr_common) calls is_isa300, defined in
> include/disas/disas.h, which is a macro that dereferences DisasContext.
> However, the struct is defined in translate.c. This one is pretty easy, I think,
> it's just turning the macro into a function, but since I'm already e-mailing,
> I figured I might as weel ask.
>
> Finally, since most read and write functions are static, I added them to
> spr_tcg.c.inc to be included only with TCG, as a quick fix, but I would really
> prefer some other solution if there is anything better. Any thoughts on this?
>
> IIRC, this is the last thing holding me back from an RFC with this motion
> patch
>
>
> Bruno Piazera Larsen
>
> Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>
>
> Departamento Computação Embarcada
>
> Analista de Software Trainee
>
> Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>

--
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: Type: text/html, Size: 12042 bytes --]

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

[Fabiano Rosas]

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.

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.

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

[David Gibson]

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

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

[Fabiano Rosas]

David Gibson <david@gibson.dropbear.id.au> writes:

> 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

The SPR code in translate_init.c.inc currently comprises of:

1) the gen_spr* functions that are called during init_proc for each
processor type;

2) the spr_register macros and _spr_register function that adds the SPRs
to env->spr, called from (1);

3) the TCG-specific SPR read|write callbacks, registered by (2);

4) the KVM specific attribute one_reg_id, registered by (2).

The intention is to have one .c file (cpu_init.c) that deals with
processor initialization, which is mostly setting PowerPCCPUClass
attributes and registering the appropriate SPRs for each processor
family (1,2). We're considering that to be shared between KVM and TCG
for now.

What is going into a separate file are the read and write SPR callbacks,
which are TCG specific (3). They are still referenced from the other
file when registering the SPRs, but are ignored when building for
KVM-only. These are kept in a TCG-only compilation unit. There's still a
decision to be made whether we should have a separate spr_tcg file for
them, or move them into translate.c along with the rest of TCG code.

The one_reg_id is just one attribute so that does not change.

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

So, if anything, the SPR callbacks are moving _closer_ to the
translation core.

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

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

[David Gibson]

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

On Fri, Apr 23, 2021 at 10:28:14AM -0300, Fabiano Rosas wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > 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
> 
> The SPR code in translate_init.c.inc currently comprises of:
> 
> 1) the gen_spr* functions that are called during init_proc for each
> processor type;

Ah... that's one part of the confusion.  I forgot about these
functions.  These should indeed be common, despite sharing the gen_*()
prefix with mostly things that are explicitly TCG only.

> 2) the spr_register macros and _spr_register function that adds the SPRs
> to env->spr, called from (1);
> 
> 3) the TCG-specific SPR read|write callbacks, registered by (2);
> 
> 4) the KVM specific attribute one_reg_id, registered by (2).
> 
> The intention is to have one .c file (cpu_init.c) that deals with
> processor initialization, which is mostly setting PowerPCCPUClass
> attributes and registering the appropriate SPRs for each processor
> family (1,2). We're considering that to be shared between KVM and TCG
> for now.

Yes, that's what I'd expect.

> What is going into a separate file are the read and write SPR callbacks,
> which are TCG specific (3). They are still referenced from the other
> file when registering the SPRs, but are ignored when building for
> KVM-only. These are kept in a TCG-only compilation unit.

Ah, right, I'd forgotten that many of the callbacks are in
translate_init.c not translate.c.  Indeed, those will have to move.

> There's still a
> decision to be made whether we should have a separate spr_tcg file for
> them, or move them into translate.c along with the rest of TCG code.

Ah, I see.  Ok, yes, in that case moving them to a new TCG only spr
file makes more sense to me.  translate.c is already enormous.

> 
> The one_reg_id is just one attribute so that does not change.
> 
> > 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.
> 
> So, if anything, the SPR callbacks are moving _closer_ to the
> translation core.

Right.  Sorry for the misunderstanding.

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

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

[Bruno Piazera Larsen]

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

> > What I was doing was to only register the spr once, and use the
> > accel-specific functions to set the relevant attributes, so spr_common
> > wouldn't need to where (and if) spr_read_* exists or not.
> > Would this work?
> >
> > Just ignoring the read and write functions means we still need
> > to compile them, or at least stub them, otherwise we'd get linker
> > problems.
>
> Not if you use a macro which will simply elide the references in the
> !TCG case.  Actually I think even an inline wrapper will do it, I'm
> pretty sure the compiler is smart enough to optimize the references
> out in that case.

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:

1. Global variables being defined in translate.c and used both there and
in spr functions. The variables in question are: cpu_gpr, cpu_so, cpu_ov,
cpu_ca, cpu_ov32, cpu_ca32, cpu_xer, cpu_lr and cpu_ctr. The easy way
out is to have global "getters" and "setters" for those, but I'm not sure
it's a good solution.

2. Static functions defined in translate.c, used there and TCG-related
spr functions. They are: gen_load_spr, gen_store_spr, gen_stop_exception,
gen_inval_exception. The easy solution is adding a prototype to internal.h
and removing the static, but again, not sure it's a good solution

3. gen_read_xer (currently in spr_common) calls is_isa300, defined in
include/disas/disas.h, which is a macro that dereferences DisasContext.
However, the struct is defined in translate.c. This one is pretty easy, I think,
it's just turning the macro into a function, but since I'm already e-mailing,
I figured I might as weel ask.

Finally, since most read and write functions are static, I added them to
spr_tcg.c.inc to be included only with TCG, as a quick fix, but I would really
prefer some other solution if there is anything better. Any thoughts on this?

IIRC, this is the last thing holding me back from an RFC with this motion
patch


Bruno Piazera Larsen

Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>

Departamento Computação Embarcada

Analista de Software Trainee

Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>

[-- Attachment #2: Type: text/html, Size: 6820 bytes --]

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

[David Gibson]

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

On Tue, Apr 20, 2021 at 07:02:36PM +0000, Bruno Piazera Larsen wrote:
> > > What I was doing was to only register the spr once, and use the
> > > accel-specific functions to set the relevant attributes, so spr_common
> > > wouldn't need to where (and if) spr_read_* exists or not.
> > > Would this work?
> > >
> > > Just ignoring the read and write functions means we still need
> > > to compile them, or at least stub them, otherwise we'd get linker
> > > problems.
> >
> > Not if you use a macro which will simply elide the references in the
> > !TCG case.  Actually I think even an inline wrapper will do it, I'm
> > pretty sure the compiler is smart enough to optimize the references
> > out in that case.
> 
> 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.

> 1. Global variables being defined in translate.c and used both there and
> in spr functions. The variables in question are: cpu_gpr, cpu_so, cpu_ov,
> cpu_ca, cpu_ov32, cpu_ca32, cpu_xer, cpu_lr and cpu_ctr. The easy way
> out is to have global "getters" and "setters" for those, but I'm not sure
> it's a good solution.

Yeah, that seems really messy, plus those are special variables used
by TCG generated code whose operation I don't really understand.

> 2. Static functions defined in translate.c, used there and TCG-related
> spr functions. They are: gen_load_spr, gen_store_spr, gen_stop_exception,
> gen_inval_exception. The easy solution is adding a prototype to internal.h
> and removing the static, but again, not sure it's a good solution

I think gen_*() functions should stay in translate.c, since they are,
explicitly, about translation ("gen" for "generating code").

> 3. gen_read_xer (currently in spr_common) calls is_isa300, defined in
> include/disas/disas.h, which is a macro that dereferences DisasContext.
> However, the struct is defined in translate.c. This one is pretty easy, I think,
> it's just turning the macro into a function, but since I'm already e-mailing,
> I figured I might as weel ask.
> 
> Finally, since most read and write functions are static, I added them to
> spr_tcg.c.inc to be included only with TCG, as a quick fix, but I would really
> prefer some other solution if there is anything better. Any thoughts on this?
> 
> IIRC, this is the last thing holding me back from an RFC with this motion
> patch
> 
> 
> Bruno Piazera Larsen
> 
> Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>
> 
> Departamento Computação Embarcada
> 
> Analista de Software Trainee
> 
> Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>

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

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

[Bruno Piazera Larsen]

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

> > > * move gen_write_xer and gen_read_xer into cpu_init.c, as they're
> > > used for some sprs, and whatever needs to be moved with it
> >
> > I'd leave them where they are currently. Instead what I think we should
> > do is to find a way to not need the uea/oea/hea|read/write callbacks
> > with KVM.
> >
> > Maybe extract a function from _spr_register that sets what is common for
> > both tcg and kvm (num, name, initial_value, AFAICT). Then alter the
> > gen_spr* functions to first create all registers and then call both
> > configs to supplement:
> >
> > //tcg.c
> > static void tcg_gen_spr_generic(CPUPPCState *env)
> > {
> >     // these only set the callbacks
> >     spr_register(env, SPR_FOO,
> >                  SPR_NOACCESS, SPR_NOACCESS,
> >                  &spr_read_foo, &spr_write_foo);
> >     spr_register(env, SPR_BAR,
> >                  SPR_NOACCESS, SPR_NOACCESS,
> >                  &spr_read_bar, &spr_write_bar);
> > }
> >
> > //kvm.c
> > static void kvm_gen_spr_generic(CPUPPCState *env)
> > {
> >     // these only set one_reg_id
> >     spr_register_kvm(env, SPR_FOO, KVM_REG_PPC_FOO);
> >     spr_register_kvm(env, SPR_BAR, KVM_REG_PPC_BAR);
> > }
>
> I really dislike the idea above - it'd be way too easy for KVM and TCG
> to get out of sync.  Instead make spr_register() itself a macro if
> necessary, so it just ignores the access functions in the !TCG case.

What I was doing was to only register the spr once, and use the
accel-specific functions to set the relevant attributes, so spr_common
wouldn't need to where (and if) spr_read_* exists or not.
Would this work?

Just ignoring the read and write functions means we still need
to compile them, or at least stub them, otherwise we'd get linker
problems. And ifdef'ing them out of the calls would be quite a
pain to understand the code later on.



Bruno Piazera Larsen

Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>

Departamento Computação Embarcada

Analista de Software Trainee

Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>

________________________________

[-- Attachment #2: Type: text/html, Size: 7276 bytes --]

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

[David Gibson]

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

On Mon, Apr 19, 2021 at 02:40:35PM +0000, Bruno Piazera Larsen wrote:
> > > > * move gen_write_xer and gen_read_xer into cpu_init.c, as they're
> > > > used for some sprs, and whatever needs to be moved with it
> > >
> > > I'd leave them where they are currently. Instead what I think we should
> > > do is to find a way to not need the uea/oea/hea|read/write callbacks
> > > with KVM.
> > >
> > > Maybe extract a function from _spr_register that sets what is common for
> > > both tcg and kvm (num, name, initial_value, AFAICT). Then alter the
> > > gen_spr* functions to first create all registers and then call both
> > > configs to supplement:
> > >
> > > //tcg.c
> > > static void tcg_gen_spr_generic(CPUPPCState *env)
> > > {
> > >     // these only set the callbacks
> > >     spr_register(env, SPR_FOO,
> > >                  SPR_NOACCESS, SPR_NOACCESS,
> > >                  &spr_read_foo, &spr_write_foo);
> > >     spr_register(env, SPR_BAR,
> > >                  SPR_NOACCESS, SPR_NOACCESS,
> > >                  &spr_read_bar, &spr_write_bar);
> > > }
> > >
> > > //kvm.c
> > > static void kvm_gen_spr_generic(CPUPPCState *env)
> > > {
> > >     // these only set one_reg_id
> > >     spr_register_kvm(env, SPR_FOO, KVM_REG_PPC_FOO);
> > >     spr_register_kvm(env, SPR_BAR, KVM_REG_PPC_BAR);
> > > }
> >
> > I really dislike the idea above - it'd be way too easy for KVM and TCG
> > to get out of sync.  Instead make spr_register() itself a macro if
> > necessary, so it just ignores the access functions in the !TCG case.
> 
> What I was doing was to only register the spr once, and use the
> accel-specific functions to set the relevant attributes, so spr_common
> wouldn't need to where (and if) spr_read_* exists or not.
> Would this work?
> 
> Just ignoring the read and write functions means we still need
> to compile them, or at least stub them, otherwise we'd get linker
> problems.

Not if you use a macro which will simply elide the references in the
!TCG case.  Actually I think even an inline wrapper will do it, I'm
pretty sure the compiler is smart enough to optimize the references
out in that case.

> And ifdef'ing them out of the calls would be quite a
> pain to understand the code later on.
> 
> 
> 
> Bruno Piazera Larsen
> 
> Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>
> 
> Departamento Computação Embarcada
> 
> Analista de Software Trainee
> 
> Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>
> 
> ________________________________

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

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

[Bruno Piazera Larsen]

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

> I'm actually not sure if we'll want translate_init.c for !tcg builds.
> It's *primarily* for TCG, but we still need at least some of the cpu
> state structure for KVM, and some of that is initialized in
> translate_init.
>
> I think it will probably make more sense to leave it in for a first
> cut.  Later refinement might end up removing it.
>
> The whole #include translate_init.c.inc thing might make for some
> awkward fiddling in this, of course.

I just checked, there is going to be some shuffling of functions
around, as there are some static variables defined on translate.c,
and used in translate_init.c.inc, some functions needed for KVM
on translate.c and some TCG only functions in the
translate_init.c.inc.

The trivial path is to:
* rename translate_init.c.inc to cpu_init.c (since it has to do with
initial definitions for CPUs, and it's not related to translating
anymore);
* move gen_write_xer and gen_read_xer into cpu_init.c, as they're
used for some sprs, and whatever needs to be moved with it
* move is_indirect_opcode and ind_table to translate.c, since they
are used to translate ppc instructions, and the things defined for
these functions
* Figure out what needs to be added to the includes for both
files to compile
* move opcodes and invalid_handler into cpu_init.c, because they
are only used by stuff in this file.

I'm just not sure about this last point. The stuff that use opcodes
create the callback tables for TCG, AFAICT. The better plan would
be to move all of that to tanslate.c, but might be a lot.

Can I follow the trivial plan for the first cut and leave a TODO in
the code for a better solution in the future? Or is there a nuance
about one of those functions that I have not understood?


Bruno Piazera Larsen

Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>

Departamento Computação Embarcada

Analista de Software Trainee

Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>

________________________________
De: David Gibson
Enviadas: Terça-feira, 13 de Abril de 2021 03:40
Para: Bruno Piazera Larsen
Cc: Fabiano Rosas; Thomas Huth; qemu-devel@nongnu.org; Luis Fernando Fujita Pires; Andre Fernando da Silva; Lucas Mateus Martins Araujo e Castro; Fernando Eckhardt Valle; qemu-ppc@nongnu.org; lagarcia@br.ibm.com; Matheus Kowalczuk Ferst
Assunto: Re: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg

On Mon, Apr 12, 2021 at 12:05:31PM +0000, Bruno Piazera Larsen wrote:
> > 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.
>
> Fair point, I should've thought about that.
>
> > > 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).
>
> There are 3 main reasons for this decision. The first is kind of silly, but when I read translate.c my mind jumped to translating machine code to TCG, and the amount of TCGv variables at the start reinforced this notion.
> The second was that both s390x and i386 removed it (translate.c) from compilation, so I had no good reason to doubt it.
> The last (and arguably most important) is that translate.c is many thousands of lines long (translate_init.c.inc alone was almost 11k). The whole point of disabling TCG is to speed up compilation and reduce the final file size, so I think it makes sense to remove that big file.
> And the final nail in the coffin is that at no point did it cross my mind to keep the init part of translation, but remove the rest
>
> Also, I'm not a fan of big ifdefs, because it's kinda hard to follow them when viewing code in vim. Adding that to already having a cpu.c file, where it makes sense (to me, at least) to add all CPU related functions, just sounded like a good idea.

I think those are all sound reasons; I think not compiling translate.c
for !tcg builds is the right choice.  We might, however, need to
"rescue" certain functions from there by moving them to another file
so they can be used by KVM code as well.

> > 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 is me misjudging what is TCG and what is not, mostly. I think that actually moving everything to cpu.c and adding ifdefs, or creating a cpu_tcg.c.inc or similar, would be the most sensible code motion, but every function I removed from translate gave me 3 new errors, at some point I felt like I should leave something behind otherwise we're compiling everything from TCG again, just in different files, so I tried to guess what was TCG and what was not (also, I really wanted the RFC out by the weekend, I _may_ have rushed a few choices).


> > > 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?
>
> Sure, I'll try and get it ready ASAP, and make sure I didn't miss one function before sending. Just a noob question... do I edit the patch manually to have it only contain the gdb code motion, or is there a way to move back to before I actually made the commit without needing to re-do the changes?
>
> Thomas, I'm adding you to this discussion since it sort of answers your message on the other one, about the functions used even in a KVM-only build.
>
> > IIRC you don't only have to exclude translate.c from the build, you also
> > have to separate translate_init.c.inc from it, i.e. turn
> > translate_init.c.inc into a proper .c file and get rid of the #include
> > "translate_init.c.inc" statement in translate.c, since many functions in the
> > translate_init.c.inc file are still needed for the KVM-only builds, too. So
> > maybe that's a good place to start as a first mini series.
>
> Now that I know that translate doesn't mean "translating to TCG", this idea makes more sense. My question is, is it a better option than the code motion I'm suggesting? From my quick check on the bits that I haven't touched it might, but at this point it's clear I'm very lost with what makes sense hahaha.
>
>
> Bruno Piazera Larsen
>
> Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>
>
> Departamento Computação Embarcada
>
> Analista de Software Trainee
>
> Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>

--
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: Type: text/html, Size: 15048 bytes --]

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

[Fabiano Rosas]

Bruno Piazera Larsen <bruno.larsen@eldorado.org.br> writes:

>> I'm actually not sure if we'll want translate_init.c for !tcg builds.
>> It's *primarily* for TCG, but we still need at least some of the cpu
>> state structure for KVM, and some of that is initialized in
>> translate_init.
>>
>> I think it will probably make more sense to leave it in for a first
>> cut.  Later refinement might end up removing it.
>>
>> The whole #include translate_init.c.inc thing might make for some
>> awkward fiddling in this, of course.
>
> I just checked, there is going to be some shuffling of functions
> around, as there are some static variables defined on translate.c,
> and used in translate_init.c.inc, some functions needed for KVM
> on translate.c and some TCG only functions in the
> translate_init.c.inc.
>
> The trivial path is to:
> * rename translate_init.c.inc to cpu_init.c (since it has to do with
> initial definitions for CPUs, and it's not related to translating
> anymore);

Below I'm assuming we have one place for TCG stuff and other for KVM
stuff, whatever this particular discussion ends up producing.

> * move gen_write_xer and gen_read_xer into cpu_init.c, as they're
> used for some sprs, and whatever needs to be moved with it

I'd leave them where they are currently. Instead what I think we should
do is to find a way to not need the uea/oea/hea|read/write callbacks
with KVM.

Maybe extract a function from _spr_register that sets what is common for
both tcg and kvm (num, name, initial_value, AFAICT). Then alter the
gen_spr* functions to first create all registers and then call both
configs to supplement:

//tcg.c
static void tcg_gen_spr_generic(CPUPPCState *env)
{
    // these only set the callbacks        
    spr_register(env, SPR_FOO,
                 SPR_NOACCESS, SPR_NOACCESS,
                 &spr_read_foo, &spr_write_foo);
    spr_register(env, SPR_BAR,
                 SPR_NOACCESS, SPR_NOACCESS,
                 &spr_read_bar, &spr_write_bar);
}

//kvm.c
static void kvm_gen_spr_generic(CPUPPCState *env)
{
    // these only set one_reg_id
    spr_register_kvm(env, SPR_FOO, KVM_REG_PPC_FOO);
    spr_register_kvm(env, SPR_BAR, KVM_REG_PPC_BAR);
}

//common.c
static void gen_spr_generic(CPUPPCState *env)
{
    // these only set name, num, initial value
    spr_register(env, SPR_FOO, "FOO", 0xf00);
    spr_register(env, SPR_BAR, "BAR", 0xb4d);
    ...

    // have these stubbed if not chosen via config
    tcg_gen_spr_generic(env);
    kvm_gen_spr_generic(env);
}

init_ppc_proc()
{
        ...
        gen_spr_generic(env);
        ...
}

Can anyone see a better way? That would be much easier if we could
afford to say that TCG and KVM are mutually exclusive for a given build,
but I don't think they are.

> * move is_indirect_opcode and ind_table to translate.c, since they
> are used to translate ppc instructions, and the things defined for
> these functions

Makes sense. This and the other part below about callback tables would
be mostly about moving code so it's a candidate for coming soon.

> * Figure out what needs to be added to the includes for both
> files to compile
> * move opcodes and invalid_handler into cpu_init.c, because they
> are only used by stuff in this file.
>
> I'm just not sure about this last point. The stuff that use opcodes
> create the callback tables for TCG, AFAICT. The better plan would
> be to move all of that to tanslate.c, but might be a lot.

translate.c seems like a better place indeed.

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

[Bruno Piazera Larsen]

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

> > * move gen_write_xer and gen_read_xer into cpu_init.c, as they're
> > used for some sprs, and whatever needs to be moved with it
>
> I'd leave them where they are currently. Instead what I think we should
> do is to find a way to not need the uea/oea/hea|read/write callbacks
> with KVM.

so we'd also move all callbacks to translate.c, right? RN, gen_write_xer
is only used in spr_read_xer, which is defined in cpu_init.c

From a quick glance, this would be almost 3k lines, so bigger patches
are incoming (side note: I tried to use that git config to show that I only
changed file names and deal better with code motion, but it doesn't
appear to have worked, is the wiki correct about this?)

> Maybe extract a function from _spr_register that sets what is common for
> both tcg and kvm (num, name, initial_value, AFAICT). Then alter the
> gen_spr* functions to first create all registers and then call both
> configs to supplement:
>
> //tcg.c
> static void tcg_gen_spr_generic(CPUPPCState *env)
> {
>     // these only set the callbacks
>     spr_register(env, SPR_FOO,
>                  SPR_NOACCESS, SPR_NOACCESS,
>                  &spr_read_foo, &spr_write_foo);
>     spr_register(env, SPR_BAR,
>                  SPR_NOACCESS, SPR_NOACCESS,
>                  &spr_read_bar, &spr_write_bar);
> }
>
> //kvm.c
> static void kvm_gen_spr_generic(CPUPPCState *env)
> {
>     // these only set one_reg_id
>     spr_register_kvm(env, SPR_FOO, KVM_REG_PPC_FOO);
>     spr_register_kvm(env, SPR_BAR, KVM_REG_PPC_BAR);
> }

by default, KVM already doesn't use the callbacks? Or would we have to
also change where these registers are accessed? If the first one is right
this looks easy enough.

> //common.c
> static void gen_spr_generic(CPUPPCState *env)
> {
>     // these only set name, num, initial value
>     spr_register(env, SPR_FOO, "FOO", 0xf00);
>     spr_register(env, SPR_BAR, "BAR", 0xb4d);
>     ...
>
>     // have these stubbed if not chosen via config
>     tcg_gen_spr_generic(env);
>     kvm_gen_spr_generic(env);
> }
>
> init_ppc_proc()
> {
>         ...
>         gen_spr_generic(env);
>         ...
> }

I'm guessing we'd need to do this to all gen_spr_* functions, this is just
an example, right?

> Can anyone see a better way? That would be much easier if we could
> afford to say that TCG and KVM are mutually exclusive for a given build,
> but I don't think they are.

Instead of stubbing, we could also create macros that turn the function call
into a nop if the config was disabled, and add "if kvm_enabled()" and
"if tcg_enabled()" if needed. I don't see how TCG and KVM being mutually
exclusive makes this easier, unless it has to do with where they are
accessed (idk yet where that is).

Another option is the solution I prototyped in [PATCH 2/4] in arch_dump.c,
having ifdef encapsulating kvm and tcg calls, and if/else blocks. I'm also
open to suggestions on how to do it better (:

> > * Figure out what needs to be added to the includes for both
> > files to compile
> > * move opcodes and invalid_handler into cpu_init.c, because they
> > are only used by stuff in this file.
> >
> > I'm just not sure about this last point. The stuff that use opcodes
> > create the callback tables for TCG, AFAICT. The better plan would
> > be to move all of that to tanslate.c, but might be a lot.
>
> translate.c seems like a better place indeed.

ok. But is it worth doing for the first cut?

Also, looking now, I see definition for exception vectors and some
exception handling code in it, which I'm not 100% sure what to do with.
It's starting to seem like should actually make this translate_init.c.inc
into a mini series of its own, if we're going to make this the best way
from the start.


Bruno Piazera Larsen

Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>

Departamento Computação Embarcada

Analista de Software Trainee

Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>


[-- Attachment #2: Type: text/html, Size: 13164 bytes --]

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

[Fabiano Rosas]

Bruno Piazera Larsen <bruno.larsen@eldorado.org.br> writes:

>> > * move gen_write_xer and gen_read_xer into cpu_init.c, as they're
>> > used for some sprs, and whatever needs to be moved with it
>>
>> I'd leave them where they are currently. Instead what I think we should
>> do is to find a way to not need the uea/oea/hea|read/write callbacks
>> with KVM.
>
> so we'd also move all callbacks to translate.c, right? RN, gen_write_xer
> is only used in spr_read_xer, which is defined in cpu_init.c

Yeah, move them away from the common file into a tcg-only file.

>
> From a quick glance, this would be almost 3k lines, so bigger patches
> are incoming (side note: I tried to use that git config to show that I only
> changed file names and deal better with code motion, but it doesn't
> appear to have worked, is the wiki correct about this?)
>
>> Maybe extract a function from _spr_register that sets what is common for
>> both tcg and kvm (num, name, initial_value, AFAICT). Then alter the
>> gen_spr* functions to first create all registers and then call both
>> configs to supplement:
>>
>> //tcg.c
>> static void tcg_gen_spr_generic(CPUPPCState *env)
>> {
>>     // these only set the callbacks
>>     spr_register(env, SPR_FOO,
>>                  SPR_NOACCESS, SPR_NOACCESS,
>>                  &spr_read_foo, &spr_write_foo);
>>     spr_register(env, SPR_BAR,
>>                  SPR_NOACCESS, SPR_NOACCESS,
>>                  &spr_read_bar, &spr_write_bar);
>> }
>>
>> //kvm.c
>> static void kvm_gen_spr_generic(CPUPPCState *env)
>> {
>>     // these only set one_reg_id
>>     spr_register_kvm(env, SPR_FOO, KVM_REG_PPC_FOO);
>>     spr_register_kvm(env, SPR_BAR, KVM_REG_PPC_BAR);
>> }
>
> by default, KVM already doesn't use the callbacks? Or would we have to
> also change where these registers are accessed? If the first one is right
> this looks easy enough.

KVM does not use the callbacks.

>> //common.c
>> static void gen_spr_generic(CPUPPCState *env)
>> {
>>     // these only set name, num, initial value
>>     spr_register(env, SPR_FOO, "FOO", 0xf00);
>>     spr_register(env, SPR_BAR, "BAR", 0xb4d);
>>     ...
>>
>>     // have these stubbed if not chosen via config
>>     tcg_gen_spr_generic(env);
>>     kvm_gen_spr_generic(env);
>> }
>>
>> init_ppc_proc()
>> {
>>         ...
>>         gen_spr_generic(env);
>>         ...
>> }
>
> I'm guessing we'd need to do this to all gen_spr_* functions, this is just
> an example, right?

Yeah, so that's one of the downsides of this change I proposed.

>> Can anyone see a better way? That would be much easier if we could
>> afford to say that TCG and KVM are mutually exclusive for a given build,
>> but I don't think they are.
>
> Instead of stubbing, we could also create macros that turn the function call
> into a nop if the config was disabled, and add "if kvm_enabled()" and
> "if tcg_enabled()" if needed. I don't see how TCG and KVM being mutually
> exclusive makes this easier, unless it has to do with where they are
> accessed (idk yet where that is).

If they were mutually exclusive we could solve most problems by having
the same signature for a function and compiling one or the other
depending on the config.

That would mean we would be able to move the whole gen_spr_* functions
to the accel-specific files. So:

//tcg.c
static void gen_spr_generic(CPUPPCState *env)
{
    spr_register(env, SPR_FOO, "FOO", 0xf00, &read_foo, &write_foo);
    spr_register(env, SPR_BAR, "BAR", 0xb4d, &read_bar, &write_bar);
}

//kvm.c
static void gen_spr_generic(CPUPPCState *env)
{
    spr_register(env, SPR_FOO, "FOO", 0xf00, KVM_REG_FOO);
    spr_register(env, SPR_BAR, "BAR", 0xb4d, KVM_REG_BAR);
}

//common.c
init_ppc_proc()
{
        ...
        gen_spr_generic(env);
        ...
}

But we can't do this because we want to have a QEMU binary that supports
both accel types in certain scenarios.

>
> Another option is the solution I prototyped in [PATCH 2/4] in arch_dump.c,
> having ifdef encapsulating kvm and tcg calls, and if/else blocks. I'm also
> open to suggestions on how to do it better (:
>
>> > * Figure out what needs to be added to the includes for both
>> > files to compile
>> > * move opcodes and invalid_handler into cpu_init.c, because they
>> > are only used by stuff in this file.
>> >
>> > I'm just not sure about this last point. The stuff that use opcodes
>> > create the callback tables for TCG, AFAICT. The better plan would
>> > be to move all of that to tanslate.c, but might be a lot.
>>
>> translate.c seems like a better place indeed.
>
> ok. But is it worth doing for the first cut?

I think it is. I don't see the issue. Aside from the opcodes destructor
you'll just move a chunk of code over. We do want cpu_init.c to be the
common file, right? So it cannot have TCG-only code in it. Better do it
now while we're (mostly) just moving code around.

>
> Also, looking now, I see definition for exception vectors and some
> exception handling code in it, which I'm not 100% sure what to do
> with.

These are tricky because there's some logic for system emulation that is
used by kvm, spapr, etc. So we'll need to do more invasive
changes. Let's tackle the easy things first.

> It's starting to seem like should actually make this translate_init.c.inc
> into a mini series of its own, if we're going to make this the best way
> from the start.
>
>
> Bruno Piazera Larsen
>
> Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>
>
> Departamento Computação Embarcada
>
> Analista de Software Trainee
>
> Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>

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

[David Gibson]

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

On Tue, Apr 13, 2021 at 06:38:57PM -0300, Fabiano Rosas wrote:
> Bruno Piazera Larsen <bruno.larsen@eldorado.org.br> writes:
> 
> >> I'm actually not sure if we'll want translate_init.c for !tcg builds.
> >> It's *primarily* for TCG, but we still need at least some of the cpu
> >> state structure for KVM, and some of that is initialized in
> >> translate_init.
> >>
> >> I think it will probably make more sense to leave it in for a first
> >> cut.  Later refinement might end up removing it.
> >>
> >> The whole #include translate_init.c.inc thing might make for some
> >> awkward fiddling in this, of course.
> >
> > I just checked, there is going to be some shuffling of functions
> > around, as there are some static variables defined on translate.c,
> > and used in translate_init.c.inc, some functions needed for KVM
> > on translate.c and some TCG only functions in the
> > translate_init.c.inc.
> >
> > The trivial path is to:
> > * rename translate_init.c.inc to cpu_init.c (since it has to do with
> > initial definitions for CPUs, and it's not related to translating
> > anymore);
> 
> Below I'm assuming we have one place for TCG stuff and other for KVM
> stuff, whatever this particular discussion ends up producing.
> 
> > * move gen_write_xer and gen_read_xer into cpu_init.c, as they're
> > used for some sprs, and whatever needs to be moved with it
> 
> I'd leave them where they are currently. Instead what I think we should
> do is to find a way to not need the uea/oea/hea|read/write callbacks
> with KVM.
> 
> Maybe extract a function from _spr_register that sets what is common for
> both tcg and kvm (num, name, initial_value, AFAICT). Then alter the
> gen_spr* functions to first create all registers and then call both
> configs to supplement:
> 
> //tcg.c
> static void tcg_gen_spr_generic(CPUPPCState *env)
> {
>     // these only set the callbacks        
>     spr_register(env, SPR_FOO,
>                  SPR_NOACCESS, SPR_NOACCESS,
>                  &spr_read_foo, &spr_write_foo);
>     spr_register(env, SPR_BAR,
>                  SPR_NOACCESS, SPR_NOACCESS,
>                  &spr_read_bar, &spr_write_bar);
> }
> 
> //kvm.c
> static void kvm_gen_spr_generic(CPUPPCState *env)
> {
>     // these only set one_reg_id
>     spr_register_kvm(env, SPR_FOO, KVM_REG_PPC_FOO);
>     spr_register_kvm(env, SPR_BAR, KVM_REG_PPC_BAR);
> }

I really dislike the idea above - it'd be way too easy for KVM and TCG
to get out of sync.  Instead make spr_register() itself a macro if
necessary, so it just ignores the access functions in the !TCG case.

> //common.c
> static void gen_spr_generic(CPUPPCState *env)
> {
>     // these only set name, num, initial value
>     spr_register(env, SPR_FOO, "FOO", 0xf00);
>     spr_register(env, SPR_BAR, "BAR", 0xb4d);
>     ...
> 
>     // have these stubbed if not chosen via config
>     tcg_gen_spr_generic(env);
>     kvm_gen_spr_generic(env);
> }
> 
> init_ppc_proc()
> {
>         ...
>         gen_spr_generic(env);
>         ...
> }
> 
> Can anyone see a better way? That would be much easier if we could
> afford to say that TCG and KVM are mutually exclusive for a given build,
> but I don't think they are.
> 
> > * move is_indirect_opcode and ind_table to translate.c, since they
> > are used to translate ppc instructions, and the things defined for
> > these functions
> 
> Makes sense. This and the other part below about callback tables would
> be mostly about moving code so it's a candidate for coming soon.
> 
> > * Figure out what needs to be added to the includes for both
> > files to compile
> > * move opcodes and invalid_handler into cpu_init.c, because they
> > are only used by stuff in this file.
> >
> > I'm just not sure about this last point. The stuff that use opcodes
> > create the callback tables for TCG, AFAICT. The better plan would
> > be to move all of that to tanslate.c, but might be a lot.
> 
> translate.c seems like a better place indeed.
> 

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

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

[Richard Henderson]

On 4/13/21 10:43 AM, Bruno Piazera Larsen wrote:
> The trivial path is to:
> * rename translate_init.c.inc to cpu_init.c (since it has to do with
> initial definitions for CPUs, and it's not related to translating
> anymore);

Anymore?  You mean after you've moved out everything related to 
create_ppc_opcodes?  Sure.

> * move gen_write_xer and gen_read_xer into cpu_init.c, as they're
> used for some sprs, and whatever needs to be moved with it

Well, gen_* things are specifically translation related, since they emit tcg 
opcodes.  But I see it's used as part of a callback from the SPRs.

I think it would be worth moving all of the SPR code out to a separate file, 
apart from cpu_init.c.  There's a lot of it.  And, yes, I would move everything 
that you can that is related out of translate.c.

> * move is_indirect_opcode and ind_table to translate.c, since they
> are used to translate ppc instructions, and the things defined for
> these functions

Yes.

> * move opcodes and invalid_handler into cpu_init.c, because they
> are only used by stuff in this file.

You could move the opcodes to a new file of its own, including invalid_handler. 
  Moving them to cpu_init.c does not seem helpful.

However, I think the surgery required to disentangle the legacy decoder and all 
its macros is probably not worth the effort.  What will be worth the effort is 
completing the decodetree conversion so that the legacy decoder goes away entirely.


r~

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

[Bruno Piazera Larsen]

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

> > The trivial path is to:
> > * rename translate_init.c.inc to cpu_init.c (since it has to do with
> > initial definitions for CPUs, and it's not related to translating
> > anymore);
>
> Anymore?  You mean after you've moved out everything related to create_ppc_opcodes?  Sure.

yeah, that. Also after removing every to destroy the opcode table
(which isn't packaged in a neat function for some reason, it's loose
in the ppc_cpu_unrealize).

> > * move gen_write_xer and gen_read_xer into cpu_init.c, as they're
> > used for some sprs, and whatever needs to be moved with it
>
> Well, gen_* things are specifically translation related, since they emit tcg
> opcodes.  But I see it's used as part of a callback from the SPRs.
>
> I think it would be worth moving all of the SPR code out to a separate file,
> apart from cpu_init.c.  There's a lot of it.  And, yes, I would move everything
> that you can that is related out of translate.c.

Yeah, now that I look at the SPR code, I'm starting to think it's easier
I think it's what fabiano had in mind too, but we'll probably have 3 files,
spr_common.c, spr_tcg.c and spr_kvm.c. It's a bit of surgery, but it's
probably worth it, to avoid a mess of ifdefs.

> > * move opcodes and invalid_handler into cpu_init.c, because they
> > are only used by stuff in this file.
> You could move the opcodes to a new file of its own, including invalid_handler.
>   Moving them to cpu_init.c does not seem helpful.

While waiting for a reply I tried this. It's really not, it creates about 6k errors.
I ended up moving everything that used it from cpu_init.c into translate.c.
create_ppc_opcodes and destroy_ppc_opcodes ended up going there, and
I added prototypes to internal.h to call them in the realize and unrealize
functions.

> However, I think the surgery required to disentangle the legacy decoder and all
>its macros is probably not worth the effort.
> What will be worth the effort is completing the decodetree conversion so that the legacy decoder goes away entirely.

Yeah, I wanted to do that, but at this point I'm just following what the client
ordered. Maybe once we compile with tcg, it could be suggested, but I
wouldn't count on it.

Anyway, I don't think the disentangling I'm doing now would make that
process harder in the future. Let me know if it is


Bruno Piazera Larsen

Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>

Departamento Computação Embarcada

Analista de Software Trainee

Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>


[-- Attachment #2: Type: text/html, Size: 9057 bytes --]

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

[Richard Henderson]

On 4/14/21 1:07 PM, Bruno Piazera Larsen wrote:
>> I think it would be worth moving all of the SPR code out to a separate file, 
>> apart from cpu_init.c.  There's a lot of it.  And, yes, I would move everything
>> that you can that is related out of translate.c.
> 
> Yeah, now that I look at the SPR code, I'm starting to think it's easier
> I think it's what fabiano had in mind too, but we'll probably have 3 files,
> spr_common.c, spr_tcg.c and spr_kvm.c. It's a bit of surgery, but it's
> probably worth it, to avoid a mess of ifdefs.

Sounds good.

> While waiting for a reply I tried this. It's really not, it creates about 6k 
> errors.
> I ended up moving everything that used it from cpu_init.c into translate.c.
> create_ppc_opcodes and destroy_ppc_opcodes ended up going there, and
> I added prototypes to internal.h to call them in the realize and unrealize
> functions.

Moving into translate.c sounds like a good option.


r~

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

[David Gibson]

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

On Tue, Apr 13, 2021 at 05:43:02PM +0000, Bruno Piazera Larsen wrote:
> > I'm actually not sure if we'll want translate_init.c for !tcg builds.
> > It's *primarily* for TCG, but we still need at least some of the cpu
> > state structure for KVM, and some of that is initialized in
> > translate_init.
> >
> > I think it will probably make more sense to leave it in for a first
> > cut.  Later refinement might end up removing it.
> >
> > The whole #include translate_init.c.inc thing might make for some
> > awkward fiddling in this, of course.
> 
> I just checked, there is going to be some shuffling of functions
> around, as there are some static variables defined on translate.c,
> and used in translate_init.c.inc, some functions needed for KVM
> on translate.c and some TCG only functions in the
> translate_init.c.inc.
> 
> The trivial path is to:
> * rename translate_init.c.inc to cpu_init.c (since it has to do with
> initial definitions for CPUs, and it's not related to translating
> anymore);
> * move gen_write_xer and gen_read_xer into cpu_init.c, as they're
> used for some sprs, and whatever needs to be moved with it

Hmm.. that doesn't seem right.  gen_*() functions are explicitly for
generating code, so it really seems like they belong in the
translation file.

> * move is_indirect_opcode and ind_table to translate.c, since they
> are used to translate ppc instructions, and the things defined for
> these functions
> * Figure out what needs to be added to the includes for both
> files to compile
> * move opcodes and invalid_handler into cpu_init.c, because they
> are only used by stuff in this file.
> 
> I'm just not sure about this last point. The stuff that use opcodes
> create the callback tables for TCG, AFAICT. The better plan would
> be to move all of that to tanslate.c, but might be a lot.
> 
> Can I follow the trivial plan for the first cut and leave a TODO in
> the code for a better solution in the future? Or is there a nuance
> about one of those functions that I have not understood?
> 
> 
> Bruno Piazera Larsen
> 
> Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>
> 
> Departamento Computação Embarcada
> 
> Analista de Software Trainee
> 
> Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>
> 
> ________________________________
> De: David Gibson
> Enviadas: Terça-feira, 13 de Abril de 2021 03:40
> Para: Bruno Piazera Larsen
> Cc: Fabiano Rosas; Thomas Huth; qemu-devel@nongnu.org; Luis Fernando Fujita Pires; Andre Fernando da Silva; Lucas Mateus Martins Araujo e Castro; Fernando Eckhardt Valle; qemu-ppc@nongnu.org; lagarcia@br.ibm.com; Matheus Kowalczuk Ferst
> Assunto: Re: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg
> 
> On Mon, Apr 12, 2021 at 12:05:31PM +0000, Bruno Piazera Larsen wrote:
> > > 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.
> >
> > Fair point, I should've thought about that.
> >
> > > > 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).
> >
> > There are 3 main reasons for this decision. The first is kind of silly, but when I read translate.c my mind jumped to translating machine code to TCG, and the amount of TCGv variables at the start reinforced this notion.
> > The second was that both s390x and i386 removed it (translate.c) from compilation, so I had no good reason to doubt it.
> > The last (and arguably most important) is that translate.c is many thousands of lines long (translate_init.c.inc alone was almost 11k). The whole point of disabling TCG is to speed up compilation and reduce the final file size, so I think it makes sense to remove that big file.
> > And the final nail in the coffin is that at no point did it cross my mind to keep the init part of translation, but remove the rest
> >
> > Also, I'm not a fan of big ifdefs, because it's kinda hard to follow them when viewing code in vim. Adding that to already having a cpu.c file, where it makes sense (to me, at least) to add all CPU related functions, just sounded like a good idea.
> 
> I think those are all sound reasons; I think not compiling translate.c
> for !tcg builds is the right choice.  We might, however, need to
> "rescue" certain functions from there by moving them to another file
> so they can be used by KVM code as well.
> 
> > > 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 is me misjudging what is TCG and what is not, mostly. I think that actually moving everything to cpu.c and adding ifdefs, or creating a cpu_tcg.c.inc or similar, would be the most sensible code motion, but every function I removed from translate gave me 3 new errors, at some point I felt like I should leave something behind otherwise we're compiling everything from TCG again, just in different files, so I tried to guess what was TCG and what was not (also, I really wanted the RFC out by the weekend, I _may_ have rushed a few choices).
> 
> 
> > > > 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?
> >
> > Sure, I'll try and get it ready ASAP, and make sure I didn't miss one function before sending. Just a noob question... do I edit the patch manually to have it only contain the gdb code motion, or is there a way to move back to before I actually made the commit without needing to re-do the changes?
> >
> > Thomas, I'm adding you to this discussion since it sort of answers your message on the other one, about the functions used even in a KVM-only build.
> >
> > > IIRC you don't only have to exclude translate.c from the build, you also
> > > have to separate translate_init.c.inc from it, i.e. turn
> > > translate_init.c.inc into a proper .c file and get rid of the #include
> > > "translate_init.c.inc" statement in translate.c, since many functions in the
> > > translate_init.c.inc file are still needed for the KVM-only builds, too. So
> > > maybe that's a good place to start as a first mini series.
> >
> > Now that I know that translate doesn't mean "translating to TCG", this idea makes more sense. My question is, is it a better option than the code motion I'm suggesting? From my quick check on the bits that I haven't touched it might, but at this point it's clear I'm very lost with what makes sense hahaha.
> >
> >
> > Bruno Piazera Larsen
> >
> > Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>
> >
> > Departamento Computação Embarcada
> >
> > Analista de Software Trainee
> >
> > Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>
> 

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

[RFC PATCH 0/4] target/ppc: add disable-tcg option

[Bruno Larsen (billionai)]

This patch series aims to add the option to build without TCG for the
powerpc target. This RFC shows mostly the strategies employed when
dealing with compilation problems, and ask for input on the bits
we don't quite understand yet.

The first patch mostly code motion, as referenced in 2021-04/msg0717.
The second patch shows the 2 strategies we've considered, and hope to
get feedback on. The third patch contains the stubs we haven't decided
on how to deal with yet, but needed to exist to compile the project.
The final patch just changes the meson.build rules

Bruno Larsen (billionai) (4):
  target/ppc: Code motion required to build disabling tcg
  target/ppc: added solutions for problems encountered when building
    with disable-tcg
  target/ppc: Add stubs for tcg functions, so it build with disable-tcg
  target/ppc: updated build rules for disable-tcg option

 target/ppc/arch_dump.c          |   17 +
 target/ppc/cpu.c                |  859 +++++++++++++++++++++++
 target/ppc/cpu.h                |   15 +
 target/ppc/gdbstub.c            |  253 +++++++
 target/ppc/kvm.c                |   30 +
 target/ppc/kvm_ppc.h            |   11 +
 target/ppc/machine.c            |   33 +-
 target/ppc/meson.build          |   22 +-
 target/ppc/tcg-stub.c           |  139 ++++
 target/ppc/translate_init.c.inc | 1148 +------------------------------
 10 files changed, 1407 insertions(+), 1120 deletions(-)
 create mode 100644 target/ppc/tcg-stub.c

-- 
2.17.1

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

[Bruno Larsen (billionai)]

This commit does the necessary code motion from translate_init.c.inc
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
All functions related to realizing the cpu have been moved to cpu.c, which
may call functions from gdbstub or translate_init

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/cpu.c                |  859 +++++++++++++++++++++++
 target/ppc/cpu.h                |   15 +
 target/ppc/gdbstub.c            |  253 +++++++
 target/ppc/translate_init.c.inc | 1148 +------------------------------
 4 files changed, 1163 insertions(+), 1112 deletions(-)

diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index e501a7ff6f..b77ea1c943 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -20,6 +20,21 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "cpu-models.h"
+#include "kvm_ppc.h"
+#include "qemu/qemu-print.h"
+#include "qemu/cutils.h"
+#include "qapi/qapi-commands-machine-target.h"
+
+#include "qapi/error.h"
+#include "sysemu/tcg.h"
+#include "helper_regs.h"
+#include "hw/ppc/ppc.h"
+#include "fpu/softfloat-helpers.h"
+#include "sysemu/hw_accel.h"
+#include "disas/capstone.h"
+#include "hw/qdev-properties.h"
+#include "internal.h"
+#include "mmu-hash64.h"
 
 target_ulong cpu_read_xer(CPUPPCState *env)
 {
@@ -45,3 +60,847 @@ void cpu_write_xer(CPUPPCState *env, target_ulong xer)
                        (1ul << XER_OV) | (1ul << XER_CA) |
                        (1ul << XER_OV32) | (1ul << XER_CA32));
 }
+
+
+static const char *ppc_cpu_lookup_alias(const char *alias)
+{
+    int ai;
+
+    for (ai = 0; ppc_cpu_aliases[ai].alias != NULL; ai++) {
+        if (strcmp(ppc_cpu_aliases[ai].alias, alias) == 0) {
+            return ppc_cpu_aliases[ai].model;
+        }
+    }
+
+    return NULL;
+}
+
+static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b)
+{
+    ObjectClass *oc = (ObjectClass *)a;
+    uint32_t pvr = *(uint32_t *)b;
+    PowerPCCPUClass *pcc = (PowerPCCPUClass *)a;
+
+    /* -cpu host does a PVR lookup during construction */
+    if (unlikely(strcmp(object_class_get_name(oc),
+                        TYPE_HOST_POWERPC_CPU) == 0)) {
+        return -1;
+    }
+
+    return pcc->pvr == pvr ? 0 : -1;
+}
+
+static gint ppc_cpu_compare_class_pvr_mask(gconstpointer a, gconstpointer b)
+{
+    ObjectClass *oc = (ObjectClass *)a;
+    uint32_t pvr = *(uint32_t *)b;
+    PowerPCCPUClass *pcc = (PowerPCCPUClass *)a;
+
+    /* -cpu host does a PVR lookup during construction */
+    if (unlikely(strcmp(object_class_get_name(oc),
+                        TYPE_HOST_POWERPC_CPU) == 0)) {
+        return -1;
+    }
+
+    if (pcc->pvr_match(pcc, pvr)) {
+        return 0;
+    }
+
+    return -1;
+}
+
+PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr)
+{
+    GSList *list, *item;
+    PowerPCCPUClass *pcc = NULL;
+
+    list = object_class_get_list(TYPE_POWERPC_CPU, false);
+    item = g_slist_find_custom(list, &pvr, ppc_cpu_compare_class_pvr);
+    if (item != NULL) {
+        pcc = POWERPC_CPU_CLASS(item->data);
+    }
+    g_slist_free(list);
+
+    return pcc;
+}
+
+PowerPCCPUClass *ppc_cpu_class_by_pvr_mask(uint32_t pvr)
+{
+    GSList *list, *item;
+    PowerPCCPUClass *pcc = NULL;
+
+    list = object_class_get_list(TYPE_POWERPC_CPU, true);
+    item = g_slist_find_custom(list, &pvr, ppc_cpu_compare_class_pvr_mask);
+    if (item != NULL) {
+        pcc = POWERPC_CPU_CLASS(item->data);
+    }
+    g_slist_free(list);
+
+    return pcc;
+}
+
+PowerPCCPUClass *ppc_cpu_get_family_class(PowerPCCPUClass *pcc)
+{
+    ObjectClass *oc = OBJECT_CLASS(pcc);
+
+    while (oc && !object_class_is_abstract(oc)) {
+        oc = object_class_get_parent(oc);
+    }
+    assert(oc);
+
+    return POWERPC_CPU_CLASS(oc);
+}
+
+/* Sort by PVR, ordering special case "host" last. */
+static gint ppc_cpu_list_compare(gconstpointer a, gconstpointer b)
+{
+    ObjectClass *oc_a = (ObjectClass *)a;
+    ObjectClass *oc_b = (ObjectClass *)b;
+    PowerPCCPUClass *pcc_a = POWERPC_CPU_CLASS(oc_a);
+    PowerPCCPUClass *pcc_b = POWERPC_CPU_CLASS(oc_b);
+    const char *name_a = object_class_get_name(oc_a);
+    const char *name_b = object_class_get_name(oc_b);
+
+    if (strcmp(name_a, TYPE_HOST_POWERPC_CPU) == 0) {
+        return 1;
+    } else if (strcmp(name_b, TYPE_HOST_POWERPC_CPU) == 0) {
+        return -1;
+    } else {
+        /* Avoid an integer overflow during subtraction */
+        if (pcc_a->pvr < pcc_b->pvr) {
+            return -1;
+        } else if (pcc_a->pvr > pcc_b->pvr) {
+            return 1;
+        } else {
+            return 0;
+        }
+    }
+}
+#ifndef CONFIG_USER_ONLY
+static const TypeInfo ppc_vhyp_type_info = {
+    .name = TYPE_PPC_VIRTUAL_HYPERVISOR,
+    .parent = TYPE_INTERFACE,
+    .class_size = sizeof(PPCVirtualHypervisorClass),
+};
+#endif
+
+static ObjectClass *ppc_cpu_class_by_name(const char *name)
+{
+    char *cpu_model, *typename;
+    ObjectClass *oc;
+    const char *p;
+    unsigned long pvr;
+
+    /*
+     * Lookup by PVR if cpu_model is valid 8 digit hex number (excl:
+     * 0x prefix if present)
+     */
+    if (!qemu_strtoul(name, &p, 16, &pvr)) {
+        int len = p - name;
+        len = (len == 10) && (name[1] == 'x') ? len - 2 : len;
+        if ((len == 8) && (*p == '\0')) {
+            return OBJECT_CLASS(ppc_cpu_class_by_pvr(pvr));
+        }
+    }
+
+    cpu_model = g_ascii_strdown(name, -1);
+    p = ppc_cpu_lookup_alias(cpu_model);
+    if (p) {
+        g_free(cpu_model);
+        cpu_model = g_strdup(p);
+    }
+
+    typename = g_strdup_printf("%s" POWERPC_CPU_TYPE_SUFFIX, cpu_model);
+    oc = object_class_by_name(typename);
+    g_free(typename);
+    g_free(cpu_model);
+
+    return oc;
+}
+
+static bool ppc_cpu_interrupts_big_endian_always(PowerPCCPU *cpu)
+{
+    return true;
+}
+
+static int ppc_fixup_cpu(PowerPCCPU *cpu)
+{
+    CPUPPCState *env = &cpu->env;
+
+    /*
+     * TCG doesn't (yet) emulate some groups of instructions that are
+     * implemented on some otherwise supported CPUs (e.g. VSX and
+     * decimal floating point instructions on POWER7).  We remove
+     * unsupported instruction groups from the cpu state's instruction
+     * masks and hope the guest can cope.  For at least the pseries
+     * machine, the unavailability of these instructions can be
+     * advertised to the guest via the device tree.
+     */
+    if ((env->insns_flags & ~PPC_TCG_INSNS)
+        || (env->insns_flags2 & ~PPC_TCG_INSNS2)) {
+        warn_report("Disabling some instructions which are not "
+                    "emulated by TCG (0x%" PRIx64 ", 0x%" PRIx64 ")",
+                    env->insns_flags & ~PPC_TCG_INSNS,
+                    env->insns_flags2 & ~PPC_TCG_INSNS2);
+    }
+    env->insns_flags &= PPC_TCG_INSNS;
+    env->insns_flags2 &= PPC_TCG_INSNS2;
+    return 0;
+}
+
+static void ppc_cpu_realize(DeviceState *dev, Error **errp)
+{
+    CPUState *cs = CPU(dev);
+    PowerPCCPU *cpu = POWERPC_CPU(dev);
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    Error *local_err = NULL;
+
+    cpu_exec_realizefn(cs, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    if (cpu->vcpu_id == UNASSIGNED_CPU_INDEX) {
+        cpu->vcpu_id = cs->cpu_index;
+    }
+
+    if (tcg_enabled()) {
+        if (ppc_fixup_cpu(cpu) != 0) {
+            error_setg(errp, "Unable to emulate selected CPU with TCG");
+            goto unrealize;
+        }
+    }
+
+    create_ppc_opcodes(cpu, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        goto unrealize;
+    }
+    init_ppc_proc(cpu);
+
+    ppc_cpu_gdb_init(cs, pcc);
+
+    qemu_init_vcpu(cs);
+
+    pcc->parent_realize(dev, errp);
+
+#if defined(PPC_DUMP_CPU)
+    {
+        CPUPPCState *env = &cpu->env;
+        const char *mmu_model, *excp_model, *bus_model;
+        switch (env->mmu_model) {
+        case POWERPC_MMU_32B:
+            mmu_model = "PowerPC 32";
+            break;
+        case POWERPC_MMU_SOFT_6xx:
+            mmu_model = "PowerPC 6xx/7xx with software driven TLBs";
+            break;
+        case POWERPC_MMU_SOFT_74xx:
+            mmu_model = "PowerPC 74xx with software driven TLBs";
+            break;
+        case POWERPC_MMU_SOFT_4xx:
+            mmu_model = "PowerPC 4xx with software driven TLBs";
+            break;
+        case POWERPC_MMU_SOFT_4xx_Z:
+            mmu_model = "PowerPC 4xx with software driven TLBs "
+                "and zones protections";
+            break;
+        case POWERPC_MMU_REAL:
+            mmu_model = "PowerPC real mode only";
+            break;
+        case POWERPC_MMU_MPC8xx:
+            mmu_model = "PowerPC MPC8xx";
+            break;
+        case POWERPC_MMU_BOOKE:
+            mmu_model = "PowerPC BookE";
+            break;
+        case POWERPC_MMU_BOOKE206:
+            mmu_model = "PowerPC BookE 2.06";
+            break;
+        case POWERPC_MMU_601:
+            mmu_model = "PowerPC 601";
+            break;
+#if defined(TARGET_PPC64)
+        case POWERPC_MMU_64B:
+            mmu_model = "PowerPC 64";
+            break;
+#endif
+        default:
+            mmu_model = "Unknown or invalid";
+            break;
+        }
+        switch (env->excp_model) {
+        case POWERPC_EXCP_STD:
+            excp_model = "PowerPC";
+            break;
+        case POWERPC_EXCP_40x:
+            excp_model = "PowerPC 40x";
+            break;
+        case POWERPC_EXCP_601:
+            excp_model = "PowerPC 601";
+            break;
+        case POWERPC_EXCP_602:
+            excp_model = "PowerPC 602";
+            break;
+        case POWERPC_EXCP_603:
+            excp_model = "PowerPC 603";
+            break;
+        case POWERPC_EXCP_603E:
+            excp_model = "PowerPC 603e";
+            break;
+        case POWERPC_EXCP_604:
+            excp_model = "PowerPC 604";
+            break;
+        case POWERPC_EXCP_7x0:
+            excp_model = "PowerPC 740/750";
+            break;
+        case POWERPC_EXCP_7x5:
+            excp_model = "PowerPC 745/755";
+            break;
+        case POWERPC_EXCP_74xx:
+            excp_model = "PowerPC 74xx";
+            break;
+        case POWERPC_EXCP_BOOKE:
+            excp_model = "PowerPC BookE";
+            break;
+#if defined(TARGET_PPC64)
+        case POWERPC_EXCP_970:
+            excp_model = "PowerPC 970";
+            break;
+#endif
+        default:
+            excp_model = "Unknown or invalid";
+            break;
+        }
+        switch (env->bus_model) {
+        case PPC_FLAGS_INPUT_6xx:
+            bus_model = "PowerPC 6xx";
+            break;
+        case PPC_FLAGS_INPUT_BookE:
+            bus_model = "PowerPC BookE";
+            break;
+        case PPC_FLAGS_INPUT_405:
+            bus_model = "PowerPC 405";
+            break;
+        case PPC_FLAGS_INPUT_401:
+            bus_model = "PowerPC 401/403";
+            break;
+        case PPC_FLAGS_INPUT_RCPU:
+            bus_model = "RCPU / MPC8xx";
+            break;
+#if defined(TARGET_PPC64)
+        case PPC_FLAGS_INPUT_970:
+            bus_model = "PowerPC 970";
+            break;
+#endif
+        default:
+            bus_model = "Unknown or invalid";
+            break;
+        }
+        printf("PowerPC %-12s : PVR %08x MSR %016" PRIx64 "\n"
+               "    MMU model        : %s\n",
+               object_class_get_name(OBJECT_CLASS(pcc)),
+               pcc->pvr, pcc->msr_mask, mmu_model);
+#if !defined(CONFIG_USER_ONLY)
+        if (env->tlb.tlb6) {
+            printf("                       %d %s TLB in %d ways\n",
+                   env->nb_tlb, env->id_tlbs ? "splitted" : "merged",
+                   env->nb_ways);
+        }
+#endif
+        printf("    Exceptions model : %s\n"
+               "    Bus model        : %s\n",
+               excp_model, bus_model);
+        printf("    MSR features     :\n");
+        if (env->flags & POWERPC_FLAG_SPE) {
+            printf("                        signal processing engine enable"
+                   "\n");
+        } else if (env->flags & POWERPC_FLAG_VRE) {
+            printf("                        vector processor enable\n");
+        }
+        if (env->flags & POWERPC_FLAG_TGPR) {
+            printf("                        temporary GPRs\n");
+        } else if (env->flags & POWERPC_FLAG_CE) {
+            printf("                        critical input enable\n");
+        }
+        if (env->flags & POWERPC_FLAG_SE) {
+            printf("                        single-step trace mode\n");
+        } else if (env->flags & POWERPC_FLAG_DWE) {
+            printf("                        debug wait enable\n");
+        } else if (env->flags & POWERPC_FLAG_UBLE) {
+            printf("                        user BTB lock enable\n");
+        }
+        if (env->flags & POWERPC_FLAG_BE) {
+            printf("                        branch-step trace mode\n");
+        } else if (env->flags & POWERPC_FLAG_DE) {
+            printf("                        debug interrupt enable\n");
+        }
+        if (env->flags & POWERPC_FLAG_PX) {
+            printf("                        inclusive protection\n");
+        } else if (env->flags & POWERPC_FLAG_PMM) {
+            printf("                        performance monitor mark\n");
+        }
+        if (env->flags == POWERPC_FLAG_NONE) {
+            printf("                        none\n");
+        }
+        printf("    Time-base/decrementer clock source: %s\n",
+               env->flags & POWERPC_FLAG_RTC_CLK ? "RTC clock" : "bus clock");
+        dump_ppc_insns(env);
+        dump_ppc_sprs(env);
+        fflush(stdout);
+    }
+#endif
+    return;
+
+unrealize:
+    cpu_exec_unrealizefn(cs);
+}
+
+static void ppc_cpu_unrealize(DeviceState *dev)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(dev);
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+    pcc->parent_unrealize(dev);
+
+    cpu_remove_sync(CPU(cpu));
+
+    destroy_ppc_opcodes(cpu);
+}
+
+static void ppc_cpu_set_pc(CPUState *cs, vaddr value)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+
+    cpu->env.nip = value;
+}
+
+static bool ppc_cpu_has_work(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    return msr_ee && (cs->interrupt_request & CPU_INTERRUPT_HARD);
+}
+
+static void ppc_cpu_reset(DeviceState *dev)
+{
+    CPUState *s = CPU(dev);
+    PowerPCCPU *cpu = POWERPC_CPU(s);
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    CPUPPCState *env = &cpu->env;
+    target_ulong msr;
+    int i;
+
+    pcc->parent_reset(dev);
+
+    msr = (target_ulong)0;
+    msr |= (target_ulong)MSR_HVB;
+    msr |= (target_ulong)0 << MSR_AP; /* TO BE CHECKED */
+    msr |= (target_ulong)0 << MSR_SA; /* TO BE CHECKED */
+    msr |= (target_ulong)1 << MSR_EP;
+#if defined(DO_SINGLE_STEP) && 0
+    /* Single step trace mode */
+    msr |= (target_ulong)1 << MSR_SE;
+    msr |= (target_ulong)1 << MSR_BE;
+#endif
+#if defined(CONFIG_USER_ONLY)
+    msr |= (target_ulong)1 << MSR_FP; /* Allow floating point usage */
+    msr |= (target_ulong)1 << MSR_FE0; /* Allow floating point exceptions */
+    msr |= (target_ulong)1 << MSR_FE1;
+    msr |= (target_ulong)1 << MSR_VR; /* Allow altivec usage */
+    msr |= (target_ulong)1 << MSR_VSX; /* Allow VSX usage */
+    msr |= (target_ulong)1 << MSR_SPE; /* Allow SPE usage */
+    msr |= (target_ulong)1 << MSR_PR;
+#if defined(TARGET_PPC64)
+    msr |= (target_ulong)1 << MSR_TM; /* Transactional memory */
+#endif
+#if !defined(TARGET_WORDS_BIGENDIAN)
+    msr |= (target_ulong)1 << MSR_LE; /* Little-endian user mode */
+    if (!((env->msr_mask >> MSR_LE) & 1)) {
+        fprintf(stderr, "Selected CPU does not support little-endian.\n");
+        exit(1);
+    }
+#endif
+#endif
+
+#if defined(TARGET_PPC64)
+    if (mmu_is_64bit(env->mmu_model)) {
+        msr |= (1ULL << MSR_SF);
+    }
+#endif
+
+    hreg_store_msr(env, msr, 1);
+
+#if !defined(CONFIG_USER_ONLY)
+    env->nip = env->hreset_vector | env->excp_prefix;
+    if (env->mmu_model != POWERPC_MMU_REAL) {
+        ppc_tlb_invalidate_all(env);
+    }
+#endif
+
+    hreg_compute_hflags(env);
+    env->reserve_addr = (target_ulong)-1ULL;
+    /* Be sure no exception or interrupt is pending */
+    env->pending_interrupts = 0;
+    s->exception_index = POWERPC_EXCP_NONE;
+    env->error_code = 0;
+    ppc_irq_reset(cpu);
+
+    /* tininess for underflow is detected before rounding */
+    set_float_detect_tininess(float_tininess_before_rounding,
+                              &env->fp_status);
+
+    for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
+        ppc_spr_t *spr = &env->spr_cb[i];
+
+        if (!spr->name) {
+            continue;
+        }
+        env->spr[i] = spr->default_value;
+    }
+}
+
+#ifndef CONFIG_USER_ONLY
+
+static bool ppc_cpu_is_big_endian(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    cpu_synchronize_state(cs);
+
+    return !msr_le;
+}
+
+#ifdef CONFIG_TCG
+static void ppc_cpu_exec_enter(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+
+    if (cpu->vhyp) {
+        PPCVirtualHypervisorClass *vhc =
+            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
+        vhc->cpu_exec_enter(cpu->vhyp, cpu);
+    }
+}
+
+static void ppc_cpu_exec_exit(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+
+    if (cpu->vhyp) {
+        PPCVirtualHypervisorClass *vhc =
+            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
+        vhc->cpu_exec_exit(cpu->vhyp, cpu);
+    }
+}
+#endif /* CONFIG_TCG */
+
+#endif /* !CONFIG_USER_ONLY */
+
+static bool ppc_pvr_match_default(PowerPCCPUClass *pcc, uint32_t pvr)
+{
+    return pcc->pvr == pvr;
+}
+
+static gchar *ppc_gdb_arch_name(CPUState *cs)
+{
+#if defined(TARGET_PPC64)
+    return g_strdup("powerpc:common64");
+#else
+    return g_strdup("powerpc:common");
+#endif
+}
+
+static void ppc_disas_set_info(CPUState *cs, disassemble_info *info)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    if ((env->hflags >> MSR_LE) & 1) {
+        info->endian = BFD_ENDIAN_LITTLE;
+    }
+    info->mach = env->bfd_mach;
+    if (!env->bfd_mach) {
+#ifdef TARGET_PPC64
+        info->mach = bfd_mach_ppc64;
+#else
+        info->mach = bfd_mach_ppc;
+#endif
+    }
+    info->disassembler_options = (char *)"any";
+    info->print_insn = print_insn_ppc;
+
+    info->cap_arch = CS_ARCH_PPC;
+#ifdef TARGET_PPC64
+    info->cap_mode = CS_MODE_64;
+#endif
+}
+
+static Property ppc_cpu_properties[] = {
+    DEFINE_PROP_BOOL("pre-2.8-migration", PowerPCCPU, pre_2_8_migration, false),
+    DEFINE_PROP_BOOL("pre-2.10-migration", PowerPCCPU, pre_2_10_migration,
+                     false),
+    DEFINE_PROP_BOOL("pre-3.0-migration", PowerPCCPU, pre_3_0_migration,
+                     false),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+#ifdef CONFIG_TCG
+#include "hw/core/tcg-cpu-ops.h"
+
+static struct TCGCPUOps ppc_tcg_ops = {
+  .initialize = ppc_translate_init,
+  .cpu_exec_interrupt = ppc_cpu_exec_interrupt,
+  .tlb_fill = ppc_cpu_tlb_fill,
+
+#ifndef CONFIG_USER_ONLY
+  .do_interrupt = ppc_cpu_do_interrupt,
+  .cpu_exec_enter = ppc_cpu_exec_enter,
+  .cpu_exec_exit = ppc_cpu_exec_exit,
+  .do_unaligned_access = ppc_cpu_do_unaligned_access,
+#endif /* !CONFIG_USER_ONLY */
+};
+#endif /* CONFIG_TCG */
+
+static void ppc_cpu_class_init(ObjectClass *oc, void *data)
+{
+    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
+    CPUClass *cc = CPU_CLASS(oc);
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    device_class_set_parent_realize(dc, ppc_cpu_realize,
+                                    &pcc->parent_realize);
+    device_class_set_parent_unrealize(dc, ppc_cpu_unrealize,
+                                      &pcc->parent_unrealize);
+    pcc->pvr_match = ppc_pvr_match_default;
+    pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_always;
+    device_class_set_props(dc, ppc_cpu_properties);
+
+    device_class_set_parent_reset(dc, ppc_cpu_reset, &pcc->parent_reset);
+
+    cc->class_by_name = ppc_cpu_class_by_name;
+    cc->has_work = ppc_cpu_has_work;
+    cc->dump_state = ppc_cpu_dump_state;
+    cc->dump_statistics = ppc_cpu_dump_statistics;
+    cc->set_pc = ppc_cpu_set_pc;
+    cc->gdb_read_register = ppc_cpu_gdb_read_register;
+    cc->gdb_write_register = ppc_cpu_gdb_write_register;
+#ifndef CONFIG_USER_ONLY
+    cc->get_phys_page_debug = ppc_cpu_get_phys_page_debug;
+    cc->vmsd = &vmstate_ppc_cpu;
+#endif
+#if defined(CONFIG_SOFTMMU)
+    cc->write_elf64_note = ppc64_cpu_write_elf64_note;
+    cc->write_elf32_note = ppc32_cpu_write_elf32_note;
+#endif
+
+    cc->gdb_num_core_regs = 71;
+#ifndef CONFIG_USER_ONLY
+    cc->gdb_get_dynamic_xml = ppc_gdb_get_dynamic_xml;
+#endif
+#ifdef USE_APPLE_GDB
+    cc->gdb_read_register = ppc_cpu_gdb_read_register_apple;
+    cc->gdb_write_register = ppc_cpu_gdb_write_register_apple;
+    cc->gdb_num_core_regs = 71 + 32;
+#endif
+
+    cc->gdb_arch_name = ppc_gdb_arch_name;
+#if defined(TARGET_PPC64)
+    cc->gdb_core_xml_file = "power64-core.xml";
+#else
+    cc->gdb_core_xml_file = "power-core.xml";
+#endif
+#ifndef CONFIG_USER_ONLY
+    cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
+#endif
+    cc->disas_set_info = ppc_disas_set_info;
+
+    dc->fw_name = "PowerPC,UNKNOWN";
+
+#ifdef CONFIG_TCG
+    cc->tcg_ops = &ppc_tcg_ops;
+#endif /* CONFIG_TCG */
+}
+
+static void ppc_cpu_instance_init(Object *obj)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(obj);
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    CPUPPCState *env = &cpu->env;
+
+    cpu_set_cpustate_pointers(cpu);
+    cpu->vcpu_id = UNASSIGNED_CPU_INDEX;
+
+    env->msr_mask = pcc->msr_mask;
+    env->mmu_model = pcc->mmu_model;
+    env->excp_model = pcc->excp_model;
+    env->bus_model = pcc->bus_model;
+    env->insns_flags = pcc->insns_flags;
+    env->insns_flags2 = pcc->insns_flags2;
+    env->flags = pcc->flags;
+    env->bfd_mach = pcc->bfd_mach;
+    env->check_pow = pcc->check_pow;
+
+    /*
+     * Mark HV mode as supported if the CPU has an MSR_HV bit in the
+     * msr_mask. The mask can later be cleared by PAPR mode but the hv
+     * mode support will remain, thus enforcing that we cannot use
+     * priv. instructions in guest in PAPR mode. For 970 we currently
+     * simply don't set HV in msr_mask thus simulating an "Apple mode"
+     * 970. If we ever want to support 970 HV mode, we'll have to add
+     * a processor attribute of some sort.
+     */
+#if !defined(CONFIG_USER_ONLY)
+    env->has_hv_mode = !!(env->msr_mask & MSR_HVB);
+#endif
+
+#ifdef CONFIG_TCG
+    ppc_hash64_init(cpu);
+#endif
+}
+
+static void ppc_cpu_instance_finalize(Object *obj)
+{
+#ifdef CONFIG_TCG
+    PowerPCCPU *cpu = POWERPC_CPU(obj);
+
+    ppc_hash64_finalize(cpu);
+#endif
+}
+
+static const TypeInfo ppc_cpu_type_info = {
+    .name = TYPE_POWERPC_CPU,
+    .parent = TYPE_CPU,
+    .instance_size = sizeof(PowerPCCPU),
+    .instance_align = __alignof__(PowerPCCPU),
+    .instance_init = ppc_cpu_instance_init,
+    .instance_finalize = ppc_cpu_instance_finalize,
+    .abstract = true,
+    .class_size = sizeof(PowerPCCPUClass),
+    .class_init = ppc_cpu_class_init,
+};
+
+static void ppc_cpu_register_types(void)
+{
+    type_register_static(&ppc_cpu_type_info);
+#ifndef CONFIG_USER_ONLY
+    type_register_static(&ppc_vhyp_type_info);
+#endif
+}
+
+type_init(ppc_cpu_register_types)
+
+static void ppc_cpu_list_entry(gpointer data, gpointer user_data)
+{
+    ObjectClass *oc = data;
+    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
+    DeviceClass *family = DEVICE_CLASS(ppc_cpu_get_family_class(pcc));
+    const char *typename = object_class_get_name(oc);
+    char *name;
+    int i;
+
+    if (unlikely(strcmp(typename, TYPE_HOST_POWERPC_CPU) == 0)) {
+        return;
+    }
+
+    name = g_strndup(typename,
+                     strlen(typename) - strlen(POWERPC_CPU_TYPE_SUFFIX));
+    qemu_printf("PowerPC %-16s PVR %08x\n", name, pcc->pvr);
+    for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
+        PowerPCCPUAlias *alias = &ppc_cpu_aliases[i];
+        ObjectClass *alias_oc = ppc_cpu_class_by_name(alias->model);
+
+        if (alias_oc != oc) {
+            continue;
+        }
+        /*
+         * If running with KVM, we might update the family alias later, so
+         * avoid printing the wrong alias here and use "preferred" instead
+         */
+        if (strcmp(alias->alias, family->desc) == 0) {
+            qemu_printf("PowerPC %-16s (alias for preferred %s CPU)\n",
+                        alias->alias, family->desc);
+        } else {
+            qemu_printf("PowerPC %-16s (alias for %s)\n",
+                        alias->alias, name);
+        }
+    }
+    g_free(name);
+}
+
+void ppc_cpu_list(void)
+{
+    GSList *list;
+
+    list = object_class_get_list(TYPE_POWERPC_CPU, false);
+    list = g_slist_sort(list, ppc_cpu_list_compare);
+    g_slist_foreach(list, ppc_cpu_list_entry, NULL);
+    g_slist_free(list);
+
+#ifdef CONFIG_KVM
+    qemu_printf("\n");
+    qemu_printf("PowerPC %-16s\n", "host");
+#endif
+}
+
+static void ppc_cpu_defs_entry(gpointer data, gpointer user_data)
+{
+    ObjectClass *oc = data;
+    CpuDefinitionInfoList **first = user_data;
+    const char *typename;
+    CpuDefinitionInfo *info;
+
+    typename = object_class_get_name(oc);
+    info = g_malloc0(sizeof(*info));
+    info->name = g_strndup(typename,
+                           strlen(typename) - strlen(POWERPC_CPU_TYPE_SUFFIX));
+
+    QAPI_LIST_PREPEND(*first, info);
+}
+
+CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
+{
+    CpuDefinitionInfoList *cpu_list = NULL;
+    GSList *list;
+    int i;
+
+    list = object_class_get_list(TYPE_POWERPC_CPU, false);
+    g_slist_foreach(list, ppc_cpu_defs_entry, &cpu_list);
+    g_slist_free(list);
+
+    for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
+        PowerPCCPUAlias *alias = &ppc_cpu_aliases[i];
+        ObjectClass *oc;
+        CpuDefinitionInfo *info;
+
+        oc = ppc_cpu_class_by_name(alias->model);
+        if (oc == NULL) {
+            continue;
+        }
+
+        info = g_malloc0(sizeof(*info));
+        info->name = g_strdup(alias->alias);
+        info->q_typename = g_strdup(object_class_get_name(oc));
+
+        QAPI_LIST_PREPEND(cpu_list, info);
+    }
+
+    return cpu_list;
+}
+#if !defined(CONFIG_USER_ONLY)
+void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
+{
+    CPUPPCState *env = &cpu->env;
+
+    cpu->vhyp = vhyp;
+
+    /*
+     * With a virtual hypervisor mode we never allow the CPU to go
+     * hypervisor mode itself
+     */
+    env->msr_mask &= ~MSR_HVB;
+}
+
+#endif /* !defined(CONFIG_USER_ONLY) */
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index e73416da68..031b04ee40 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2612,4 +2612,19 @@ static inline ppc_avr_t *cpu_avr_ptr(CPUPPCState *env, int i)
 void dump_mmu(CPUPPCState *env);
 
 void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len);
+
+/*
+ * functions used by cpu_ppc_realize, but that dont necessarily make sense
+ * to be added to cpu.c, because they seem very related to TCG or gdb
+ */
+
+/* gdbstub.c */
+void ppc_cpu_gdb_init(CPUState *cs, PowerPCCPUClass *ppc);
+
+/* translate_init.c.inc */
+void create_ppc_opcodes(PowerPCCPU *cpu, Error **errp);
+void destroy_ppc_opcodes(PowerPCCPU *cpu);
+void gen_spr_generic(CPUPPCState *env);
+void init_ppc_proc(PowerPCCPU *cpu);
+
 #endif /* PPC_CPU_H */
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index c28319fb97..e6a6c0a6a0 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -20,6 +20,10 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "exec/gdbstub.h"
+#ifdef CONFIG_TCG
+#include "exec/helper-proto.h"
+#endif
+#include "kvm_ppc.h"
 
 static int ppc_gdb_register_len_apple(int n)
 {
@@ -387,3 +391,252 @@ const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name)
     return NULL;
 }
 #endif
+
+static bool avr_need_swap(CPUPPCState *env)
+{
+#ifdef HOST_WORDS_BIGENDIAN
+    return msr_le;
+#else
+    return !msr_le;
+#endif
+}
+
+#if !defined(CONFIG_USER_ONLY)
+static int gdb_find_spr_idx(CPUPPCState *env, int n)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
+        ppc_spr_t *spr = &env->spr_cb[i];
+
+        if (spr->name && spr->gdb_id == n) {
+            return i;
+        }
+    }
+    return -1;
+}
+
+static int gdb_get_spr_reg(CPUPPCState *env, GByteArray *buf, int n)
+{
+    int reg;
+    int len;
+
+    reg = gdb_find_spr_idx(env, n);
+    if (reg < 0) {
+        return 0;
+    }
+
+    len = TARGET_LONG_SIZE;
+    gdb_get_regl(buf, env->spr[reg]);
+    ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, len), len);
+    return len;
+}
+
+static int gdb_set_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
+{
+    int reg;
+    int len;
+
+    reg = gdb_find_spr_idx(env, n);
+    if (reg < 0) {
+        return 0;
+    }
+
+    len = TARGET_LONG_SIZE;
+    ppc_maybe_bswap_register(env, mem_buf, len);
+    env->spr[reg] = ldn_p(mem_buf, len);
+
+    return len;
+}
+#endif
+
+static int gdb_get_float_reg(CPUPPCState *env, GByteArray *buf, int n)
+{
+    uint8_t *mem_buf;
+    if (n < 32) {
+        gdb_get_reg64(buf, *cpu_fpr_ptr(env, n));
+        mem_buf = gdb_get_reg_ptr(buf, 8);
+        ppc_maybe_bswap_register(env, mem_buf, 8);
+        return 8;
+    }
+    if (n == 32) {
+        gdb_get_reg32(buf, env->fpscr);
+        mem_buf = gdb_get_reg_ptr(buf, 4);
+        ppc_maybe_bswap_register(env, mem_buf, 4);
+        return 4;
+    }
+    return 0;
+}
+
+static int gdb_set_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
+{
+    if (n < 32) {
+        ppc_maybe_bswap_register(env, mem_buf, 8);
+        *cpu_fpr_ptr(env, n) = ldq_p(mem_buf);
+        return 8;
+    }
+    if (n == 32) {
+        ppc_maybe_bswap_register(env, mem_buf, 4);
+        store_fpscr(env, ldl_p(mem_buf), 0xffffffff);
+        return 4;
+    }
+    return 0;
+}
+
+static int gdb_get_avr_reg(CPUPPCState *env, GByteArray *buf, int n)
+{
+    uint8_t *mem_buf;
+
+    if (n < 32) {
+        ppc_avr_t *avr = cpu_avr_ptr(env, n);
+        if (!avr_need_swap(env)) {
+            gdb_get_reg128(buf, avr->u64[0] , avr->u64[1]);
+        } else {
+            gdb_get_reg128(buf, avr->u64[1] , avr->u64[0]);
+        }
+        mem_buf = gdb_get_reg_ptr(buf, 16);
+        ppc_maybe_bswap_register(env, mem_buf, 8);
+        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
+        return 16;
+    }
+    if (n == 32) {
+        gdb_get_reg32(buf, helper_mfvscr(env));
+        mem_buf = gdb_get_reg_ptr(buf, 4);
+        ppc_maybe_bswap_register(env, mem_buf, 4);
+        return 4;
+    }
+    if (n == 33) {
+        gdb_get_reg32(buf, (uint32_t)env->spr[SPR_VRSAVE]);
+        mem_buf = gdb_get_reg_ptr(buf, 4);
+        ppc_maybe_bswap_register(env, mem_buf, 4);
+        return 4;
+    }
+    return 0;
+}
+
+static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
+{
+    if (n < 32) {
+        ppc_avr_t *avr = cpu_avr_ptr(env, n);
+        ppc_maybe_bswap_register(env, mem_buf, 8);
+        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
+        if (!avr_need_swap(env)) {
+            avr->u64[0] = ldq_p(mem_buf);
+            avr->u64[1] = ldq_p(mem_buf + 8);
+        } else {
+            avr->u64[1] = ldq_p(mem_buf);
+            avr->u64[0] = ldq_p(mem_buf + 8);
+        }
+        return 16;
+    }
+    if (n == 32) {
+        ppc_maybe_bswap_register(env, mem_buf, 4);
+        helper_mtvscr(env, ldl_p(mem_buf));
+        return 4;
+    }
+    if (n == 33) {
+        ppc_maybe_bswap_register(env, mem_buf, 4);
+        env->spr[SPR_VRSAVE] = (target_ulong)ldl_p(mem_buf);
+        return 4;
+    }
+    return 0;
+}
+
+static int gdb_get_spe_reg(CPUPPCState *env, GByteArray *buf, int n)
+{
+    if (n < 32) {
+#if defined(TARGET_PPC64)
+        gdb_get_reg32(buf, env->gpr[n] >> 32);
+        ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, 4), 4);
+#else
+        gdb_get_reg32(buf, env->gprh[n]);
+#endif
+        return 4;
+    }
+    if (n == 32) {
+        gdb_get_reg64(buf, env->spe_acc);
+        ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, 8), 8);
+        return 8;
+    }
+    if (n == 33) {
+        gdb_get_reg32(buf, env->spe_fscr);
+        ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, 4), 4);
+        return 4;
+    }
+    return 0;
+}
+
+static int gdb_set_spe_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
+{
+    if (n < 32) {
+#if defined(TARGET_PPC64)
+        target_ulong lo = (uint32_t)env->gpr[n];
+        target_ulong hi;
+
+        ppc_maybe_bswap_register(env, mem_buf, 4);
+
+        hi = (target_ulong)ldl_p(mem_buf) << 32;
+        env->gpr[n] = lo | hi;
+#else
+        env->gprh[n] = ldl_p(mem_buf);
+#endif
+        return 4;
+    }
+    if (n == 32) {
+        ppc_maybe_bswap_register(env, mem_buf, 8);
+        env->spe_acc = ldq_p(mem_buf);
+        return 8;
+    }
+    if (n == 33) {
+        ppc_maybe_bswap_register(env, mem_buf, 4);
+        env->spe_fscr = ldl_p(mem_buf);
+        return 4;
+    }
+    return 0;
+}
+
+static int gdb_get_vsx_reg(CPUPPCState *env, GByteArray *buf, int n)
+{
+    if (n < 32) {
+        gdb_get_reg64(buf, *cpu_vsrl_ptr(env, n));
+        ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, 8), 8);
+        return 8;
+    }
+    return 0;
+}
+
+static int gdb_set_vsx_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
+{
+    if (n < 32) {
+        ppc_maybe_bswap_register(env, mem_buf, 8);
+        *cpu_vsrl_ptr(env, n) = ldq_p(mem_buf);
+        return 8;
+    }
+    return 0;
+}
+
+
+void ppc_cpu_gdb_init(CPUState *cs, PowerPCCPUClass *pcc)
+{
+
+    if (pcc->insns_flags & PPC_FLOAT) {
+        gdb_register_coprocessor(cs, gdb_get_float_reg, gdb_set_float_reg,
+                                 33, "power-fpu.xml", 0);
+    }
+    if (pcc->insns_flags & PPC_ALTIVEC) {
+        gdb_register_coprocessor(cs, gdb_get_avr_reg, gdb_set_avr_reg,
+                                 34, "power-altivec.xml", 0);
+    }
+    if (pcc->insns_flags & PPC_SPE) {
+        gdb_register_coprocessor(cs, gdb_get_spe_reg, gdb_set_spe_reg,
+                                 34, "power-spe.xml", 0);
+    }
+    if (pcc->insns_flags2 & PPC2_VSX) {
+        gdb_register_coprocessor(cs, gdb_get_vsx_reg, gdb_set_vsx_reg,
+                                 32, "power-vsx.xml", 0);
+    }
+#ifndef CONFIG_USER_ONLY
+    gdb_register_coprocessor(cs, gdb_get_spr_reg, gdb_set_spr_reg,
+                             pcc->gdb_num_sprs, "power-spr.xml", 0);
+#endif
+}
diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
index c03a7c4f52..e1228a4b92 100644
--- a/target/ppc/translate_init.c.inc
+++ b/target/ppc/translate_init.c.inc
@@ -820,7 +820,7 @@ static inline void _spr_register(CPUPPCState *env, int num,
 }
 
 /* Generic PowerPC SPRs */
-static void gen_spr_generic(CPUPPCState *env)
+void gen_spr_generic(CPUPPCState *env)
 {
     /* Integer processing */
     spr_register(env, SPR_XER, "XER",
@@ -3498,11 +3498,6 @@ static int check_pow_hid0_74xx(CPUPPCState *env)
     return 0;
 }
 
-static bool ppc_cpu_interrupts_big_endian_always(PowerPCCPU *cpu)
-{
-    return true;
-}
-
 #ifdef TARGET_PPC64
 static bool ppc_cpu_interrupts_big_endian_lpcr(PowerPCCPU *cpu)
 {
@@ -9329,27 +9324,12 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data)
     pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr;
 }
 
-#if !defined(CONFIG_USER_ONLY)
-void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
-{
-    CPUPPCState *env = &cpu->env;
-
-    cpu->vhyp = vhyp;
-
-    /*
-     * With a virtual hypervisor mode we never allow the CPU to go
-     * hypervisor mode itself
-     */
-    env->msr_mask &= ~MSR_HVB;
-}
-
-#endif /* !defined(CONFIG_USER_ONLY) */
 
 #endif /* defined(TARGET_PPC64) */
 
 /*****************************************************************************/
 /* Generic CPU instantiation routine                                         */
-static void init_ppc_proc(PowerPCCPU *cpu)
+void init_ppc_proc(PowerPCCPU *cpu)
 {
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
@@ -9783,7 +9763,7 @@ static void fix_opcode_tables(opc_handler_t **ppc_opcodes)
 }
 
 /*****************************************************************************/
-static void create_ppc_opcodes(PowerPCCPU *cpu, Error **errp)
+void create_ppc_opcodes(PowerPCCPU *cpu, Error **errp)
 {
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     opcode_t *opc;
@@ -9805,6 +9785,39 @@ static void create_ppc_opcodes(PowerPCCPU *cpu, Error **errp)
     fflush(stderr);
 }
 
+void destroy_ppc_opcodes(PowerPCCPU *cpu) {
+    opc_handler_t **table, **table_2;
+    int i, j, k;
+
+    for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
+        if (cpu->opcodes[i] == &invalid_handler) {
+            continue;
+        }
+        if (is_indirect_opcode(cpu->opcodes[i])) {
+            table = ind_table(cpu->opcodes[i]);
+            for (j = 0; j < PPC_CPU_INDIRECT_OPCODES_LEN; j++) {
+                if (table[j] == &invalid_handler) {
+                    continue;
+                }
+                if (is_indirect_opcode(table[j])) {
+                    table_2 = ind_table(table[j]);
+                    for (k = 0; k < PPC_CPU_INDIRECT_OPCODES_LEN; k++) {
+                        if (table_2[k] != &invalid_handler &&
+                            is_indirect_opcode(table_2[k])) {
+                            g_free((opc_handler_t *)((uintptr_t)table_2[k] &
+                                                     ~PPC_INDIRECT));
+                        }
+                    }
+                    g_free((opc_handler_t *)((uintptr_t)table[j] &
+                                             ~PPC_INDIRECT));
+                }
+            }
+            g_free((opc_handler_t *)((uintptr_t)cpu->opcodes[i] &
+                ~PPC_INDIRECT));
+        }
+    }
+}
+
 #if defined(PPC_DUMP_CPU)
 static void dump_ppc_insns(CPUPPCState *env)
 {
@@ -9895,1092 +9908,3 @@ static void dump_ppc_insns(CPUPPCState *env)
     }
 }
 #endif
-
-static bool avr_need_swap(CPUPPCState *env)
-{
-#ifdef HOST_WORDS_BIGENDIAN
-    return msr_le;
-#else
-    return !msr_le;
-#endif
-}
-
-#if !defined(CONFIG_USER_ONLY)
-static int gdb_find_spr_idx(CPUPPCState *env, int n)
-{
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
-        ppc_spr_t *spr = &env->spr_cb[i];
-
-        if (spr->name && spr->gdb_id == n) {
-            return i;
-        }
-    }
-    return -1;
-}
-
-static int gdb_get_spr_reg(CPUPPCState *env, GByteArray *buf, int n)
-{
-    int reg;
-    int len;
-
-    reg = gdb_find_spr_idx(env, n);
-    if (reg < 0) {
-        return 0;
-    }
-
-    len = TARGET_LONG_SIZE;
-    gdb_get_regl(buf, env->spr[reg]);
-    ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, len), len);
-    return len;
-}
-
-static int gdb_set_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
-{
-    int reg;
-    int len;
-
-    reg = gdb_find_spr_idx(env, n);
-    if (reg < 0) {
-        return 0;
-    }
-
-    len = TARGET_LONG_SIZE;
-    ppc_maybe_bswap_register(env, mem_buf, len);
-    env->spr[reg] = ldn_p(mem_buf, len);
-
-    return len;
-}
-#endif
-
-static int gdb_get_float_reg(CPUPPCState *env, GByteArray *buf, int n)
-{
-    uint8_t *mem_buf;
-    if (n < 32) {
-        gdb_get_reg64(buf, *cpu_fpr_ptr(env, n));
-        mem_buf = gdb_get_reg_ptr(buf, 8);
-        ppc_maybe_bswap_register(env, mem_buf, 8);
-        return 8;
-    }
-    if (n == 32) {
-        gdb_get_reg32(buf, env->fpscr);
-        mem_buf = gdb_get_reg_ptr(buf, 4);
-        ppc_maybe_bswap_register(env, mem_buf, 4);
-        return 4;
-    }
-    return 0;
-}
-
-static int gdb_set_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
-{
-    if (n < 32) {
-        ppc_maybe_bswap_register(env, mem_buf, 8);
-        *cpu_fpr_ptr(env, n) = ldq_p(mem_buf);
-        return 8;
-    }
-    if (n == 32) {
-        ppc_maybe_bswap_register(env, mem_buf, 4);
-        helper_store_fpscr(env, ldl_p(mem_buf), 0xffffffff);
-        return 4;
-    }
-    return 0;
-}
-
-static int gdb_get_avr_reg(CPUPPCState *env, GByteArray *buf, int n)
-{
-    uint8_t *mem_buf;
-
-    if (n < 32) {
-        ppc_avr_t *avr = cpu_avr_ptr(env, n);
-        if (!avr_need_swap(env)) {
-            gdb_get_reg128(buf, avr->u64[0] , avr->u64[1]);
-        } else {
-            gdb_get_reg128(buf, avr->u64[1] , avr->u64[0]);
-        }
-        mem_buf = gdb_get_reg_ptr(buf, 16);
-        ppc_maybe_bswap_register(env, mem_buf, 8);
-        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
-        return 16;
-    }
-    if (n == 32) {
-        gdb_get_reg32(buf, helper_mfvscr(env));
-        mem_buf = gdb_get_reg_ptr(buf, 4);
-        ppc_maybe_bswap_register(env, mem_buf, 4);
-        return 4;
-    }
-    if (n == 33) {
-        gdb_get_reg32(buf, (uint32_t)env->spr[SPR_VRSAVE]);
-        mem_buf = gdb_get_reg_ptr(buf, 4);
-        ppc_maybe_bswap_register(env, mem_buf, 4);
-        return 4;
-    }
-    return 0;
-}
-
-static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
-{
-    if (n < 32) {
-        ppc_avr_t *avr = cpu_avr_ptr(env, n);
-        ppc_maybe_bswap_register(env, mem_buf, 8);
-        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
-        if (!avr_need_swap(env)) {
-            avr->u64[0] = ldq_p(mem_buf);
-            avr->u64[1] = ldq_p(mem_buf + 8);
-        } else {
-            avr->u64[1] = ldq_p(mem_buf);
-            avr->u64[0] = ldq_p(mem_buf + 8);
-        }
-        return 16;
-    }
-    if (n == 32) {
-        ppc_maybe_bswap_register(env, mem_buf, 4);
-        helper_mtvscr(env, ldl_p(mem_buf));
-        return 4;
-    }
-    if (n == 33) {
-        ppc_maybe_bswap_register(env, mem_buf, 4);
-        env->spr[SPR_VRSAVE] = (target_ulong)ldl_p(mem_buf);
-        return 4;
-    }
-    return 0;
-}
-
-static int gdb_get_spe_reg(CPUPPCState *env, GByteArray *buf, int n)
-{
-    if (n < 32) {
-#if defined(TARGET_PPC64)
-        gdb_get_reg32(buf, env->gpr[n] >> 32);
-        ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, 4), 4);
-#else
-        gdb_get_reg32(buf, env->gprh[n]);
-#endif
-        return 4;
-    }
-    if (n == 32) {
-        gdb_get_reg64(buf, env->spe_acc);
-        ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, 8), 8);
-        return 8;
-    }
-    if (n == 33) {
-        gdb_get_reg32(buf, env->spe_fscr);
-        ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, 4), 4);
-        return 4;
-    }
-    return 0;
-}
-
-static int gdb_set_spe_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
-{
-    if (n < 32) {
-#if defined(TARGET_PPC64)
-        target_ulong lo = (uint32_t)env->gpr[n];
-        target_ulong hi;
-
-        ppc_maybe_bswap_register(env, mem_buf, 4);
-
-        hi = (target_ulong)ldl_p(mem_buf) << 32;
-        env->gpr[n] = lo | hi;
-#else
-        env->gprh[n] = ldl_p(mem_buf);
-#endif
-        return 4;
-    }
-    if (n == 32) {
-        ppc_maybe_bswap_register(env, mem_buf, 8);
-        env->spe_acc = ldq_p(mem_buf);
-        return 8;
-    }
-    if (n == 33) {
-        ppc_maybe_bswap_register(env, mem_buf, 4);
-        env->spe_fscr = ldl_p(mem_buf);
-        return 4;
-    }
-    return 0;
-}
-
-static int gdb_get_vsx_reg(CPUPPCState *env, GByteArray *buf, int n)
-{
-    if (n < 32) {
-        gdb_get_reg64(buf, *cpu_vsrl_ptr(env, n));
-        ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, 8), 8);
-        return 8;
-    }
-    return 0;
-}
-
-static int gdb_set_vsx_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
-{
-    if (n < 32) {
-        ppc_maybe_bswap_register(env, mem_buf, 8);
-        *cpu_vsrl_ptr(env, n) = ldq_p(mem_buf);
-        return 8;
-    }
-    return 0;
-}
-
-static int ppc_fixup_cpu(PowerPCCPU *cpu)
-{
-    CPUPPCState *env = &cpu->env;
-
-    /*
-     * TCG doesn't (yet) emulate some groups of instructions that are
-     * implemented on some otherwise supported CPUs (e.g. VSX and
-     * decimal floating point instructions on POWER7).  We remove
-     * unsupported instruction groups from the cpu state's instruction
-     * masks and hope the guest can cope.  For at least the pseries
-     * machine, the unavailability of these instructions can be
-     * advertised to the guest via the device tree.
-     */
-    if ((env->insns_flags & ~PPC_TCG_INSNS)
-        || (env->insns_flags2 & ~PPC_TCG_INSNS2)) {
-        warn_report("Disabling some instructions which are not "
-                    "emulated by TCG (0x%" PRIx64 ", 0x%" PRIx64 ")",
-                    env->insns_flags & ~PPC_TCG_INSNS,
-                    env->insns_flags2 & ~PPC_TCG_INSNS2);
-    }
-    env->insns_flags &= PPC_TCG_INSNS;
-    env->insns_flags2 &= PPC_TCG_INSNS2;
-    return 0;
-}
-
-static void ppc_cpu_realize(DeviceState *dev, Error **errp)
-{
-    CPUState *cs = CPU(dev);
-    PowerPCCPU *cpu = POWERPC_CPU(dev);
-    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
-    Error *local_err = NULL;
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
-    if (cpu->vcpu_id == UNASSIGNED_CPU_INDEX) {
-        cpu->vcpu_id = cs->cpu_index;
-    }
-
-    if (tcg_enabled()) {
-        if (ppc_fixup_cpu(cpu) != 0) {
-            error_setg(errp, "Unable to emulate selected CPU with TCG");
-            goto unrealize;
-        }
-    }
-
-    create_ppc_opcodes(cpu, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        goto unrealize;
-    }
-    init_ppc_proc(cpu);
-
-    if (pcc->insns_flags & PPC_FLOAT) {
-        gdb_register_coprocessor(cs, gdb_get_float_reg, gdb_set_float_reg,
-                                 33, "power-fpu.xml", 0);
-    }
-    if (pcc->insns_flags & PPC_ALTIVEC) {
-        gdb_register_coprocessor(cs, gdb_get_avr_reg, gdb_set_avr_reg,
-                                 34, "power-altivec.xml", 0);
-    }
-    if (pcc->insns_flags & PPC_SPE) {
-        gdb_register_coprocessor(cs, gdb_get_spe_reg, gdb_set_spe_reg,
-                                 34, "power-spe.xml", 0);
-    }
-    if (pcc->insns_flags2 & PPC2_VSX) {
-        gdb_register_coprocessor(cs, gdb_get_vsx_reg, gdb_set_vsx_reg,
-                                 32, "power-vsx.xml", 0);
-    }
-#ifndef CONFIG_USER_ONLY
-    gdb_register_coprocessor(cs, gdb_get_spr_reg, gdb_set_spr_reg,
-                             pcc->gdb_num_sprs, "power-spr.xml", 0);
-#endif
-    qemu_init_vcpu(cs);
-
-    pcc->parent_realize(dev, errp);
-
-#if defined(PPC_DUMP_CPU)
-    {
-        CPUPPCState *env = &cpu->env;
-        const char *mmu_model, *excp_model, *bus_model;
-        switch (env->mmu_model) {
-        case POWERPC_MMU_32B:
-            mmu_model = "PowerPC 32";
-            break;
-        case POWERPC_MMU_SOFT_6xx:
-            mmu_model = "PowerPC 6xx/7xx with software driven TLBs";
-            break;
-        case POWERPC_MMU_SOFT_74xx:
-            mmu_model = "PowerPC 74xx with software driven TLBs";
-            break;
-        case POWERPC_MMU_SOFT_4xx:
-            mmu_model = "PowerPC 4xx with software driven TLBs";
-            break;
-        case POWERPC_MMU_SOFT_4xx_Z:
-            mmu_model = "PowerPC 4xx with software driven TLBs "
-                "and zones protections";
-            break;
-        case POWERPC_MMU_REAL:
-            mmu_model = "PowerPC real mode only";
-            break;
-        case POWERPC_MMU_MPC8xx:
-            mmu_model = "PowerPC MPC8xx";
-            break;
-        case POWERPC_MMU_BOOKE:
-            mmu_model = "PowerPC BookE";
-            break;
-        case POWERPC_MMU_BOOKE206:
-            mmu_model = "PowerPC BookE 2.06";
-            break;
-        case POWERPC_MMU_601:
-            mmu_model = "PowerPC 601";
-            break;
-#if defined(TARGET_PPC64)
-        case POWERPC_MMU_64B:
-            mmu_model = "PowerPC 64";
-            break;
-#endif
-        default:
-            mmu_model = "Unknown or invalid";
-            break;
-        }
-        switch (env->excp_model) {
-        case POWERPC_EXCP_STD:
-            excp_model = "PowerPC";
-            break;
-        case POWERPC_EXCP_40x:
-            excp_model = "PowerPC 40x";
-            break;
-        case POWERPC_EXCP_601:
-            excp_model = "PowerPC 601";
-            break;
-        case POWERPC_EXCP_602:
-            excp_model = "PowerPC 602";
-            break;
-        case POWERPC_EXCP_603:
-            excp_model = "PowerPC 603";
-            break;
-        case POWERPC_EXCP_603E:
-            excp_model = "PowerPC 603e";
-            break;
-        case POWERPC_EXCP_604:
-            excp_model = "PowerPC 604";
-            break;
-        case POWERPC_EXCP_7x0:
-            excp_model = "PowerPC 740/750";
-            break;
-        case POWERPC_EXCP_7x5:
-            excp_model = "PowerPC 745/755";
-            break;
-        case POWERPC_EXCP_74xx:
-            excp_model = "PowerPC 74xx";
-            break;
-        case POWERPC_EXCP_BOOKE:
-            excp_model = "PowerPC BookE";
-            break;
-#if defined(TARGET_PPC64)
-        case POWERPC_EXCP_970:
-            excp_model = "PowerPC 970";
-            break;
-#endif
-        default:
-            excp_model = "Unknown or invalid";
-            break;
-        }
-        switch (env->bus_model) {
-        case PPC_FLAGS_INPUT_6xx:
-            bus_model = "PowerPC 6xx";
-            break;
-        case PPC_FLAGS_INPUT_BookE:
-            bus_model = "PowerPC BookE";
-            break;
-        case PPC_FLAGS_INPUT_405:
-            bus_model = "PowerPC 405";
-            break;
-        case PPC_FLAGS_INPUT_401:
-            bus_model = "PowerPC 401/403";
-            break;
-        case PPC_FLAGS_INPUT_RCPU:
-            bus_model = "RCPU / MPC8xx";
-            break;
-#if defined(TARGET_PPC64)
-        case PPC_FLAGS_INPUT_970:
-            bus_model = "PowerPC 970";
-            break;
-#endif
-        default:
-            bus_model = "Unknown or invalid";
-            break;
-        }
-        printf("PowerPC %-12s : PVR %08x MSR %016" PRIx64 "\n"
-               "    MMU model        : %s\n",
-               object_class_get_name(OBJECT_CLASS(pcc)),
-               pcc->pvr, pcc->msr_mask, mmu_model);
-#if !defined(CONFIG_USER_ONLY)
-        if (env->tlb.tlb6) {
-            printf("                       %d %s TLB in %d ways\n",
-                   env->nb_tlb, env->id_tlbs ? "splitted" : "merged",
-                   env->nb_ways);
-        }
-#endif
-        printf("    Exceptions model : %s\n"
-               "    Bus model        : %s\n",
-               excp_model, bus_model);
-        printf("    MSR features     :\n");
-        if (env->flags & POWERPC_FLAG_SPE) {
-            printf("                        signal processing engine enable"
-                   "\n");
-        } else if (env->flags & POWERPC_FLAG_VRE) {
-            printf("                        vector processor enable\n");
-        }
-        if (env->flags & POWERPC_FLAG_TGPR) {
-            printf("                        temporary GPRs\n");
-        } else if (env->flags & POWERPC_FLAG_CE) {
-            printf("                        critical input enable\n");
-        }
-        if (env->flags & POWERPC_FLAG_SE) {
-            printf("                        single-step trace mode\n");
-        } else if (env->flags & POWERPC_FLAG_DWE) {
-            printf("                        debug wait enable\n");
-        } else if (env->flags & POWERPC_FLAG_UBLE) {
-            printf("                        user BTB lock enable\n");
-        }
-        if (env->flags & POWERPC_FLAG_BE) {
-            printf("                        branch-step trace mode\n");
-        } else if (env->flags & POWERPC_FLAG_DE) {
-            printf("                        debug interrupt enable\n");
-        }
-        if (env->flags & POWERPC_FLAG_PX) {
-            printf("                        inclusive protection\n");
-        } else if (env->flags & POWERPC_FLAG_PMM) {
-            printf("                        performance monitor mark\n");
-        }
-        if (env->flags == POWERPC_FLAG_NONE) {
-            printf("                        none\n");
-        }
-        printf("    Time-base/decrementer clock source: %s\n",
-               env->flags & POWERPC_FLAG_RTC_CLK ? "RTC clock" : "bus clock");
-        dump_ppc_insns(env);
-        dump_ppc_sprs(env);
-        fflush(stdout);
-    }
-#endif
-    return;
-
-unrealize:
-    cpu_exec_unrealizefn(cs);
-}
-
-static void ppc_cpu_unrealize(DeviceState *dev)
-{
-    PowerPCCPU *cpu = POWERPC_CPU(dev);
-    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
-    opc_handler_t **table, **table_2;
-    int i, j, k;
-
-    pcc->parent_unrealize(dev);
-
-    cpu_remove_sync(CPU(cpu));
-
-    for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
-        if (cpu->opcodes[i] == &invalid_handler) {
-            continue;
-        }
-        if (is_indirect_opcode(cpu->opcodes[i])) {
-            table = ind_table(cpu->opcodes[i]);
-            for (j = 0; j < PPC_CPU_INDIRECT_OPCODES_LEN; j++) {
-                if (table[j] == &invalid_handler) {
-                    continue;
-                }
-                if (is_indirect_opcode(table[j])) {
-                    table_2 = ind_table(table[j]);
-                    for (k = 0; k < PPC_CPU_INDIRECT_OPCODES_LEN; k++) {
-                        if (table_2[k] != &invalid_handler &&
-                            is_indirect_opcode(table_2[k])) {
-                            g_free((opc_handler_t *)((uintptr_t)table_2[k] &
-                                                     ~PPC_INDIRECT));
-                        }
-                    }
-                    g_free((opc_handler_t *)((uintptr_t)table[j] &
-                                             ~PPC_INDIRECT));
-                }
-            }
-            g_free((opc_handler_t *)((uintptr_t)cpu->opcodes[i] &
-                ~PPC_INDIRECT));
-        }
-    }
-}
-
-static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b)
-{
-    ObjectClass *oc = (ObjectClass *)a;
-    uint32_t pvr = *(uint32_t *)b;
-    PowerPCCPUClass *pcc = (PowerPCCPUClass *)a;
-
-    /* -cpu host does a PVR lookup during construction */
-    if (unlikely(strcmp(object_class_get_name(oc),
-                        TYPE_HOST_POWERPC_CPU) == 0)) {
-        return -1;
-    }
-
-    return pcc->pvr == pvr ? 0 : -1;
-}
-
-PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr)
-{
-    GSList *list, *item;
-    PowerPCCPUClass *pcc = NULL;
-
-    list = object_class_get_list(TYPE_POWERPC_CPU, false);
-    item = g_slist_find_custom(list, &pvr, ppc_cpu_compare_class_pvr);
-    if (item != NULL) {
-        pcc = POWERPC_CPU_CLASS(item->data);
-    }
-    g_slist_free(list);
-
-    return pcc;
-}
-
-static gint ppc_cpu_compare_class_pvr_mask(gconstpointer a, gconstpointer b)
-{
-    ObjectClass *oc = (ObjectClass *)a;
-    uint32_t pvr = *(uint32_t *)b;
-    PowerPCCPUClass *pcc = (PowerPCCPUClass *)a;
-
-    /* -cpu host does a PVR lookup during construction */
-    if (unlikely(strcmp(object_class_get_name(oc),
-                        TYPE_HOST_POWERPC_CPU) == 0)) {
-        return -1;
-    }
-
-    if (pcc->pvr_match(pcc, pvr)) {
-        return 0;
-    }
-
-    return -1;
-}
-
-PowerPCCPUClass *ppc_cpu_class_by_pvr_mask(uint32_t pvr)
-{
-    GSList *list, *item;
-    PowerPCCPUClass *pcc = NULL;
-
-    list = object_class_get_list(TYPE_POWERPC_CPU, true);
-    item = g_slist_find_custom(list, &pvr, ppc_cpu_compare_class_pvr_mask);
-    if (item != NULL) {
-        pcc = POWERPC_CPU_CLASS(item->data);
-    }
-    g_slist_free(list);
-
-    return pcc;
-}
-
-static const char *ppc_cpu_lookup_alias(const char *alias)
-{
-    int ai;
-
-    for (ai = 0; ppc_cpu_aliases[ai].alias != NULL; ai++) {
-        if (strcmp(ppc_cpu_aliases[ai].alias, alias) == 0) {
-            return ppc_cpu_aliases[ai].model;
-        }
-    }
-
-    return NULL;
-}
-
-static ObjectClass *ppc_cpu_class_by_name(const char *name)
-{
-    char *cpu_model, *typename;
-    ObjectClass *oc;
-    const char *p;
-    unsigned long pvr;
-
-    /*
-     * Lookup by PVR if cpu_model is valid 8 digit hex number (excl:
-     * 0x prefix if present)
-     */
-    if (!qemu_strtoul(name, &p, 16, &pvr)) {
-        int len = p - name;
-        len = (len == 10) && (name[1] == 'x') ? len - 2 : len;
-        if ((len == 8) && (*p == '\0')) {
-            return OBJECT_CLASS(ppc_cpu_class_by_pvr(pvr));
-        }
-    }
-
-    cpu_model = g_ascii_strdown(name, -1);
-    p = ppc_cpu_lookup_alias(cpu_model);
-    if (p) {
-        g_free(cpu_model);
-        cpu_model = g_strdup(p);
-    }
-
-    typename = g_strdup_printf("%s" POWERPC_CPU_TYPE_SUFFIX, cpu_model);
-    oc = object_class_by_name(typename);
-    g_free(typename);
-    g_free(cpu_model);
-
-    return oc;
-}
-
-PowerPCCPUClass *ppc_cpu_get_family_class(PowerPCCPUClass *pcc)
-{
-    ObjectClass *oc = OBJECT_CLASS(pcc);
-
-    while (oc && !object_class_is_abstract(oc)) {
-        oc = object_class_get_parent(oc);
-    }
-    assert(oc);
-
-    return POWERPC_CPU_CLASS(oc);
-}
-
-/* Sort by PVR, ordering special case "host" last. */
-static gint ppc_cpu_list_compare(gconstpointer a, gconstpointer b)
-{
-    ObjectClass *oc_a = (ObjectClass *)a;
-    ObjectClass *oc_b = (ObjectClass *)b;
-    PowerPCCPUClass *pcc_a = POWERPC_CPU_CLASS(oc_a);
-    PowerPCCPUClass *pcc_b = POWERPC_CPU_CLASS(oc_b);
-    const char *name_a = object_class_get_name(oc_a);
-    const char *name_b = object_class_get_name(oc_b);
-
-    if (strcmp(name_a, TYPE_HOST_POWERPC_CPU) == 0) {
-        return 1;
-    } else if (strcmp(name_b, TYPE_HOST_POWERPC_CPU) == 0) {
-        return -1;
-    } else {
-        /* Avoid an integer overflow during subtraction */
-        if (pcc_a->pvr < pcc_b->pvr) {
-            return -1;
-        } else if (pcc_a->pvr > pcc_b->pvr) {
-            return 1;
-        } else {
-            return 0;
-        }
-    }
-}
-
-static void ppc_cpu_list_entry(gpointer data, gpointer user_data)
-{
-    ObjectClass *oc = data;
-    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
-    DeviceClass *family = DEVICE_CLASS(ppc_cpu_get_family_class(pcc));
-    const char *typename = object_class_get_name(oc);
-    char *name;
-    int i;
-
-    if (unlikely(strcmp(typename, TYPE_HOST_POWERPC_CPU) == 0)) {
-        return;
-    }
-
-    name = g_strndup(typename,
-                     strlen(typename) - strlen(POWERPC_CPU_TYPE_SUFFIX));
-    qemu_printf("PowerPC %-16s PVR %08x\n", name, pcc->pvr);
-    for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
-        PowerPCCPUAlias *alias = &ppc_cpu_aliases[i];
-        ObjectClass *alias_oc = ppc_cpu_class_by_name(alias->model);
-
-        if (alias_oc != oc) {
-            continue;
-        }
-        /*
-         * If running with KVM, we might update the family alias later, so
-         * avoid printing the wrong alias here and use "preferred" instead
-         */
-        if (strcmp(alias->alias, family->desc) == 0) {
-            qemu_printf("PowerPC %-16s (alias for preferred %s CPU)\n",
-                        alias->alias, family->desc);
-        } else {
-            qemu_printf("PowerPC %-16s (alias for %s)\n",
-                        alias->alias, name);
-        }
-    }
-    g_free(name);
-}
-
-void ppc_cpu_list(void)
-{
-    GSList *list;
-
-    list = object_class_get_list(TYPE_POWERPC_CPU, false);
-    list = g_slist_sort(list, ppc_cpu_list_compare);
-    g_slist_foreach(list, ppc_cpu_list_entry, NULL);
-    g_slist_free(list);
-
-#ifdef CONFIG_KVM
-    qemu_printf("\n");
-    qemu_printf("PowerPC %-16s\n", "host");
-#endif
-}
-
-static void ppc_cpu_defs_entry(gpointer data, gpointer user_data)
-{
-    ObjectClass *oc = data;
-    CpuDefinitionInfoList **first = user_data;
-    const char *typename;
-    CpuDefinitionInfo *info;
-
-    typename = object_class_get_name(oc);
-    info = g_malloc0(sizeof(*info));
-    info->name = g_strndup(typename,
-                           strlen(typename) - strlen(POWERPC_CPU_TYPE_SUFFIX));
-
-    QAPI_LIST_PREPEND(*first, info);
-}
-
-CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
-{
-    CpuDefinitionInfoList *cpu_list = NULL;
-    GSList *list;
-    int i;
-
-    list = object_class_get_list(TYPE_POWERPC_CPU, false);
-    g_slist_foreach(list, ppc_cpu_defs_entry, &cpu_list);
-    g_slist_free(list);
-
-    for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
-        PowerPCCPUAlias *alias = &ppc_cpu_aliases[i];
-        ObjectClass *oc;
-        CpuDefinitionInfo *info;
-
-        oc = ppc_cpu_class_by_name(alias->model);
-        if (oc == NULL) {
-            continue;
-        }
-
-        info = g_malloc0(sizeof(*info));
-        info->name = g_strdup(alias->alias);
-        info->q_typename = g_strdup(object_class_get_name(oc));
-
-        QAPI_LIST_PREPEND(cpu_list, info);
-    }
-
-    return cpu_list;
-}
-
-static void ppc_cpu_set_pc(CPUState *cs, vaddr value)
-{
-    PowerPCCPU *cpu = POWERPC_CPU(cs);
-
-    cpu->env.nip = value;
-}
-
-static bool ppc_cpu_has_work(CPUState *cs)
-{
-    PowerPCCPU *cpu = POWERPC_CPU(cs);
-    CPUPPCState *env = &cpu->env;
-
-    return msr_ee && (cs->interrupt_request & CPU_INTERRUPT_HARD);
-}
-
-static void ppc_cpu_reset(DeviceState *dev)
-{
-    CPUState *s = CPU(dev);
-    PowerPCCPU *cpu = POWERPC_CPU(s);
-    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
-    CPUPPCState *env = &cpu->env;
-    target_ulong msr;
-    int i;
-
-    pcc->parent_reset(dev);
-
-    msr = (target_ulong)0;
-    msr |= (target_ulong)MSR_HVB;
-    msr |= (target_ulong)0 << MSR_AP; /* TO BE CHECKED */
-    msr |= (target_ulong)0 << MSR_SA; /* TO BE CHECKED */
-    msr |= (target_ulong)1 << MSR_EP;
-#if defined(DO_SINGLE_STEP) && 0
-    /* Single step trace mode */
-    msr |= (target_ulong)1 << MSR_SE;
-    msr |= (target_ulong)1 << MSR_BE;
-#endif
-#if defined(CONFIG_USER_ONLY)
-    msr |= (target_ulong)1 << MSR_FP; /* Allow floating point usage */
-    msr |= (target_ulong)1 << MSR_FE0; /* Allow floating point exceptions */
-    msr |= (target_ulong)1 << MSR_FE1;
-    msr |= (target_ulong)1 << MSR_VR; /* Allow altivec usage */
-    msr |= (target_ulong)1 << MSR_VSX; /* Allow VSX usage */
-    msr |= (target_ulong)1 << MSR_SPE; /* Allow SPE usage */
-    msr |= (target_ulong)1 << MSR_PR;
-#if defined(TARGET_PPC64)
-    msr |= (target_ulong)1 << MSR_TM; /* Transactional memory */
-#endif
-#if !defined(TARGET_WORDS_BIGENDIAN)
-    msr |= (target_ulong)1 << MSR_LE; /* Little-endian user mode */
-    if (!((env->msr_mask >> MSR_LE) & 1)) {
-        fprintf(stderr, "Selected CPU does not support little-endian.\n");
-        exit(1);
-    }
-#endif
-#endif
-
-#if defined(TARGET_PPC64)
-    if (mmu_is_64bit(env->mmu_model)) {
-        msr |= (1ULL << MSR_SF);
-    }
-#endif
-
-    hreg_store_msr(env, msr, 1);
-
-#if !defined(CONFIG_USER_ONLY)
-    env->nip = env->hreset_vector | env->excp_prefix;
-    if (env->mmu_model != POWERPC_MMU_REAL) {
-        ppc_tlb_invalidate_all(env);
-    }
-#endif
-
-    hreg_compute_hflags(env);
-    env->reserve_addr = (target_ulong)-1ULL;
-    /* Be sure no exception or interrupt is pending */
-    env->pending_interrupts = 0;
-    s->exception_index = POWERPC_EXCP_NONE;
-    env->error_code = 0;
-    ppc_irq_reset(cpu);
-
-    /* tininess for underflow is detected before rounding */
-    set_float_detect_tininess(float_tininess_before_rounding,
-                              &env->fp_status);
-
-    for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
-        ppc_spr_t *spr = &env->spr_cb[i];
-
-        if (!spr->name) {
-            continue;
-        }
-        env->spr[i] = spr->default_value;
-    }
-}
-
-#ifndef CONFIG_USER_ONLY
-
-static bool ppc_cpu_is_big_endian(CPUState *cs)
-{
-    PowerPCCPU *cpu = POWERPC_CPU(cs);
-    CPUPPCState *env = &cpu->env;
-
-    cpu_synchronize_state(cs);
-
-    return !msr_le;
-}
-
-#ifdef CONFIG_TCG
-static void ppc_cpu_exec_enter(CPUState *cs)
-{
-    PowerPCCPU *cpu = POWERPC_CPU(cs);
-
-    if (cpu->vhyp) {
-        PPCVirtualHypervisorClass *vhc =
-            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
-        vhc->cpu_exec_enter(cpu->vhyp, cpu);
-    }
-}
-
-static void ppc_cpu_exec_exit(CPUState *cs)
-{
-    PowerPCCPU *cpu = POWERPC_CPU(cs);
-
-    if (cpu->vhyp) {
-        PPCVirtualHypervisorClass *vhc =
-            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
-        vhc->cpu_exec_exit(cpu->vhyp, cpu);
-    }
-}
-#endif /* CONFIG_TCG */
-
-#endif /* !CONFIG_USER_ONLY */
-
-static void ppc_cpu_instance_init(Object *obj)
-{
-    PowerPCCPU *cpu = POWERPC_CPU(obj);
-    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
-    CPUPPCState *env = &cpu->env;
-
-    cpu_set_cpustate_pointers(cpu);
-    cpu->vcpu_id = UNASSIGNED_CPU_INDEX;
-
-    env->msr_mask = pcc->msr_mask;
-    env->mmu_model = pcc->mmu_model;
-    env->excp_model = pcc->excp_model;
-    env->bus_model = pcc->bus_model;
-    env->insns_flags = pcc->insns_flags;
-    env->insns_flags2 = pcc->insns_flags2;
-    env->flags = pcc->flags;
-    env->bfd_mach = pcc->bfd_mach;
-    env->check_pow = pcc->check_pow;
-
-    /*
-     * Mark HV mode as supported if the CPU has an MSR_HV bit in the
-     * msr_mask. The mask can later be cleared by PAPR mode but the hv
-     * mode support will remain, thus enforcing that we cannot use
-     * priv. instructions in guest in PAPR mode. For 970 we currently
-     * simply don't set HV in msr_mask thus simulating an "Apple mode"
-     * 970. If we ever want to support 970 HV mode, we'll have to add
-     * a processor attribute of some sort.
-     */
-#if !defined(CONFIG_USER_ONLY)
-    env->has_hv_mode = !!(env->msr_mask & MSR_HVB);
-#endif
-
-    ppc_hash64_init(cpu);
-}
-
-static void ppc_cpu_instance_finalize(Object *obj)
-{
-    PowerPCCPU *cpu = POWERPC_CPU(obj);
-
-    ppc_hash64_finalize(cpu);
-}
-
-static bool ppc_pvr_match_default(PowerPCCPUClass *pcc, uint32_t pvr)
-{
-    return pcc->pvr == pvr;
-}
-
-static gchar *ppc_gdb_arch_name(CPUState *cs)
-{
-#if defined(TARGET_PPC64)
-    return g_strdup("powerpc:common64");
-#else
-    return g_strdup("powerpc:common");
-#endif
-}
-
-static void ppc_disas_set_info(CPUState *cs, disassemble_info *info)
-{
-    PowerPCCPU *cpu = POWERPC_CPU(cs);
-    CPUPPCState *env = &cpu->env;
-
-    if ((env->hflags >> MSR_LE) & 1) {
-        info->endian = BFD_ENDIAN_LITTLE;
-    }
-    info->mach = env->bfd_mach;
-    if (!env->bfd_mach) {
-#ifdef TARGET_PPC64
-        info->mach = bfd_mach_ppc64;
-#else
-        info->mach = bfd_mach_ppc;
-#endif
-    }
-    info->disassembler_options = (char *)"any";
-    info->print_insn = print_insn_ppc;
-
-    info->cap_arch = CS_ARCH_PPC;
-#ifdef TARGET_PPC64
-    info->cap_mode = CS_MODE_64;
-#endif
-}
-
-static Property ppc_cpu_properties[] = {
-    DEFINE_PROP_BOOL("pre-2.8-migration", PowerPCCPU, pre_2_8_migration, false),
-    DEFINE_PROP_BOOL("pre-2.10-migration", PowerPCCPU, pre_2_10_migration,
-                     false),
-    DEFINE_PROP_BOOL("pre-3.0-migration", PowerPCCPU, pre_3_0_migration,
-                     false),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-#ifdef CONFIG_TCG
-#include "hw/core/tcg-cpu-ops.h"
-
-static struct TCGCPUOps ppc_tcg_ops = {
-  .initialize = ppc_translate_init,
-  .cpu_exec_interrupt = ppc_cpu_exec_interrupt,
-  .tlb_fill = ppc_cpu_tlb_fill,
-
-#ifndef CONFIG_USER_ONLY
-  .do_interrupt = ppc_cpu_do_interrupt,
-  .cpu_exec_enter = ppc_cpu_exec_enter,
-  .cpu_exec_exit = ppc_cpu_exec_exit,
-  .do_unaligned_access = ppc_cpu_do_unaligned_access,
-#endif /* !CONFIG_USER_ONLY */
-};
-#endif /* CONFIG_TCG */
-
-static void ppc_cpu_class_init(ObjectClass *oc, void *data)
-{
-    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
-    CPUClass *cc = CPU_CLASS(oc);
-    DeviceClass *dc = DEVICE_CLASS(oc);
-
-    device_class_set_parent_realize(dc, ppc_cpu_realize,
-                                    &pcc->parent_realize);
-    device_class_set_parent_unrealize(dc, ppc_cpu_unrealize,
-                                      &pcc->parent_unrealize);
-    pcc->pvr_match = ppc_pvr_match_default;
-    pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_always;
-    device_class_set_props(dc, ppc_cpu_properties);
-
-    device_class_set_parent_reset(dc, ppc_cpu_reset, &pcc->parent_reset);
-
-    cc->class_by_name = ppc_cpu_class_by_name;
-    cc->has_work = ppc_cpu_has_work;
-    cc->dump_state = ppc_cpu_dump_state;
-    cc->dump_statistics = ppc_cpu_dump_statistics;
-    cc->set_pc = ppc_cpu_set_pc;
-    cc->gdb_read_register = ppc_cpu_gdb_read_register;
-    cc->gdb_write_register = ppc_cpu_gdb_write_register;
-#ifndef CONFIG_USER_ONLY
-    cc->get_phys_page_debug = ppc_cpu_get_phys_page_debug;
-    cc->vmsd = &vmstate_ppc_cpu;
-#endif
-#if defined(CONFIG_SOFTMMU)
-    cc->write_elf64_note = ppc64_cpu_write_elf64_note;
-    cc->write_elf32_note = ppc32_cpu_write_elf32_note;
-#endif
-
-    cc->gdb_num_core_regs = 71;
-#ifndef CONFIG_USER_ONLY
-    cc->gdb_get_dynamic_xml = ppc_gdb_get_dynamic_xml;
-#endif
-#ifdef USE_APPLE_GDB
-    cc->gdb_read_register = ppc_cpu_gdb_read_register_apple;
-    cc->gdb_write_register = ppc_cpu_gdb_write_register_apple;
-    cc->gdb_num_core_regs = 71 + 32;
-#endif
-
-    cc->gdb_arch_name = ppc_gdb_arch_name;
-#if defined(TARGET_PPC64)
-    cc->gdb_core_xml_file = "power64-core.xml";
-#else
-    cc->gdb_core_xml_file = "power-core.xml";
-#endif
-#ifndef CONFIG_USER_ONLY
-    cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
-#endif
-    cc->disas_set_info = ppc_disas_set_info;
-
-    dc->fw_name = "PowerPC,UNKNOWN";
-
-#ifdef CONFIG_TCG
-    cc->tcg_ops = &ppc_tcg_ops;
-#endif /* CONFIG_TCG */
-}
-
-static const TypeInfo ppc_cpu_type_info = {
-    .name = TYPE_POWERPC_CPU,
-    .parent = TYPE_CPU,
-    .instance_size = sizeof(PowerPCCPU),
-    .instance_align = __alignof__(PowerPCCPU),
-    .instance_init = ppc_cpu_instance_init,
-    .instance_finalize = ppc_cpu_instance_finalize,
-    .abstract = true,
-    .class_size = sizeof(PowerPCCPUClass),
-    .class_init = ppc_cpu_class_init,
-};
-
-#ifndef CONFIG_USER_ONLY
-static const TypeInfo ppc_vhyp_type_info = {
-    .name = TYPE_PPC_VIRTUAL_HYPERVISOR,
-    .parent = TYPE_INTERFACE,
-    .class_size = sizeof(PPCVirtualHypervisorClass),
-};
-#endif
-
-static void ppc_cpu_register_types(void)
-{
-    type_register_static(&ppc_cpu_type_info);
-#ifndef CONFIG_USER_ONLY
-    type_register_static(&ppc_vhyp_type_info);
-#endif
-}
-
-type_init(ppc_cpu_register_types)
-- 
2.17.1

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

[Fabiano Rosas]

"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.

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

[David Gibson]

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