Re: [PATCH 1/5] target/arm: only build psci for TCG

[Alexander Graf]

On 20.12.22 14:53, Fabiano Rosas wrote:
> Alexander Graf <agraf@csgraf.de> writes:
>
> > Hey Fabiano,
> >
> > On 19.12.22 12:42, Fabiano Rosas wrote:
> > > Claudio Fontana <cfontana@suse.de> writes:
> > >
> > > > Ciao Alex,
> > > >
> > > > On 12/19/22 11:47, Alexander Graf wrote:
> > > > > Hey Claudio,
> > > > >
> > > > > On 19.12.22 09:37, Claudio Fontana wrote:
> > > > > > On 12/16/22 22:59, Alexander Graf wrote:
> > > > > > > Hi Claudio,
> > > > > > >
> > > > > > > If the PSCI implementation becomes TCG only, can we also move to a tcg accel 
> > > > > > > directory? It slowly gets super confusing to keep track of which files are 
> > > > > > > supposed to be generic target code and which ones TCG specific>
> > > > > > > Alex
> > > > > > Hi Alex, Fabiano, Peter and all,
> > > > > >
> > > > > > that was the plan but at the time of:
> > > > > >
> > > > > > https://lore.kernel.org/all/20210416162824.25131-1-cfontana@suse.de/
> > > > > >
> > > > > > Peter mentioned that HVF AArch64 might use that code too:
> > > > > >
> > > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg00509.html
> > > > > >
> > > > > > so from v2 to v3 the series changed to not move the code under tcg/ , expecting HVF to be 
> > > > > > reusing that code "soon".
> > > > > >
> > > > > > I see that your hvf code in hvf/ implements psci, is there some plan to reuse 
> > > > > > pieces from the tcg implementation now?
> > > > > I originally reused the PSCI code in earlier versions of my hvf patch
> > > > > set, but then we realized that some functions like remote CPU reset are
> > > > > wired in a TCG specific view of the world with full target CPU register
> > > > > ownership. So if we want to actually share it, we'll need to abstract it
> > > > > up a level.
> > > > >
> > > > > Hence I'd suggest to move it to a TCG directory for now and then later
> > > > > move it back into a generic helper if we want / need to. The code just
> > > > > simply isn't generic yet.
> > > > >
> > > > > Or alternatively, you create a patch (set) to actually merge the 2
> > > > > implementations into a generic one again which then can live at a
> > > > > generic place :)
> > > > >
> > > > >
> > > > > Alex
> > > > Thanks for the clarification, I'll leave the choice up to Fabiano now, since he 
> > > > is working on the series currently :-)
> > > >
> > > > Ciao,
> > > >
> > > > Claudio
> > > Hello, thank you all for the comments.
> > >
> > > I like the idea of merging the two implementations. However, I won't get
> > > to it anytime soon. There's still ~70 patches in the original series
> > > that I need to understand, rebase and test, including the introduction
> > > of the tcg directory.
> >
> > Sure, I am definitely fine with leaving them separate for now as well :).
> >
> >
> > > I'd say we merge this as is now, since this patch has no
> > > dependencies. Later when I introduce the tcg directory I can move the
> > > code there along with the other tcg-only files. I'll take note to come
> > > back to the PSCI code as well.
> > I'm confused about the patch ordering :). Why is it easier to move the
> > psci.c compilation target from generic to an if(CONFIG_TCG) only to
> > later move it into a tcg/ directory?
> It's a simple patch, so the overhead didn't cross my mind. But you are
> right, this could go directly into tcg/ without having to put it under
> CONFIG_TCG first.


I'm sure more like this will follow, and it will be a lot easier on 
everyone if the pattern is going to be "move tcg specific code to tcg/ 
and leave generic code in the main directory".


> > Wouldn't it be easier to create a
> > tcg/ directory from the start and just put it there?
> I don't know about "from the start". At this point we cannot have a
> single commit moving everything into the tcg/ directory because some
> files still contain tcg + non-tcg code.


Yes, the only thing the initial commit at the start would do is create 
the directory and ninja config, pretty much nothing else. All follow-on 
commits then split the currently entangled code into tcg/ once code is 
clearly separate :).


I believe this was also the approach Claudio took in his patch set last 
year, and I find it very reasonable. It allows you to stop at any point 
mid-way.


> We would end up with several
> commits moving files into tcg/ along the history, which I think results
> in a poor experience when inspecting the log later on (git blame and so
> on). So my idea was to split as much as I can upfront and only later
> move everything into the directory.


Quite the opposite: Please make sure to move everything slowly at a 
digestible pace. There is no way you will get 100 patches in at once. 
Make sure you can cut off at any point in between.

What you basically want is to move from "target/arm is tcg+generic code" 
to "target/arm is generic, target/arm/tcg is tcg code". You will be in a 
transitional phase along the way whatever you do, so just make it 
"target/arm is tcg+generic code, target/arm/tcg is tcg code" while 
things are in flight and have a final commit that indicates the 
conversion is done.


> I'm also rebasing this series [1] from 2021, which means I'd rather have
> small chunks of code moved under CONFIG_TCG that I can build-test with
> --disable-tcg (even though the build doesn't finish, I can see the
> number of errors going down), than to move non-tcg code into tcg/ and
> then pull it out later like in the original series.


I think we're saying the same thing. Please don't move non-tcg code into 
tcg/. Just move files that are absolutely clearly TCG into tcg/ right 
from the start. The psci.c is a good example for that. translate*.c and 
op-helper.c would be another.


Alex


> 1- https://lore.kernel.org/r/20210416162824.25131-1-cfontana@suse.de
>
> But hey, that's just my reasoning, no strong feelings about it.