diff mbox series

selftests: kvm/set_memory_region_test: Fix race in move region test

Message ID 0fdddb94bb0e31b7da129a809a308d91c10c0b5e.1606941224.git.maciej.szmigiero@oracle.com (mailing list archive)
State Accepted
Commit 0c55f867f0c96dff93d4e0b5973975d65afb26d8
Headers show
Series selftests: kvm/set_memory_region_test: Fix race in move region test | expand

Commit Message

Maciej S. Szmigiero Dec. 2, 2020, 8:35 p.m. UTC
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(-)

Comments

Paolo Bonzini Dec. 3, 2020, 5:39 p.m. UTC | #1
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
diff mbox series

Patch

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);