Re: [PATCH v6 07/10] iommufd: Add a nested HW pagetable object

On 2023/10/25 01:30, Jason Gunthorpe wrote:

On Tue, Oct 24, 2023 at 02:18:10PM -0300, Jason Gunthorpe wrote:

...

On Tue, Oct 24, 2023 at 08:06:06AM -0700, Yi Liu wrote:

...

@@ -195,6 +279,10 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd) if (pt_obj->type == IOMMUFD_OBJ_IOAS) { struct iommufd_hwpt_paging *hwpt_paging;

• if (cmd->data_type != IOMMU_HWPT_DATA_NONE) {
  

• 	rc = -EINVAL;
  

• 	goto out_put_pt;
  

• }
  

  ioas = container_of(pt_obj, struct iommufd_ioas, obj); mutex_lock(&ioas->mutex); hwpt_paging = iommufd_hwpt_paging_alloc(ucmd->ictx, ioas, idev,

?? What is this?

Ah something went wrong earlier in "iommu: Pass in parent domain with user_data to domain_alloc_user op"

Bah, I got confused because that had half the uapi, so in this pathc

...

Once we added the user_data we should flow it through to the op always.

Like this:

ack.

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index 92859b907bb93c..a3382811af8a81 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -586,8 +586,8 @@ iommufd_device_auto_get_domain(struct iommufd_device *idev, goto out_unlock; }

• hwpt_paging = iommufd_hwpt_paging_alloc(idev->ictx, ioas, idev,

• 				 0, immediate_attach);
  

• hwpt_paging = iommufd_hwpt_paging_alloc(idev->ictx, ioas, idev, 0,

• 				immediate_attach, NULL);
  

  if (IS_ERR(hwpt_paging)) { destroy_hwpt = ERR_CAST(hwpt_paging); goto out_unlock;

diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c index cfd85df693d7b2..324a6d73f032ee 100644 --- a/drivers/iommu/iommufd/hw_pagetable.c +++ b/drivers/iommu/iommufd/hw_pagetable.c @@ -96,7 +96,8 @@ iommufd_hwpt_paging_enforce_cc(struct iommufd_hwpt_paging *hwpt_paging) struct iommufd_hwpt_paging * iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, struct iommufd_device *idev, u32 flags,

• 	  bool immediate_attach)
  

• 	  bool immediate_attach,
  

• 	  const struct iommu_user_data *user_data)
  

  { const u32 valid_flags = IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING;

@@ -107,7 +108,7 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, lockdep_assert_held(&ioas->mutex);

• if (flags && !ops->domain_alloc_user)

• if ((flags || user_data) && !ops->domain_alloc_user) return ERR_PTR(-EOPNOTSUPP); if (flags & ~valid_flags) return ERR_PTR(-EOPNOTSUPP);

@@ -127,7 +128,7 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, if (ops->domain_alloc_user) { hwpt->domain = ops->domain_alloc_user(idev->dev, flags,

• 				      NULL, NULL);
  

• 				      NULL, user_data);
  

  if (IS_ERR(hwpt->domain)) { rc = PTR_ERR(hwpt->domain); hwpt->domain = NULL;

@@ -210,8 +211,7 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx, struct iommufd_hw_pagetable *hwpt; int rc;

• if (flags || user_data->type == IOMMU_HWPT_DATA_NONE ||

•    !ops->domain_alloc_user)
  

• if (flags || !user_data->len || !ops->domain_alloc_user) return ERR_PTR(-EOPNOTSUPP); if (parent->auto_domain || !parent->nest_parent) return ERR_PTR(-EINVAL);

@@ -249,6 +249,11 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx, int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd) { struct iommu_hwpt_alloc *cmd = ucmd->cmd;

• const struct iommu_user_data user_data = {

• .type = cmd->data_type,
  

• .uptr = u64_to_user_ptr(cmd->data_uptr),
  

• .len = cmd->data_len,
  

• }; struct iommufd_hw_pagetable *hwpt; struct iommufd_ioas *ioas = NULL; struct iommufd_object *pt_obj;

@@ -273,25 +278,17 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd) if (pt_obj->type == IOMMUFD_OBJ_IOAS) { struct iommufd_hwpt_paging *hwpt_paging;

• if (cmd->data_type != IOMMU_HWPT_DATA_NONE) {
  

• 	rc = -EINVAL;
  

• 	goto out_put_pt;
  

• }
  

it can be covered by iommu driver when checking user data pointer. hence remove above if.

ioas = container_of(pt_obj, struct iommufd_ioas, obj);
mutex_lock(&ioas->mutex);

• hwpt_paging = iommufd_hwpt_paging_alloc(ucmd->ictx, ioas, idev,
  

• 					cmd->flags, false);
  

• hwpt_paging = iommufd_hwpt_paging_alloc(
  

• 	ucmd->ictx, ioas, idev, cmd->flags, false,
  

• 	user_data.len ? &user_data : NULL);
  

  if (IS_ERR(hwpt_paging)) { rc = PTR_ERR(hwpt_paging); goto out_unlock; } hwpt = &hwpt_paging->common; } else if (pt_obj->type == IOMMUFD_OBJ_HWPT_PAGING) {

• const struct iommu_user_data user_data = {
  

• 	.type = cmd->data_type,
  

• 	.uptr = u64_to_user_ptr(cmd->data_uptr),
  

• 	.len = cmd->data_len,
  

• };
  

  struct iommufd_hwpt_nested *hwpt_nested; struct iommufd_hwpt_paging *parent;

diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 6fdad56893af4d..24e5a36fc875e0 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -290,7 +290,8 @@ int iommufd_hwpt_get_dirty_bitmap(struct iommufd_ucmd *ucmd); struct iommufd_hwpt_paging * iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, struct iommufd_device *idev, u32 flags,

• 	  bool immediate_attach);
  

• 	  bool immediate_attach,
  

• 	  const struct iommu_user_data *user_data);
  

  int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt, struct iommufd_device *idev); struct iommufd_hw_pagetable *