[PATCH 0/3] v3 - Paravirtualization Patch Series

[Sebastian Huber]

----- Am 23. Feb 2018 um 7:41 schrieb Chris Johns chrisj at rtems.org:

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

Ok, but then could we please not introduce CPU_* defines in this paravirt.h.  The rationale for this header should be documented somewhere, maybe the general section of the CPU supplement. In one year nobody knows why we added this paravirt.h.

The paravirt.h should be also added for sparc.

[...] 

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

The ARM ABI. The ABI is defined by ARM and the platform. We currently use the GCC default platform definitions (e.g. Linux). Maybe we should use the TPIDRURW on RTEMS for the thread pointer. In this case, the hypervisor must add this register to the per-processor partition context. For XtratuM this was not an option.

[...]