| Message ID | 1443556138-21085-5-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On 09/29, Rob Clark wrote: > diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c > index c1e4325..e1ac97f 100644 > --- a/drivers/firmware/qcom_scm-32.c > +++ b/drivers/firmware/qcom_scm-32.c > @@ -500,6 +500,59 @@ int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp) > req, req_cnt * sizeof(*req), resp, sizeof(*resp)); > } > > +int __qcom_scm_ocmem_secure_cfg(unsigned sec_id) > +{ > + int ret, scm_ret = 0; > + struct msm_scm_sec_cfg { We've left these as anonymous structs for things like qcom_scm_set_boot_addr(), maybe we should do the same here. > + __le32 id; > + __le32 spare; Also, the iommu driver would use this API and it uses this "spare" element, so perhaps this whole function should be renamed to be more generic and take two values. Downstream the function is called scm_restore_sec_cfg, so maybe something similar. And the service id is MP for "memory protection", so QCOM_SCM_OCMEM_SECURE_SVC could be QCOM_SCM_MEMORY_PROTECTION? Otherwise this patch looks good. > + } cfg; > + > + cfg.id = cpu_to_le32(sec_id); > + > + ret = qcom_scm_call(QCOM_SCM_OCMEM_SECURE_SVC, QCOM_SCM_OCMEM_SECURE_CFG, > + &cfg, sizeof(cfg), &scm_ret, sizeof(scm_ret)); > + > + if (ret || scm_ret)
On Tue, Sep 29, 2015 at 5:38 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 09/29, Rob Clark wrote: >> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c >> index c1e4325..e1ac97f 100644 >> --- a/drivers/firmware/qcom_scm-32.c >> +++ b/drivers/firmware/qcom_scm-32.c >> @@ -500,6 +500,59 @@ int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp) >> req, req_cnt * sizeof(*req), resp, sizeof(*resp)); >> } >> >> +int __qcom_scm_ocmem_secure_cfg(unsigned sec_id) >> +{ >> + int ret, scm_ret = 0; >> + struct msm_scm_sec_cfg { > > We've left these as anonymous structs for things like > qcom_scm_set_boot_addr(), maybe we should do the same here. > >> + __le32 id; >> + __le32 spare; > > Also, the iommu driver would use this API and it uses this > "spare" element, so perhaps this whole function should be renamed > to be more generic and take two values. Downstream the function > is called scm_restore_sec_cfg, so maybe something similar. And > the service id is MP for "memory protection", so > QCOM_SCM_OCMEM_SECURE_SVC could be QCOM_SCM_MEMORY_PROTECTION? heh, #define SCM_SVC_MP 0xC #define IOMMU_SECURE_CFG 2 vs. #define OCMEM_SECURE_SVC_ID 12 #define OCMEM_SECURE_CFG_ID 0x2 that wasn't obscure at all! Maybe then there is a better name than spare? Looks like downstream iommu calls it cb_num? BR, -R > Otherwise this patch looks good. > >> + } cfg; >> + >> + cfg.id = cpu_to_le32(sec_id); >> + >> + ret = qcom_scm_call(QCOM_SCM_OCMEM_SECURE_SVC, QCOM_SCM_OCMEM_SECURE_CFG, >> + &cfg, sizeof(cfg), &scm_ret, sizeof(scm_ret)); >> + >> + if (ret || scm_ret) > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/29, Rob Clark wrote: > On Tue, Sep 29, 2015 at 5:38 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > On 09/29, Rob Clark wrote: > >> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c > >> index c1e4325..e1ac97f 100644 > >> --- a/drivers/firmware/qcom_scm-32.c > >> +++ b/drivers/firmware/qcom_scm-32.c > >> @@ -500,6 +500,59 @@ int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp) > >> req, req_cnt * sizeof(*req), resp, sizeof(*resp)); > >> } > >> > >> +int __qcom_scm_ocmem_secure_cfg(unsigned sec_id) > >> +{ > >> + int ret, scm_ret = 0; > >> + struct msm_scm_sec_cfg { > > > > We've left these as anonymous structs for things like > > qcom_scm_set_boot_addr(), maybe we should do the same here. > > > >> + __le32 id; > >> + __le32 spare; > > > > Also, the iommu driver would use this API and it uses this > > "spare" element, so perhaps this whole function should be renamed > > to be more generic and take two values. Downstream the function > > is called scm_restore_sec_cfg, so maybe something similar. And > > the service id is MP for "memory protection", so > > QCOM_SCM_OCMEM_SECURE_SVC could be QCOM_SCM_MEMORY_PROTECTION? > > heh, > > #define SCM_SVC_MP 0xC > #define IOMMU_SECURE_CFG 2 > > vs. > > #define OCMEM_SECURE_SVC_ID 12 > #define OCMEM_SECURE_CFG_ID 0x2 > > that wasn't obscure at all! :) > > Maybe then there is a better name than spare? Looks like downstream > iommu calls it cb_num? Yeah I think that's the only use to indicate which context bank it is. Maybe we can have a single id configure API and a special iommu context bank API that both funnel into the same private two number API. Otherwise we have a bunch of callers passing 0 for the second argument because they don't care.
On Tue, Sep 29, 2015 at 6:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 09/29, Rob Clark wrote: >> On Tue, Sep 29, 2015 at 5:38 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> > On 09/29, Rob Clark wrote: >> >> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c >> >> index c1e4325..e1ac97f 100644 >> >> --- a/drivers/firmware/qcom_scm-32.c >> >> +++ b/drivers/firmware/qcom_scm-32.c >> >> @@ -500,6 +500,59 @@ int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp) >> >> req, req_cnt * sizeof(*req), resp, sizeof(*resp)); >> >> } >> >> >> >> +int __qcom_scm_ocmem_secure_cfg(unsigned sec_id) >> >> +{ >> >> + int ret, scm_ret = 0; >> >> + struct msm_scm_sec_cfg { >> > >> > We've left these as anonymous structs for things like >> > qcom_scm_set_boot_addr(), maybe we should do the same here. >> > >> >> + __le32 id; >> >> + __le32 spare; >> > >> > Also, the iommu driver would use this API and it uses this >> > "spare" element, so perhaps this whole function should be renamed >> > to be more generic and take two values. Downstream the function >> > is called scm_restore_sec_cfg, so maybe something similar. And >> > the service id is MP for "memory protection", so >> > QCOM_SCM_OCMEM_SECURE_SVC could be QCOM_SCM_MEMORY_PROTECTION? >> >> heh, >> >> #define SCM_SVC_MP 0xC >> #define IOMMU_SECURE_CFG 2 >> >> vs. >> >> #define OCMEM_SECURE_SVC_ID 12 >> #define OCMEM_SECURE_CFG_ID 0x2 >> >> that wasn't obscure at all! > > :) > >> >> Maybe then there is a better name than spare? Looks like downstream >> iommu calls it cb_num? > > Yeah I think that's the only use to indicate which context bank > it is. Maybe we can have a single id configure API and a special > iommu context bank API that both funnel into the same private two > number API. Otherwise we have a bunch of callers passing 0 for > the second argument because they don't care. so fwiw, I went thru all the downstream scm_call() callers.. there are a lot of callers to SCM_SVC_MP service (through a couple different #defines), but most of them are different cmd-id's. The ones using SECURE_CFG (0x2) are: * dwc3_msm_restore_sec_config() * ocmem_restore_sec_program() * msm_iommu_sec_program_iommu() so we have two points passing in zero for ctx-bank, one that does not. I don't think it is worth having two API's to save hard-coding zero in two places ;-) BR, -R > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/01, Rob Clark wrote: > On Tue, Sep 29, 2015 at 6:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > On 09/29, Rob Clark wrote: > >> On Tue, Sep 29, 2015 at 5:38 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > > > Yeah I think that's the only use to indicate which context bank > > it is. Maybe we can have a single id configure API and a special > > iommu context bank API that both funnel into the same private two > > number API. Otherwise we have a bunch of callers passing 0 for > > the second argument because they don't care. > > so fwiw, I went thru all the downstream scm_call() callers.. there > are a lot of callers to SCM_SVC_MP service (through a couple different > #defines), but most of them are different cmd-id's. The ones using > SECURE_CFG (0x2) are: > > * dwc3_msm_restore_sec_config() > * ocmem_restore_sec_program() > * msm_iommu_sec_program_iommu() > > so we have two points passing in zero for ctx-bank, one that does not. > I don't think it is worth having two API's to save hard-coding zero in > two places ;-) > What sources are you looking at? It seems like whatever you have is over a year old. About a year ago, we consolidated all calls to this specific SCM call into a single API called scm_restore_sec_cfg (see commit 9933a272db9a5612bcc2ee0ef9149f70c8166eb3 "qcom: scm: Provide an API that restores security configuration" on msm-3.10). Looking at our latest msm-3.10 branch I see drivers/crypto/msm/ice.c: ret = scm_restore_sec_cfg(cbuf.device_id, cbuf.spare, &scm_ret); cbuf.spare is 0 here. drivers/iommu/msm_iommu_sec.c: ret = scm_restore_sec_cfg(drvdata->sec_id, ctx_drvdata->num, &scm_ret); This is the only real user of spare drivers/pci/host/pci-msm.c: ret = scm_restore_sec_cfg(dev->scm_dev_id, 0, &scm_ret); drivers/scsi/ufs/ufs-qcom.c: ret = scm_restore_sec_cfg(cbuf.device_id, cbuf.spare, &scm_ret); cbuf.spare is 0 here. drivers/soc/qcom/ocmem_core.c: rc = scm_restore_sec_cfg(sec_id, 0, &scm_ret); drivers/video/msm/mdss/mdss_mdp.c: ret = scm_restore_sec_cfg(SEC_DEVICE_MDSS, 0, &scm_ret); So that's 6 callers and 1 uses the second argument.
diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c index c1e4325..e1ac97f 100644 --- a/drivers/firmware/qcom_scm-32.c +++ b/drivers/firmware/qcom_scm-32.c @@ -500,6 +500,59 @@ int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp) req, req_cnt * sizeof(*req), resp, sizeof(*resp)); } +int __qcom_scm_ocmem_secure_cfg(unsigned sec_id) +{ + int ret, scm_ret = 0; + struct msm_scm_sec_cfg { + __le32 id; + __le32 spare; + } cfg; + + cfg.id = cpu_to_le32(sec_id); + + ret = qcom_scm_call(QCOM_SCM_OCMEM_SECURE_SVC, QCOM_SCM_OCMEM_SECURE_CFG, + &cfg, sizeof(cfg), &scm_ret, sizeof(scm_ret)); + + if (ret || scm_ret) + return ret ? ret : -EINVAL; + + return 0; +} + +int __qcom_scm_ocmem_lock(u32 id, u32 offset, u32 size, u32 mode) +{ + struct ocmem_tz_lock { + __le32 id; + __le32 offset; + __le32 size; + __le32 mode; + } request; + + request.id = cpu_to_le32(id); + request.offset = cpu_to_le32(offset); + request.size = cpu_to_le32(size); + request.mode = cpu_to_le32(mode); + + return qcom_scm_call(QCOM_SCM_OCMEM_SVC, QCOM_SCM_OCMEM_LOCK_CMD, + &request, sizeof(request), NULL, 0); +} + +int __qcom_scm_ocmem_unlock(u32 id, u32 offset, u32 size) +{ + struct ocmem_tz_unlock { + __le32 id; + __le32 offset; + __le32 size; + } request; + + request.id = cpu_to_le32(id); + request.offset = cpu_to_le32(offset); + request.size = cpu_to_le32(size); + + return qcom_scm_call(QCOM_SCM_OCMEM_SVC, QCOM_SCM_OCMEM_UNLOCK_CMD, + &request, sizeof(request), NULL, 0); +} + bool __qcom_scm_pas_supported(u32 peripheral) { __le32 out; diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c index e64fd92..ef5c59e 100644 --- a/drivers/firmware/qcom_scm-64.c +++ b/drivers/firmware/qcom_scm-64.c @@ -62,6 +62,22 @@ int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp) return -ENOTSUPP; } +int __qcom_scm_ocmem_secure_cfg(unsigned sec_id) +{ + return -ENOTSUPP; +} + +int __qcom_scm_ocmem_lock(uint32_t id, uint32_t offset, uint32_t size, + uint32_t mode) +{ + return -ENOTSUPP; +} + +int __qcom_scm_ocmem_unlock(uint32_t id, uint32_t offset, uint32_t size) +{ + return -ENOTSUPP; +} + bool __qcom_scm_pas_supported(u32 peripheral) { return false; diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c index 118df0a..83a6b77 100644 --- a/drivers/firmware/qcom_scm.c +++ b/drivers/firmware/qcom_scm.c @@ -154,6 +154,67 @@ int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp) EXPORT_SYMBOL(qcom_scm_hdcp_req); /** + * qcom_scm_ocmem_secure_available() - Check if secure environment supports + * OCMEM. + * + * Return true if OCMEM secure interface is supported, false if not. + */ +bool qcom_scm_ocmem_secure_available(void) +{ + return __qcom_scm_is_call_available(QCOM_SCM_OCMEM_SECURE_SVC, + QCOM_SCM_OCMEM_SECURE_CFG); +} +EXPORT_SYMBOL(qcom_scm_ocmem_secure_available); + +/** + * qcom_scm_ocmem_secure_cfg() - call OCMEM secure cfg interface + */ +int qcom_scm_ocmem_secure_cfg(unsigned sec_id) +{ + return __qcom_scm_ocmem_secure_cfg(sec_id); +} +EXPORT_SYMBOL(qcom_scm_ocmem_secure_cfg); + +/** + * qcom_scm_ocmem_lock_available() - is OCMEM lock/unlock interface available + */ +bool qcom_scm_ocmem_lock_available(void) +{ + return __qcom_scm_is_call_available(QCOM_SCM_OCMEM_SVC, + QCOM_SCM_OCMEM_LOCK_CMD); +} +EXPORT_SYMBOL(qcom_scm_ocmem_lock_available); + +/** + * qcom_scm_ocmem_lock() - call OCMEM lock interface to assign an OCMEM + * region to the specified initiator + * + * @id: tz initiator id + * @offset: OCMEM offset + * @size: OCMEM size + * @mode: access mode (WIDE/NARROW) + */ +int qcom_scm_ocmem_lock(u32 id, u32 offset, u32 size, u32 mode) +{ + return __qcom_scm_ocmem_lock(id, offset, size, mode); +} +EXPORT_SYMBOL(qcom_scm_ocmem_lock); + +/** + * qcom_scm_ocmem_unlock() - call OCMEM unlock interface to release an OCMEM + * region from the specified initiator + * + * @id: tz initiator id + * @offset: OCMEM offset + * @size: OCMEM size + */ +int qcom_scm_ocmem_unlock(u32 id, u32 offset, u32 size) +{ + return __qcom_scm_ocmem_unlock(id, offset, size); +} +EXPORT_SYMBOL(qcom_scm_ocmem_unlock); + +/** * qcom_scm_pas_supported() - Check if the peripheral authentication service is * available for the given peripherial * @peripheral: peripheral id diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h index 220d19c..c41228d 100644 --- a/drivers/firmware/qcom_scm.h +++ b/drivers/firmware/qcom_scm.h @@ -36,6 +36,18 @@ extern int __qcom_scm_is_call_available(u32 svc_id, u32 cmd_id); extern int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp); +#define QCOM_SCM_OCMEM_SECURE_SVC 0xc +#define QCOM_SCM_OCMEM_SECURE_CFG 0x2 + +extern int __qcom_scm_ocmem_secure_cfg(unsigned sec_id); + +#define QCOM_SCM_OCMEM_SVC 0xf +#define QCOM_SCM_OCMEM_LOCK_CMD 0x1 +#define QCOM_SCM_OCMEM_UNLOCK_CMD 0x2 + +extern int __qcom_scm_ocmem_lock(u32 id, u32 offset, u32 size, u32 mode); +extern int __qcom_scm_ocmem_unlock(u32 id, u32 offset, u32 size); + #define QCOM_SCM_SVC_PIL 0x2 #define QCOM_SCM_PAS_INIT_IMAGE_CMD 0x1 #define QCOM_SCM_PAS_MEM_SETUP_CMD 0x2 diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h index e407c0a..86bc6ba7 100644 --- a/include/linux/qcom_scm.h +++ b/include/linux/qcom_scm.h @@ -32,6 +32,13 @@ extern bool qcom_scm_hdcp_available(void); extern int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp); +extern bool qcom_scm_ocmem_secure_available(void); +extern int qcom_scm_ocmem_secure_cfg(unsigned sec_id); + +extern bool qcom_scm_ocmem_lock_available(void); +extern int qcom_scm_ocmem_lock(u32 id, u32 offset, u32 size, u32 mode); +extern int qcom_scm_ocmem_unlock(u32 id, u32 offset, u32 size); + extern bool qcom_scm_pas_supported(u32 peripheral); extern int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size); extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size);
Add interfaces needed for configuring OCMEM. Signed-off-by: Rob Clark <robdclark@gmail.com> --- drivers/firmware/qcom_scm-32.c | 53 ++++++++++++++++++++++++++++++++++++ drivers/firmware/qcom_scm-64.c | 16 +++++++++++ drivers/firmware/qcom_scm.c | 61 ++++++++++++++++++++++++++++++++++++++++++ drivers/firmware/qcom_scm.h | 12 +++++++++ include/linux/qcom_scm.h | 7 +++++ 5 files changed, 149 insertions(+)