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

Yi Liu
2023-10-25T10:17:53Z

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 *

--
Regards,
Yi Liu