Re: PVHVM defines.

["Mathew, Cherry G." <cherry%zyx.in@localhost>]

On December 5, 2018 3:33:13 PM GMT+05:30, Maxime Villard <max%m00nbsd.net@localhost> wrote:
>Le 04/12/2018 à 06:50, Cherry G.Mathew a écrit :
>> Hi,
>> 
>> Towards getting PVHVM code into source, I'm thinking of committing
>this
>> patch, that has no functional changes. I want to "ratchet" in the
>code,
>> so that there are clear bisection points.
>> 
>> The basic idea is to have an 'options XENPVHVM' in the config file of
>> the PVHVM kernel, and no other changes anywhere else.
>> 
>> Once things have settled in, we can invert the sense of the -D from
>> XENPVHVM being the novelty, to -D XENPV being the novelty.
>> 
>> In other words, PV will have to be explicityly enabled by
>> 'options XENPV' while -DXEN will imply XENPVHVM. This is a bit into
>the
>> future though, for after we have PVHVM and PVH stable.
>> 
>> For now, this is purely elbowing into -current.
>> 
>> Thoughts ?
>> 
>
>Style
>
>	-#ifdef XEN
>	+#if defined(XEN) && ! defined(XENPVHVM)
>
>	-#ifndef XEN
>	+#if !defined (XEN) || defined(XENPVHVM)
>
>	-#ifdef XEN
>	+#if defined(XEN) && ! defined(XENPVHVM)
>
>	-#if defined(DIAGNOSTIC) || defined(XEN)
>	+#if defined(DIAGNOSTIC) || defined(XEN)  && !defined(XENPVHVM)
>
>Please be careful because you're also adding useless whitespaces, and
>I've repeatedly told you that in previous patches. The code should be
>KNF as soon as it lands the tree, not after when someone comes in and
>fixes everything.
>


Hi maxv,

Thanks for the repeated admonitions on KNF. I will do a lint style pass before checking them in. I'm sorry for the distress previously caused by you having to whitespace nitpick through my mess.

I'm glad we still care about KNF (although I have some disagreement with certain constructs - for eg: lack of braces around single line conditional blocks.)

>Apart from style, this change is wrong:
>
>	-#ifndef XEN
>	+#if !defined(XEN) || defined(XENPVHVM)
>	+#if defined(SPECTRE_V2_GCC_MITIGATION)	
>
>The GCC mitigation has nothing to do with the hardware mitigation;
>same for the other SpectreV2-related places.
>

Iirc there was some kind of dependancy with hotpatching which I wanted to disable for the first round of PVHVM at least, but I'll have a relook and get back to you. 

Is there a 'furthest back' cpu model where GCC mitigation is not relevant? Or is it purely a compiler related ifdef? 

>Overall, this kind of change is pretty bad from a code quality PoV, but
>I'm not blaming you, because I don't see another way of adding PVHVM
>without introducing a ton of ifdefs.
>

This is just a first step along a path of least resistance. It is by no means something I like or want to preserve.

>I guess it validates my point about "too many shades of x86", the fact
>that our x86 port is growing into spaghetti code, and that we can't do
>anything about it apart from dropping the least interesting modes.

That's one approach - but it's a policy prescription which needs more consensus than just your opinion. A better approach would be to look at re-factoring with modularity in mind. I'll probably take that approach when I come across native vs. PVHVM for now, at least.

Many Thanks,

Cherry


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.