Re: [PATCH 30/30] KVM: nSVM: implement KVM_GET_NESTED_STATE and KVM_SET_NESTED_STATE

[Paolo Bonzini]

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