Re: PVHVM defines.

[Maxime Villard <max%m00nbsd.net@localhost>]

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.

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.

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.

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.