On Mon, 2018-09-24 at 09:26 -0700, Jim Mattson wrote: > On Mon, Sep 24, 2018 at 6:31 AM, Sean Christopherson > <sean.j.christopherson@xxxxxxxxx> wrote: > > > > On Thu, 2018-08-30 at 16:05 -0700, Jim Mattson wrote: > > > > > > As specified in Intel's SDM, do not allow the L1 hypervisor to launch > > > an L2 guest with the VM-execution controls for "unrestricted guest" or > > > "mode-based execute control for EPT" set and the VM-execution control > > > for "enable EPT" clear. > > > > > > Note that the VM-execution control for "mode-based execute control for > > > EPT" is not yet virtualized by kvm. > > The EPT violation #VE control also falls into this category. > Though that makes sense to me, the "enable EPT" requirement isn't documented. Huh. I didn't actually check anything, I just assumed this was the case. The SDM appears to be correct, I can't find anything that suggests that setting EPT_VIOLATION_VE requires EPT to be enabled, which is hilarious. > > nested_vmx_check_ept_enable() is a bit weird, it makes it sound like > > we're always requiring EPT to be enabled. And checking for EPT being > > enabled isn't unique to this function, e.g. EPT is also required for > > PML and is checked by nested_vmx_check_pml_controls(). > And EPTP switching... > > I was just trying to implement one bullet point from section 26.2.1.1 > of the SDM: > > o If either the “unrestricted guest” VM-execution control or the > “mode-based execute control for EPT” VM-execution control is 1, the > “enable EPT” VM-execution control must also be 1. > > > > > That being said, I actually like the approach of checking all controls > > that depend EPT in a single function, i.e. what about moving the EPT > > part of the PML check to this function? > I think that mixing the checks for multiple bullet points from the SDM > makes it harder to verify that all of the checks have been properly > implemented. However, the obvious > "nested_vmx_check_unrestricted_guest_or_mode_based_exec_controls" > seems a bit long. I'd rather split one bullet point into multiple > checks than merge portions of multiple bullet points into a single > check. Maybe nested_vmx_check_unrestricted_guest_controls and > nested_vmx_check_mode_based_exec_controls? Makes sense, I didn't realize the SDM put those under a single bullet. I'd vote to separate the two checks, they're completely unrelated other than the EPT dependency. I'm guessing they're lumped together in the SDM only because they don't have any other consistency checks. > > What about borrowing the style of nested_vmx_check_shadow_vmcs_controls() > > and returning 0 immediately if nested_cpu_has_ept() is true?! > Sure. Feel free to ignore this, it doesn't make as much sense if each check is in a separate function. I might argue the other way in that case :)
