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

[Fabiano Rosas]

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.

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

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.

1- https://lore.kernel.org/r/20210416162824.25131-1-cfontana@suse.de

But hey, that's just my reasoning, no strong feelings about it.