----- Am 22. Feb 2018 um 17:57 schrieb joel j...@rtems.org: > On Thu, Feb 22, 2018 at 5:42 AM, Sebastian Huber < > sebastian.hu...@embedded-brains.de> wrote: > >> ----- Am 22. Feb 2018 um 6:06 schrieb Chris Johns chr...@rtems.org: >> >> > On 22/02/2018 13:37, Sebastian Huber wrote: >> >>> >> >>> Architecture-specific names should use an ARCH_ or _Arch prefix and >> not CPU_ARCH >> >>> or _CPU_Arch. >> >>> >> >>> This >> >>> >> >>> CPU_DISABLE_INLINE_ISR_DISABLE_ENABLE >> >>> >> >>> is an architecture-specific implementation detail which doesn't >> propagate to >> >>> generic files, e.g. rtems/score/isrlevel.h, so it should not be >> introduced from >> >>> my point of view. >> >> OK. I can add an architecture to the constant but my hope was that > isrlevel.h would eventually be where the cut off to the paravirtualized > ISR disable/enable methods would be made.
This RTEMS_PARAVIRT is a remnant of our failed attempt to upstream the XtratuM support for SPARC. For XtratuM on ARM, this didn't work, since here we needed different tweaks. I think that this paravirtualization is difficult to generalize. How many hypervisors do we support? Two? With a special header file we are less flexible and I think it will only complicate things. In the next hypervisor you will have some stuff in paravirt.h and then some more stuff somewhere else. > > >> >>> I don't think it is worth to add a rtems/score/paravirt.h for each >> architecture. >> >>> The changes introduced by RTEMS_PARAVIRT are too small to justify >> this. I am >> >>> also not sure if you can encapsulate this in one header in all cases. >> >> >> >> Please don't ignore this. >> >> >> > >> > I felt spreading the RTEMS_PARAVIRT across the code was hiding the >> reason in >> > some cases. When I reviewed the v2 patches I felt changes in a specific >> area >> > needed more information to aid long term maintenance. For example look >> at the >> > ARM thread id register. It is clear what is happening and if that change >> flows >> > out to other parts of the system it is clear what is happening if there >> is a >> > dependence on that register. >> >> Yes, this is all right, but do we really need a special header file for >> this? We can do this in one area of cpu.h or cpuimpl.h. >> > > It might be able to go near the top of cpu.h. But the inclusion of > paravirt.h is already > wrapped in a conditional so it isn't impacting normal compiles. > > Given that this file exists to specifically map an RTEMS configuration onto > architectural tweaks and document them, I still think they should be in > their > own file. Putting it at the top of cpu.h is likely technically feasible but > doesn't > achieve the goal of having a clear place to document paravirtualization > tweaks, etc. I think that one block with comments for the RTEMS_PARAVIRT in cpu.h is quite clear. Why is it better to have two files open to figure out what is going on? > > >> >> One long term goal is to reduce the implementation details visible via >> <rtems.h> and move more and more stuff into cpuimpl.h. This paravirt.h is a >> step back under this point of view. >> > > It was actually hiding the reason for the conditional in all cases. > > Chris' suggestion was because none of this can be tested without special > setups which are not generally available. Plus it is completely undocumented > what the host restrictions and requirements were that led to the > conditionals > being put in place. > > This header file is primarily to document (and do a good job of it) the > paravirtualization changes. Your thread id response is a perfect example. It is a good example that the paravirtualization support is hard to generalize. > The need to compile with that option should have been documented. As > the code is today, the definition of that register is only implied by > a GCC architectural revision. The register is indeed present in our > paravirtualized environment but we can't set it because it can't > be set in user mode. You already solved this problem and could > have even provided the helper routine for paravirtualized environments. No, I don't think so. It is a workaround for a particular hypervisor implementation which has some particular shortcomings. With this workaround we violate the ABI. > > Scattering around a bunch of undocumented tweaks without a > way to document them is bad. Especially when only the person > who did the paravirtualization adapter for a specific host knows the > details. > > I agreed with Chris that in-code documentation was the answer > and providing a nice container for it was the answer. We could easily document this in the CPU architecture supplement. > > Having paravirt.h could have the configure process give you an > error if the architecture doesn't have it -- which implies it doesn't > support it. You will probably also notice that there is no BSP. _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel