[PATCH 0/3] v3 - Paravirtualization Patch Series

[Sebastian Huber]

----- Am 22. Feb 2018 um 3:25 schrieb Sebastian Huber sebastian.huber at embedded-brains.de:

> ----- Am 21. Feb 2018 um 21:07 schrieb joel joel at rtems.org:
> 
>> Hi
>> 
>> This patch series reworks the ARM and PowerPC support addition
>> as well as reworks the i386 paravirtualization support based
>> on Chris Johns' suggestion. He suggested adding a
>> rtems/score/paravirt.h for each architecture
> 
> This is not what you are doing. You added one rtems/score/paravirt.h for all
> architectures.

Sorry, I did misread the patches. I have currently only a web based e-mail client.

>> and using
>> derived feature macros instead of RTEMS_PARAVIRT directly.
>> This has the nice side-effect of paravirt.h documenting
>> what the paravirtualization environment is for each
>> architecture as well as precisely the purpose of each
>> introduced conditional compilation.
> 
> This new rtems/score/paravirt.h is completely against the current score/cpu/*
> structure. Architecture-specific stuff should not be aggregated in one file,
> instead the existing architecture-specific cpu.h, etc. files should be used.

Please ignore this.

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

In addition, this

+#endif  /* END CPU_POWERPC_DISABLE_MSR_ACCESS */

style is a bit odd.