Re: [PATCH v6 02/12] iommu: Add pasid_bits field in struct dev_iommu
On Wed, May 11, 2022 at 09:00:50AM +0100, Jean-Philippe Brucker wrote: > > /** > > * struct iommu_device - IOMMU core representation of one IOMMU hardware > > * instance > > * @list: Used by the iommu-core to keep a list of registered iommus > > * @ops: iommu-ops for talking to this iommu > > * @dev: struct device for sysfs handling > > */ > > struct iommu_device { > > struct list_head list; > > const struct iommu_ops *ops; > > struct fwnode_handle *fwnode; > > struct device *dev; > > }; > > > > I haven't checked ARM code yet, but it works for x86 as far as I can > > see. > > Arm also supports non-PCI PASID by reading a firmware property: > > device_property_read_u32(dev, "pasid-num-bits", >ssid_bits); > > should be the only difference That is not "ARM" that is generic DT/ACPI for platform devices and should be handled by the core code in the same place it does PCI discovery. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 02/12] iommu: Add pasid_bits field in struct dev_iommu
On Wed, May 11, 2022 at 10:25:48AM +0800, Baolu Lu wrote: > On 2022/5/10 22:34, Jason Gunthorpe wrote: > > On Tue, May 10, 2022 at 02:17:28PM +0800, Lu Baolu wrote: > > > > > int iommu_device_register(struct iommu_device *iommu, > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > > index 627a3ed5ee8f..afc63fce6107 100644 > > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > > @@ -2681,6 +2681,8 @@ static struct iommu_device > > > *arm_smmu_probe_device(struct device *dev) > > > smmu->features & ARM_SMMU_FEAT_STALL_FORCE) > > > master->stall_enabled = true; > > > + dev->iommu->pasid_bits = master->ssid_bits; > > > return >iommu; > > > err_free_master: > > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > > > index 2990f80c5e08..99643f897f26 100644 > > > +++ b/drivers/iommu/intel/iommu.c > > > @@ -4624,8 +4624,11 @@ static struct iommu_device > > > *intel_iommu_probe_device(struct device *dev) > > > if (pasid_supported(iommu)) { > > > int features = pci_pasid_features(pdev); > > > - if (features >= 0) > > > + if (features >= 0) { > > > info->pasid_supported = > > > features | 1; > > > + dev->iommu->pasid_bits = > > > + fls(pci_max_pasids(pdev)) - 1; > > > + } > > > > It is not very nice that both the iommu drivers have to duplicate the > > code to read the pasid capability out of the PCI device. > > > > IMHO it would make more sense for the iommu layer to report the > > capability of its own HW block only, and for the core code to figure > > out the master's limitation using a bus-specific approach. > > Fair enough. The iommu hardware capability could be reported in > > /** > * struct iommu_device - IOMMU core representation of one IOMMU hardware > * instance > * @list: Used by the iommu-core to keep a list of registered iommus > * @ops: iommu-ops for talking to this iommu > * @dev: struct device for sysfs handling > */ > struct iommu_device { > struct list_head list; > const struct iommu_ops *ops; > struct fwnode_handle *fwnode; > struct device *dev; > }; > > I haven't checked ARM code yet, but it works for x86 as far as I can > see. Arm also supports non-PCI PASID by reading a firmware property: device_property_read_u32(dev, "pasid-num-bits", >ssid_bits); should be the only difference Thanks, Jean > > > > > It is also unfortunate that the enable/disable pasid is inside the > > iommu driver as well - ideally the PCI driver itself would do this > > when it knows it wants to use PASIDs. > > > > The ordering interaction with ATS makes this look quite annoying > > though. :( > > > > I'm also not convinced individual IOMMU drivers should be forcing ATS > > on, there are performance and functional implications here. Using ATS > > or not is possibly best left as an administrator policy controlled by > > the core code. Again we seem to have some mess. > > Agreed with you. This has already been in my task list. I will start to > solve it after the iommufd tasks. > > Best regards, > baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 02/12] iommu: Add pasid_bits field in struct dev_iommu
On 2022/5/10 22:34, Jason Gunthorpe wrote: On Tue, May 10, 2022 at 02:17:28PM +0800, Lu Baolu wrote: int iommu_device_register(struct iommu_device *iommu, diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 627a3ed5ee8f..afc63fce6107 100644 +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2681,6 +2681,8 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev) smmu->features & ARM_SMMU_FEAT_STALL_FORCE) master->stall_enabled = true; + dev->iommu->pasid_bits = master->ssid_bits; return >iommu; err_free_master: diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 2990f80c5e08..99643f897f26 100644 +++ b/drivers/iommu/intel/iommu.c @@ -4624,8 +4624,11 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev) if (pasid_supported(iommu)) { int features = pci_pasid_features(pdev); -if (features >= 0) + if (features >= 0) { info->pasid_supported = features | 1; + dev->iommu->pasid_bits = + fls(pci_max_pasids(pdev)) - 1; + } It is not very nice that both the iommu drivers have to duplicate the code to read the pasid capability out of the PCI device. IMHO it would make more sense for the iommu layer to report the capability of its own HW block only, and for the core code to figure out the master's limitation using a bus-specific approach. Fair enough. The iommu hardware capability could be reported in /** * struct iommu_device - IOMMU core representation of one IOMMU hardware * instance * @list: Used by the iommu-core to keep a list of registered iommus * @ops: iommu-ops for talking to this iommu * @dev: struct device for sysfs handling */ struct iommu_device { struct list_head list; const struct iommu_ops *ops; struct fwnode_handle *fwnode; struct device *dev; }; I haven't checked ARM code yet, but it works for x86 as far as I can see. It is also unfortunate that the enable/disable pasid is inside the iommu driver as well - ideally the PCI driver itself would do this when it knows it wants to use PASIDs. The ordering interaction with ATS makes this look quite annoying though. :( I'm also not convinced individual IOMMU drivers should be forcing ATS on, there are performance and functional implications here. Using ATS or not is possibly best left as an administrator policy controlled by the core code. Again we seem to have some mess. Agreed with you. This has already been in my task list. I will start to solve it after the iommufd tasks. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 02/12] iommu: Add pasid_bits field in struct dev_iommu
On Tue, May 10, 2022 at 02:17:28PM +0800, Lu Baolu wrote: > int iommu_device_register(struct iommu_device *iommu, > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > index 627a3ed5ee8f..afc63fce6107 100644 > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2681,6 +2681,8 @@ static struct iommu_device > *arm_smmu_probe_device(struct device *dev) > smmu->features & ARM_SMMU_FEAT_STALL_FORCE) > master->stall_enabled = true; > > + dev->iommu->pasid_bits = master->ssid_bits; > return >iommu; > > err_free_master: > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > index 2990f80c5e08..99643f897f26 100644 > +++ b/drivers/iommu/intel/iommu.c > @@ -4624,8 +4624,11 @@ static struct iommu_device > *intel_iommu_probe_device(struct device *dev) > if (pasid_supported(iommu)) { > int features = pci_pasid_features(pdev); > > - if (features >= 0) > + if (features >= 0) { > info->pasid_supported = features | 1; > + dev->iommu->pasid_bits = > + fls(pci_max_pasids(pdev)) - 1; > + } It is not very nice that both the iommu drivers have to duplicate the code to read the pasid capability out of the PCI device. IMHO it would make more sense for the iommu layer to report the capability of its own HW block only, and for the core code to figure out the master's limitation using a bus-specific approach. It is also unfortunate that the enable/disable pasid is inside the iommu driver as well - ideally the PCI driver itself would do this when it knows it wants to use PASIDs. The ordering interaction with ATS makes this look quite annoying though. :( I'm also not convinced individual IOMMU drivers should be forcing ATS on, there are performance and functional implications here. Using ATS or not is possibly best left as an administrator policy controlled by the core code. Again we seem to have some mess. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v6 02/12] iommu: Add pasid_bits field in struct dev_iommu
Use this field to save the pasid/ssid bits that a device is able to support with its IOMMU hardware. It is a generic attribute of a device and lifting it into the per-device dev_iommu struct makes it possible to allocate a PASID for device without calls into the IOMMU drivers. Any iommu driver which suports PASID related features should set this field before features are enabled on the devices. For initialization of this field in the VT-d driver, the info->pasid_supported is only set for PCI devices. So the status is that non-PCI SVA hasn't been supported yet. Setting this field only for PCI devices has no functional change. Signed-off-by: Lu Baolu Reviewed-by: Jean-Philippe Brucker --- include/linux/iommu.h | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 ++ drivers/iommu/intel/iommu.c | 5 - 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 5e1afe169549..b8ffaf2cb1d0 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -373,6 +373,7 @@ struct dev_iommu { struct iommu_fwspec *fwspec; struct iommu_device *iommu_dev; void*priv; + unsigned intpasid_bits; }; int iommu_device_register(struct iommu_device *iommu, diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 627a3ed5ee8f..afc63fce6107 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2681,6 +2681,8 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev) smmu->features & ARM_SMMU_FEAT_STALL_FORCE) master->stall_enabled = true; + dev->iommu->pasid_bits = master->ssid_bits; + return >iommu; err_free_master: diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 2990f80c5e08..99643f897f26 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4624,8 +4624,11 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev) if (pasid_supported(iommu)) { int features = pci_pasid_features(pdev); - if (features >= 0) + if (features >= 0) { info->pasid_supported = features | 1; + dev->iommu->pasid_bits = + fls(pci_max_pasids(pdev)) - 1; + } } if (info->ats_supported && ecap_prs(iommu->ecap) && -- 2.25.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu