[PATCH 0/3] v3 - Paravirtualization Patch Series

Fri Feb 23 06:41:02 UTC 2018

On 23/02/2018 14:15, Sebastian Huber wrote:
> ----- 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.

Sorry, I am not following what you are saying. I am reading the above as either
provide a full generalisation or do not provide any framework. I suspect my
interpretation is wrong so could you please explain why a paravirt header makes
"things" more difficult?

> In the next hypervisor you will have some stuff in paravirt.h and then some more stuff somewhere else.

Yes I agree another will add other "stuff" "elsewhere" and this is all the more
reason to not spread RTEMS_PARAVIRT and to overload CPU specifics and I am
concerned adding this stuff to cpu.h is only going to create a mess for that
header if we get more hypervisors. I feel adding this to cpu.h is wrong because
this is nothing to do with *the* CPU and it's interface to RTEMS, it is all
about virtualising software that hosts RTEMS and how that effects the CPU.

All we should see in paravirt.h is the controls for _a_ hypervisor. The first to
do this work gets to add their stuff and others who come later may need to
refactor this header to support multiple hypervisors. I cannot say what that
means, there is nothing else on the table to consider but I hope there are more
to come. And it is fine if nothing else is ever supported.

>   
>>
>>
>>>>>> 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?

The cpu.h header describes a CPU to RTEMS. The paravirt header describes a
hypervisor to a CPU. Yes it could be added as comment to the cpu.h header but
this does not define a framework to others and so users will keep on adding to
cpu.h. How many hypervisors and how much do we add to cpu.h before we have to stop?

> 
>>
>>
>>>
>>> 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.
> 

Here again you have lost me. Yes it is hard to generalise however I feel doing
nothing or not discussing this is not good either. I would like to make it
easier and simpler for users to added support for other hypervisors.

>> 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.

Which 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.
> 

Hmmm.

Chris