| Message ID | 20221216212944.28229-2-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Some CONFIG_TCG code movement | expand |
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 > Am 16.12.2022 um 22:37 schrieb Fabiano Rosas <farosas@suse.de>: > > From: Claudio Fontana <cfontana@suse.de> > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > Cc: Alexander Graf <agraf@csgraf.de> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > Originally from: > [RFC v14 09/80] target/arm: only build psci for TCG > https://lore.kernel.org/r/20210416162824.25131-10-cfontana@suse.de > --- > target/arm/meson.build | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/target/arm/meson.build b/target/arm/meson.build > index 87e911b27f..26e425418f 100644 > --- a/target/arm/meson.build > +++ b/target/arm/meson.build > @@ -61,10 +61,13 @@ arm_softmmu_ss.add(files( > 'arm-powerctl.c', > 'machine.c', > 'monitor.c', > - 'psci.c', > 'ptw.c', > )) > > +arm_softmmu_ss.add(when: 'CONFIG_TCG', if_true: files( > + 'psci.c', > +)) > + > subdir('hvf') > > target_arch += {'arm': arm_ss} > -- > 2.35.3 >
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? Ciao, Claudio > >> Am 16.12.2022 um 22:37 schrieb Fabiano Rosas <farosas@suse.de>: >> >> From: Claudio Fontana <cfontana@suse.de> >> >> Signed-off-by: Claudio Fontana <cfontana@suse.de> >> Cc: Alexander Graf <agraf@csgraf.de> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >> Signed-off-by: Fabiano Rosas <farosas@suse.de> >> --- >> Originally from: >> [RFC v14 09/80] target/arm: only build psci for TCG >> https://lore.kernel.org/r/20210416162824.25131-10-cfontana@suse.de >> --- >> target/arm/meson.build | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/target/arm/meson.build b/target/arm/meson.build >> index 87e911b27f..26e425418f 100644 >> --- a/target/arm/meson.build >> +++ b/target/arm/meson.build >> @@ -61,10 +61,13 @@ arm_softmmu_ss.add(files( >> 'arm-powerctl.c', >> 'machine.c', >> 'monitor.c', >> - 'psci.c', >> 'ptw.c', >> )) >> >> +arm_softmmu_ss.add(when: 'CONFIG_TCG', if_true: files( >> + 'psci.c', >> +)) >> + >> subdir('hvf') >> >> target_arch += {'arm': arm_ss} >> -- >> 2.35.3 >>
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
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
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. 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.
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? Wouldn't it be easier to create a tcg/ directory from the start and just put it there? The current approach just looks like duplicate effort to me. Alex
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.
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.
Alexander Graf <agraf@csgraf.de> writes: >>> 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". Ok, so I'll drop this patch from this series and resend it along with the rest of the code movement to the tcg/ 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. I meant having code under CONFIG_TCG first and later moving to tcg/. So we separate moving the code from figuring out if it should be moved. There was no implication of speed, size or indigestibility =). > > 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. Yeah, I think we agree. Thanks for taking the time to spell it out.
diff --git a/target/arm/meson.build b/target/arm/meson.build index 87e911b27f..26e425418f 100644 --- a/target/arm/meson.build +++ b/target/arm/meson.build @@ -61,10 +61,13 @@ arm_softmmu_ss.add(files( 'arm-powerctl.c', 'machine.c', 'monitor.c', - 'psci.c', 'ptw.c', )) +arm_softmmu_ss.add(when: 'CONFIG_TCG', if_true: files( + 'psci.c', +)) + subdir('hvf') target_arch += {'arm': arm_ss}