Re: [PATCH v1 06/12] iommufd/selftest: Implement mock_get_viommu_size and mock_viommu_init

[Nicolin Chen <nicolinc@nvidia.com>]

On Fri, Jun 13, 2025 at 10:45:22AM -0300, Jason Gunthorpe wrote:
> On Mon, Jun 09, 2025 at 10:13:29AM -0700, Nicolin Chen wrote:
> > Sanitize the inputs and report the size of struct mock_viommu on success,
> > in mock_get_viommu_size().
> > 
> > The core will ensure the viommu_type is set to the core vIOMMU object, so
> > simply init the driver part in mock_viommu_init().
> > 
> > The mock_viommu_alloc() will be cleaned up once the transition is done.
> 
> Note the addition of the missed s2_parent store

I am adding a new patch in prep-v2 dropping the unused s2_parent:

-------------------------------------------------------------------
iommufd/selftest: Drop parent domain from mock_iommu_domain_nested

This is no use of this parent domain. Delete the dead code.

Note that the s2_parent in struct mock_viommu will be a deadcode too. Yet,
keep it because it will be soon used by HW queue objects, i.e. no point in
adding it back and forth in such a short window. Besides, keeping it could
cover the majority of vIOMMU use cases where a driver-level structure will
be larger in size than the core structure.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
-------------------------------------------------------------------

In this case, we don't need to assign mock_viommu->s2_parent here
since it's a dead code. The HW queue series will assign it and add
a use case:

+/* Test iommufd_hw_queue_depend/undepend() */
+static int mock_hw_queue_init_phys(struct iommufd_hw_queue *hw_queue, u32 index,
+                                  phys_addr_t base_addr_pa)
...
+       /*
+        * Test to catch a kernel bug if the core converted the physical address
+        * incorrectly. Let mock_domain_iova_to_phys() WARN_ON if it fails.
+        */
+       if (base_addr_pa != iommu_iova_to_phys(&mock_viommu->s2_parent->domain,
+                                              hw_queue->base_addr)) {
+               rc = -EFAULT;
+               goto unlock;
+       }

> > +static int mock_viommu_init(struct iommufd_viommu *viommu,
> > +			    struct iommu_domain *parent_domain)
> > +{
> > +	struct mock_iommu_device *mock_iommu = container_of(
> > +		viommu->iommu_dev, struct mock_iommu_device, iommu_dev);
> > +	struct mock_viommu *mock_viommu = to_mock_viommu(viommu);
> > +
> > +	refcount_inc(&mock_iommu->users);
> > +	mock_viommu->s2_parent = to_mock_domain(parent_domain);
> > +
> > +	viommu->ops = &mock_viommu_ops;
> > +	return 0;
> > +}
> 
> The patches will read better if you add the call logic for init along
> side alloc based on init or alloc ops being non-NULL in the prior
> patch and then have these driver patches replace alloc with init.
> 
> Duplicating alloc into init and leaving both makes the patch harder to
> check.

I see. That will add an additional patch tentatively supporting
both ops.

Thanks
Nicolin