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

[Gleb Natapov <gleb@redhat.com>]

On Mon, Jul 29, 2013 at 03:11:39PM +0200, Paolo Bonzini wrote:
> Il 29/07/2013 14:34, Gleb Natapov ha scritto:
> >>>> But I think what you _really_ want is not avoiding conditional branches.
> >>> The idea is that it is hard for branch prediction to predict correct
> >>> result when correct result depends on guest's page table that can
> >>> contain anything, so in some places shadow paging code uses boolean
> >>> logic to avoid branches, in this case it is hard to avoid if() anyway
> >>> since the function invocation is in the if().
> >>
> >> Yes, I get the idea, but is_rsvd_bits_set should be predicted unlikely,
> >> no?  If the guest has to run, it must use mostly valid ptes. :)
> >>
> > You see, you are confused and you want branch prediction not to be? :)
> > If your guest is KVM is_rsvd_bits_set() will be likely much more then
> > unlikely because KVM misconfigures EPT entries to cache MMIO addresses,
> > so all the "unlikely" cases will be fixed by shadow pages and will not
> > reappear (until shadow pages are zapped), but misconfigured entries will
> > continue to produces violations.
> 
> But then:
> 
> 1) MMIO is a slow path anyway, losing 10 cycles on a mispredicted branch
> is not going to help much.  Fast page faults are all I would optimize for.
> 
Of course, for that the check should be fast.

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

> >> Especially if you change prefetch_invalid_gpte to do the reserved bits
> >> test after the present test (so that is_rsvd_bits_set is only called on
> >> present pagetables), is_rsvd_bits_set's result should be really
> >> well-predicted. 
> > Nope, for ept page tables present is not a single bit, it is three bits
> > which by themselves can have invalid values.
> 
> We're not checking the validity of the bits in the is_present_gpte test,
> we're checking it in the is_rsvd_bits_set test (is_present_gpte is doing
> just "(pte & 7) != 0").  It doesn't change anything in the outcome of
> prefetch_invalid_gpte, and it makes the ordering consistent with
> walk_addr_generic which already tests presence before reserved bits.
> 
> So doing this swap should be a win anyway.
> 
> >>                   At this point (and especially since function invocation
> >> is always in "if"s), using boolean logic to avoid branches does not make
> >> much sense anymore for this function.
> > 
> > That's true.
> 
> So are you going to change to "if"s?
> 
I think it will be better just to check mmu->bad_mt_xwr always. (I
dislike ifdefs if you haven't noticed :)).

--
			Gleb.