[PATCH 0/3] v3 - Paravirtualization Patch Series

[Sebastian Huber]

----- Am 22. Feb 2018 um 17:57 schrieb joel joel at rtems.org:

> On Thu, Feb 22, 2018 at 5:42 AM, Sebastian Huber <
> sebastian.huber at embedded-brains.de> wrote:
> 
>> ----- Am 22. Feb 2018 um 6:06 schrieb Chris Johns chrisj at 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.