On Sun, 2018-10-07 at 23:14 +0300, Liran Alon wrote:
>
> >
> > On 1 Oct 2018, at 19:49, Jim Mattson <jmattson@xxxxxxxxxx> wrote:
> >
> > On Mon, Oct 1, 2018 at 5:56 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> > >
> > > On 06/09/2018 18:06, Jim Mattson wrote:
> > > >
> > > > On Thu, Sep 6, 2018 at 6:32 AM, Liran Alon <liran.alon@xxxxxxxxxx> wrote:
> > > > >
> > > > > If CPU use both VPID and EPT, TLB entries populated by CPU are tagged
> > > > > with both EPTP and VPID. Therefore, if L1 uses EPT, L2 TLB entries
> > > > > are separated from L1 TLB entries by the EPTP tags as vmcs02 use
> > > > > EPTP02 while vmcs01 use EPTP01.
> > > > >
> > > > > Thus, we don't need to make sure that vmcs02->vpid != vmcs01->vpid.
> > > > > Therefore, we can just set vmcs02->vpid to vmcs12->vpid.
> > > > >
> > > > > Reviewed-by: Mihai Carabas <mihai.carabas@xxxxxxxxxx>
> > > > > Reviewed-by: Darren Kenny <darren.kenny@xxxxxxxxxx>
> > > > > Reviewed-by: Nikita Leshchenko <nikita.leshchenko@xxxxxxxxxx>
> > > > > Signed-off-by: Liran Alon <liran.alon@xxxxxxxxxx>
> > > > I suggested this back in July, but Paolo didn't like it. I still like it. :-)
> > > >
> > > > Reviewed-by: Jim Mattson <jmattson@xxxxxxxxxx>
> > > The problem with this is still the same as in July, namely that if all
> > > guests (at any level) share the VPID space, then L1 can force an
> > > invalidation of any VPID (and thus slow down execution of other guests,
> > > including siblings of L1) through INVVPID.
> Funny fact: This discussion have lead me to look at handle_invvpid() which L0 use to
> emulate INVVPID for L1 and it seems to me to contain a serious issue.
> All calls to __vmx_flush_tlb() is called with invalidate_gpa set to true which actually results
> in L0 executing INVEPT(eptp01) instead of INVVPID(vmx->nested.vpid02).
> (Bug was introduced by c2ba05ccfde2 ("KVM: X86: introduce invalidate_gpa argument to tlb flush”)).
> I will send a very simple patch to fix this issue :)
record_steal_time() calls __vmx_flush_tlb() with invalidate_gpa=false via
kvm_vcpu_flush_tlb(). And if you look closely at the diff, you can see
that prior to invalidate_gpa we unconditionally did INVEPT if enable_ept.
This was also discussed in depth in the context of a VPID series[1].
IIRC, the consensus at thay time was that while the VPID code is horribly
buggy, none of the bugs actually cause problems because we always flush
on a nested transition.
[1] https://www.spinics.net/lists/kvm/msg169752.html
> In addition, it seems that the patch I have submitted here also lacks changing handle_invvpid()
> to execute INVVPID(vmcs12->vpid) instead of INVVPID(vmx->nested.vpid02) in case vmcs12 use EPT.
> I can fix this in v2 of this patch if we will decide it should be applied… (See below)
>
> I understand the concern raised here as even a non-malicious L1 guest will likely hurt other guests
> performance when executing a non-individual-address type INVVPID.
> What we really wish to have here is a new Intel VMX instruction which allows invalidating TLB entries
> by given EPTP *and* VPID. Too bad there isn’t such an instruction…
>
> >
> >
> > Can't L1 already do this by filling the TLB with its own entries,
> > thereby evicting its siblings' entries?
> Yes, but I agree that with this change, it is much easier for a L1 to just invalidate combined mappings used by other guests.
> So I tend to agree with Paolo that maybe this patch should be dropped…
>
> -Liran
>
>
>
