On Fri, May 30, 2025 at 01:14:55PM -0300, Jason Gunthorpe wrote:
On Sat, May 17, 2025 at 08:21:31PM -0700, Nicolin Chen wrote:
- offset =
cmd->nesting_parent_iova - PAGE_ALIGN(cmd->nesting_parent_iova);- max_npages = DIV_ROUND_UP(offset + cmd->length, PAGE_SIZE);
This should probably be capped to PAGE_SIZE/sizeof(void *), return EINVAL if not
Hmm, mind elaborating where this PAGE_SIZE/sizeof comes from?
- hw_queue = viommu->ops->hw_queue_alloc(ucmd, viommu, cmd->type,
cmd->index,cmd->nesting_parent_iova,cmd->length);- if (IS_ERR(hw_queue)) {
rc = PTR_ERR(hw_queue);goto out_unpin;- }
- /* The iommufd_hw_queue_alloc helper saves ictx in hw_queue->ictx */
- if (WARN_ON_ONCE(hw_queue->ictx != ucmd->ictx)) {
rc = -EINVAL;goto out_unpin;- }
There is another technique from RDMA which may actually be very helpful here considering how things are getting split up..
Put a size_t in the driver ops:
size_t size_viommu; size_t size_hw_queue;
Have the driver set it via a macro like INIT_RDMA_OBJ_SIZE
#define INIT_RDMA_OBJ_SIZE(ib_struct, drv_struct, member) \ .size_##ib_struct = \ (sizeof(struct drv_struct) + \ BUILD_BUG_ON_ZERO(offsetof(struct drv_struct, member)) + \ BUILD_BUG_ON_ZERO( \ !__same_type(((struct drv_struct *)NULL)->member, \ struct ib_struct)))
Which proves the core structure is at the front.
Then the core code can allocate the object along with enough space for the driver and call a driver function to init the driver portion of the already allocated object.
That's interesting! Then all "_alloc" ops would be "_init" instead.
Then you don't need these dances where the driver helper has to do things like set uctx, or the core structure is partially initialized:
- hw_queue->viommu = viommu;
- refcount_inc(&viommu->obj.users);
- hw_queue->length = cmd->length;
- hw_queue->base_addr = cmd->nesting_parent_iova;
When the driver is running, which can be a source of bugs.
Hmm, I don't quite follow the "bugs" here. Any example?
This would be useful for the existing ops too.
May reduce the size of the linked in code.
Yes! Haven't tried that yet, but sounds like we could move quite a few things back to the private header.
Perhaps a smaller cleanup series first to the existing code.
Thanks! Nicolin