| Message ID | a4e454630e57aedd9da6a4ba40c8e1c415bb6836.1604048969.git.saiprakash.ranjan@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Fri, Oct 30, 2020 at 02:53:09PM +0530, Sai Prakash Ranjan wrote: > Add iommu domain attribute for using system cache aka last level > cache by client drivers like GPU to set right attributes for caching > the hardware pagetables into the system cache. > > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> > --- > drivers/iommu/arm/arm-smmu/arm-smmu.c | 17 +++++++++++++++++ > drivers/iommu/arm/arm-smmu/arm-smmu.h | 1 + > include/linux/iommu.h | 1 + > 3 files changed, 19 insertions(+) > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c > index b1cf8f0abc29..070d13f80c7e 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c > @@ -789,6 +789,9 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain, > if (smmu_domain->non_strict) > pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT; > > + if (smmu_domain->sys_cache) > + pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_SYS_CACHE; > + > pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain); > if (!pgtbl_ops) { > ret = -ENOMEM; > @@ -1520,6 +1523,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain, > case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE: > *(int *)data = smmu_domain->non_strict; > return 0; > + case DOMAIN_ATTR_SYS_CACHE: > + *((int *)data) = smmu_domain->sys_cache; > + return 0; > default: > return -ENODEV; > } > @@ -1551,6 +1557,17 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain, > else > smmu_domain->stage = ARM_SMMU_DOMAIN_S1; > break; > + case DOMAIN_ATTR_SYS_CACHE: > + if (smmu_domain->smmu) { > + ret = -EPERM; > + goto out_unlock; > + } > + > + if (*((int *)data)) > + smmu_domain->sys_cache = true; > + else > + smmu_domain->sys_cache = false; > + break; > default: > ret = -ENODEV; > } > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h > index 885840f3bec8..dfc44d806671 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h > @@ -373,6 +373,7 @@ struct arm_smmu_domain { > struct mutex init_mutex; /* Protects smmu pointer */ > spinlock_t cb_lock; /* Serialises ATS1* ops and TLB syncs */ > struct iommu_domain domain; > + bool sys_cache; > }; > > struct arm_smmu_master_cfg { > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index b95a6f8db6ff..4f4bb9c6f8f6 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -118,6 +118,7 @@ enum iommu_attr { > DOMAIN_ATTR_FSL_PAMUV1, > DOMAIN_ATTR_NESTING, /* two stages of translation */ > DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE, > + DOMAIN_ATTR_SYS_CACHE, I think you're trying to make this look generic, but it's really not. If we need to funnel io-pgtable quirks through domain attributes, then I think we should be open about that and add something like DOMAIN_ATTR_IO_PGTABLE_CFG which could take a struct of page-table configuration data for the domain (this could just be quirks initially, but maybe it's worth extending to take ias, oas and page size) Will
On 2020-11-10 17:48, Will Deacon wrote: > On Fri, Oct 30, 2020 at 02:53:09PM +0530, Sai Prakash Ranjan wrote: >> Add iommu domain attribute for using system cache aka last level >> cache by client drivers like GPU to set right attributes for caching >> the hardware pagetables into the system cache. >> >> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> >> --- >> drivers/iommu/arm/arm-smmu/arm-smmu.c | 17 +++++++++++++++++ >> drivers/iommu/arm/arm-smmu/arm-smmu.h | 1 + >> include/linux/iommu.h | 1 + >> 3 files changed, 19 insertions(+) >> >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c >> b/drivers/iommu/arm/arm-smmu/arm-smmu.c >> index b1cf8f0abc29..070d13f80c7e 100644 >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c >> @@ -789,6 +789,9 @@ static int arm_smmu_init_domain_context(struct >> iommu_domain *domain, >> if (smmu_domain->non_strict) >> pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT; >> >> + if (smmu_domain->sys_cache) >> + pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_SYS_CACHE; >> + >> pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain); >> if (!pgtbl_ops) { >> ret = -ENOMEM; >> @@ -1520,6 +1523,9 @@ static int arm_smmu_domain_get_attr(struct >> iommu_domain *domain, >> case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE: >> *(int *)data = smmu_domain->non_strict; >> return 0; >> + case DOMAIN_ATTR_SYS_CACHE: >> + *((int *)data) = smmu_domain->sys_cache; >> + return 0; >> default: >> return -ENODEV; >> } >> @@ -1551,6 +1557,17 @@ static int arm_smmu_domain_set_attr(struct >> iommu_domain *domain, >> else >> smmu_domain->stage = ARM_SMMU_DOMAIN_S1; >> break; >> + case DOMAIN_ATTR_SYS_CACHE: >> + if (smmu_domain->smmu) { >> + ret = -EPERM; >> + goto out_unlock; >> + } >> + >> + if (*((int *)data)) >> + smmu_domain->sys_cache = true; >> + else >> + smmu_domain->sys_cache = false; >> + break; >> default: >> ret = -ENODEV; >> } >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h >> b/drivers/iommu/arm/arm-smmu/arm-smmu.h >> index 885840f3bec8..dfc44d806671 100644 >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h >> @@ -373,6 +373,7 @@ struct arm_smmu_domain { >> struct mutex init_mutex; /* Protects smmu pointer */ >> spinlock_t cb_lock; /* Serialises ATS1* ops and TLB syncs */ >> struct iommu_domain domain; >> + bool sys_cache; >> }; >> >> struct arm_smmu_master_cfg { >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h >> index b95a6f8db6ff..4f4bb9c6f8f6 100644 >> --- a/include/linux/iommu.h >> +++ b/include/linux/iommu.h >> @@ -118,6 +118,7 @@ enum iommu_attr { >> DOMAIN_ATTR_FSL_PAMUV1, >> DOMAIN_ATTR_NESTING, /* two stages of translation */ >> DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE, >> + DOMAIN_ATTR_SYS_CACHE, > > I think you're trying to make this look generic, but it's really not. > If we need to funnel io-pgtable quirks through domain attributes, then > I > think we should be open about that and add something like > DOMAIN_ATTR_IO_PGTABLE_CFG which could take a struct of page-table > configuration data for the domain (this could just be quirks initially, > but maybe it's worth extending to take ias, oas and page size) > Actually the initial versions used DOMAIN_ATTR_QCOM_SYS_CACHE to make it QCOM specific and not generic, I don't see anyone else using this attribute, would that work? Thanks, Sai -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
On Wed, Nov 11, 2020 at 12:10:50PM +0530, Sai Prakash Ranjan wrote: > On 2020-11-10 17:48, Will Deacon wrote: > > On Fri, Oct 30, 2020 at 02:53:09PM +0530, Sai Prakash Ranjan wrote: > > > Add iommu domain attribute for using system cache aka last level > > > cache by client drivers like GPU to set right attributes for caching > > > the hardware pagetables into the system cache. > > > > > > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> > > > --- > > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 17 +++++++++++++++++ > > > drivers/iommu/arm/arm-smmu/arm-smmu.h | 1 + > > > include/linux/iommu.h | 1 + > > > 3 files changed, 19 insertions(+) > > > > > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c > > > b/drivers/iommu/arm/arm-smmu/arm-smmu.c > > > index b1cf8f0abc29..070d13f80c7e 100644 > > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c > > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c > > > @@ -789,6 +789,9 @@ static int arm_smmu_init_domain_context(struct > > > iommu_domain *domain, > > > if (smmu_domain->non_strict) > > > pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT; > > > > > > + if (smmu_domain->sys_cache) > > > + pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_SYS_CACHE; > > > + > > > pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain); > > > if (!pgtbl_ops) { > > > ret = -ENOMEM; > > > @@ -1520,6 +1523,9 @@ static int arm_smmu_domain_get_attr(struct > > > iommu_domain *domain, > > > case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE: > > > *(int *)data = smmu_domain->non_strict; > > > return 0; > > > + case DOMAIN_ATTR_SYS_CACHE: > > > + *((int *)data) = smmu_domain->sys_cache; > > > + return 0; > > > default: > > > return -ENODEV; > > > } > > > @@ -1551,6 +1557,17 @@ static int arm_smmu_domain_set_attr(struct > > > iommu_domain *domain, > > > else > > > smmu_domain->stage = ARM_SMMU_DOMAIN_S1; > > > break; > > > + case DOMAIN_ATTR_SYS_CACHE: > > > + if (smmu_domain->smmu) { > > > + ret = -EPERM; > > > + goto out_unlock; > > > + } > > > + > > > + if (*((int *)data)) > > > + smmu_domain->sys_cache = true; > > > + else > > > + smmu_domain->sys_cache = false; > > > + break; > > > default: > > > ret = -ENODEV; > > > } > > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h > > > b/drivers/iommu/arm/arm-smmu/arm-smmu.h > > > index 885840f3bec8..dfc44d806671 100644 > > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h > > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h > > > @@ -373,6 +373,7 @@ struct arm_smmu_domain { > > > struct mutex init_mutex; /* Protects smmu pointer */ > > > spinlock_t cb_lock; /* Serialises ATS1* ops and TLB syncs */ > > > struct iommu_domain domain; > > > + bool sys_cache; > > > }; > > > > > > struct arm_smmu_master_cfg { > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > > index b95a6f8db6ff..4f4bb9c6f8f6 100644 > > > --- a/include/linux/iommu.h > > > +++ b/include/linux/iommu.h > > > @@ -118,6 +118,7 @@ enum iommu_attr { > > > DOMAIN_ATTR_FSL_PAMUV1, > > > DOMAIN_ATTR_NESTING, /* two stages of translation */ > > > DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE, > > > + DOMAIN_ATTR_SYS_CACHE, > > > > I think you're trying to make this look generic, but it's really not. > > If we need to funnel io-pgtable quirks through domain attributes, then I > > think we should be open about that and add something like > > DOMAIN_ATTR_IO_PGTABLE_CFG which could take a struct of page-table > > configuration data for the domain (this could just be quirks initially, > > but maybe it's worth extending to take ias, oas and page size) > > > > Actually the initial versions used DOMAIN_ATTR_QCOM_SYS_CACHE > to make it QCOM specific and not generic, I don't see anyone else > using this attribute, would that work? No -- I'd prefer to have _one_ domain attribute for funneling all the IP_PGTABLE_CFG data. Otherwise, we'll just end up with things like DOMAIN_ATTR_QCOM_SYS_CACHE_EXT or DOMAIN_ATTR_QCOM_QUIRKS later on. Will
On 2020-11-12 15:05, Will Deacon wrote: > On Wed, Nov 11, 2020 at 12:10:50PM +0530, Sai Prakash Ranjan wrote: >> On 2020-11-10 17:48, Will Deacon wrote: >> > On Fri, Oct 30, 2020 at 02:53:09PM +0530, Sai Prakash Ranjan wrote: >> > > Add iommu domain attribute for using system cache aka last level >> > > cache by client drivers like GPU to set right attributes for caching >> > > the hardware pagetables into the system cache. >> > > >> > > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> >> > > --- >> > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 17 +++++++++++++++++ >> > > drivers/iommu/arm/arm-smmu/arm-smmu.h | 1 + >> > > include/linux/iommu.h | 1 + >> > > 3 files changed, 19 insertions(+) >> > > >> > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c >> > > b/drivers/iommu/arm/arm-smmu/arm-smmu.c >> > > index b1cf8f0abc29..070d13f80c7e 100644 >> > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c >> > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c >> > > @@ -789,6 +789,9 @@ static int arm_smmu_init_domain_context(struct >> > > iommu_domain *domain, >> > > if (smmu_domain->non_strict) >> > > pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT; >> > > >> > > + if (smmu_domain->sys_cache) >> > > + pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_SYS_CACHE; >> > > + >> > > pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain); >> > > if (!pgtbl_ops) { >> > > ret = -ENOMEM; >> > > @@ -1520,6 +1523,9 @@ static int arm_smmu_domain_get_attr(struct >> > > iommu_domain *domain, >> > > case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE: >> > > *(int *)data = smmu_domain->non_strict; >> > > return 0; >> > > + case DOMAIN_ATTR_SYS_CACHE: >> > > + *((int *)data) = smmu_domain->sys_cache; >> > > + return 0; >> > > default: >> > > return -ENODEV; >> > > } >> > > @@ -1551,6 +1557,17 @@ static int arm_smmu_domain_set_attr(struct >> > > iommu_domain *domain, >> > > else >> > > smmu_domain->stage = ARM_SMMU_DOMAIN_S1; >> > > break; >> > > + case DOMAIN_ATTR_SYS_CACHE: >> > > + if (smmu_domain->smmu) { >> > > + ret = -EPERM; >> > > + goto out_unlock; >> > > + } >> > > + >> > > + if (*((int *)data)) >> > > + smmu_domain->sys_cache = true; >> > > + else >> > > + smmu_domain->sys_cache = false; >> > > + break; >> > > default: >> > > ret = -ENODEV; >> > > } >> > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h >> > > b/drivers/iommu/arm/arm-smmu/arm-smmu.h >> > > index 885840f3bec8..dfc44d806671 100644 >> > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h >> > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h >> > > @@ -373,6 +373,7 @@ struct arm_smmu_domain { >> > > struct mutex init_mutex; /* Protects smmu pointer */ >> > > spinlock_t cb_lock; /* Serialises ATS1* ops and TLB syncs */ >> > > struct iommu_domain domain; >> > > + bool sys_cache; >> > > }; >> > > >> > > struct arm_smmu_master_cfg { >> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h >> > > index b95a6f8db6ff..4f4bb9c6f8f6 100644 >> > > --- a/include/linux/iommu.h >> > > +++ b/include/linux/iommu.h >> > > @@ -118,6 +118,7 @@ enum iommu_attr { >> > > DOMAIN_ATTR_FSL_PAMUV1, >> > > DOMAIN_ATTR_NESTING, /* two stages of translation */ >> > > DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE, >> > > + DOMAIN_ATTR_SYS_CACHE, >> > >> > I think you're trying to make this look generic, but it's really not. >> > If we need to funnel io-pgtable quirks through domain attributes, then I >> > think we should be open about that and add something like >> > DOMAIN_ATTR_IO_PGTABLE_CFG which could take a struct of page-table >> > configuration data for the domain (this could just be quirks initially, >> > but maybe it's worth extending to take ias, oas and page size) >> > >> >> Actually the initial versions used DOMAIN_ATTR_QCOM_SYS_CACHE >> to make it QCOM specific and not generic, I don't see anyone else >> using this attribute, would that work? > > No -- I'd prefer to have _one_ domain attribute for funneling all the > IP_PGTABLE_CFG data. Otherwise, we'll just end up with things like > DOMAIN_ATTR_QCOM_SYS_CACHE_EXT or DOMAIN_ATTR_QCOM_QUIRKS later on. > Right, that makes sense. I will add this in next version. Thanks, Sai -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c index b1cf8f0abc29..070d13f80c7e 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c @@ -789,6 +789,9 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain, if (smmu_domain->non_strict) pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT; + if (smmu_domain->sys_cache) + pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_SYS_CACHE; + pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain); if (!pgtbl_ops) { ret = -ENOMEM; @@ -1520,6 +1523,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain, case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE: *(int *)data = smmu_domain->non_strict; return 0; + case DOMAIN_ATTR_SYS_CACHE: + *((int *)data) = smmu_domain->sys_cache; + return 0; default: return -ENODEV; } @@ -1551,6 +1557,17 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain, else smmu_domain->stage = ARM_SMMU_DOMAIN_S1; break; + case DOMAIN_ATTR_SYS_CACHE: + if (smmu_domain->smmu) { + ret = -EPERM; + goto out_unlock; + } + + if (*((int *)data)) + smmu_domain->sys_cache = true; + else + smmu_domain->sys_cache = false; + break; default: ret = -ENODEV; } diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h index 885840f3bec8..dfc44d806671 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h @@ -373,6 +373,7 @@ struct arm_smmu_domain { struct mutex init_mutex; /* Protects smmu pointer */ spinlock_t cb_lock; /* Serialises ATS1* ops and TLB syncs */ struct iommu_domain domain; + bool sys_cache; }; struct arm_smmu_master_cfg { diff --git a/include/linux/iommu.h b/include/linux/iommu.h index b95a6f8db6ff..4f4bb9c6f8f6 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -118,6 +118,7 @@ enum iommu_attr { DOMAIN_ATTR_FSL_PAMUV1, DOMAIN_ATTR_NESTING, /* two stages of translation */ DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE, + DOMAIN_ATTR_SYS_CACHE, DOMAIN_ATTR_MAX, };
Add iommu domain attribute for using system cache aka last level cache by client drivers like GPU to set right attributes for caching the hardware pagetables into the system cache. Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> --- drivers/iommu/arm/arm-smmu/arm-smmu.c | 17 +++++++++++++++++ drivers/iommu/arm/arm-smmu/arm-smmu.h | 1 + include/linux/iommu.h | 1 + 3 files changed, 19 insertions(+)