> On 8 Oct 2018, at 16:24, Sean Christopherson <sean.j.christopherson@xxxxxxxxx> wrote:
>
> 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.
Yes I am aware of that.
I currently already have a simple series which I believe fixes the remaining issues in VPID code.
Only waiting for a bit more testing and reviews. Will send shortly.
It is true that probably none of these bugs manifested because most L1 hypervisors use
INVEPT instead of INVVPID when they enable EPT. The only case which this is not true
is for TLB shootdown hypercall, which in this case L1 hypervisor may use INVVPID.
>
> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_kvm_msg169752.html&d=DwIDaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=ahHvfxys3ePYk78-Y2po88fibK8IsEgjLvC-5mRcOb8&s=t94xDNDrS8RKCYG83SJWc3xOcMR4M-RQ_whTU73VI04&e=
>
>> 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
>>
>>
>>
