[PATCH 0/3] v3 - Paravirtualization Patch Series

[Joel Sherrill]

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.


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


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

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.

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.

--joel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20180222/343c4d48/attachment.html>