Re: [PATCH v5 10/11] KVM: VMX: Enable SGX virtualization for SGX1, SGX2 and LC

From: Paolo Bonzini <pbonzini@redhat.com>
To: Sean Christopherson <seanjc@google.com>, Kai Huang <kai.huang@intel.com>
Cc: kvm@vger.kernel.org, linux-sgx@vger.kernel.org, bp@alien8.de,
	jarkko@kernel.org, dave.hansen@intel.com, luto@kernel.org,
	rick.p.edgecombe@intel.com, haitao.huang@intel.com
Subject: Re: [PATCH v5 10/11] KVM: VMX: Enable SGX virtualization for SGX1, SGX2 and LC
Date: Mon, 19 Apr 2021 19:14:14 +0200	[thread overview]
Message-ID: <acff71e3-4a38-b858-551c-8b34cbf344af@redhat.com> (raw)
In-Reply-To: <YH2e1lIckOBY2OGa@google.com>

On 19/04/21 17:16, Sean Christopherson wrote:
> On Mon, Apr 19, 2021, Kai Huang wrote:
>> On Sat, 2021-04-17 at 16:11 +0200, Paolo Bonzini wrote:
>>> On 12/04/21 06:21, Kai Huang wrote:
>>>> @@ -4377,6 +4380,15 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
>>>>    	if (!vcpu->kvm->arch.bus_lock_detection_enabled)
>>>>    		exec_control &= ~SECONDARY_EXEC_BUS_LOCK_DETECTION;
>>>>    
>>>>
>>>> +	if (cpu_has_vmx_encls_vmexit() && nested) {
>>>> +		if (guest_cpuid_has(vcpu, X86_FEATURE_SGX))
>>>> +			vmx->nested.msrs.secondary_ctls_high |=
>>>> +				SECONDARY_EXEC_ENCLS_EXITING;
>>>> +		else
>>>> +			vmx->nested.msrs.secondary_ctls_high &=
>>>> +				~SECONDARY_EXEC_ENCLS_EXITING;
>>>> +	}
>>>> +
>>>
>>> This is incorrect, I've removed it.  The MSRs can only be written by
>>> userspace.
> 
> vmx_compute_secondary_exec_control() violates that left, right, and center, it's
> just buried down in vmx_adjust_secondary_exec_control().  This is an open coded
> version of that helper, sans the actual update to exec_control since ENCLS needs
> to be conditionally intercepted even when it's exposed to the guest.

Hmm, that's true.  I'll place back the version that you had.

Paolo

>>> If SGX is disabled in the guest CPUID, nested_vmx_exit_handled_encls can
>>> just do:
>>>
>>> 	if (!guest_cpuid_has(vcpu, X86_FEATURE_SGX) ||
>>> 	    !nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENCLS_EXITING))
>>> 		return false;
>>>
>>> and the useless ENCLS exiting bitmap in vmcs12 will be ignored.
>>>
>>> Paolo
>>>
>>
>> Thanks for queuing this series!
>>
>> Looks good to me. However if I read code correctly, in this way a side effect would be
>> vmx->nested.msrs.secondary_ctls_high will always have SECONDARY_EXEC_ENCLS_EXITING bit
>> set, even SGX is not exposed to guest, which means a guest can set this even SGX is not
>> present, but I think it is OK since ENCLS exiting bitmap in vmcs12 will be ignored anyway
>> in nested_vmx_exit_handled_encls() as you mentioned above.
>>
>> Anyway, I have tested this code and it works at my side (by creating L2 with SGX support
>> and running SGX workloads inside it).
>>
>> Sean, please also comment if you have any.
>>
>>
> 