Re: target/ppc: sPAPR invalid function calls when compiling without TCG

Re: target/ppc: sPAPR invalid function calls when compiling without TCG

[Lucas Mateus Martins Araujo e Castro]

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




________________________________

>> spapr_hcall.c:
>>     function h_enter call ppc_hash64_hpte_page_shift_noslb, ppc_hash64_map_hptes and ppc_hash64_unmap_hptes
>>     function remove_hpte call ppc_hash64_map_hptes, ppc_hash64_unmap_hptes and ppc_hash64_tlb_flush_hpte
>>     function h_protect call ppc_hash64_map_hptes, ppc_hash64_unmap_hptes and ppc_hash64_tlb_flush_hpte
>>     function h_read call ppc_hash64_map_hptes and ppc_hash64_unmap_hptes
>>     function rehash_hpte call ppc_hash64_hpte_page_shift_noslb
>>     function rehash_hpt call ppc_hash64_map_hptes and ppc_hash64_unmap_hptes
>
>h_enter, remove_hpte, h_protect, h_read and rehash_hpte should never
>be called for a KVM guest (KVM will intercept the relevant hcalls
>before they reach qemu).  I think it would make sense to split of a
>TCG only portion of spapr_hcall.c into a new file.  You'll then
>probably need !TCG stubs for those hypercalls, which should trip a
>fatal error (if KVM hasn't handled the calls, there's nothing we can
>do that will salvage the situation).

Besides the aforementioned functions, will all spapr hypercalls beginning with H_* be intercepted
(and therefore, should trip a fatal error in the KVM-only file) or only a subset of these functions will
be intercepted? If the latter is there an interval of opcodes that'll be intercepted?

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

Re: target/ppc: sPAPR invalid function calls when compiling without TCG

[David Gibson]

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

On Tue, Apr 20, 2021 at 06:27:38PM +0000, Lucas Mateus Martins Araujo e Castro wrote:
> 
> 
> 
> ________________________________
> 
> >> spapr_hcall.c:
> >>     function h_enter call ppc_hash64_hpte_page_shift_noslb, ppc_hash64_map_hptes and ppc_hash64_unmap_hptes
> >>     function remove_hpte call ppc_hash64_map_hptes, ppc_hash64_unmap_hptes and ppc_hash64_tlb_flush_hpte
> >>     function h_protect call ppc_hash64_map_hptes, ppc_hash64_unmap_hptes and ppc_hash64_tlb_flush_hpte
> >>     function h_read call ppc_hash64_map_hptes and ppc_hash64_unmap_hptes
> >>     function rehash_hpte call ppc_hash64_hpte_page_shift_noslb
> >>     function rehash_hpt call ppc_hash64_map_hptes and ppc_hash64_unmap_hptes
> >
> >h_enter, remove_hpte, h_protect, h_read and rehash_hpte should never
> >be called for a KVM guest (KVM will intercept the relevant hcalls
> >before they reach qemu).  I think it would make sense to split of a
> >TCG only portion of spapr_hcall.c into a new file.  You'll then
> >probably need !TCG stubs for those hypercalls, which should trip a
> >fatal error (if KVM hasn't handled the calls, there's nothing we can
> >do that will salvage the situation).
> 
> Besides the aforementioned functions, will all spapr hypercalls
> beginning with H_* be intercepted (and therefore, should trip a
> fatal error in the KVM-only file) or only a subset of these
> functions will be intercepted?

No, only a few hcalls will be KVM intercepted.  There are basically
two categories:
  1) hcalls which *must* be implemented in KVM, because qemu doesn't
     have access to the KVM data structures to implement them.  I
     think this is pretty much just the MMU related hypercalls.
  2) performance critical hcalls where we have a KVM accelerated
     implementation, but *could* be implemented in qemu (and in a few
     cases the kernel fast path will fall back to the qemu slow path
     sometimes)

> If the latter is there an interval of
> opcodes that'll be intercepted?

No.  The opcode numbers aren't defined by KVM or qemu, they're from
from PAPR/PowerVM and have no bearing on how concerns are divided in
the KVM/qemu world.  I'd start by only separating hypercall
implementations which are actively causing you problems (calling into
TCG code is a strong hint that its in category (1)).

-- 
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: target/ppc: sPAPR invalid function calls when compiling without TCG

[Lucas Mateus Martins Araujo e Castro]

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



>> spapr_hcall.c:
>>     function h_enter call ppc_hash64_hpte_page_shift_noslb, ppc_hash64_map_hptes and ppc_hash64_unmap_hptes
>>     function remove_hpte call ppc_hash64_map_hptes, ppc_hash64_unmap_hptes and ppc_hash64_tlb_flush_hpte
>>     function h_protect call ppc_hash64_map_hptes, ppc_hash64_unmap_hptes and ppc_hash64_tlb_flush_hpte
>>     function h_read call ppc_hash64_map_hptes and ppc_hash64_unmap_hptes
>>     function rehash_hpte call ppc_hash64_hpte_page_shift_noslb
>>     function rehash_hpt call ppc_hash64_map_hptes and ppc_hash64_unmap_hptes

>h_enter, remove_hpte, h_protect, h_read and rehash_hpte should never
>be called for a KVM guest (KVM will intercept the relevant hcalls
>before they reach qemu).  I think it would make sense to split of a
>TCG only portion of spapr_hcall.c into a new file.  You'll then
>probably need !TCG stubs for those hypercalls, which should trip a
>fatal error (if KVM hasn't handled the calls, there's nothing we can
>do that will salvage the situation).

These functions are static, so I thought about turning them into non-static functions and creating the prototype in
internal.h and changing hw/ppc/meson.build to add the correct .c file but hw/ppc/ doesn't have an internal.h

What is the best way to handle this static functions?


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

Re: target/ppc: sPAPR invalid function calls when compiling without TCG

[David Gibson]

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

On Mon, Apr 19, 2021 at 07:04:34PM +0000, Lucas Mateus Martins Araujo e Castro wrote:
> 
> 
> >> spapr_hcall.c:
> >>     function h_enter call ppc_hash64_hpte_page_shift_noslb, ppc_hash64_map_hptes and ppc_hash64_unmap_hptes
> >>     function remove_hpte call ppc_hash64_map_hptes, ppc_hash64_unmap_hptes and ppc_hash64_tlb_flush_hpte
> >>     function h_protect call ppc_hash64_map_hptes, ppc_hash64_unmap_hptes and ppc_hash64_tlb_flush_hpte
> >>     function h_read call ppc_hash64_map_hptes and ppc_hash64_unmap_hptes
> >>     function rehash_hpte call ppc_hash64_hpte_page_shift_noslb
> >>     function rehash_hpt call ppc_hash64_map_hptes and ppc_hash64_unmap_hptes
> 
> >h_enter, remove_hpte, h_protect, h_read and rehash_hpte should never
> >be called for a KVM guest (KVM will intercept the relevant hcalls
> >before they reach qemu).  I think it would make sense to split of a
> >TCG only portion of spapr_hcall.c into a new file.  You'll then
> >probably need !TCG stubs for those hypercalls, which should trip a
> >fatal error (if KVM hasn't handled the calls, there's nothing we can
> >do that will salvage the situation).
> 
> These functions are static, so I thought about turning them into non-static functions and creating the prototype in
> internal.h and changing hw/ppc/meson.build to add the correct .c file but hw/ppc/ doesn't have an internal.h
> 
> What is the best way to handle this static functions?

They're static because their only direct use is passing to
spapr_register_hypercall() in hypercall_register_types().  You can put
your own register types function in the new KVM-only file, with those
calls, so everything will stay static.

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

target/ppc: sPAPR invalid function calls when compiling without TCG

[Lucas Mateus Martins Araujo e Castro]

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

Hi, I have been working on billionai's patch to enable the --disable-tcg option on PowerPC and one of the problems is that 5 files in hw/ppc use functions implemented in mmu-hash64.c which is not compiled with --disable-tcg, I'd like to know how to correct the spapr function call, should I

  *   change the calls to generic functions that should call the correct function based on if kvm is being used or not,
  *   should I just implement said functions independently of mmu-hash64.c and always call them,
  *   should I just implement said functions independently of mmu-hash64.c and only call it with --disable-tcg option turned on,
  *   find a way to not have said calls when compiling without TCG as they're not necessary,
  *   just create a stub

Here's when each function is called by each .c:
spapr.c:
    function do_lpcr_sync call ppc_store_lpcr

spapr_hcall.c:
    function h_enter call ppc_hash64_hpte_page_shift_noslb, ppc_hash64_map_hptes and ppc_hash64_unmap_hptes
    function remove_hpte call ppc_hash64_map_hptes, ppc_hash64_unmap_hptes and ppc_hash64_tlb_flush_hpte
    function h_protect call ppc_hash64_map_hptes, ppc_hash64_unmap_hptes and ppc_hash64_tlb_flush_hpte
    function h_read call ppc_hash64_map_hptes and ppc_hash64_unmap_hptes
    function rehash_hpte call ppc_hash64_hpte_page_shift_noslb
    function rehash_hpt call ppc_hash64_map_hptes and ppc_hash64_unmap_hptes

spapr_rtas.c:
    function rtas_start_cpu call ppc_store_lpcr
    function rtas_stop_self call ppc_store_lpcr

spapr_spapr_cpu_core.c
    function spapr_reset_vcpu call ppc_store_lpcr
    function spapr_cpu_set_entry_state call ppc_store_lpcr

spapr_caps.c:
    function cap_large_decr_cpu_apply call ppc_store_lpcr
    function cap_hpt_maxpagesize_cpu_apply call ppc_hash64_filter_pagesizes



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

Re: target/ppc: sPAPR invalid function calls when compiling without TCG

[David Gibson]

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

On Wed, Apr 14, 2021 at 06:28:26PM +0000, Lucas Mateus Martins Araujo e Castro wrote:
> Hi, I have been working on billionai's patch to enable the --disable-tcg option on PowerPC and one of the problems is that 5 files in hw/ppc use functions implemented in mmu-hash64.c which is not compiled with --disable-tcg, I'd like to know how to correct the spapr function call, should I
> 
>   *   change the calls to generic functions that should call the correct function based on if kvm is being used or not,
>   *   should I just implement said functions independently of mmu-hash64.c and always call them,
>   *   should I just implement said functions independently of mmu-hash64.c and only call it with --disable-tcg option turned on,
>   *   find a way to not have said calls when compiling without TCG as they're not necessary,
>   *   just create a stub
> 
> Here's when each function is called by each .c:
> spapr.c:
>     function do_lpcr_sync call ppc_store_lpcr

I think ppc_store_lpcr() needs to be moved so that it's compiled with
KVM as well, but #ifdef out the hreg_compute_hflags() line in that case.

> 
> spapr_hcall.c:
>     function h_enter call ppc_hash64_hpte_page_shift_noslb, ppc_hash64_map_hptes and ppc_hash64_unmap_hptes
>     function remove_hpte call ppc_hash64_map_hptes, ppc_hash64_unmap_hptes and ppc_hash64_tlb_flush_hpte
>     function h_protect call ppc_hash64_map_hptes, ppc_hash64_unmap_hptes and ppc_hash64_tlb_flush_hpte
>     function h_read call ppc_hash64_map_hptes and ppc_hash64_unmap_hptes
>     function rehash_hpte call ppc_hash64_hpte_page_shift_noslb
>     function rehash_hpt call ppc_hash64_map_hptes and ppc_hash64_unmap_hptes

h_enter, remove_hpte, h_protect, h_read and rehash_hpte should never
be called for a KVM guest (KVM will intercept the relevant hcalls
before they reach qemu).  I think it would make sense to split of a
TCG only portion of spapr_hcall.c into a new file.  You'll then
probably need !TCG stubs for those hypercalls, which should trip a
fatal error (if KVM hasn't handled the calls, there's nothing we can
do that will salvage the situation).

> spapr_rtas.c:
>     function rtas_start_cpu call ppc_store_lpcr
>     function rtas_stop_self call ppc_store_lpcr

As above.

> spapr_spapr_cpu_core.c
>     function spapr_reset_vcpu call ppc_store_lpcr
>     function spapr_cpu_set_entry_state call ppc_store_lpcr

As above.

> spapr_caps.c:
>     function cap_large_decr_cpu_apply call ppc_store_lpcr

As above.

>     function cap_hpt_maxpagesize_cpu_apply call ppc_hash64_filter_pagesizes

Ok, you definitely need the filter_pagesizes logic, even with KVM, so
that will also have to be moved out of the TCG only file.

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