diff mbox series

[2/3] firmware: qcom: scm: Add function to set the maximum IOMMU pool size

Message ID 20211208083423.22037-3-marijn.suijten@somainline.org (mailing list archive)
State Accepted
Headers show
Series firmware: qcom: scm: Add IOMMU pool size and | expand

Commit Message

Marijn Suijten Dec. 8, 2021, 8:34 a.m. UTC 
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

This is not necessary for basic functionality of the IOMMU, but
it's an optimization that tells to the TZ what's the maximum
mappable size for the secure IOMMUs, so that it can optimize
the data structures in the TZ itself.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
[Marijn: ported from 5.3 to the unified architecture in 5.11]
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: Konrad Dybcio <konrad.dybcio@somainline.org>
---
 drivers/firmware/qcom_scm.c | 15 +++++++++++++++
 drivers/firmware/qcom_scm.h |  1 +
 include/linux/qcom_scm.h    |  1 +
 3 files changed, 17 insertions(+)

Comments

Alex Elder Dec. 8, 2021, 1:30 p.m. UTC | #1 
On 12/8/21 2:34 AM, Marijn Suijten wrote:
> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> 
> This is not necessary for basic functionality of the IOMMU, but
> it's an optimization that tells to the TZ what's the maximum
> mappable size for the secure IOMMUs, so that it can optimize
> the data structures in the TZ itself.

Are there no users of this function?	-Alex

> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> [Marijn: ported from 5.3 to the unified architecture in 5.11]
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@somainline.org>
> ---
>   drivers/firmware/qcom_scm.c | 15 +++++++++++++++
>   drivers/firmware/qcom_scm.h |  1 +
>   include/linux/qcom_scm.h    |  1 +
>   3 files changed, 17 insertions(+)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 3f67bf774821..d5a9ba15e2ba 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -759,6 +759,21 @@ int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare)
>   }
>   EXPORT_SYMBOL(qcom_scm_iommu_secure_ptbl_init);
>   
> +int qcom_scm_iommu_set_cp_pool_size(u32 spare, u32 size)
> +{
> +	struct qcom_scm_desc desc = {
> +		.svc = QCOM_SCM_SVC_MP,
> +		.cmd = QCOM_SCM_MP_IOMMU_SET_CP_POOL_SIZE,
> +		.arginfo = QCOM_SCM_ARGS(2),
> +		.args[0] = size,
> +		.args[1] = spare,
> +		.owner = ARM_SMCCC_OWNER_SIP,
> +	};
> +
> +	return qcom_scm_call(__scm->dev, &desc, NULL);
> +}
> +EXPORT_SYMBOL(qcom_scm_iommu_set_cp_pool_size);
> +
>   int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
>   				   u32 cp_nonpixel_start,
>   				   u32 cp_nonpixel_size)
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index d92156ceb3ac..bb627941702b 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -100,6 +100,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
>   #define QCOM_SCM_MP_RESTORE_SEC_CFG		0x02
>   #define QCOM_SCM_MP_IOMMU_SECURE_PTBL_SIZE	0x03
>   #define QCOM_SCM_MP_IOMMU_SECURE_PTBL_INIT	0x04
> +#define QCOM_SCM_MP_IOMMU_SET_CP_POOL_SIZE	0x05
>   #define QCOM_SCM_MP_VIDEO_VAR			0x08
>   #define QCOM_SCM_MP_ASSIGN			0x16
>   
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index 81cad9e1e412..8a065f8660c1 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -83,6 +83,7 @@ extern bool qcom_scm_restore_sec_cfg_available(void);
>   extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
>   extern int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size);
>   extern int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare);
> +extern int qcom_scm_iommu_set_cp_pool_size(u32 spare, u32 size);
>   extern int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
>   					  u32 cp_nonpixel_start,
>   					  u32 cp_nonpixel_size);
>
Marijn Suijten Dec. 8, 2021, 11:44 p.m. UTC | #2 
On 2021-12-08 07:30:28, Alex Elder wrote:
> On 12/8/21 2:34 AM, Marijn Suijten wrote:
> > From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > 
> > This is not necessary for basic functionality of the IOMMU, but
> > it's an optimization that tells to the TZ what's the maximum
> > mappable size for the secure IOMMUs, so that it can optimize
> > the data structures in the TZ itself.
> 
> Are there no users of this function?	-Alex

I should have probably mentioned in the cover letter that this function
and the one introduced in patch 3/3 are going to be used in upcoming
patches for IOMMUs found in msm8976, msm8974 and related SoCs (with the
side-note that I don't see this particular set_cp_pool_size used in the
branch that this was submitted from, but it's most likely used elsewhere
or planned ahead to be used in the near future - I expect Angelo to be
able to comment on that more accurately).

> > 
> > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > [Marijn: ported from 5.3 to the unified architecture in 5.11]
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@somainline.org>
> > ---
> >   drivers/firmware/qcom_scm.c | 15 +++++++++++++++
> >   drivers/firmware/qcom_scm.h |  1 +
> >   include/linux/qcom_scm.h    |  1 +
> >   3 files changed, 17 insertions(+)
> > 
> > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > index 3f67bf774821..d5a9ba15e2ba 100644
> > --- a/drivers/firmware/qcom_scm.c
> > +++ b/drivers/firmware/qcom_scm.c
> > @@ -759,6 +759,21 @@ int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare)
> >   }
> >   EXPORT_SYMBOL(qcom_scm_iommu_secure_ptbl_init);
> >   
> > +int qcom_scm_iommu_set_cp_pool_size(u32 spare, u32 size)
> > +{
> > +	struct qcom_scm_desc desc = {
> > +		.svc = QCOM_SCM_SVC_MP,
> > +		.cmd = QCOM_SCM_MP_IOMMU_SET_CP_POOL_SIZE,
> > +		.arginfo = QCOM_SCM_ARGS(2),
> > +		.args[0] = size,
> > +		.args[1] = spare,
> > +		.owner = ARM_SMCCC_OWNER_SIP,
> > +	};
> > +
> > +	return qcom_scm_call(__scm->dev, &desc, NULL);
> > +}
> > +EXPORT_SYMBOL(qcom_scm_iommu_set_cp_pool_size);
> > +
> >   int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
> >   				   u32 cp_nonpixel_start,
> >   				   u32 cp_nonpixel_size)
> > diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> > index d92156ceb3ac..bb627941702b 100644
> > --- a/drivers/firmware/qcom_scm.h
> > +++ b/drivers/firmware/qcom_scm.h
> > @@ -100,6 +100,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
> >   #define QCOM_SCM_MP_RESTORE_SEC_CFG		0x02
> >   #define QCOM_SCM_MP_IOMMU_SECURE_PTBL_SIZE	0x03
> >   #define QCOM_SCM_MP_IOMMU_SECURE_PTBL_INIT	0x04
> > +#define QCOM_SCM_MP_IOMMU_SET_CP_POOL_SIZE	0x05
> >   #define QCOM_SCM_MP_VIDEO_VAR			0x08
> >   #define QCOM_SCM_MP_ASSIGN			0x16
> >   
> > diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> > index 81cad9e1e412..8a065f8660c1 100644
> > --- a/include/linux/qcom_scm.h
> > +++ b/include/linux/qcom_scm.h
> > @@ -83,6 +83,7 @@ extern bool qcom_scm_restore_sec_cfg_available(void);
> >   extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
> >   extern int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size);
> >   extern int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare);
> > +extern int qcom_scm_iommu_set_cp_pool_size(u32 spare, u32 size);
> >   extern int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
> >   					  u32 cp_nonpixel_start,
> >   					  u32 cp_nonpixel_size);
> > 
>
AngeloGioacchino Del Regno Dec. 10, 2021, 4:28 p.m. UTC | #3 
Il 09/12/21 00:44, Marijn Suijten ha scritto:
> On 2021-12-08 07:30:28, Alex Elder wrote:
>> On 12/8/21 2:34 AM, Marijn Suijten wrote:
>>> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>>>
>>> This is not necessary for basic functionality of the IOMMU, but
>>> it's an optimization that tells to the TZ what's the maximum
>>> mappable size for the secure IOMMUs, so that it can optimize
>>> the data structures in the TZ itself.
>>
>> Are there no users of this function?	-Alex
> 
> I should have probably mentioned in the cover letter that this function
> and the one introduced in patch 3/3 are going to be used in upcoming
> patches for IOMMUs found in msm8976, msm8974 and related SoCs (with the
> side-note that I don't see this particular set_cp_pool_size used in the
> branch that this was submitted from, but it's most likely used elsewhere
> or planned ahead to be used in the near future - I expect Angelo to be
> able to comment on that more accurately).
> 

This function is used in the secured iommu pagetable setup, but not for
all of the "SCM feature versions" (only for version >= 1.1.1, downstream
reads it with a call to scm_feat_version()).

It's not strictly necessary for functionality, hence why Marijn isn't
seeing any call to this in the branch that he was browsing: the spirit here
is to first introduce code that does a minimal (but, of course, working)
setup of the IOMMUs found in MSM8956/76 (which can be adapted with very
minimal changes to other SoCs), and *only then* to add these performance
enhancements to the mix.

...and this is why this commit is here :))

By the way, if my memory isn't failing me, that SCM call should be usable
on all AArch64 SoCs (so, 8956, 8994 and the rest).
Alex Elder Dec. 10, 2021, 9:44 p.m. UTC | #4 
On 12/10/21 10:28 AM, AngeloGioacchino Del Regno wrote:
> Il 09/12/21 00:44, Marijn Suijten ha scritto:
>> On 2021-12-08 07:30:28, Alex Elder wrote:
>>> On 12/8/21 2:34 AM, Marijn Suijten wrote:
>>>> From: AngeloGioacchino Del Regno 
>>>> <angelogioacchino.delregno@somainline.org>
>>>>
>>>> This is not necessary for basic functionality of the IOMMU, but
>>>> it's an optimization that tells to the TZ what's the maximum
>>>> mappable size for the secure IOMMUs, so that it can optimize
>>>> the data structures in the TZ itself.
>>>
>>> Are there no users of this function?    -Alex
>>
>> I should have probably mentioned in the cover letter that this function
>> and the one introduced in patch 3/3 are going to be used in upcoming
>> patches for IOMMUs found in msm8976, msm8974 and related SoCs (with the
>> side-note that I don't see this particular set_cp_pool_size used in the
>> branch that this was submitted from, but it's most likely used elsewhere
>> or planned ahead to be used in the near future - I expect Angelo to be
>> able to comment on that more accurately).
>>
> 
> This function is used in the secured iommu pagetable setup, but not for
> all of the "SCM feature versions" (only for version >= 1.1.1, downstream
> reads it with a call to scm_feat_version()).
> 
> It's not strictly necessary for functionality, hence why Marijn isn't
> seeing any call to this in the branch that he was browsing: the spirit here
> is to first introduce code that does a minimal (but, of course, working)
> setup of the IOMMUs found in MSM8956/76 (which can be adapted with very
> minimal changes to other SoCs), and *only then* to add these performance
> enhancements to the mix.
> 
> ...and this is why this commit is here :))
> 
> By the way, if my memory isn't failing me, that SCM call should be usable
> on all AArch64 SoCs (so, 8956, 8994 and the rest).

Thanks for your responses.  I don't actually know what
you use as downstream for this.  If I did, I might
compare what I see there to what you do and confirm
it does the same thing...

So I'm not offering any review on this or patch 3.
But I'm glad to know the users of these functions
will be coming soon.

					-Alex
diff mbox series

Patch

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 3f67bf774821..d5a9ba15e2ba 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -759,6 +759,21 @@  int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare)
 }
 EXPORT_SYMBOL(qcom_scm_iommu_secure_ptbl_init);
 
+int qcom_scm_iommu_set_cp_pool_size(u32 spare, u32 size)
+{
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_MP,
+		.cmd = QCOM_SCM_MP_IOMMU_SET_CP_POOL_SIZE,
+		.arginfo = QCOM_SCM_ARGS(2),
+		.args[0] = size,
+		.args[1] = spare,
+		.owner = ARM_SMCCC_OWNER_SIP,
+	};
+
+	return qcom_scm_call(__scm->dev, &desc, NULL);
+}
+EXPORT_SYMBOL(qcom_scm_iommu_set_cp_pool_size);
+
 int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
 				   u32 cp_nonpixel_start,
 				   u32 cp_nonpixel_size)
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index d92156ceb3ac..bb627941702b 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -100,6 +100,7 @@  extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
 #define QCOM_SCM_MP_RESTORE_SEC_CFG		0x02
 #define QCOM_SCM_MP_IOMMU_SECURE_PTBL_SIZE	0x03
 #define QCOM_SCM_MP_IOMMU_SECURE_PTBL_INIT	0x04
+#define QCOM_SCM_MP_IOMMU_SET_CP_POOL_SIZE	0x05
 #define QCOM_SCM_MP_VIDEO_VAR			0x08
 #define QCOM_SCM_MP_ASSIGN			0x16
 
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index 81cad9e1e412..8a065f8660c1 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -83,6 +83,7 @@  extern bool qcom_scm_restore_sec_cfg_available(void);
 extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
 extern int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size);
 extern int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare);
+extern int qcom_scm_iommu_set_cp_pool_size(u32 spare, u32 size);
 extern int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
 					  u32 cp_nonpixel_start,
 					  u32 cp_nonpixel_size);