Messages in this thread | | | Date | Wed, 1 Feb 2023 09:46:23 -0800 | From | Nicolin Chen <> | Subject | Re: [PATCH v2 1/3] iommufd: Add devices_users to track the hw_pagetable usage by device |
| |
On Wed, Feb 01, 2023 at 11:53:02AM -0400, Jason Gunthorpe wrote: > On Tue, Jan 31, 2023 at 10:57:13PM -0800, Nicolin Chen wrote: > > On Mon, Jan 30, 2023 at 04:35:35PM -0400, Jason Gunthorpe wrote: > > > > > IMHO I would structure the smmu driver so that all the different > > > iommu_domain formats have their own ops pointer. The special > > > "undecided" format would have a special ops with only attach_dev and > > > at first attach it would switch the ops to whatever format it > > > selected. > > > > > > I think this could get rid of a lot of the 'if undecided/S1/S2/CD' > > > complexity all over the place. You know what type it is because you > > > were called on a op that is only called on its type. > > > > An auto/unmanaged domain allocation via iommu_domain_alloc() would > > be S1, while an allocation via ops->domain_alloc_user can be S1 or > > S2 with a given parameter/flag. So, actually the format is always > > decided. > > No, it can't decide the S1/S2 format until it knows the smmu because > of this: > > /* Restrict the stage to what we can actually support */ > if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1)) > smmu_domain->stage = ARM_SMMU_DOMAIN_S2; > if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2)) > smmu_domain->stage = ARM_SMMU_DOMAIN_S1; > > So the format is never decided.
OK. That's right. And the solution to that is also passing a dev pointer in regular ->domain_alloc() op.
> > that we don't pass the dev pointer down to ops->domain_alloc. So, > > the SMMU driver can't know which SMMU device the device is behind, > > resulting in being unable to finalizing the domain. Robin mentioned > > that he has a patch "iommu: Pass device through ops->domain_alloc". > > Perhaps that is required for us to entirely fix the add_domain() > > problem? > > Robin is making progress, hopefully soon > > So the issue is with replace you need to have the domain populated > before we can call replace but you can't populate the domain until it > is bound because of the above issue? That seems unsovlable without > fixing up the driver.
Not really. A REPLACE ioctl is just an ATTACH, if the device just gets BIND-ed. So the SMMU driver will initialize ("finalise") the domain during the replace() call, then iopt_table_add_domain() can be done.
So, not a blocker here.
> I'd say replace can go ahead ingoring that issue and that for now > replace will only work on ARM with domains created by > domain_alloc_user that are fully configured. > > It will start working correctly for auto domains once Robin's changes > get finished. > > Is there another issue?
Oh. I think we mixed the topics here. These three patches were not to unblock but to clean up a way for the replace series and the nesting series, for the device locking issue:
if (cur_hwpt != hwpt) mutex_lock(&cur_hwpt->device_lock); mutex_lock(&hwpt->device_lock); ... if (iommufd_hw_pagetabe_has_group()) { // touching device list ... iommu_group_replace_domain(); ... } if (cur_hwpt && hwpt) list_del(&idev->devices_item); list_add(&idev->devices_item, &cur_hwpt->devices); ... mutex_unlock(&hwpt->device_lock); if (cur_hwpt != hwpt) mutex_unlock(&cur_hwpt->device_lock);
I just gave another thought about it. Since we have the patch-2 from this series moving the ioas->mutex, it already serializes attach/detach routines. And I see that all the places touching idev->device_item and hwpt->devices are protected by ioas->mutex. So, perhaps we can simply remove the device_lock?
do_attach(): mutex_lock(&ioas->mutex); // protect both devices_item and hwpt_item ... if (iommufd_hw_pagetabe_has_group()) { // touching device list ... iommu_group_replace_domain(); ... } if (cur_hwpt && hwpt) list_del(&idev->devices_item); list_add(&idev->devices_item, &cur_hwpt->devices); ... mutex_unlock(&ioas->mutex);
do_detach(): mutex_lock(&ioas->mutex); // protect both devices_item and hwpt_item ... if (iommufd_hw_pagetabe_has_group()) { // touching device list ... iommu_detach_group(); ... } list_del(&idev->devices_item); ... mutex_unlock(&ioas->mutex);
If this is correct, I think I can prepare the replace series and send it by the end of the day.
Thanks Nic
| |