On Mon, Mar 28, 2022 at 5:30 PM Jukka Laitinen <jukka.laiti...@iki.fi> wrote:
> Another example: > > https://github.com/apache/incubator-nuttx/pull/5731 > > A PR which looks like 0 gain, 100 risk of breaking everything (breaks > our stuff at least), This patch can improve the context switch performance, since it could avoid the follow memory copy in every context switch: 1. 32 integer register(128B on 32bit, 256B on 64bit) 2. 32 float register(128B single precision, 256B double precision) 3. Two status register And at the same time, each tcb can also reduce the same amount of memory. So, could you give more info why this change is zero gain? > and can only work in FLAT_BUILD mode (if even in > that?). > This patch should work fine for both flat and protected builds, both Xiaomi and Sony test on several boards. > > How does stuff like this get in? Xiaomi does and approves by themselves? > Sorry for being blunt, but this is really irritating. > > This patch is developed by Xiaomi, but reviewed, tested and merged by Sony. Normally, big changes like this will be reviewed by different groups and will be held at least several days before merging. Anyone could give the feedback on github and all comments must be addressed before merging. If you look at this patch closely: 1. This patch was created 15 days ago 2. And has more than 20 comments > -Jukka > > > On 25.3.2022 21.47, Xiang Xiao wrote: > > On Fri, Mar 25, 2022 at 6:53 PM Jukka Laitinen <jukka.laiti...@iki.fi> > > wrote: > > > >> Hi, > >> > >> I was trying to make a more general statement than starting discussion > >> on separate PRs, but let me shortly answer still > >> > >> On 25.3.2022 10.32, Xiang Xiao wrote: > >>> On Fri, Mar 25, 2022 at 3:35 PM Jukka Laitinen <jukka.laiti...@iki.fi> > >>> wrote: > >>> > >>>> Hi, > >>>> > >>>> > >>>> As an another example, we would very much like to bring in > >>>> CONFIG_BUILD_KERNEL support for RISC-V for NuttX, as we have worked > hard > >>>> on this for some time, and have it working. Now, even when this work > it > >>>> is only additions, not breaking anything for FLAT_BUILD users, > >>> Yes, it doesn't break FLAT_BUILD, but doesn't mean KERNEL design or > code > >> is > >>> perfect, that's why the interesting people can give the comment. > >> But of course! > >>>> is stuck > >>>> in endless debate where half of the comments show that the reviewer > >>>> doesn't know the RISC-V ISA thoroughly, > >>>> > >>>> > >>>> Sorry, I give you this impression, but I have worked on RiSCV since > >> 2018:(. > >> > >> I wasn't referring to you with the above comment... and I am sorry about > >> this. I'll try to avoid anything that can be interpreted as comments to > >> person, in the future. > >> > >>>> and is referring to how things > >>>> are done in ARM world. And the other half is largely useless style > >>>> nitpicking. > >>>> > >>> I think the most debate is about how S-mode talks with M-mode in this > PR. > >>> The module design and standard compliant is always NuttX core value and > >>> highlight in: > >>> https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md > >>> Since OpenSBI is adopted by many OS to interact with M-mode firmware: > >>> https://github.com/riscv-software-src/opensbi > >>> It's always good to follow the best practice and what I insist in this > >> PR: > >>> we can implement a minimal subset of OpenSBI to support S-mode NuttX, > >> We (TII, it's research partners and subcontractors) will not be using > >> OpenSBI for interfacing between S-mode and M-mode inside NuttX, since it > >> doesn't make any sense for us. For running NuttX in kernel mode, we will > >> not "implement a subset of OpenSBI". We (Ville) have implemented the > >> "native SBI for NuttX". OpenSBI is not a standard, it is just a library > >> maintained by a few people. It is much better to just do what is needed > >> to do what is good for NuttX, following the standard RISC-V ISA spec, > >> and not blindly go with a 3rd party bloated library. > >> > > Ok, if you still prefer native SBI, please at least add a choice option > in > > Kconfig, > > so other people can support OpenSBI later. > > > > > >>> but > >>> don't invent a private interface since M-mode firmware mayn't run NuttX > >> at > >>> all. > >> Now this is fair request, and I don't see any problems in implementing > >> this also for the mpfs in the future. Currently that PR (IMHO) doesn't > >> prevent having another M-mode firmware+any custom SBI such as OpenSBI on > >> other platforms. Currently there are also parts under mpfs board, which > >> can be generalized later on to avoid code copy paste, if others want to > >> use our implementation on other risc-v archs. Right now there is no > >> copy-paste code added, since this is not used in any other risc-v archs. > >> > >> I see bringing in CONFIG_BUILD_KERNEL as a very important step, and I > >> see it a huge step forward if we can get in even one new architecture in > >> supporting that. But putting too much pressure on making world perfect > >> for the whole risc-v/nuttx community at once is just too much. > >> Incremental steps forward is much better way... First make it work on > >> one board, then generalize functionality when taking it into use in > >> another and so on... > >> > > Sure, but since it's a bigger change and the implementation is unique in > > many places, it's expected that the review process will be a bit longer. > > > > > >> Anyhow, let's try to keep the discussion regarding a single PR within > >> that PR in github. And I am really sorry if you felt that I was accusing > >> you of something. it was not my intention. I truly appreciate that you > >> are putting so much time and effort for reviewing patches to NuttX. > >> > >> Thanks, > >> > >> Jukka > >> > >> > >> >
