From: Jason Gunthorpe jgg@nvidia.com Sent: Saturday, July 29, 2023 1:56 AM
On Mon, Jul 24, 2023 at 04:03:57AM -0700, Yi Liu wrote:
- switch (pt_obj->type) {
- case IOMMUFD_OBJ_IOAS:
ioas = container_of(pt_obj, struct iommufd_ioas, obj);break;- case IOMMUFD_OBJ_HW_PAGETABLE:
/* pt_id points HWPT only when hwpt_typeis !IOMMU_HWPT_TYPE_DEFAULT */
if (cmd->hwpt_type == IOMMU_HWPT_TYPE_DEFAULT) {rc = -EINVAL;goto out_put_pt;}parent = container_of(pt_obj, struct iommufd_hw_pagetable,obj);
/** Cannot allocate user-managed hwpt linking toauto_created
* hwpt. If the parent hwpt is already a user-managed hwpt,* don't allocate another user-managed hwpt linking to it.*/if (parent->auto_domain || parent->parent) {rc = -EINVAL;goto out_put_pt;}ioas = parent->ioas;Why do we set ioas here? I would think it should be NULL.
I think it is looking like a mistake to try and re-use iommufd_hw_pagetable_alloc() directly for the nested case. It should not have a IOAS argument, it should not do enforce_cc, or iopt_* functions
enforce_cc is still required since it's about memory accesses post page table walking (no matter the walked table is single stage or nested).
So must of the function is removed. Probably better to make a new ioas-less function for the nesting case.
diff --git a/drivers/iommu/iommufd/main.c
b/drivers/iommu/iommufd/main.c
index 510db114fc61..5f4420626421 100644 --- a/drivers/iommu/iommufd/main.c +++ b/drivers/iommu/iommufd/main.c @@ -426,7 +426,7 @@ static const struct iommufd_ioctl_op
iommufd_ioctl_ops[] = {
IOCTL_OP(IOMMU_GET_HW_INFO, iommufd_get_hw_info, struct
iommu_hw_info,
__reserved),IOCTL_OP(IOMMU_HWPT_ALLOC, iommufd_hwpt_alloc, struct
iommu_hwpt_alloc,
__reserved),
data_uptr),Nono, these can never change once we put them it. It specifies the hard minimum size that userspace must provide. If userspace gives less than this then the ioctl always fails. Changing it breaks all the existing software.
The core code ensures that the trailing part of the cmd struct is zero'd the extended implementation must cope with Zero'd values, which this does.
Ideally expanding uAPI structure size should come with new flag bits.
this one might be a bit special. hwpt_alloc() series was just queued to iommufd-next. If the nesting series could come together in one cycle then probably changing it in current way is fine since there is no existing software. Otherwise we need follow common practice. 😊