Re: [PATCH 1/2] vfio: selftests: Add support of creating multiple iommus from iommufd

[Samiullah Khawaja <skhawaja@google.com>]

On Mon, May 11, 2026 at 08:59:53PM +0000, David Matlack wrote:
>On 2026-05-11 08:21 PM, Samiullah Khawaja wrote:
>> On Fri, May 08, 2026 at 06:17:19PM +0000, David Matlack wrote:
>> > On 2026-05-05 10:14 PM, Samiullah Khawaja wrote:
>
>> > @@ -478,11 +483,7 @@ struct iommu *iommufd_iommu_init(int iommufd, u32 dev_id, u32 flags)
>> >        struct iommu *iommu;
>> >
>> >        iommu = iommu_alloc("iommufd");
>> > -
>> > -       iommu->iommufd = dup(iommufd);
>> > -       VFIO_ASSERT_GT(iommu->iommufd, 0);
>> > -
>> > -       iommu->ioas_id = iommufd_ioas_alloc(iommu->iommufd);
>> > +       iommufd_init(iommu, dup(iommufd));
>> >
>> >        if (flags & IOMMUFD_IOMMU_INIT_CREATE_PT)
>> >                iommu->hwpt_id = iommufd_hwpt_alloc(iommu, dev_id);
>> >
>> > > +
>> > > +	if (flags & IOMMUFD_IOMMU_INIT_CREATE_PT)
>> > > +		iommu->hwpt_id = iommufd_hwpt_alloc(iommu, dev_id);
>> >
>> > Does this need to be part of iommufd_iommu_init()? Maybe it would be
>> > better to expose a separate helper to allocate a HWPT for a given struct
>> > iommu *.
>>
>> Hmm.. that is interesting.. I think we can have following immediate
>> possibilities when creating a struct iommu (there will be more down the
>> road, but can be handled in similar way):
>>
>> - create using struct iommu with a new IOAS.
>> - create using struct iommu with existing IOAS but new HWPT.
>> - create using struct iommu with new IOAS and new HWPT.
>>
>> I think I should probably add following flags:
>>
>> IOMMUFD_IOMMU_INIT_CREATE_IOPT (IO Page Table) or _PT
>> IOMMUFD_IOMMU_INIT_CREATE_IOAS (IO address Space) or _AS
>>
>> At least one of those should be required when creating new struct iommu
>> from an existing one. WDYT?
>
>I don't love the idea of passing in flags to control the logic,
>especially since not every combination of flags is valid. And also tests
>would be required to pass in dev_id even if it's not needed (first
>possibility in your list).

Agreed.
>
>Maybe add explicit helpers for each case?
>
>/*** static (private) helpers ***/
>
>static u32 iommufd_hwpt_alloc(struct iommu *iommu, u32 dev_id)
>{
>	struct iommu_hwpt_alloc args = {
>		.size = sizeof(args),
>		.pt_id = iommu->ioas_id,
>		.dev_id = dev_id,
>	};
>
>	VFIO_ASSERT_EQ(iommu->hwpt_id, 0);
>	ioctl_assert(iommu->iommufd, IOMMU_HWPT_ALLOC, &args);
>
>	iommu->hwpt_id = args.out_hwpt_id;
>}
>
>static struct iommu *iommufd_new(int iommufd, u32 ioas_id)
>{
>	struct iommu *new;
>
>	new = iommu_alloc("iommufd");
>
>	new->iommufd = dup(iommufd);
>	VFIO_ASSERT_GT(new->iommufd, 0);
>
>	new->ioas_id = ioas_id;
>
>	return new;
>}
>
>/*** Public API for tests ***/
>
>struct iommu *iommufd_new_ioas(struct iommu *cur)
>{
>	return iommufd_new(cur->iommufd, iommufd_ioas_alloc(cur->iommufd));
>}
>
>struct iommu *iommufd_new_hwpt(struct iommu *cur, u32 dev_id)
>{
>	struct iommu *new = iommufd_new(cur->iommufd, cur->ioas_id);
>
>	iommufd_hwpt_alloc(new, dev_id);
>	return new;
>}
>
>struct iommu *iommufd_new_ioas_hwpt(struct iommu *cur, u32 dev_id)
>{
>	struct iommu *new = iommufd_new_ioas(cur);
>
>	iommufd_hwpt_alloc(new, dev_id);
>	return new;
>}

This looks great. I will update in next revision.