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.
>
> 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.
