Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] selftests: kvm/set_memory_region_test: Fix race in move
 region test
To:     "Maciej S. Szmigiero" <mail@maciej.szmigiero.name>,
        Shuah Khan <shuah@kernel.org>
Cc:     Sean Christopherson <seanjc@google.com>, kvm@vger.kernel.org,
        linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
References: <0fdddb94bb0e31b7da129a809a308d91c10c0b5e.1606941224.git.maciej.szmigiero@oracle.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <6e0f1fcf-c8ac-c05a-778b-eeb7a4cd50e7@redhat.com>
Date:   Thu, 3 Dec 2020 18:39:11 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.0
MIME-Version: 1.0
In-Reply-To: <0fdddb94bb0e31b7da129a809a308d91c10c0b5e.1606941224.git.maciej.szmigiero@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 02/12/20 21:35, Maciej S. Szmigiero wrote:
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> 
> The current memory region move test correctly handles the situation that
> the second (realigning) memslot move operation would temporarily trigger
> MMIO until it completes, however it does not handle the case in which the
> first (misaligning) move operation does this, too.
> This results in false test assertions in case it does so.
> 
> Fix this by handling temporary MMIO from the first memslot move operation
> in the test guest code, too.
> 
> Fixes: 8a0639fe9201 ("KVM: sefltests: Add explicit synchronization to move mem region test")
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> ---
>      The race is pretty hard to trigger on the current KVM memslot code,
>      to trigger it reliably an extra delay in memslot move op is needed:
>      --- a/virt/kvm/kvm_main.c
>      +++ b/virt/kvm/kvm_main.c
>      @@ -1173,7 +1173,7 @@ static struct kvm_memslots *kvm_dup_memslots(struct kvm_memslots *old,
>      
>              return slots;
>       }
>      -
>      +#include <linux/delay.h>
>       static int kvm_set_memslot(struct kvm *kvm,
>                                 const struct kvm_userspace_memory_region *mem,
>                                 struct kvm_memory_slot *old,
>      @@ -1212,6 +1212,8 @@ static int kvm_set_memslot(struct kvm *kvm,
>                       *      - kvm_is_visible_gfn (mmu_check_root)
>                       */
>                      kvm_arch_flush_shadow_memslot(kvm, slot);
>      +
>      +               if (change == KVM_MR_MOVE) mdelay(100);
>              }
>      
>              r = kvm_arch_prepare_memory_region(kvm, new, mem, change);
> 
>   .../selftests/kvm/set_memory_region_test.c      | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
> index b3ece55a2da6..6f441dd9f33c 100644
> --- a/tools/testing/selftests/kvm/set_memory_region_test.c
> +++ b/tools/testing/selftests/kvm/set_memory_region_test.c
> @@ -156,14 +156,23 @@ static void guest_code_move_memory_region(void)
>   	GUEST_SYNC(0);
>   
>   	/*
> -	 * Spin until the memory region is moved to a misaligned address.  This
> -	 * may or may not trigger MMIO, as the window where the memslot is
> -	 * invalid is quite small.
> +	 * Spin until the memory region starts getting moved to a
> +	 * misaligned address.
> +	 * Every region move may or may not trigger MMIO, as the
> +	 * window where the memslot is invalid is usually quite small.
>   	 */
>   	val = guest_spin_on_val(0);
>   	GUEST_ASSERT_1(val == 1 || val == MMIO_VAL, val);
>   
> -	/* Spin until the memory region is realigned. */
> +	/* Spin until the misaligning memory region move completes. */
> +	val = guest_spin_on_val(MMIO_VAL);
> +	GUEST_ASSERT_1(val == 1 || val == 0, val);
> +
> +	/* Spin until the memory region starts to get re-aligned. */
> +	val = guest_spin_on_val(0);
> +	GUEST_ASSERT_1(val == 1 || val == MMIO_VAL, val);
> +
> +	/* Spin until the re-aligning memory region move completes. */
>   	val = guest_spin_on_val(MMIO_VAL);
>   	GUEST_ASSERT_1(val == 1, val);
>   
> 

Queued, thanks.

paolo