Sorry I missed this.
On 02/06/20 02:11, Krish Sadhukhan wrote:
>>
>> +
>> + /* SMM temporarily disables SVM, so we cannot be in guest mode. */
>> + if (is_smm(vcpu) && (kvm_state->flags &
>> KVM_STATE_NESTED_GUEST_MODE))
>> + return -EINVAL;
>> +
>> + if (!(kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) {
>
>
> Should this be done up at the beginning of the function ? If this flag
> isn't set, we probably don't want to come this far.
So far we have only done consistency checks. These have to be done no
matter what (for example checking that GIF=1 if SVME=0).
>> + svm_leave_nested(svm);
>> + goto out_set_gif;
>> + }
>> +
>> + if (!page_address_valid(vcpu, kvm_state->hdr.svm.vmcb_pa))
>> + return -EINVAL;
>> + if (kvm_state->size < sizeof(*kvm_state) +
>> KVM_STATE_NESTED_SVM_VMCB_SIZE)
>> + return -EINVAL;
>> + if (copy_from_user(&ctl, &user_vmcb->control, sizeof(ctl)))
>> + return -EFAULT;
>> + if (copy_from_user(&save, &user_vmcb->save, sizeof(save)))
>> + return -EFAULT;
>> +
>> + if (!nested_vmcb_check_controls(&ctl))
>> + return -EINVAL;
>> +
>> + /*
>> + * Processor state contains L2 state. Check that it is
>> + * valid for guest mode (see nested_vmcb_checks).
>> + */
>> + cr0 = kvm_read_cr0(vcpu);
>> + if (((cr0 & X86_CR0_CD) == 0) && (cr0 & X86_CR0_NW))
>> + return -EINVAL;
>
>
> Does it make sense to create a wrapper for the CR0 checks ? We have
> these checks in nested_vmcb_check_controls() also.
Not in nested_vmcb_check_controls (rather nested_vmcb_checks as
mentioned in the comments).
If there are more checks it certainly makes sense to have them. Right
now however there are only two checks in svm_set_nested_state, and they
come from two different functions so I chose to duplicate them.
Paolo
