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

[Gleb Natapov <gleb@redhat.com>]

On Mon, Jul 29, 2013 at 02:05:44PM +0200, Paolo Bonzini wrote:
> Il 29/07/2013 13:43, Gleb Natapov ha scritto:
> > On Mon, Jul 29, 2013 at 12:59:54PM +0200, Paolo Bonzini wrote:
> >> Il 29/07/2013 12:52, Gleb Natapov ha scritto:
> >>> On Mon, Jul 29, 2013 at 10:59:31AM +0200, Paolo Bonzini wrote:
> >>>> Il 25/07/2013 12:59, Gleb Natapov ha scritto:
> >>>>> +#if PTTYPE == PTTYPE_EPT
> >>>>> +#define CHECK_BAD_MT_XWR(G) mmu->bad_mt_xwr & (1ull << ((G) & 0x3f));
> >>>>> +#else
> >>>>> +#define CHECK_BAD_MT_XWR(G) 0;
> >>
> >> No semicolons here, BTW.
> >>
> >>>>> +#endif
> >>>>> +
> >>>>>  static bool FNAME(is_rsvd_bits_set)(struct kvm_mmu *mmu, u64 gpte, int level)
> >>>>>  {
> >>>>>  	int bit7;
> >>>>>  
> >>>>>  	bit7 = (gpte >> 7) & 1;
> >>>>> -	return (gpte & mmu->rsvd_bits_mask[bit7][level-1]) != 0;
> >>>>> +	return ((gpte & mmu->rsvd_bits_mask[bit7][level-1]) != 0) ||
> >>>>> +		CHECK_BAD_MT_XWR(gpte);
> >>>>>  }
> >>>>>  
> >>>>> +#undef CHECK_BAD_MT_XWR
> >>>>
> >>>> Instead of a macro, you can do
> >>>>
> >>>> 	if (...)
> >>>> 		return true;
> >>>> #if PTTYPE == PTTYPE_EPT
> >>>> 	if (...)
> >>>> 		return true;
> >>>> #endif
> >>>> 	return false;
> >>>>
> >>>> The compiler should be smart enough to generate the same code for
> >>>> non-EPT PTTYPE.
> >>>>
> >>> The idea behind this rsvd_bits_mask trickery is to  produce code that
> >>> does not have conditional branches.
> >>
> >> If you want to have no conditional branches, you need to use "|" not
> >> "||" (and you also need an "!= 0" in CHECK_BAD_MT_XWR).  As you wrote
> >> it, the compiler is most likely to generate exactly the same code that I
> >> suggested.
> > OK. I can add that :) I still prefer to have ifdefs outside the
> > function, not inside.
> > 
> >>
> >> 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.

> 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. For regular paging you are
probably right.

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

> 
> The compiler may still use setCC and boolean logic, even if you write it
> as "if"s; and in simple cases like this, after inlining and seeing an
> "if" the compiler may undo all your careful choice of "|" vs. "||".  So
> it's better anyway to ensure is_rsvd_bits_set is called only when it's
> unlikely.
> 
> Paolo

--
			Gleb.