Re: [PATCH v8 02/11] iommu: Add max_pasids field in struct dev_iommu
On 2022/6/10 17:01, Tian, Kevin wrote:
From: Baolu Lu
Sent: Friday, June 10, 2022 2:47 PM
On 2022/6/10 03:01, Raj, Ashok wrote:
On Tue, Jun 07, 2022 at 09:49:33AM +0800, Lu Baolu wrote:
@@ -218,6 +219,30 @@ static void dev_iommu_free(struct device *dev)
kfree(param);
}
+static u32 dev_iommu_get_max_pasids(struct device *dev)
+{
+ u32 max_pasids = dev->iommu->iommu_dev->max_pasids;
+ u32 num_bits;
+ int ret;
+
+ if (!max_pasids)
+ return 0;
+
+ if (dev_is_pci(dev)) {
+ ret = pci_max_pasids(to_pci_dev(dev));
+ if (ret < 0)
+ return 0;
+
+ return min_t(u32, max_pasids, ret);
Ah.. that answers my other question to consider device pasid-max. I guess
if we need any enforcement of restricting devices that aren't supporting
the full PASID, that will be done by some higher layer?
The mm->pasid style of SVA is explicitly enabled through
iommu_dev_enable_feature(IOMMU_DEV_FEAT_SVA). The IOMMU driver
specific
restriction might be put there?
too many returns in this function, maybe setup all returns to the end of
the function might be elegant?
I didn't find cleanup room after a quick scan of the code. But sure, let
me go through code again offline.
If we do care:
+static u32 dev_iommu_get_max_pasids(struct device *dev)
+{
+ u32 max_pasids = 0,
+ u32 num_bits = 0;
+ int ret;
+
+ if (dev_is_pci(dev)) {
+ ret = pci_max_pasids(to_pci_dev(dev));
+ if (ret > 0)
+ max_pasids = ret;
+ } else {
+ ret = device_property_read_u32(dev, "pasid-num-bits",
_bits);
+ if (!ret)
+ max_pasids = 1UL << num_bits;
+ }
+
+ return min_t(u32, max_pasids, dev->iommu->iommu_dev->max_pasids);
+}
Great! Cleaner and more compact than mine. Thank you!
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v8 02/11] iommu: Add max_pasids field in struct dev_iommu
> From: Baolu Lu
> Sent: Friday, June 10, 2022 2:47 PM
>
> On 2022/6/10 03:01, Raj, Ashok wrote:
> > On Tue, Jun 07, 2022 at 09:49:33AM +0800, Lu Baolu wrote:
> >> @@ -218,6 +219,30 @@ static void dev_iommu_free(struct device *dev)
> >>kfree(param);
> >> }
> >>
> >> +static u32 dev_iommu_get_max_pasids(struct device *dev)
> >> +{
> >> + u32 max_pasids = dev->iommu->iommu_dev->max_pasids;
> >> + u32 num_bits;
> >> + int ret;
> >> +
> >> + if (!max_pasids)
> >> + return 0;
> >> +
> >> + if (dev_is_pci(dev)) {
> >> + ret = pci_max_pasids(to_pci_dev(dev));
> >> + if (ret < 0)
> >> + return 0;
> >> +
> >> + return min_t(u32, max_pasids, ret);
> >
> > Ah.. that answers my other question to consider device pasid-max. I guess
> > if we need any enforcement of restricting devices that aren't supporting
> > the full PASID, that will be done by some higher layer?
>
> The mm->pasid style of SVA is explicitly enabled through
> iommu_dev_enable_feature(IOMMU_DEV_FEAT_SVA). The IOMMU driver
> specific
> restriction might be put there?
>
> >
> > too many returns in this function, maybe setup all returns to the end of
> > the function might be elegant?
>
> I didn't find cleanup room after a quick scan of the code. But sure, let
> me go through code again offline.
>
If we do care:
+static u32 dev_iommu_get_max_pasids(struct device *dev)
+{
+ u32 max_pasids = 0,
+ u32 num_bits = 0;
+ int ret;
+
+ if (dev_is_pci(dev)) {
+ ret = pci_max_pasids(to_pci_dev(dev));
+ if (ret > 0)
+ max_pasids = ret;
+ } else {
+ ret = device_property_read_u32(dev, "pasid-num-bits",
_bits);
+ if (!ret)
+ max_pasids = 1UL << num_bits;
+ }
+
+ return min_t(u32, max_pasids, dev->iommu->iommu_dev->max_pasids);
+}
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 02/11] iommu: Add max_pasids field in struct dev_iommu
On 2022/6/10 03:01, Raj, Ashok wrote:
On Tue, Jun 07, 2022 at 09:49:33AM +0800, Lu Baolu wrote:
Use this field to save the number of PASIDs that a device is able to
consume. It is a generic attribute of a device and lifting it into the
per-device dev_iommu struct could help to avoid the boilerplate code
in various IOMMU drivers.
Signed-off-by: Lu Baolu
---
include/linux/iommu.h | 2 ++
drivers/iommu/iommu.c | 26 ++
2 files changed, 28 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 03fbb1b71536..d50afb2c9a09 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -364,6 +364,7 @@ struct iommu_fault_param {
* @fwspec:IOMMU fwspec data
* @iommu_dev: IOMMU device this device is linked to
* @priv: IOMMU Driver private data
+ * @max_pasids: number of PASIDs device can consume
*
* TODO: migrate other per device data pointers under iommu_dev_data, e.g.
*struct iommu_group *iommu_group;
@@ -375,6 +376,7 @@ struct dev_iommu {
struct iommu_fwspec *fwspec;
struct iommu_device *iommu_dev;
void*priv;
+ u32 max_pasids;
};
int iommu_device_register(struct iommu_device *iommu,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 847ad47a2dfd..adac85ccde73 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
Is this needed for this patch?
Yes. It's for pci_max_pasids().
#include
#include
#include
@@ -218,6 +219,30 @@ static void dev_iommu_free(struct device *dev)
kfree(param);
}
+static u32 dev_iommu_get_max_pasids(struct device *dev)
+{
+ u32 max_pasids = dev->iommu->iommu_dev->max_pasids;
+ u32 num_bits;
+ int ret;
+
+ if (!max_pasids)
+ return 0;
+
+ if (dev_is_pci(dev)) {
+ ret = pci_max_pasids(to_pci_dev(dev));
+ if (ret < 0)
+ return 0;
+
+ return min_t(u32, max_pasids, ret);
Ah.. that answers my other question to consider device pasid-max. I guess
if we need any enforcement of restricting devices that aren't supporting
the full PASID, that will be done by some higher layer?
The mm->pasid style of SVA is explicitly enabled through
iommu_dev_enable_feature(IOMMU_DEV_FEAT_SVA). The IOMMU driver specific
restriction might be put there?
too many returns in this function, maybe setup all returns to the end of
the function might be elegant?
I didn't find cleanup room after a quick scan of the code. But sure, let
me go through code again offline.
+ }
+
+ ret = device_property_read_u32(dev, "pasid-num-bits", _bits);
+ if (ret)
+ return 0;
+
+ return min_t(u32, max_pasids, 1UL << num_bits);
+}
+
static int __iommu_probe_device(struct device *dev, struct list_head
*group_list)
{
const struct iommu_ops *ops = dev->bus->iommu_ops;
@@ -243,6 +268,7 @@ static int __iommu_probe_device(struct device *dev, struct
list_head *group_list
}
dev->iommu->iommu_dev = iommu_dev;
+ dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
group = iommu_group_get_for_dev(dev);
if (IS_ERR(group)) {
--
2.25.1
Best regards,
Baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 02/11] iommu: Add max_pasids field in struct dev_iommu
On Tue, Jun 07, 2022 at 09:49:33AM +0800, Lu Baolu wrote:
> Use this field to save the number of PASIDs that a device is able to
> consume. It is a generic attribute of a device and lifting it into the
> per-device dev_iommu struct could help to avoid the boilerplate code
> in various IOMMU drivers.
>
> Signed-off-by: Lu Baolu
> ---
> include/linux/iommu.h | 2 ++
> drivers/iommu/iommu.c | 26 ++
> 2 files changed, 28 insertions(+)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 03fbb1b71536..d50afb2c9a09 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -364,6 +364,7 @@ struct iommu_fault_param {
> * @fwspec: IOMMU fwspec data
> * @iommu_dev:IOMMU device this device is linked to
> * @priv: IOMMU Driver private data
> + * @max_pasids: number of PASIDs device can consume
> *
> * TODO: migrate other per device data pointers under iommu_dev_data, e.g.
> * struct iommu_group *iommu_group;
> @@ -375,6 +376,7 @@ struct dev_iommu {
> struct iommu_fwspec *fwspec;
> struct iommu_device *iommu_dev;
> void*priv;
> + u32 max_pasids;
> };
>
> int iommu_device_register(struct iommu_device *iommu,
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 847ad47a2dfd..adac85ccde73 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -20,6 +20,7 @@
> #include
> #include
> #include
> +#include
Is this needed for this patch?
> #include
> #include
> #include
> @@ -218,6 +219,30 @@ static void dev_iommu_free(struct device *dev)
> kfree(param);
> }
>
> +static u32 dev_iommu_get_max_pasids(struct device *dev)
> +{
> + u32 max_pasids = dev->iommu->iommu_dev->max_pasids;
> + u32 num_bits;
> + int ret;
> +
> + if (!max_pasids)
> + return 0;
> +
> + if (dev_is_pci(dev)) {
> + ret = pci_max_pasids(to_pci_dev(dev));
> + if (ret < 0)
> + return 0;
> +
> + return min_t(u32, max_pasids, ret);
Ah.. that answers my other question to consider device pasid-max. I guess
if we need any enforcement of restricting devices that aren't supporting
the full PASID, that will be done by some higher layer?
too many returns in this function, maybe setup all returns to the end of
the function might be elegant?
> + }
> +
> + ret = device_property_read_u32(dev, "pasid-num-bits", _bits);
> + if (ret)
> + return 0;
> +
> + return min_t(u32, max_pasids, 1UL << num_bits);
> +}
> +
> static int __iommu_probe_device(struct device *dev, struct list_head
> *group_list)
> {
> const struct iommu_ops *ops = dev->bus->iommu_ops;
> @@ -243,6 +268,7 @@ static int __iommu_probe_device(struct device *dev,
> struct list_head *group_list
> }
>
> dev->iommu->iommu_dev = iommu_dev;
> + dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
>
> group = iommu_group_get_for_dev(dev);
> if (IS_ERR(group)) {
> --
> 2.25.1
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
