Re: [PATCH v2 5/5] KVM: selftests: List the known IOMMU modes in irq_test's help output

[David Matlack]

On 2026-11-04 06:53 AM, Sean Christopherson wrote:
> On Tue, Nov 03, 2026, David Matlack wrote:
> > @@ -159,7 +159,8 @@ static void help(const char *name)
> 
> If you're going to do s/type/mode below, then the initial help lines should also
> use iommu_mode.

Will do. I'll also rename the iommu_type variable in main() to
iommu_mode.

> > -	printf("-t	Override the IOMMU type to use (vfio_type1_iommu or iommufd)\n");
> > +	printf("-t	Override the IOMMU mode to use instead of auto-detecting it (%s)\n",
> 
> IMO, the "auto-detecting" blurb does more harm than good.  It makes the code harder
> to read, and for me at least it's just extra noise in understanding what the '-t'
> option does.  It also makes the line reaaaaaaally long.
> 
> I know the parentheses are pre-existing, but that was somewhat ok when there were
> only two options.  With a big list, it's again noisy to reach the end and have to
> think about that the closing ')' is terminating.

Agreed that the auto-detect blurb and the parentheses are just noise.
I went with a slight variation, see below.

> This really should spit out only the available modes.  For the internal assert,
> it makes sense to print all modes since that's just doing a lookup, but it's
> annoying to have to manually probe to see what's available.
> 
> Juggling all vs. available is mildly tricky to avoid having to dynamically
> allocate, but not that hard in the end.  And IMO, it's totally ok to print a
> misleading message if a mode becomes (un)available mid-test, especially since
> this helper is unlikely to be used except in terminal situations.
> 
> E.g.
> 
> static const char *__iommu_mode_names(bool include_unavailable)
> {
> 	static char all_names[256];
> 	static char avl_names[sizeof(all_names)];
> 	char *names = include_unavailable ? all_names : avl_names;
> 	int i, len = 0;
> 
> 	if (names[0])
> 		return names;
> 
> 	for (i = 0; i < ARRAY_SIZE(iommu_modes); i++) {
> 		if (!include_unavailable && !iommu_mode_probe(&iommu_modes[i]))
> 			continue;
> 
> 		len += snprintf(names + len, sizeof(all_names) - len, "%s%s",
> 				names[0] ? ", " : "", iommu_modes[i].name);
> 		VFIO_ASSERT_LT(len, (int)sizeof(all_names));
> 	}
> 
> 	if (!names[0])
> 		return NO_IOMMU_MODE_AVAILABLE_MSG;
> 
> 	return names;
> }
> 
> const char *iommu_mode_names(void)
> {
> 	return __iommu_mode_names(false);
> }

How about listing both? That keeps -h consistent with how other KVM
selftests list the options for their arguments (e.g. backing_src_help()
lists every backing src type), while also showing which modes can
actually be used. e.g. on a system with only the legacy VFIO container:

  # ./irq_test -h
  ...
  -t	Override the IOMMU mode to use
  	Options: iommufd, vfio_type1v2_iommu, vfio_type1_iommu, iommufd_compat_type1v2, iommufd_compat_type1
  	Available: vfio_type1v2_iommu, vfio_type1_iommu

The available modes also get printed when the requested mode is not
available, i.e. when the __TEST_REQUIRE() fails:

  # ./irq_test -d 0000:01:00.0 -t iommufd
  1..0 # SKIP - IOMMU mode 'iommufd' is not available.  Available modes: vfio_type1v2_iommu, vfio_type1_iommu.

libvfio exposes iommu_mode_names_all() and iommu_mode_names_available()
for this, built on your helper for juggling all vs. available.

> Coupled with the above, how about this:
> 
> 	VFIO_FAIL("Unrecognized IOMMU mode '%s'.  Known modes: %s.\n",
> 		  iommu_mode, __iommu_mode_names(true));

Will do.

Thanks!