Re: [PATCH v4 09/13] nEPT: Add nEPT violation/misconfigration support

[Gleb Natapov <gleb@redhat.com>]

On Mon, Jul 29, 2013 at 06:36:12PM +0200, Paolo Bonzini wrote:
> Il 29/07/2013 18:24, Gleb Natapov ha scritto:
> > On Mon, Jul 29, 2013 at 04:12:33PM +0200, Paolo Bonzini wrote:
> >> Il 29/07/2013 15:20, Gleb Natapov ha scritto:
> >>>> 2) in cases like this you just do not use likely/unlikely; the branch
> >>>> will be very unlikely in the beginning, and very likely once shadow
> >>>> pages are filled or in the no-EPT case.  Just let the branch predictor
> >>>> adjust, it will probably do better than boolean tricks.
> >>>>
> >>> likely/unlikely are usually useless anyway. If you can avoid if()
> >>> altogether this is a win since there is no branch to predict.
> >>
> >> However, if the branches are dynamically well-predicted,
> >>
> >>    if (simple)
> >>        ...
> >>    if (complex)
> >>        ...
> >>
> >> is likely faster than
> >>
> >>    if (simple | complex)
> >>
> >> because the branches then are very very cheap, and it pays off to not
> >> always evaluate the complex branch.
> >
> > Good point about about "|" always evaluating both. Is this the case
> > with if (simple !=0 | complex != 0) too where theoretically compiler may
> > see that if simple !=0 is true no need to evaluate the second one?
> 
> Yes (only if complex doesn't have any side effects, which is the case here).
> 
> >> Yeah, I also thought of always checking bad_mt_xwr and even using it to
> >> subsume the present check too, i.e. turning it into
> >> is_rsvd_bits_set_or_nonpresent.  It checks the same bits that are used
> >> in the present check (well, a superset).  You can then check for
> >> presence separately if you care, which you don't in
> >> prefetch_invalid_gpte.  It requires small changes in the callers but
> >> nothing major.
> > 
> > I do not get what is_rsvd_bits_set_or_nonpresent() will check exactly
> > and why do we needed it, there are two places where we check
> > present/reserved and in one of them we need to know which one it is.
> 
> You can OR bad_mt_xwr with 0x5555555555555555ULL (I think).  Then your
With 0x1010101.

> implementation of is_rsvd_bits_set() using bad_mt_xwr will return true
> in all cases where the pte is non-present.  You can then call
> is_present_pte to discriminate the two cases.
> 
>     if (is_rsvd_bits_set_or_nonpresent) {
>         if (!present)
>             ...
>         else
>             ...
>     }
> 
> In more abstract terms this is:
> 
>     if (simple)
>         ...
>     if (complex)
>         ...
> 
> to
> 
>     if (simple_or_complex) {
>         if (simple)
>             ...
>         else
>             ...
>     }
> 
> This can actually make sense if simple is almost always false, because
> then you save something from not evaluating it on the fast path.
> 
> But in this case, adding bad_mt_xwr to the non-EPT case is a small loss.
> 
> > Anyway order of checks in prefetch_invalid_gpte() is not relevant to
> > that patchset, so lets better leave it to a separate discussion.
> 
> Yes.
> 
> Paolo
> 
> >>
> >> But it still seems to me that we're in the above "if (simple ||
> >> complex)" case and having a separate "if (!present)" check will be faster.
> >>
> >> Paolo
> > 
> > --
> > 			Gleb.
> > 

--
			Gleb.