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