Re: [RFC PATCH 2/4] hw/ppc: Add kvm-only file spapr_hcall_tcg_stub.c

[David Gibson <david@gibson.dropbear.id.au>]

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

On Fri, Apr 23, 2021 at 07:06:15PM -0300, Fabiano Rosas wrote:
> "Lucas Mateus Castro (alqotel)" <lucas.araujo@eldorado.org.br> writes:
> 
> > This file should be used instead of spapr_hcall.c when compiling
> > without tcg (--disable-tcg) as it does not call tcg-only functions and
> > trips fatal error when invalid functions are called
> 
> Not calling any TCG-specific function is not an indication of the code
> being "kvm only" in this case. So I think this patch is backwards, we
> should instead aim to remove tcg-only code from spapr_hcall.c.

Right.

> > As of right now some functions are repeated here and in spapr_hcall.c,
> > as they are static, is some other method to deal with this
> > recommended?
> 
> Yeah, you should not be repeating the functions. From previous
> discussions on this topic I understood that we'd have another
> hypercall_register_types for TCG. So we could have a spapr_hcall_tcg.c
> that contains tcg-only functions. And they would only be used in that
> file so they would continue being static.

> > Also some functions should only cause a fatal error as KVM should
> > intercept and handle their call, but as I'm not sure which ones I just
> > did this to functions that called tcg-only code.
> >
> > Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br>
> > ---
> >  hw/ppc/spapr_hcall_tcg_stub.c | 1824 +++++++++++++++++++++++++++++++++
> >  1 file changed, 1824 insertions(+)
> >  create mode 100644 hw/ppc/spapr_hcall_tcg_stub.c
> >
> > diff --git a/hw/ppc/spapr_hcall_tcg_stub.c
> > b/hw/ppc/spapr_hcall_tcg_stub.c
> 
> Your usage of stub here is a bit confusing. Take a look at
> target/ppc/kvm-stub.c and accel/stubs/kvm-stub.c. These are files that
> are only included in the build to satisfy any references to the symbols
> they contain. The implementation is just an empty body or an error
> return. So if the feature is included, the actual foo.c will be present
> with the proper implementation; if not, we get the empty stub.
> 
> Also, look at target/ppc/kvm.h under #ifndef CONFIG_USER_ONLY. There's
> some similar ideas there that could be of help.
> 
> So my suggestion for this patch is take a step back and move first all
> of the TCG-only functions that are certainly not needed. We can then
> figure out what patterns we are going to use to stub them in the
> KVM-only build. After that we take a look at what's left and go from
> there.

Right.  You should be able to stub these much more simply than this.
Just a single say "h_tcg_only()" stub, then in the !TCG case you
register that instead of the real implementation with
spapr_register_hypercall().

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