Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191008180808.14181-1-vkuznets@redhat.com>
In-Reply-To: <20191008180808.14181-1-vkuznets@redhat.com>
From:   Jim Mattson <jmattson@google.com>
Date:   Tue, 8 Oct 2019 11:29:32 -0700
Message-ID: <CALMp9eTqu2zff2g4pX-aR3P-AFNochKfJGZrA6zecE864FuwVQ@mail.gmail.com>
Subject: Re: [PATCH] selftests: kvm: fix sync_regs_test with newer gccs
To:     Vitaly Kuznetsov <vkuznets@redhat.com>
Cc:     kvm list <kvm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Oct 8, 2019 at 11:08 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> Commit 204c91eff798a ("KVM: selftests: do not blindly clobber registers in
>  guest asm") was intended to make test more gcc-proof, however, the result
> is exactly the opposite: on newer gccs (e.g. 8.2.1) the test breaks with
>
> ==== Test Assertion Failure ====
>   x86_64/sync_regs_test.c:168: run->s.regs.regs.rbx == 0xBAD1DEA + 1
>   pid=14170 tid=14170 - Invalid argument
>      1  0x00000000004015b3: main at sync_regs_test.c:166 (discriminator 6)
>      2  0x00007f413fb66412: ?? ??:0
>      3  0x000000000040191d: _start at ??:?
>   rbx sync regs value incorrect 0x1.
>
> Apparently, compile is still free to play games with registers even
> when they have variables attaches.
>
> Re-write guest code with 'asm volatile' by embedding ucall there and
> making sure rbx is preserved.
>
> Fixes: 204c91eff798a ("KVM: selftests: do not blindly clobber registers in guest asm")
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  .../selftests/kvm/x86_64/sync_regs_test.c     | 21 ++++++++++---------
>  1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
> index 11c2a70a7b87..5c8224256294 100644
> --- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
> @@ -22,18 +22,19 @@
>
>  #define VCPU_ID 5
>
> +#define UCALL_PIO_PORT ((uint16_t)0x1000)
> +
> +/*
> + * ucall is embedded here to protect against compiler reshuffling registers
> + * before calling a function. In this test we only need to get KVM_EXIT_IO
> + * vmexit and preserve RBX, no additional information is needed.
> + */
>  void guest_code(void)
>  {
> -       /*
> -        * use a callee-save register, otherwise the compiler
> -        * saves it around the call to GUEST_SYNC.
> -        */
> -       register u32 stage asm("rbx");
> -       for (;;) {
> -               GUEST_SYNC(0);
> -               stage++;
> -               asm volatile ("" : : "r" (stage));
> -       }
> +       asm volatile("1: in %[port], %%al\n"
> +                    "add $0x1, %%rbx\n"
> +                    "jmp 1b"
> +                    : : [port] "d" (UCALL_PIO_PORT) : "rax", "rbx");
>  }
A better solution might be something like:

register u32 stage = 0;
for (;;) {
        asm volatile("in %[port], %%al"
             :
             : "b" (stage), [port] "d" (UCALL_PIO_PORT)
             : "rax");
        stage++;
}

(Gmail no doubt has mangled the indentation. Sorry.)