lkml.org 
[lkml]   [2023]   [May]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v2 04/11] iommufd: Pass parent hwpt and user_data to iommufd_hw_pagetable_alloc()
    On Fri, May 19, 2023 at 09:06:20AM +0000, Tian, Kevin wrote:

    > > @@ -73,14 +77,22 @@ int iommufd_hw_pagetable_enforce_cc(struct
    > > iommufd_hw_pagetable *hwpt)
    > > */
    > > struct iommufd_hw_pagetable *
    > > iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct
    > > iommufd_ioas *ioas,
    > > - struct iommufd_device *idev, bool
    > > immediate_attach)
    > > + struct iommufd_device *idev,
    > > + struct iommufd_hw_pagetable *parent,
    > > + union iommu_domain_user_data *user_data,
    > > + bool immediate_attach)
    > > {
    > > const struct iommu_ops *ops = dev_iommu_ops(idev->dev);
    > > + struct iommu_domain *parent_domain = NULL;
    > > struct iommufd_hw_pagetable *hwpt;
    > > + bool type_unmanaged, type_nested;
    > > int rc;
    > >
    > > lockdep_assert_held(&ioas->mutex);
    > >
    > > + if ((user_data || parent) && !ops->domain_alloc_user)
    > > + return ERR_PTR(-EOPNOTSUPP);
    >
    > Do we allow specifying parent w/o user_data?

    I don't think so. Perhaps we should do a double check:

    + if (!!user_data ^ !!parent)
    + return ERR_PTR(-EINVAL);
    + if (user_data && !ops->domain_alloc_user)
    + return ERR_PTR(-EOPNOTSUPP);

    > > @@ -99,6 +117,15 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx
    > > *ictx, struct iommufd_ioas *ioas,
    > > goto out_abort;
    > > }
    > >
    > > + /* It must be either NESTED or UNMANAGED, depending on
    > > parent_domain */
    > > + type_nested = hwpt->domain->type == IOMMU_DOMAIN_NESTED;
    > > + type_unmanaged = hwpt->domain->type ==
    > > IOMMU_DOMAIN_UNMANAGED;
    >
    > no need of one-time used variables. Just put the conditions directly
    > in WARN_ON.

    It is to improve the readability. Otherwise, we'd have:

    if (WARN_ON((parent_domain &&
    hwpt->domain->type != IOMMU_DOMAIN_NESTED) ||
    (!parent_domain &&
    hwpt->domain->type != IOMMU_DOMAIN_UNMANAGED)))

    > > + if (WARN_ON((parent_domain && !type_nested) ||
    > > + (!parent_domain && !type_unmanaged))) {
    > > + rc = -EINVAL;
    > > + goto out_abort;
    > > + }
    > > +
    >
    > probably just WARN_ON_ONCE() to mark that driver has problem?

    Yea. I think we could do that.

    Thanks
    Nic
    \
     
     \ /
      Last update: 2023-05-19 21:11    [W:4.080 / U:0.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site