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