Haozhong Zhang <haozhong.zhang@xxxxxxxxx> writes:
> On 11/25/15 10:45, Bandan Das wrote:
>> Haozhong Zhang <haozhong.zhang@xxxxxxxxx> writes:
>>
>> > This patch removes the vpid check when emulating nested invvpid
>> > instruction of type all-contexts invalidation. The existing code is
>> > incorrect because:
>> > (1) According to Intel SDM Vol 3, Section "INVVPID - Invalidate
>> > Translations Based on VPID", invvpid instruction does not check
>> > vpid in the invvpid descriptor when its type is all-contexts
>> > invalidation.
>>
>> But iirc isn't vpid=0 reserved for root mode ?
> Yes,
>
>> I think we don't want
>> L1 hypervisor to be able do a invvpid(0).
>
> but the invvpid emulated here is doing the all-contexts invalidation that
> does not use the given vpid and "invalidates all mappings tagged with all
> VPIDs except VPID 0000H" (from Intel SDM).
Actually, vpid_sync_context will always try single invalidation first and
I was concerned that we will end up calling vpid_sync_context(0). But that
will not happen since nested.vpid02 is always allocated a vpid.
So... we are good :)
>>
>> > (2) According to the same document, invvpid of type all-contexts
>> > invalidation does not require there is an active VMCS, so/and
>> > get_vmcs12() in the existing code may result in a NULL-pointer
>> > dereference. In practice, it can crash both KVM itself and L1
>> > hypervisors that use invvpid (e.g. Xen).
>>
>> If that is the case, then just check if it's null and return without
>> doing anything.
>
> (according to Intel SDM) invvpid of type all-contexts invalidation
> should not trigger a valid vmx fail if vpid in the current VMCS is 0.
No, I meant just skip instruction and return but I doubt if there's
any overhead of flushing mappings that don't exist in the first place.
Anyway, better to do as the spec says.
> However, this check and its following operation do change this semantics
> in nested VMX, so it should be completely removed.
>
>>
>> > Signed-off-by: Haozhong Zhang <haozhong.zhang@xxxxxxxxx>
>> > ---
>> > arch/x86/kvm/vmx.c | 5 -----
>> > 1 file changed, 5 deletions(-)
>> >
>> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> > index 87acc52..af823a3 100644
>> > --- a/arch/x86/kvm/vmx.c
>> > +++ b/arch/x86/kvm/vmx.c
>> > @@ -7394,11 +7394,6 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
>> >
>> > switch (type) {
>> > case VMX_VPID_EXTENT_ALL_CONTEXT:
>> > - if (get_vmcs12(vcpu)->virtual_processor_id == 0) {
>> > - nested_vmx_failValid(vcpu,
>> > - VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
>> > - return 1;
>> > - }
>> > __vmx_flush_tlb(vcpu, to_vmx(vcpu)->nested.vpid02);
>> > nested_vmx_succeed(vcpu);
>> > break;
>>
>> I also noticed a BUG() here in the default. It might be a good idea to replace
>> it with a WARN.
>
> Or, in nested_vmx_setup_ctls_msrs():
>
> if (enable_vpid)
> - vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT |
> - VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT;
> + vmx->nested.nested_vmx_vpid_caps = VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT;
>
> because the current handle_invvpid() only handles all-contexts invalidation.
>
> Haozhong
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
