RE: Discussion: Patch series that adds disable-tcg option for ppc targets

RE: Discussion: Patch series that adds disable-tcg option for ppc targets

[Bruno Piazera Larsen]

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

> t's usually best to focus on logical changes, rather than
> file-by-file.  That said, I'd probably suggest changing the .c files
> first, then changing the meson file.

OK, will do!

> I'd lean towards building a whole series, since I think the individual
> file changes won't make a lot of sense on their own.  That said, it's
> ok and encouraged to post a relatively early draft of the series as an
> RFC, so that the overall idea can be reviewed, even if it has obvious
> omissions (just mention them in the cover letter).

Good to know. I'm guessing that the RFC should at least compile, though, right? That's mostly what is holding me from sharing what we have so far.

> I'd expect  mmu-*.c to be excluded.  Those are softmmu implementations
> which shouldn't be used with KVM.  It's possible there are a few
> helpers that will need to be extracted, though.

weirdly enough, removing the mmu-* don't give compilation errors, but linker errors. There are a few helpers that we'll have to see how to deal with, but something that I found odd is that cpu_list and all it's related functions are defined in translate_init.c.inc, instead of cpu.c like most other targets. I'm thinking about moving those to the cpu.c and follow the general standard of the rest

> You'll probably also need changes in hw/ppc/spapr_hcall.c and maybe
> some other parts of the spapr code: there are a number of hypercalls
> that we implement in qemu for TCG, but which are (and must be)
> implemented in KVM when KVM is in use.  So, I expect you'll need to
> suppress compilation of h_enter, h_remove, h_protect, h_read and
> h_bulk_remove at least in the !TCG case.

Yeah. there are 6 files dealing with spapr that are having linking problems, though some might be because of the cpu_list problem... But I'm a bit confused on how to interface with KVM, do I use an ioctl explicitly, or can it handle it implicitly? wenever I google or ask on IRC for KVM stuff the answer is usually "KVM handles it" so I never know if/when to call it

> Sounds fine so far.

Awesome! As the first contribution, I always feel like I'm missing something, thanks for the help (:


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, 06 de Abril de 2021 03:11
Para: Bruno Piazera Larsen
Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; Lucas Mateus Martins Araujo e Castro; Luis Fernando Fujita Pires; Fernando Eckhardt Valle; Matheus Kowalczuk Ferst; Andre Fernando da Silva
Assunto: Re: Discussion: Patch series that adds disable-tcg option for ppc targets

On Mon, Apr 05, 2021 at 01:32:18PM +0000, Bruno Piazera Larsen wrote:
> Hi all,
>
> The team I'm working on started to work on adding support for
> building the ppc target with the disable-tcg option. However, we're
> not quite sure on where to start with such a big patch.
>
>     * Should we focus first on changing the .c files, so that it
>       will build when we finally patch the meson.build everything
>       build correctly? Or should we start by changing the meson
>       file, so that everyone agrees that the excluded files can
>       indeed be excluded?

It's usually best to focus on logical changes, rather than
file-by-file.  That said, I'd probably suggest changing the .c files
first, then changing the meson file.

>     * Should we first finish the whole series to then mail it, or
>       should we send one file at a time, as soon as we have them
>       ready?

I'd lean towards building a whole series, since I think the individual
file changes won't make a lot of sense on their own.  That said, it's
ok and encouraged to post a relatively early draft of the series as an
RFC, so that the overall idea can be reviewed, even if it has obvious
omissions (just mention them in the cover letter).

> So far we're thinking we'll need to:
>     * change 6 files (arch_dump.c, machine.c, mmu-hash32.c,
>       mmu-hash64.c, mmu-radix64.c and meson.build);

I'd expect  mmu-*.c to be excluded.  Those are softmmu implementations
which shouldn't be used with KVM.  It's possible there are a few
helpers that will need to be extracted, though.

You'll probably also need changes in hw/ppc/spapr_hcall.c and maybe
some other parts of the spapr code: there are a number of hypercalls
that we implement in qemu for TCG, but which are (and must be)
implemented in KVM when KVM is in use.  So, I expect you'll need to
suppress compilation of h_enter, h_remove, h_protect, h_read and
h_bulk_remove at least in the !TCG case.

>     * exclude 8 files from the build (dfp_helper.c, excp_helper.c,
>       fpu_helper.c, int_helper.c, mem_helper.c, misc_helper.c, *
>       translate.c, timebase_helper.c);

That looks about right.

>     * create a new one that holds stubs.

Sounds reasonable.

> Does this sound about right? This is mostly guesswork based on how
> x86, s390x and ARM are doing it.

Sounds fine so far.

--
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: 11503 bytes --]

RE: Discussion: Patch series that adds disable-tcg option for ppc targets

[Fabiano Rosas]

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

>> You'll probably also need changes in hw/ppc/spapr_hcall.c and maybe
>> some other parts of the spapr code: there are a number of hypercalls
>> that we implement in qemu for TCG, but which are (and must be)
>> implemented in KVM when KVM is in use.  So, I expect you'll need to
>> suppress compilation of h_enter, h_remove, h_protect, h_read and
>> h_bulk_remove at least in the !TCG case.
>
> Yeah. there are 6 files dealing with spapr that are having linking
> problems, though some might be because of the cpu_list problem... But
> I'm a bit confused on how to interface with KVM, do I use an ioctl
> explicitly, or can it handle it implicitly? wenever I google or ask on
> IRC for KVM stuff the answer is usually "KVM handles it" so I never
> know if/when to call it

Maybe a general overview would help:

KVM runs in an actual ppc machine that can execute the Power instruction
set and provide the hardware facilities the guest expects from a ppc
hypervisor. So when using KVM, QEMU only needs to provide things that
are out of the scope of KVM or that for whatever reason KVM cannot
provide.

When running a ppc guest on top of a machine that's from another
architecture or that has KVM disabled, QEMU will need to do all the
heavy-lifting and translate the guest code to the target architecture
(TCG), emulate all of the hardware facilties and so on.

The point with disabling TCG is to make sure we identify which parts of
the code are needed when running natively (KVM) and have a way to build
only those, without the parts that are only needed when running on
another arch (TCG).

So the interfacing with KVM is something that is already in place. If
you grep for ioctl in accel/kvm/kvm-all.c and target/ppc/kvm.c you'll
see how QEMU interacts with KVM to get a guest to run and behave
properly.

About hypercalls (a kind of system call that the guest uses to talk to
its hypervisor) you'll see that spapr_hcall.c contains some hypercall
implementations:

 - The ones used when running TCG (i.e. QEMU pretending we're in an
actual ppc machine), such as h_enter, h_remove, etc;

 - The ones used when running KVM (i.e. KVM says it cannot handle this
hcall and then QEMU has to do it).

If you cross-reference the hcalls from spapr_hcall.c with the
implementations in the kernel at arch/powerpc/kvm/ you'll start to see
which hypercalls are handled by QEMU because we're replacing KVM and
which are handled by QEMU because we're helping KVM.

Note that there are nuances to what I have said, so if you encounter
something that differs we can discuss further.

Hope it helps. =)

Re: Discussion: Patch series that adds disable-tcg option for ppc targets

[David Gibson]

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

On Tue, Apr 06, 2021 at 06:38:21PM +0000, Bruno Piazera Larsen wrote:
> > t's usually best to focus on logical changes, rather than
> > file-by-file.  That said, I'd probably suggest changing the .c files
> > first, then changing the meson file.
> 
> OK, will do!
> 
> > I'd lean towards building a whole series, since I think the individual
> > file changes won't make a lot of sense on their own.  That said, it's
> > ok and encouraged to post a relatively early draft of the series as an
> > RFC, so that the overall idea can be reviewed, even if it has obvious
> > omissions (just mention them in the cover letter).
> 
> Good to know. I'm guessing that the RFC should at least compile,
> though, right? That's mostly what is holding me from sharing what we
> have so far.

Yes, compiling would definitely be preferable.  If you're able to
reduce scope of your initial draft to accomplish that, then I'd
recommend it, so that you can get something out for initial review
sooner.

> 
> > I'd expect  mmu-*.c to be excluded.  Those are softmmu implementations
> > which shouldn't be used with KVM.  It's possible there are a few
> > helpers that will need to be extracted, though.
> 
> weirdly enough, removing the mmu-* don't give compilation errors,
> but linker errors.

That's not surprising - the prototypes will be in a header somewhere,
so the compiler will be happy.  It's only at link time it will
discover it doesn't have the helpers that come from those files.

> There are a few helpers that we'll have to see
> how to deal with, but something that I found odd is that cpu_list
> and all it's related functions are defined in translate_init.c.inc,
> instead of cpu.c like most other targets. I'm thinking about moving
> those to the cpu.c and follow the general standard of the rest

That sounds like a good idea (note I haven't looked in any depth to
see if there are some gotchas with that).

> > You'll probably also need changes in hw/ppc/spapr_hcall.c and maybe
> > some other parts of the spapr code: there are a number of hypercalls
> > that we implement in qemu for TCG, but which are (and must be)
> > implemented in KVM when KVM is in use.  So, I expect you'll need to
> > suppress compilation of h_enter, h_remove, h_protect, h_read and
> > h_bulk_remove at least in the !TCG case.
> 
> Yeah. there are 6 files dealing with spapr that are having linking
> problems, though some might be because of the cpu_list
> problem... But I'm a bit confused on how to interface with KVM, do I
> use an ioctl explicitly, or can it handle it implicitly? wenever I
> google or ask on IRC for KVM stuff the answer is usually "KVM
> handles it" so I never know if/when to call it

I'm afraid this will mostly need to be figured out on a case by case
basis, with some knowledge of the PAPR interfaces.

One general guideline I can give you: look for the (mostly indirect)
calls to kvmppc_enable_hcall().  That call instructs KVM to handle the
designated hypercall, so it's a hint that you may not need the qemu
implementation of it in the KVM case.  However, there are still some
gotchas - in some cases the hypercall must be implemented by KVM for a
KVM guest or it won't work (e.g. H_ENTER), in other cases the KVM
implementation is just an optimization or fast path and the qemu
implementation is needed as well (e.g. H_PUT_TCE).

The other thing to do is to actually look at the KVM code which
dispatches KVM handled hypercalls, specifically
kvmppc_pseries_do_hcall() in arch/powerpc/kvm/book3s_hv.c and
hcall_real_table in arch/powerpc/kvm/book3s_hv_rmhandlers.S.

At the moment, the TCG-only and always-needed hcall implementations in
qemu aren't split from each other.  Doing so would probably be a good
idea, and I'd be happy to add a new TCG-only .c file for it.

Feel free to ask about specific hypercalls, and I'll try to help out.

-- 
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: Discussion: Patch series that adds disable-tcg option for ppc targets

[Thomas Huth]

On 06/04/2021 08.11, David Gibson wrote:
> On Mon, Apr 05, 2021 at 01:32:18PM +0000, Bruno Piazera Larsen wrote:
[...]
>>      * exclude 8 files from the build (dfp_helper.c, excp_helper.c,
>>        fpu_helper.c, int_helper.c, mem_helper.c, misc_helper.c, *
>>        translate.c, timebase_helper.c);
> 
> That looks about right.

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.

  Thomas

Re: Discussion: Patch series that adds disable-tcg option for ppc targets

[David Gibson]

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

On Mon, Apr 05, 2021 at 01:32:18PM +0000, Bruno Piazera Larsen wrote:
> Hi all,
> 
> The team I'm working on started to work on adding support for
> building the ppc target with the disable-tcg option. However, we're
> not quite sure on where to start with such a big patch.
> 
>     * Should we focus first on changing the .c files, so that it
>       will build when we finally patch the meson.build everything
>       build correctly? Or should we start by changing the meson
>       file, so that everyone agrees that the excluded files can
>       indeed be excluded?

It's usually best to focus on logical changes, rather than
file-by-file.  That said, I'd probably suggest changing the .c files
first, then changing the meson file.

>     * Should we first finish the whole series to then mail it, or
>       should we send one file at a time, as soon as we have them
>       ready?

I'd lean towards building a whole series, since I think the individual
file changes won't make a lot of sense on their own.  That said, it's
ok and encouraged to post a relatively early draft of the series as an
RFC, so that the overall idea can be reviewed, even if it has obvious
omissions (just mention them in the cover letter).

> So far we're thinking we'll need to:
>     * change 6 files (arch_dump.c, machine.c, mmu-hash32.c,
>       mmu-hash64.c, mmu-radix64.c and meson.build);

I'd expect  mmu-*.c to be excluded.  Those are softmmu implementations
which shouldn't be used with KVM.  It's possible there are a few
helpers that will need to be extracted, though.

You'll probably also need changes in hw/ppc/spapr_hcall.c and maybe
some other parts of the spapr code: there are a number of hypercalls
that we implement in qemu for TCG, but which are (and must be)
implemented in KVM when KVM is in use.  So, I expect you'll need to
suppress compilation of h_enter, h_remove, h_protect, h_read and
h_bulk_remove at least in the !TCG case.

>     * exclude 8 files from the build (dfp_helper.c, excp_helper.c,
>       fpu_helper.c, int_helper.c, mem_helper.c, misc_helper.c, *
>       translate.c, timebase_helper.c);

That looks about right.

>     * create a new one that holds stubs.

Sounds reasonable.

> Does this sound about right? This is mostly guesswork based on how
> x86, s390x and ARM are doing it.

Sounds fine so far.

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

Discussion: Patch series that adds disable-tcg option for ppc targets

[Bruno Piazera Larsen]

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

Hi all,

The team I'm working on started to work on adding support for building the ppc target with the disable-tcg option. However, we're not quite sure on where to start with such a big patch.

    * Should we focus first on changing the .c files, so that it will build when we finally patch the meson.build everything build correctly? Or should we start by changing the meson file, so that everyone agrees that the excluded files can indeed be excluded?
    * Should we first finish the whole series to then mail it, or should we send one file at a time, as soon as we have them ready?

So far we're thinking we'll need to:
    * change 6 files (arch_dump.c, machine.c, mmu-hash32.c, mmu-hash64.c, mmu-radix64.c and meson.build);
    * exclude 8 files from the build (dfp_helper.c, excp_helper.c, fpu_helper.c, int_helper.c, mem_helper.c, misc_helper.c, translate.c, timebase_helper.c);
    * create a new one that holds stubs.
Does this sound about right? This is mostly guesswork based on how x86, s390x and ARM are doing it.

Thank in advance


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: 4850 bytes --]