On Wed, Sep 20, 2017 at 10:13 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> On 20/09/2017 09:53, Paolo Bonzini wrote:
>> On 19/09/2017 17:37, Ladi Prosek wrote:
>>> + if (nested_cpu_has_ept(vmcs12)) {
>>> + /*
>>> + * 34.14.1 Default Treatment of SMI Delivery
>>> + * Bit 0 of the 32-bit field at offset SMBASE + 8000H + 7EE0H
>>> + * indicates whether the cpu was in VMX non-root operation with
>>> + * EPT enabled.
>>> + * The 64-bit field at offset SMBASE + 8000H + 7ED8H holds the
>>> + * value of the EPT pointer.
>>> + */
>>> + put_smstate(u32, smstate, 0x7ee0,
>>> + get_smstate(u32, smstate, 0x7ee0) | 1);
>>
>> This is zero, so it should be enough to just write 1 here.
>>
>> But, it is not clear to me why this is needed. Since we use the AMD
>> format for the SMM state save area anyway, I'm inclined to omit it...
>
> To clarify more what I mean, my guess is that the EPT state is meant for
> SMI state save handlers that want to emulate e.g. REP INS or REP OUTS,
> and therefore need to walk the caller's page tables. We do not have
> such magic SMI state save handlers because we don't use SMM for crazy
> things such as emulating the 8042 on top of a USB keyboard. Therefore,
> since we document our SMM state save map version as the AMD format, we
> can omit this information.
Sure, this was just for completeness since I noticed it in the manual.
Can be omitted.
> I probably should also clarify my different opinion on vmx->nested.smm
> versus svm->nested.smm. AMD lets you use SVM in system management mode,
> without all the complication of dual-monitor treatment and the like.
> They do it simply by storing the SVM state (which is not much) in the
> state save map.
>
> On the other hand, Intel's VMX state (VMXON, VMXON area, current VMCS
> pointer) is inaccessible to SMM under the default treatment of SMIs.
> That's because CR4.VMXE is reserved for SMM under default treatment (KVM
> currenty does not implement this and it should be changed---not
> necessarily by you, though I won't complain if it is part of v3 :)).
Will do :)
> 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!
