Re: [PATCH v5 06/13] iommufd: Allow pt_id to carry viommu_id for IOMMU_HWPT_ALLOC

[Nicolin Chen <nicolinc@nvidia.com>]

On Mon, Oct 28, 2024 at 07:52:10AM -0700, Nicolin Chen wrote:
> On Mon, Oct 28, 2024 at 10:03:09AM -0300, Jason Gunthorpe wrote:
> > On Mon, Oct 28, 2024 at 11:24:10AM +0800, Zhangfei Gao wrote:
> >
> > > > +/**
> > > > + * iommufd_viommu_alloc_hwpt_nested() - Get a hwpt_nested for a vIOMMU
> > > > + * @viommu: vIOMMU ojbect to associate the hwpt_nested/domain with
> > > > + * @user_data: user_data pointer. Must be valid
> > > > + *
> > > > + * Allocate a new IOMMU_DOMAIN_NESTED for a vIOMMU and return it as a NESTED
> > > > + * hw_pagetable.
> > > > + */
> > > > +static struct iommufd_hwpt_nested *
> > > > +iommufd_viommu_alloc_hwpt_nested(struct iommufd_viommu *viommu, u32 flags,
> > > > +                                const struct iommu_user_data *user_data)
> > > > +{
> > > > +       struct iommufd_hwpt_nested *hwpt_nested;
> > > > +       struct iommufd_hw_pagetable *hwpt;
> > > > +       int rc;
> > > > +
> > > > +       if (flags)
> > > > +               return ERR_PTR(-EOPNOTSUPP);
> > >
> > > This check should be removed.
> > >
> > > When a user page fault is required, IOMMU_HWPT_FAULT_ID_VALID is set.
> > > if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) {
> >
> > It can't just be removed..
> >
> > I suspect that IOMMU_HWPT_FAULT_ID_VALID should not be set on the
> > nested domain but on the parent?
> 
> By giving another look,
> 
> In iommufd_hwpt_paging_alloc(), we reject IOMMU_HWPT_FAULT_ID_VALID:
>         const u32 valid_flags = IOMMU_HWPT_ALLOC_NEST_PARENT |
>                                 IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>         ...
>         if (flags & ~valid_flags)
>                 return ERR_PTR(-EOPNOTSUPP);
> 
> In iommufd_hwpt_nested_alloc(), we mask the flag away:
>         if ((flags & ~IOMMU_HWPT_FAULT_ID_VALID) ||
>             !user_data->len || !ops->domain_alloc_user)
>                 return ERR_PTR(-EOPNOTSUPP);
>         ...
>         hwpt->domain = ops->domain_alloc_user(idev->dev,
>                                               flags & ~IOMMU_HWPT_FAULT_ID_VALID,
>                                               parent->common.domain, user_data);
> 
> Then, in the common function it has a section of
>         if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) {
>         ...
> 
> It seems that this IOMMU_HWPT_FAULT_ID_VALID is for nested domains?
> 
> So, aligning with that, here we need:
>         if ((flags & ~IOMMU_HWPT_FAULT_ID_VALID) || !user_data->len)
>                 return ERR_PTR(-EOPNOTSUPP);

I added a TEST_F for the coverage:

+TEST_F(iommufd_viommu, viommu_alloc_nested_iopf)
+{
+       struct iommu_hwpt_selftest data = {
+               .iotlb = IOMMU_TEST_IOTLB_DEFAULT,
+       };
+       uint32_t viommu_id = self->viommu_id;
+       uint32_t dev_id = self->device_id;
+       uint32_t iopf_hwpt_id;
+       uint32_t fault_id;
+       uint32_t fault_fd;
+
+       if (self->device_id) {
+               test_ioctl_fault_alloc(&fault_id, &fault_fd);
+               test_err_hwpt_alloc_iopf(
+                       ENOENT, dev_id, viommu_id, UINT32_MAX,
+                       IOMMU_HWPT_FAULT_ID_VALID, &iopf_hwpt_id,
+                       IOMMU_HWPT_DATA_SELFTEST, &data, sizeof(data));
+               test_err_hwpt_alloc_iopf(
+                       EOPNOTSUPP, dev_id, viommu_id, fault_id,
+                       IOMMU_HWPT_FAULT_ID_VALID | (1 << 31), &iopf_hwpt_id,
+                       IOMMU_HWPT_DATA_SELFTEST, &data, sizeof(data));
+               test_cmd_hwpt_alloc_iopf(
+                       dev_id, viommu_id, fault_id, IOMMU_HWPT_FAULT_ID_VALID,
+                       &iopf_hwpt_id, IOMMU_HWPT_DATA_SELFTEST, &data,
+                       sizeof(data));
+
+               test_cmd_mock_domain_replace(self->stdev_id, iopf_hwpt_id);
+               EXPECT_ERRNO(EBUSY,
+                            _test_ioctl_destroy(self->fd, iopf_hwpt_id));
+               test_cmd_trigger_iopf(dev_id, fault_fd);
+
+               test_cmd_mock_domain_replace(self->stdev_id, self->ioas_id);
+               test_ioctl_destroy(iopf_hwpt_id);
+               close(fault_fd);
+               test_ioctl_destroy(fault_id);
+       }
+}

Thanks
Nicolin