linux-kernel - Re: [PATCH 2/2] KVM: selftests: Enable dirty_log

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190731084405.tn3x6nmt2svkjkiq@kamzik.brq.redhat.com>
Date:   Wed, 31 Jul 2019 10:44:05 +0200
From:   Andrew Jones <drjones@...hat.com>
To:     Thomas Huth <thuth@...hat.com>
Cc:     kvm@...r.kernel.org,
        Christian Borntraeger <borntraeger@...ibm.com>,
        Janosch Frank <frankja@...ux.ibm.com>,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org, David Hildenbrand <david@...hat.com>,
        Cornelia Huck <cohuck@...hat.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Shuah Khan <shuah@...nel.org>, Peter Xu <peterx@...hat.com>
Subject: Re: [PATCH 2/2] KVM: selftests: Enable dirty_log_test on s390x

On Wed, Jul 31, 2019 at 10:19:57AM +0200, Thomas Huth wrote:
> 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@...hat.com>
> >> ---
> [...]
> >> 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?

Tested it. It works on aarch64 too.

> 
> >> +/* 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.
>

OK

Thanks,
drew