| Message ID | 20190610184714.6786-4-jean-philippe.brucker@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu: Add auxiliary domain and PASID support to Arm SMMUv3 | expand |
On Mon, 10 Jun 2019 19:47:09 +0100 Jean-Philippe Brucker <jean-philippe.brucker@arm.com> wrote: > For platform devices that support SubstreamID (SSID), firmware provides > the number of supported SSID bits. Restrict it to what the SMMU supports > and cache it into master->ssid_bits. > > Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com> Missing kernel-doc. Thanks, Jonathan > --- > drivers/iommu/arm-smmu-v3.c | 11 +++++++++++ > drivers/iommu/of_iommu.c | 6 +++++- > include/linux/iommu.h | 1 + > 3 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index 4d5a694f02c2..3254f473e681 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -604,6 +604,7 @@ struct arm_smmu_master { > struct list_head domain_head; > u32 *sids; > unsigned int num_sids; > + unsigned int ssid_bits; > bool ats_enabled :1; > }; > > @@ -2097,6 +2098,16 @@ static int arm_smmu_add_device(struct device *dev) > } > } > > + master->ssid_bits = min(smmu->ssid_bits, fwspec->num_pasid_bits); > + > + /* > + * If the SMMU doesn't support 2-stage CD, limit the linear > + * tables to a reasonable number of contexts, let's say > + * 64kB / sizeof(ctx_desc) = 1024 = 2^10 > + */ > + if (!(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB)) > + master->ssid_bits = min(master->ssid_bits, 10U); > + > group = iommu_group_get_for_dev(dev); > if (!IS_ERR(group)) { > iommu_group_put(group); > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index f04a6df65eb8..04f4f6b95d82 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -206,8 +206,12 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, > if (err) > break; > } > - } > > + fwspec = dev_iommu_fwspec_get(dev); > + if (!err && fwspec) > + of_property_read_u32(master_np, "pasid-num-bits", > + &fwspec->num_pasid_bits); > + } > > /* > * Two success conditions can be represented by non-negative err here: > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 519e40fb23ce..b91df613385f 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -536,6 +536,7 @@ struct iommu_fwspec { > struct fwnode_handle *iommu_fwnode; > void *iommu_priv; > u32 flags; > + u32 num_pasid_bits; This structure has kernel doc so you need to add something for this. > unsigned int num_ids; > u32 ids[1]; > };
On 11/06/2019 10:42, Jonathan Cameron wrote: >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h >> index 519e40fb23ce..b91df613385f 100644 >> --- a/include/linux/iommu.h >> +++ b/include/linux/iommu.h >> @@ -536,6 +536,7 @@ struct iommu_fwspec { >> struct fwnode_handle *iommu_fwnode; >> void *iommu_priv; >> u32 flags; >> + u32 num_pasid_bits; > > This structure has kernel doc so you need to add something for this. Good catch Thanks, Jean
On Mon, Jun 10, 2019 at 07:47:09PM +0100, Jean-Philippe Brucker wrote: > For platform devices that support SubstreamID (SSID), firmware provides > the number of supported SSID bits. Restrict it to what the SMMU supports > and cache it into master->ssid_bits. > > Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com> > --- > drivers/iommu/arm-smmu-v3.c | 11 +++++++++++ > drivers/iommu/of_iommu.c | 6 +++++- > include/linux/iommu.h | 1 + > 3 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index 4d5a694f02c2..3254f473e681 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -604,6 +604,7 @@ struct arm_smmu_master { > struct list_head domain_head; > u32 *sids; > unsigned int num_sids; > + unsigned int ssid_bits; > bool ats_enabled :1; > }; > > @@ -2097,6 +2098,16 @@ static int arm_smmu_add_device(struct device *dev) > } > } > > + master->ssid_bits = min(smmu->ssid_bits, fwspec->num_pasid_bits); > + > + /* > + * If the SMMU doesn't support 2-stage CD, limit the linear > + * tables to a reasonable number of contexts, let's say > + * 64kB / sizeof(ctx_desc) = 1024 = 2^10 > + */ > + if (!(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB)) > + master->ssid_bits = min(master->ssid_bits, 10U); Please introduce a #define for the 10, so that it is computed in the way you describe in the comment (a bit like we do for things like queue sizes). > + > group = iommu_group_get_for_dev(dev); > if (!IS_ERR(group)) { > iommu_group_put(group); > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index f04a6df65eb8..04f4f6b95d82 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -206,8 +206,12 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, > if (err) > break; > } > - } > > + fwspec = dev_iommu_fwspec_get(dev); > + if (!err && fwspec) > + of_property_read_u32(master_np, "pasid-num-bits", > + &fwspec->num_pasid_bits); > + } Hmm. Do you know if there's anything in ACPI for this? Otherwise, patch looks fine. Thanks. Will
On 18/06/2019 19:08, Will Deacon wrote: >> + /* >> + * If the SMMU doesn't support 2-stage CD, limit the linear >> + * tables to a reasonable number of contexts, let's say >> + * 64kB / sizeof(ctx_desc) = 1024 = 2^10 >> + */ >> + if (!(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB)) >> + master->ssid_bits = min(master->ssid_bits, 10U); > > Please introduce a #define for the 10, so that it is computed in the way > you describe in the comment (a bit like we do for things like queue sizes). Ok >> + >> group = iommu_group_get_for_dev(dev); >> if (!IS_ERR(group)) { >> iommu_group_put(group); >> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c >> index f04a6df65eb8..04f4f6b95d82 100644 >> --- a/drivers/iommu/of_iommu.c >> +++ b/drivers/iommu/of_iommu.c >> @@ -206,8 +206,12 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, >> if (err) >> break; >> } >> - } >> >> + fwspec = dev_iommu_fwspec_get(dev); >> + if (!err && fwspec) >> + of_property_read_u32(master_np, "pasid-num-bits", >> + &fwspec->num_pasid_bits); >> + } > > Hmm. Do you know if there's anything in ACPI for this? Yes, IORT version D introduced a "substream width" field for the Named component node (platform device). I don't think it existed last time I checked, so I'll see about supporting it. Thanks, Jean
Hi Jean, On 6/10/19 8:47 PM, Jean-Philippe Brucker wrote: > For platform devices that support SubstreamID (SSID), firmware provides > the number of supported SSID bits. Restrict it to what the SMMU supports > and cache it into master->ssid_bits. The commit message may give the impression the master's ssid_bits field only is used for platform devices. > > Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com> > --- > drivers/iommu/arm-smmu-v3.c | 11 +++++++++++ > drivers/iommu/of_iommu.c | 6 +++++- > include/linux/iommu.h | 1 + > 3 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index 4d5a694f02c2..3254f473e681 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -604,6 +604,7 @@ struct arm_smmu_master { > struct list_head domain_head; > u32 *sids; > unsigned int num_sids; > + unsigned int ssid_bits; > bool ats_enabled :1; > }; > > @@ -2097,6 +2098,16 @@ static int arm_smmu_add_device(struct device *dev) > } > } > > + master->ssid_bits = min(smmu->ssid_bits, fwspec->num_pasid_bits); In case the device is a PCI device, what is the value taken by fwspec->num_pasid_bits? > + > + /* > + * If the SMMU doesn't support 2-stage CD, limit the linear > + * tables to a reasonable number of contexts, let's say > + * 64kB / sizeof(ctx_desc) = 1024 = 2^10 ctx_desc is 26B so 11bits would be OK > + */ > + if (!(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB)) > + master->ssid_bits = min(master->ssid_bits, 10U); > + > group = iommu_group_get_for_dev(dev); > if (!IS_ERR(group)) { > iommu_group_put(group); > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index f04a6df65eb8..04f4f6b95d82 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -206,8 +206,12 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, > if (err) > break; > } > - } > > + fwspec = dev_iommu_fwspec_get(dev); > + if (!err && fwspec) > + of_property_read_u32(master_np, "pasid-num-bits", > + &fwspec->num_pasid_bits); > + } > > /* > * Two success conditions can be represented by non-negative err here: > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 519e40fb23ce..b91df613385f 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -536,6 +536,7 @@ struct iommu_fwspec { > struct fwnode_handle *iommu_fwnode; > void *iommu_priv; > u32 flags; > + u32 num_pasid_bits; > unsigned int num_ids; > u32 ids[1]; > }; > Thanks Eric
Hi Eric, Sorry for the delay. I'll see if I can resend this for v5.5, although I can't do much testing at the moment. On Mon, Jul 08, 2019 at 09:58:22AM +0200, Auger Eric wrote: > Hi Jean, > > On 6/10/19 8:47 PM, Jean-Philippe Brucker wrote: > > For platform devices that support SubstreamID (SSID), firmware provides > > the number of supported SSID bits. Restrict it to what the SMMU supports > > and cache it into master->ssid_bits. > The commit message may give the impression the master's ssid_bits field > only is used for platform devices. Ok maybe I should add that this field will be used for PCI PASID as well. > > @@ -2097,6 +2098,16 @@ static int arm_smmu_add_device(struct device *dev) > > } > > } > > > > + master->ssid_bits = min(smmu->ssid_bits, fwspec->num_pasid_bits); > In case the device is a PCI device, what is the value taken by > fwspec->num_pasid_bits? It would be zero, as firmware only specifies a value for platform devices. For a PCI device, patch 8/8 fills master->ssid_bits from the PCIe PASID capability. > > + /* > > + * If the SMMU doesn't support 2-stage CD, limit the linear > > + * tables to a reasonable number of contexts, let's say > > + * 64kB / sizeof(ctx_desc) = 1024 = 2^10 > ctx_desc is 26B so 11bits would be OK This refers to the size of the hardware context descriptor, not struct arm_smmu_ctx_desc. Next version moves this to a define and makes it clearer. Thanks, Jean
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index 4d5a694f02c2..3254f473e681 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -604,6 +604,7 @@ struct arm_smmu_master { struct list_head domain_head; u32 *sids; unsigned int num_sids; + unsigned int ssid_bits; bool ats_enabled :1; }; @@ -2097,6 +2098,16 @@ static int arm_smmu_add_device(struct device *dev) } } + master->ssid_bits = min(smmu->ssid_bits, fwspec->num_pasid_bits); + + /* + * If the SMMU doesn't support 2-stage CD, limit the linear + * tables to a reasonable number of contexts, let's say + * 64kB / sizeof(ctx_desc) = 1024 = 2^10 + */ + if (!(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB)) + master->ssid_bits = min(master->ssid_bits, 10U); + group = iommu_group_get_for_dev(dev); if (!IS_ERR(group)) { iommu_group_put(group); diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index f04a6df65eb8..04f4f6b95d82 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -206,8 +206,12 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, if (err) break; } - } + fwspec = dev_iommu_fwspec_get(dev); + if (!err && fwspec) + of_property_read_u32(master_np, "pasid-num-bits", + &fwspec->num_pasid_bits); + } /* * Two success conditions can be represented by non-negative err here: diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 519e40fb23ce..b91df613385f 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -536,6 +536,7 @@ struct iommu_fwspec { struct fwnode_handle *iommu_fwnode; void *iommu_priv; u32 flags; + u32 num_pasid_bits; unsigned int num_ids; u32 ids[1]; };
For platform devices that support SubstreamID (SSID), firmware provides the number of supported SSID bits. Restrict it to what the SMMU supports and cache it into master->ssid_bits. Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com> --- drivers/iommu/arm-smmu-v3.c | 11 +++++++++++ drivers/iommu/of_iommu.c | 6 +++++- include/linux/iommu.h | 1 + 3 files changed, 17 insertions(+), 1 deletion(-)