Re: [PATCH 2/2] KVM: selftests: Enable dirty_log_test on s390x

[Andrew Jones <drjones@redhat.com>]

On Tue, Jul 30, 2019 at 12:01:12PM +0200, Thomas Huth wrote:
> To run the dirty_log_test on s390x, we have to make sure that we
> access the dirty log bitmap with little endian byte ordering and
> we have to properly align the memslot of the guest.
> Also all dirty bits of a segment are set once on s390x when one
> of the pages of a segment are written to for the first time, so
> we have to make sure that we touch all pages during the first
> iteration to keep the test in sync here.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tools/testing/selftests/kvm/Makefile         |  1 +
>  tools/testing/selftests/kvm/dirty_log_test.c | 70 ++++++++++++++++++--
>  2 files changed, 66 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index ba7849751989..ac7e63e00fee 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -33,6 +33,7 @@ TEST_GEN_PROGS_aarch64 += dirty_log_test
>  TEST_GEN_PROGS_aarch64 += kvm_create_max_vcpus
>  
>  TEST_GEN_PROGS_s390x += s390x/sync_regs_test
> +TEST_GEN_PROGS_s390x += dirty_log_test
>  TEST_GEN_PROGS_s390x += kvm_create_max_vcpus
>  
>  TEST_GEN_PROGS += $(TEST_GEN_PROGS_$(UNAME_M))
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index ceb52b952637..7a1223ad0ff3 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -26,9 +26,22 @@
>  /* The memory slot index to track dirty pages */
>  #define TEST_MEM_SLOT_INDEX		1
>  
> +#ifdef __s390x__
> +
> +/*
> + * On s390x, the ELF program is sometimes linked at 0x80000000, so we can
> + * not use 0x40000000 here without overlapping into that region. Thus let's
> + * use 0xc0000000 as base address there instead.
> + */
> +#define DEFAULT_GUEST_TEST_MEM		0xc0000000

I think both x86 and aarch64 should be ok with this offset. If testing
proves it does, then we can just change it for all architecture.

> +
> +#else
> +
>  /* Default guest test memory offset, 1G */
>  #define DEFAULT_GUEST_TEST_MEM		0x40000000
>  
> +#endif
> +
>  /* How many pages to dirty for each guest loop */
>  #define TEST_PAGES_PER_LOOP		1024
>  
> @@ -38,6 +51,27 @@
>  /* Interval for each host loop (ms) */
>  #define TEST_HOST_LOOP_INTERVAL		10UL
>  
> +/* Dirty bitmaps are always little endian, so we need to swap on big endian */
> +#if defined(__s390x__)
> +# define BITOP_LE_SWIZZLE	((BITS_PER_LONG-1) & ~0x7)
> +# define test_bit_le(nr, addr) \
> +	test_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
> +# define set_bit_le(nr, addr) \
> +	set_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
> +# define clear_bit_le(nr, addr) \
> +	clear_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
> +# define test_and_set_bit_le(nr, addr) \
> +	test_and_set_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
> +# define test_and_clear_bit_le(nr, addr) \
> +	test_and_clear_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
> +#else
> +# define test_bit_le	test_bit
> +# define set_bit_le	set_bit
> +# define clear_bit_le	clear_bit
> +# define test_and_set_bit_le	test_and_set_bit
> +# define test_and_clear_bit_le	test_and_clear_bit
> +#endif

nit: does the formatting above look right after applying the patch?

> +
>  /*
>   * Guest/Host shared variables. Ensure addr_gva2hva() and/or
>   * sync_global_to/from_guest() are used when accessing from
> @@ -69,11 +103,25 @@ static uint64_t guest_test_virt_mem = DEFAULT_GUEST_TEST_MEM;
>   */
>  static void guest_code(void)
>  {
> +	uint64_t addr;
>  	int i;
>  
> +#ifdef __s390x__
> +	/*
> +	 * On s390x, all pages of a 1M segment are initially marked as dirty
> +	 * when a page of the segment is written to for the very first time.
> +	 * To compensate this specialty in this test, we need to touch all
> +	 * pages during the first iteration.
> +	 */
> +	for (i = 0; i < guest_num_pages; i++) {
> +		addr = guest_test_virt_mem + i * guest_page_size;
> +		*(uint64_t *)addr = READ_ONCE(iteration);
> +	}
> +#endif
> +
>  	while (true) {
>  		for (i = 0; i < TEST_PAGES_PER_LOOP; i++) {
> -			uint64_t addr = guest_test_virt_mem;
> +			addr = guest_test_virt_mem;
>  			addr += (READ_ONCE(random_array[i]) % guest_num_pages)
>  				* guest_page_size;
>  			addr &= ~(host_page_size - 1);
> @@ -158,15 +206,15 @@ static void vm_dirty_log_verify(unsigned long *bmap)
>  		value_ptr = host_test_mem + page * host_page_size;
>  
>  		/* If this is a special page that we were tracking... */
> -		if (test_and_clear_bit(page, host_bmap_track)) {
> +		if (test_and_clear_bit_le(page, host_bmap_track)) {
>  			host_track_next_count++;
> -			TEST_ASSERT(test_bit(page, bmap),
> +			TEST_ASSERT(test_bit_le(page, bmap),
>  				    "Page %"PRIu64" should have its dirty bit "
>  				    "set in this iteration but it is missing",
>  				    page);
>  		}
>  
> -		if (test_bit(page, bmap)) {
> +		if (test_bit_le(page, bmap)) {
>  			host_dirty_count++;
>  			/*
>  			 * If the bit is set, the value written onto
> @@ -209,7 +257,7 @@ static void vm_dirty_log_verify(unsigned long *bmap)
>  				 * should report its dirtyness in the
>  				 * next run
>  				 */
> -				set_bit(page, host_bmap_track);
> +				set_bit_le(page, host_bmap_track);
>  			}
>  		}
>  	}
> @@ -293,6 +341,10 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
>  	 * case where the size is not aligned to 64 pages.
>  	 */
>  	guest_num_pages = (1ul << (30 - guest_page_shift)) + 16;
> +#ifdef __s390x__
> +	/* Round up to multiple of 1M (segment size) */
> +	guest_num_pages = (guest_num_pages + 0xff) & ~0xffUL;

We could maybe do this for all architectures as well.

> +#endif
>  	host_page_size = getpagesize();
>  	host_num_pages = (guest_num_pages * guest_page_size) / host_page_size +
>  			 !!((guest_num_pages * guest_page_size) % host_page_size);
> @@ -304,6 +356,11 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
>  		guest_test_phys_mem = phys_offset;
>  	}
>  
> +#ifdef __s390x__
> +	/* Align to 1M (segment size) */
> +	guest_test_phys_mem &= ~((1 << 20) - 1);

and this

> +#endif
> +
>  	DEBUG("guest physical test memory offset: 0x%lx\n", guest_test_phys_mem);
>  
>  	bmap = bitmap_alloc(host_num_pages);
> @@ -454,6 +511,9 @@ int main(int argc, char *argv[])
>  		vm_guest_mode_params_init(VM_MODE_P48V48_64K, true, true);
>  	}
>  #endif
> +#ifdef __s390x__
> +	vm_guest_mode_params_init(VM_MODE_P40V48_4K, true, true);
> +#endif
>  
>  	while ((opt = getopt(argc, argv, "hi:I:p:m:")) != -1) {
>  		switch (opt) {
> -- 
> 2.21.0
>

Thanks,
drew