> On 7 Oct 2018, at 23:50, Liran Alon <liran.alon@xxxxxxxxxx> wrote:
>
>
>
>> On 7 Oct 2018, at 23:14, Liran Alon <liran.alon@xxxxxxxxxx> 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 :)
>>
>> 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
>
> I have an idea on how to preserve the wanted functionality of this patch:
> We could modify handle_invvpid() to emulate INVVPID of all types besides individual-address by executing INVEPT(eptp02) when vmcs12 uses EPT.
> This will prevent L1 from invalidating TLB entries of other guests by executing INVVPID.
>
> This will also result in every INVVPID of non-individual-address type to invalidate all L2 TLB entries of given eptp02 of all various L2 VPIDs.
> However, it is not different than today in that regard, as today all L2 TLB entries are tagged with a single vmx->nested.vpid02.
>
> Paolo, if you agree to this, I will create relevant patches for v2 of this series.
>
> -Liran
>
>
Please ignore last email idea. Not really practical…
I don’t know what is worse in it: The fact there is no vmcs12 to check if it uses EPT
or the fact that there is no easy way to translate between a vmcs12->vpid to all the active eptp02 that it was used on…
Sorry for the spam. :)
I will just submit a fix for the bug of calling __vmx_flush_tlb() with bad parameter.
-Liran
