On 30/07/2019 12.57, Andrew Jones wrote: > 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@xxxxxxxxxx> >> --- [...] >> 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. Ok. It seems to work on x86 - could you please check aarch64, since I don't have such a system available right now? >> +/* 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? It looked ok to me, but I can add some more tabs to even make it nicer :) >> @@ -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. It's really only needed on s390x, so I think we should keep the #ifdef here. Thomas
