Re: [PATCH] iommufd/selftest: Fix dirty_bitmap tests

[Joao Martins]

On 16/11/2023 16:52, Robin Murphy wrote:
> The ASSERT_EQ() macro sneakily expands to two statements, so the loop
> here needs braces to ensure it captures both and actually terminates the
> test upon failure. 

Ugh

> Where these tests are currently failing on my arm64
> machine, this reduces the number of logged lines from a rather
> unreasonable ~197,000 down to 10. While we're at it, we can also clean
> up the tautologous "count" calculations whose assertions can never fail
> unless mathematics and/or the C language become fundamentally broken.
> 
> Fixes: a9af47e382a4 ("iommufd/selftest: Test IOMMU_HWPT_GET_DIRTY_BITMAP")
> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>

I was going to say that the second assert is useful, but we are already test the
number of bits we set against what the mock domain set after
mock_domain_set_dirty(). So the second is redundantly testing the same, and can
be removed as you are doing. Thanks for fixing this.

I would suggest the subject to:

	iommufd/selftest: Fix _test_mock_dirty_bitmaps()

Because dirty-bitmap tests seems to imply the whole fixture, which covers more
than the bitmaps.

> ---
>  tools/testing/selftests/iommu/iommufd_utils.h | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h
> index 050e9751321c..ad9202335656 100644
> --- a/tools/testing/selftests/iommu/iommufd_utils.h
> +++ b/tools/testing/selftests/iommu/iommufd_utils.h
> @@ -293,15 +293,13 @@ static int _test_mock_dirty_bitmaps(int fd, __u32 hwpt_id, size_t length,
>  				    __u64 bitmap_size, __u32 flags,
>  				    struct __test_metadata *_metadata)
>  {
> -	unsigned long i, count, nbits = bitmap_size * BITS_PER_BYTE;
> +	unsigned long i, nbits = bitmap_size * BITS_PER_BYTE;
>  	unsigned long nr = nbits / 2;
>  	__u64 out_dirty = 0;
>  
>  	/* Mark all even bits as dirty in the mock domain */
> -	for (count = 0, i = 0; i < nbits; count += !(i % 2), i++)
> -		if (!(i % 2))
> -			set_bit(i, (unsigned long *)bitmap);
> -	ASSERT_EQ(nr, count);
> +	for (i = 0; i < nbits; i += 2)
> +		set_bit(i, (unsigned long *)bitmap);
>  
>  	test_cmd_mock_domain_set_dirty(fd, hwpt_id, length, iova, page_size,
>  				       bitmap, &out_dirty);
> @@ -311,9 +309,10 @@ static int _test_mock_dirty_bitmaps(int fd, __u32 hwpt_id, size_t length,
>  	memset(bitmap, 0, bitmap_size);
>  	test_cmd_get_dirty_bitmap(fd, hwpt_id, length, iova, page_size, bitmap,
>  				  flags);
> -	for (count = 0, i = 0; i < nbits; count += !(i % 2), i++)
> +	/* Beware ASSERT_EQ() is two statements -- braces are not redundant! */
> +	for (i = 0; i < nbits; i++) {
>  		ASSERT_EQ(!(i % 2), test_bit(i, (unsigned long *)bitmap));
> -	ASSERT_EQ(count, out_dirty);
> +	}
>  
>  	memset(bitmap, 0, bitmap_size);
>  	test_cmd_get_dirty_bitmap(fd, hwpt_id, length, iova, page_size, bitmap,

Reviewed-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
Tested-by: Joao Martins <joao.m.martins@xxxxxxxxxx>