Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     Andrei Vagin <avagin@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        Andrei Vagin <avagin@google.com>,
        Sean Christopherson <seanjc@google.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jianfeng Tan <henry.tjf@antfin.com>,
        Adin Scannell <ascannell@google.com>,
        Konstantin Bogomolov <bogomolov@google.com>,
        Etienne Perot <eperot@google.com>
Subject: Re: [PATCH 4/5] selftests/kvm/x86_64: set rax before vmcall
In-Reply-To: <20220722230241.1944655-5-avagin@google.com>
References: <20220722230241.1944655-1-avagin@google.com>
 <20220722230241.1944655-5-avagin@google.com>
Date:   Mon, 01 Aug 2022 13:32:24 +0200
Message-ID: <87y1w819o7.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Andrei Vagin <avagin@google.com> writes:

> kvm_hypercall has to place the hypercall number in rax.
>
> Trace events show that kvm_pv_test doesn't work properly:
>      kvm_pv_test-53132: kvm_hypercall: nr 0x0 a0 0x0 a1 0x0 a2 0x0 a3 0x0
>      kvm_pv_test-53132: kvm_hypercall: nr 0x0 a0 0x0 a1 0x0 a2 0x0 a3 0x0
>      kvm_pv_test-53132: kvm_hypercall: nr 0x0 a0 0x0 a1 0x0 a2 0x0 a3 0x0
>
> With this change, it starts working as expected:
>      kvm_pv_test-54285: kvm_hypercall: nr 0x5 a0 0x0 a1 0x0 a2 0x0 a3 0x0
>      kvm_pv_test-54285: kvm_hypercall: nr 0xa a0 0x0 a1 0x0 a2 0x0 a3 0x0
>      kvm_pv_test-54285: kvm_hypercall: nr 0xb a0 0x0 a1 0x0 a2 0x0 a3 0x0
>

Fixes: ac4a4d6de22e ("selftests: kvm: test enforcement of paravirtual cpuid features")

> Signed-off-by: Andrei Vagin <avagin@google.com>
> ---
>  tools/testing/selftests/kvm/lib/x86_64/processor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index ead7011ee8f6..5d85e1c021da 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -1422,7 +1422,7 @@ uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
>  
>  	asm volatile("vmcall"
>  		     : "=a"(r)
> -		     : "b"(a0), "c"(a1), "d"(a2), "S"(a3));
> +		     : "a"(nr), "b"(a0), "c"(a1), "d"(a2), "S"(a3));

Wouldn't '"+a"(r)' instead of '"=a"(r)' suffice (assuming we also assing
'r' to 'nr' in the beginning, something like

diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index ead7011ee8f6..fdd6554b94a1 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1418,10 +1418,10 @@ bool set_cpuid(struct kvm_cpuid2 *cpuid,
 uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
                       uint64_t a3)
 {
-       uint64_t r;
+       uint64_t r = nr;
 
        asm volatile("vmcall"
-                    : "=a"(r)
+                    : "+a"(r)
                     : "b"(a0), "c"(a1), "d"(a2), "S"(a3));
        return r;
 }

-- 
Vitaly