Re: [PATCH v5 10/14] nEPT: Add nEPT violation/misconfigration support

[Paolo Bonzini <pbonzini@redhat.com>]

> > > > > > Not sure this explicit "inline" is needed... Gcc always inlines the small and
> > > > > > static functions.
> > > > > 
> > > > > Paolo asked for it, but now I see that I did in in a wrong patch. I do
> > > > > not care much personally either way, I agree with you though, compiler
> > > > > will inline it anyway.
> > > > 
> > > > The point here was that if we use "||" below (or multiple "if"s as I
> > > > suggested in my review), we really want to inline the function to ensure
> > > > that the branches here is merged with the one in the caller's "if()".
> > > > 
> > > > With the "|" there is not much effect.
> > > 
> > > Even with if() do you really think there is a chance the function will not be
> > > inlined? I see that much much bigger functions are inlined.
> > 
> > I don't know, it depends on the compiler flags, how much the function
> > is used...  Zeroing the chance is not bad.
> 
> AFAIK inline is just a hint anyway and compiler is free to ignore it.
> That is why we have __always_inline, but compiler should know better
> here, do not see the reason for __always_inline.

Yes, but with "inline" the compiler will use more generous thresholds.

The GCC docs say that without "inline", -O2 only inlines "functions into
their callers when their body is smaller than expected function call
code (so overall size of program gets smaller)".  I'm not at all sure
this is the case for the new is_rsvd_bits_set, and anyway "making the
program smaller" is not the reason why we want the compiler to inline it.

Did you check that the compiler inlines it?  Perhaps you should really
use __always_inline since that's what we really want.

> > Because the function is "bool".  I dislike the magic "!= 0"
> > that the compiler adds on conversion to bool.  It always seemed
> > like a recipe for trouble since "int" and "bool" are otherwise
> > interchangeable...  Without that "!= 0", s/bool/int/ would ignore
> > the upper 32 bits and break.
> 
> I actually checked that before proposing.
> 
> printf("%d\n", (bool)0x1000000000) prints one, but of course if bool is
> typedefed to int it will not work, but it should be not.

No, it should not be indeed, but not everyone uses bool in the same way;
it is quite common to use "int" for something that is 0/1, and the magic
"!= 0" is dangerous if you cut-and-paste the expression where the compiler
will not do it...  It can even be a function argument where you do not
see directly if it is bool, int, u64 or what.

I don't think omitting "!= 0" is common at all in the kernel, so I would
not start doing it here. :)

> > > > On the other hand, it starts to look like useless complication not
> > > > backed by any benchmark (and probably unmeasurable anyway).  Neither of
> > > > the conditions are particularly easy to compute, and they are probably
> > > > more expensive than a well-predicted branch.  Thus, in this case I
> > > > would prefer to have clearer code and just use two "if"s, the second
> > > > of which can be guarded to be done only for EPT (it doesn't have to
> > > > be an #ifdef, no?).
> > > 
> > > return a | b
> > > 
> > > is less clear than:
> > > 
> > > if (a)
> > >  return true;
> > > if (b)
> > >  return true;
> > 
> > If each of a and b is actually a complicated expression having
> > AND/SHIFT/arrays, then yes. :)
>
> This is not the most complex expression possible :)
> Just having them on different lines is not enough?

No big deal, especially if we plan to merge the present check together.

Paolo