Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <72f0e75b-6c91-4c17-beb2-3f198ed05cd0@oracle.com>
Date:   Thu, 16 Nov 2023 17:28:55 +0000
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] iommufd/selftest: Fix dirty_bitmap tests
Content-Language: en-US
To:     Robin Murphy <robin.murphy@arm.com>, kevin.tian@intel.com,
        jgg@ziepe.ca
Cc:     iommu@lists.linux.dev, linux-kernel@vger.kernel.org
References: <90e083045243ef407dd592bb1deec89cd1f4ddf2.1700153535.git.robin.murphy@arm.com>
From:   Joao Martins <joao.m.martins@oracle.com>
In-Reply-To: <90e083045243ef407dd592bb1deec89cd1f4ddf2.1700153535.git.robin.murphy@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?c0hRWHloR01NRjU1SFVORHV6R1RRb0pjMGx2RGRCQ0ZHM2I3TFUrUHpLM2lx?=
 =?utf-8?B?cWc2VENUeWI3V0hlUjJqNk5RTGtYVG5jRkFHdHhtbnhHbW90aDEzbnlFZXZ4?=
 =?utf-8?B?WDNodkVkbnpJKzdhTlJCZmlmcjFwTWR2a2JkNUlqOVJQRVc1anpZTXIyU0JJ?=
 =?utf-8?B?NEo0ekV0WDlCOEQyTTk1VnQ5c0RBRWVrNXhvMU5GclFxT24yRitDMzBDVVhy?=
 =?utf-8?B?VDVWTnFnN296NVVUbHBzYUdWUUFUcXZrT0ZOZGphQno4eS9USnBoS1ZJMFRQ?=
 =?utf-8?B?d0IvWlFocmIyM1RWNkxkN3QxZ3VkSVdzUysrTElCeXdiYnBjQWhCRWo0cGVn?=
 =?utf-8?B?WENxRzcwRTF0Vi9SaFlycS9hcmNrUmFnc3Ruamk0Qm5rV3VyaFIxc3c5VER5?=
 =?utf-8?B?TGJydjk2ZjVpMjcySjRaalQvMElyK0VQelpFbkJyNi91Wm5tVFhBTDRHY3JN?=
 =?utf-8?B?UnY2Y0liYVJsVW9zQWE2VzY0bjlGdlliTG1vNDlkY2Y4OFNMVmpTUVpJSlh3?=
 =?utf-8?B?TTQyemZZYmlGcHdQZ25yTFRIK3FPdGIzU2ZZSVl1Q3VMUFY0NmIrVWdvSkNy?=
 =?utf-8?B?L0h6K0pYT0lJUzE4M1ZQN296K01rYUVzelVPYzF0TmVzQWlYRWRnTFl6RHFE?=
 =?utf-8?B?aWxoZks5d3R4dFI3MlhGdEZXeHpMY1hVdDl0WWw1TWJHTnpwazhnTWtHdkVR?=
 =?utf-8?B?VlpyV21nekM1QVVKeVk3TEVMZEJrRjY4eXJLT0Q1SEZ5cFhIVWk5dFdQSVgw?=
 =?utf-8?B?aTEzekk0a0FjY0ZXellMM0RYaTdPNTJCdEw4TEhiU0JKN0d2TmMzVDEyam4x?=
 =?utf-8?B?V3NBdk5NUitVQy83Tzh5SndIL2djMWl1N3ZNSDkrK2VJdlVNd3p5ejI4TDdL?=
 =?utf-8?B?bmhiSHFieVVzczYxTEd1b2QrWm9LSkRaRUI1NFY5eHU1ekRiZXZ5QTVvQThu?=
 =?utf-8?B?ajN3YTM1ZnBPVnRDZWk2c2NpemFOWHdiSmxSU0FFZStaMGFDV2FSazNpemtr?=
 =?utf-8?B?Tm5mREc2UG14bTRJcW9IS09GNkpaVlFiUmR0SzNody9WZEVlVUZOMUZVMEtF?=
 =?utf-8?B?cnJST2V5bkhRRm5mZTBKOGZaMEppNDBRUCt2MHAySm80M2JxSmJsb1M5MHJa?=
 =?utf-8?B?QkgzaEV4M3duQXZROHFtS3dGVU5tV2hlYStQRWNrdHJDMHhhUXNmZm5tRGNZ?=
 =?utf-8?B?cWRVNkdHT2ppdFZOR20wWkNlNkIzQjBmY205Vkd6NmlYOGJ4YXhMWVljd2M4?=
 =?utf-8?B?bi9GMXpYcUYzaTE1RUFhMjBvWjAxY3lYblhrVDJCdUI2SWprR21VWm4vNmww?=
 =?utf-8?B?azNXTGV3Mkx2RVdmL0lub21hUUtKekJlSXk5R1h5MFVzMXQvY0I5WjZYZjVI?=
 =?utf-8?B?MFFGSHVIMFVWSFJRV012bTZ1NzgrM2JNYW9lZ244ajhRaUFld2oyYW1EUUpk?=
 =?utf-8?B?M0NzZ2tHWkpWb2ZpQnB1eVFKUnVjZTBpWER5RVVFenNMenlIVWZGQ2huRWx3?=
 =?utf-8?B?TThKL1Yxbk5UNW5TS0VXYTlIam5FaHcxUnRzNGdsSHk4VWNNd0pnUkJNWGxV?=
 =?utf-8?B?RkhEck9YcStLZFRocUtOTzBwcndTNFVvSk9WNVEzMnQ2ZTI3WS9KWUZjaVp2?=
 =?utf-8?B?OE1TUzZmMURYaVRmSHBDV25mLzhIZlIyb2N5OHhoMjRUT1Y2ajVTUWFySGx2?=
 =?utf-8?B?RDMzZlpjdm43RHE2SFBYdGNaRWNQSk5pUTZFa2F4bGdSM1hrY2lsdWZnZmFC?=
 =?utf-8?B?Y29Vb1Z4YUtDK2JXUXBDMzhwYy84a3ZwdndDaldvc0hxc011bHBxTzdzb3VW?=
 =?utf-8?B?NVRlSml6NEZZZHozenF0bERHdFZwZU1iTzFxUFQwZWpWd2JrcU9HeCtZNVBq?=
 =?utf-8?B?c2x3K2VmRDVucG50S2QxWEJjNDVxR01zMWlXMCtTME1iZVVVRGk5T1JWM1JL?=
 =?utf-8?B?ZGZyWCtmMDBhVXJGTk9jc2RaY29sbUwyY20yR0FvRmxNWWlLK054ZGJzOXh5?=
 =?utf-8?B?aHh5aFNRUUZwcTlyZk4wU0lDMnhUalFoVXgwUktQTndCNG1XTVRrdzUwRWRD?=
 =?utf-8?B?MkFTZUNCcS9TaFdaOVBlR3JvUGFxTHZWTHJyNnRwRzRXM1l0bnZ1Rys5c1Zq?=
 =?utf-8?B?TS9kL04rMDBnL3V6NURQd0VvSjN5MXFVdjIyREtYQ0daSWY3YXB4UURxVVVQ?=
 =?utf-8?B?TUE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: kjDtF6afdW29tQkPkyIQfoVsw6YJ03uJ9ZsE/YheVYxI9S1zHCLaS/Cu+RAFRe/QEuWnmHvxqn9B5VnW/wf3KK8vKibN4NtGO2zeP8LfyvdaIQdKLh/mnw61YVBt89/a8p4c6+aktaE+thxN0Tu5FOU7KJRCtdbNCNWi2H36j24iu8QDmSxfChkT2KT+ALmRaIB/2CZuWhYD6WStIQdCRU+RU6ZG9A9rZj6Zw1SZI+29qz85ac23EAt051ynBzMw5hK2e5kiXuXm/9lch0dKT95Q3z+ww7Ozhml9cBqLjgZSxRga5ldzfEOzkJ9eClc4Q+t2kSTCuhAuPObP72fcYM8PlRIZqF61QUv1KhOoq+29EqZdGKe63QmnEPn9ofjrMe00QyGDKTHV3rK1NClBXEyr92fVIWZyp4wMSPu8vY5+HxCSBdI0feKxYA6BJOF6DKNDoD5f5yfMO2tBgXH4bvipP8xgK27D7lUwmbmk7cxm/TNaU+u1C7FyGlO+AShy5IlhD+ICAJNHSWXigOtPmr1RXk6kD/wh2pkZYeeJtxv1TuvQmBCyI0G+I1u6AjGyjEb1/6VjnTS+AYNo7gJh43nJA174NtInB54zh2jJ8US+ZU0hrVnehMHIp9wJkaX++odMkbZ2XnAIYsvnKZsMsnJBXZopLrVhO0w1gthvHVk1ZNhYcYDIDmhATg+Ioov1Qa8M8cDuificaloG9ds5ef2kw6mnz3l5AbF+F541xezb4zU2RtsSIIFUaAOtDPLteSHpLOXecf2VwmWxyVHsTBTDji4OmARZm09uGgA+KD2bbIsejGy57CrPsvbeVvaGGrwxl+uRVf2Ka6CjAfr+KsNtQglEtR85xrsm4mwfWVlSzA6JYsIU8/3vUJ449Gp1
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9d341134-163e-4659-9042-08dbe6c98b43
X-MS-Exchange-CrossTenant-AuthSource: BLAPR10MB4835.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Nov 2023 17:29:10.3411
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: b6BvfmktkKQWbLmUV0mqbKumNH3BpTm4x86+uskDGFVVcwrvv6M3qYfLpS0hynye/OxH7OVX3by2GX8Cj32+9Y0YfHBYbYNwdUiOOkaSo8I=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR10MB6558
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.987,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-11-16_18,2023-11-16_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 adultscore=0
 mlxlogscore=999 mlxscore=0 malwarescore=0 phishscore=0 suspectscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311060000 definitions=main-2311160137
X-Proofpoint-ORIG-GUID: TrHYNqkk7EnuIrusEUss8GVu2XFSdSd3
X-Proofpoint-GUID: TrHYNqkk7EnuIrusEUss8GVu2XFSdSd3
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Thu, 16 Nov 2023 09:29:48 -0800 (PST)

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@arm.com>

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@oracle.com>
Tested-by: Joao Martins <joao.m.martins@oracle.com>