<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Feb 22, 2018 at 5:42 AM, Sebastian Huber <span dir="ltr"><<a href="mailto:sebastian.huber@embedded-brains.de" target="_blank">sebastian.huber@embedded-brains.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">----- Am 22. Feb 2018 um 6:06 schrieb Chris Johns <a href="mailto:chrisj@rtems.org">chrisj@rtems.org</a>:<br>
<br>
> On 22/02/2018 13:37, Sebastian Huber wrote:<br>
>>><br>
>>> Architecture-specific names should use an ARCH_ or _Arch prefix and not CPU_ARCH<br>
>>> or _CPU_Arch.<br>
>>><br>
>>> This<br>
>>><br>
>>> CPU_DISABLE_INLINE_ISR_<wbr>DISABLE_ENABLE<br>
>>><br>
>>> is an architecture-specific implementation detail which doesn't propagate to<br>
>>> generic files, e.g. rtems/score/isrlevel.h, so it should not be introduced from<br>
>>> my point of view.<br><br></span></blockquote><div>OK. I can add an architecture to the constant but my hope was that </div><div>isrlevel.h would eventually be where the cut off to the paravirtualized </div><div>ISR disable/enable methods would be made. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
>>> I don't think it is worth to add a rtems/score/paravirt.h for each architecture.<br>
>>> The changes introduced by RTEMS_PARAVIRT are too small to justify this.  I am<br>
>>> also not sure if you can encapsulate this in one header in all cases.<br>
>><br>
>> Please don't ignore this.<br>
>><br>
><br>
> I felt spreading the RTEMS_PARAVIRT across the code was hiding the reason in<br>
> some cases. When I reviewed the v2 patches I felt changes in a specific area<br>
> needed more information to aid long term maintenance. For example look at the<br>
> ARM thread id register. It is clear what is happening and if that change flows<br>
> out to other parts of the system it is clear what is happening if there is a<br>
> dependence on that register.<br>
<br>
</span>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.<br></blockquote><div><br></div><div>It might be able to go near the top of cpu.h. But the inclusion of paravirt.h is already</div><div>wrapped in a conditional so it isn't impacting normal compiles.</div><div><br></div><div>Given that this file exists to specifically map an RTEMS configuration onto</div><div>architectural tweaks and document them, I still think they should be in their</div><div>own file. Putting it at the top of cpu.h is likely technically feasible but doesn't</div><div>achieve the goal of having a clear place to document paravirtualization</div><div>tweaks, etc.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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.<br></blockquote><div><br></div><div>It was actually hiding the reason for the conditional in all cases.  </div><div><br></div><div>Chris' suggestion was because none of this can be tested without special</div><div>setups which are not generally available. Plus it is completely undocumented</div><div>what the host restrictions and requirements were that led to the conditionals</div><div>being put in place.</div></div><br></div><div class="gmail_extra">This header file is primarily to document (and do a good job of it) the</div><div class="gmail_extra">paravirtualization changes. Your thread id response is a perfect example.</div><div class="gmail_extra">The need to compile with that option should have been documented.  As</div><div class="gmail_extra">the code is today, the definition of that register is only implied by </div><div class="gmail_extra">a GCC architectural revision. The register is indeed present in our</div><div class="gmail_extra">paravirtualized environment but we can't set it because it can't <br></div><div class="gmail_extra">be set in user mode. You already solved this problem and could</div><div class="gmail_extra">have even provided the helper routine for paravirtualized environments.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Scattering around a bunch of undocumented tweaks without a</div><div class="gmail_extra">way to document them is bad. Especially when only the person</div><div class="gmail_extra">who did the paravirtualization adapter for a specific host knows the</div><div class="gmail_extra">details.</div><div class="gmail_extra"><br></div><div class="gmail_extra">I agreed with Chris that in-code documentation was the answer</div><div class="gmail_extra">and providing a nice container for it was the answer.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Having paravirt.h could have the configure process give you an</div><div class="gmail_extra">error if the architecture doesn't have it -- which implies it doesn't</div><div class="gmail_extra">support it.</div><div class="gmail_extra"><br></div><div class="gmail_extra">--joel</div></div>