Re: [PATCH] KVM: Fix some obsolete comments

[Vitaly Kuznetsov]

linmiaohe <linmiaohe@xxxxxxxxxx> writes:

> From: Miaohe Lin <linmiaohe@xxxxxxxxxx>
>
> Remove some obsolete comments, fix wrong function name and description.
>
> Signed-off-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
> ---
>  arch/x86/kvm/svm.c        | 3 ---
>  arch/x86/kvm/vmx/nested.c | 4 ++--
>  arch/x86/kvm/vmx/vmx.c    | 2 +-
>  3 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index fd3fc9fbefff..ee114a9913eb 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -3228,9 +3228,6 @@ static int nested_svm_exit_special(struct vcpu_svm *svm)
>  	return NESTED_EXIT_CONTINUE;
>  }
>  
> -/*
> - * If this function returns true, this #vmexit was already handled
> - */
>  static int nested_svm_intercept(struct vcpu_svm *svm)
>  {

Thank you for the cleanup, I looked at nested_svm_intercept() and I see
room for improvement, e.g. (completely untested!)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 76c24b3491f6..fcb26d64d3c7 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3280,42 +3280,36 @@ static int nested_svm_intercept(struct vcpu_svm *svm)
        case SVM_EXIT_IOIO:
                vmexit = nested_svm_intercept_ioio(svm);
                break;
-       case SVM_EXIT_READ_CR0 ... SVM_EXIT_WRITE_CR8: {
-               u32 bit = 1U << (exit_code - SVM_EXIT_READ_CR0);
-               if (svm->nested.intercept_cr & bit)
+       case SVM_EXIT_READ_CR0 ... SVM_EXIT_WRITE_CR8:
+               if (svm->nested.intercept_cr &
+                   BIT(exit_code - SVM_EXIT_READ_CR0))
                        vmexit = NESTED_EXIT_DONE;
                break;
-       }
-       case SVM_EXIT_READ_DR0 ... SVM_EXIT_WRITE_DR7: {
-               u32 bit = 1U << (exit_code - SVM_EXIT_READ_DR0);
-               if (svm->nested.intercept_dr & bit)
+       case SVM_EXIT_READ_DR0 ... SVM_EXIT_WRITE_DR7:
+               if (svm->nested.intercept_dr &
+                   BIT(exit_code - SVM_EXIT_READ_DR0))
                        vmexit = NESTED_EXIT_DONE;
                break;
-       }
-       case SVM_EXIT_EXCP_BASE ... SVM_EXIT_EXCP_BASE + 0x1f: {
-               u32 excp_bits = 1 << (exit_code - SVM_EXIT_EXCP_BASE);
-               if (svm->nested.intercept_exceptions & excp_bits) {
+       case SVM_EXIT_EXCP_BASE ... SVM_EXIT_EXCP_BASE + 0x1f:
+               if (svm->nested.intercept_exceptions &
+                   BIT(exit_code - SVM_EXIT_EXCP_BASE)) {
                        if (exit_code == SVM_EXIT_EXCP_BASE + DB_VECTOR)
                                vmexit = nested_svm_intercept_db(svm);
                        else
                                vmexit = NESTED_EXIT_DONE;
-               }
-               /* async page fault always cause vmexit */
-               else if ((exit_code == SVM_EXIT_EXCP_BASE + PF_VECTOR) &&
-                        svm->vcpu.arch.exception.nested_apf != 0)
+               } else if ((exit_code == SVM_EXIT_EXCP_BASE + PF_VECTOR) &&
+                          svm->vcpu.arch.exception.nested_apf != 0) {
+                       /* async page fault always cause vmexit */
                        vmexit = NESTED_EXIT_DONE;
+               }
                break;
-       }
-       case SVM_EXIT_ERR: {
+       case SVM_EXIT_ERR:
                vmexit = NESTED_EXIT_DONE;
                break;
-       }
-       default: {
-               u64 exit_bits = 1ULL << (exit_code - SVM_EXIT_INTR);
-               if (svm->nested.intercept & exit_bits)
+       default:
+               if (svm->nested.intercept & BIT_ULL(exit_code - SVM_EXIT_INTR))
                        vmexit = NESTED_EXIT_DONE;
        }
-       }
 
        return vmexit;
 }

Feel free to pick stuff you like and split your changes to this function
in a separate patch.

>  	u32 exit_code = svm->vmcb->control.exit_code;
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 0946122a8d3b..46c5f63136a8 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2960,7 +2960,7 @@ static int nested_vmx_check_vmentry_hw(struct kvm_vcpu *vcpu)
>  	/*
>  	 * Induce a consistency check VMExit by clearing bit 1 in GUEST_RFLAGS,
>  	 * which is reserved to '1' by hardware.  GUEST_RFLAGS is guaranteed to
> -	 * be written (by preparve_vmcs02()) before the "real" VMEnter, i.e.
> +	 * be written (by prepare_vmcs02()) before the "real" VMEnter, i.e.
>  	 * there is no need to preserve other bits or save/restore the field.
>  	 */
>  	vmcs_writel(GUEST_RFLAGS, 0);
> @@ -4382,7 +4382,7 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
>   * Decode the memory-address operand of a vmx instruction, as recorded on an
>   * exit caused by such an instruction (run by a guest hypervisor).
>   * On success, returns 0. When the operand is invalid, returns 1 and throws
> - * #UD or #GP.
> + * #UD, #GP or #SS.

Oxford comma, anyone? :-)))

>   */
>  int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification,
>  			u32 vmx_instruction_info, bool wr, int len, gva_t *ret)
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 69948aa1b127..8d91fa9acbb2 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -808,7 +808,7 @@ void update_exception_bitmap(struct kvm_vcpu *vcpu)
>  	if (to_vmx(vcpu)->rmode.vm86_active)
>  		eb = ~0;
>  	if (enable_ept)
> -		eb &= ~(1u << PF_VECTOR); /* bypass_guest_pf = 0 */
> +		eb &= ~(1u << PF_VECTOR);
>  
>  	/* When we are running a nested L2 guest and L1 specified for it a
>  	 * certain exception bitmap, we must trap the same exceptions and pass

All your changes look correct, so

Reviewed-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>

-- 
Vitaly