On 20/09/2017 15:05, Ladi Prosek wrote: > On Wed, Sep 20, 2017 at 12:14 PM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote: >> On 20/09/2017 10:52, Ladi Prosek wrote: >>>> Therefore, storing such hidden state or a subset of it in >>>> vmx->nested.smm is merely a convenience that lets you keep >>>> vmx->nested.vmxon == false while in SMM. This is different from nSVM, >>>> and I think it's an acceptable difference for the sake of convenience. >>> So for nSVM, you'd save to and restore from the state save area? I can >>> certainly do that but it would be hard to guarantee that KVM won't >>> blow up if the SMM handler pokes at these fields. It feels safer to >>> keep it internal or just save, not restore from it. I know that this >>> comment belongs to the nSVM patch but it looks similar to your >>> suggestion to protect CR4.VMXE on Intel. If we want to prevent SMM >>> from wreaking havoc by trying to enable VMX, shouldn't we use the same >>> measures on AMD and prevent it from switching VMCB while the nested >>> guest is running. Thanks! >> >> The difference is that on AMD you're actually allowed to do that. The >> SMM handler can also poke the general purpose register or even the >> descriptor cache, so I think we should stay close to what the manual >> says---even if it does two opposite things on Intel vs. AMD. > > The AMD manual seems to list the SVM fields as Read-Only. > "Software should not modify offsets specified as read-only or > reserved, otherwise unpredictable results can occur." In practice, that means "otherwise you'll get exactly what you think you'll get, with possibly entertaining effects". However, this is not relevant: the point is that on AMD the SMM handler can do VMRUN, while on VMX it cannot even do VMXON. So on AMD the handler can change the active VMCB (via VMRUN, which writes to svm->nested.vmcb). On RSM however you must get back to L2 using the VMCB stored in the SMM state save area. This is exactly what patch 6 does but, because AMD doesn't have hidden registers outside guest mode, I think restoring from the SMM state save area is more in line with the spirit of AMD's extensions. > Would this convince you to keep patch 6 as is? Saving and restoring > from the save state area eliminates a bit of internal state but it > adds complexity to other places (need to remember smbase before em_rsm > overwrites it, for example). It's also a bit faster to restore from > internal than from guest memory and it would match VMX. What if you instead reached the kvm_x86_ops post_leave_smm callback from rsm_load_state_64 (via a new emulator op)? Paolo
