On Tue, Mar 26, 2019 at 12:08 AM Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote:
>
> Paolo Bonzini <pbonzini@xxxxxxxxxx> writes:
>
> > On 25/03/19 18:30, Vitaly Kuznetsov wrote:
> >> static int handle_set_cr4(struct kvm_vcpu *vcpu, unsigned long val)
> >> {
> >> + /* VMXE can't be enabled from SMM */
> >> + if (val & X86_CR4_VMXE && is_smm(vcpu))
> >> + return 1;
> >> +
> >> if (is_guest_mode(vcpu)) {
> >> struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
> >> unsigned long orig_val = val;
> >>
> >>
> >> Paolo, as the author of the original commit, what do you think? Would
> >> this protection be enough or do you envision any other scenarios?
> >
> > It would still be possible to set it via KVM_SET_SREGS I think, so you'd
> > need a similar check in kvm_valid_sregs; but it's ugly to put it there
> > since it's VMX specific.
>
> True;
>
> >
> > I like Liran's idea, instead.
>
> (Honestly, I slightly dislike the 'drop HF_SMM_MASK, call set_cr4(),
> restore HF_SMM_MASK()' approach because this looks like an implicit
> parameter to that function and requires readers of this code to use
> stack in their brains. But an alternative approach to add an explicit
> 'from_rsm' parameter is also ugly. Spreading this VMX specifics to
> non-vmx code, however, is probably the worst).
>
> Thank you both for your comments, v1 is coming!
>
> --
> Vitaly
I have to wonder why the kvm implementation of SMI/RSM is *so*
different from the architected behavior in the SDM. Regarding the VMXE
bit in particular, the SDM says that SMI is handled as follows...
enter SMM;
save the following internal to the processor:
CR4.VMXE
an indication of whether the logical processor was in VMX operation
(root or non-root)
...
CR4.VMXE ← 0;
perform ordinary SMI delivery:
save processor state in SMRAM;
It looks like vmx->nested.smm.guest_mode is the "indication of whether
the logical processor was in VMX operation (root or non-root)," but I
don't see where the VMXE bit is saved.
According to the SDM, VMXE on RSM is handled as follows:
IF VMXE = 1 in CR4 image in SMRAM
THEN fail and enter shutdown state;
ELSE
restore state normally from SMRAM;
...
FI;
CR4.VMXE ← value stored internally;
