| Message ID | 1492031718-28477-3-git-send-email-jcrouse@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed 12 Apr 14:15 PDT 2017, Jordan Crouse wrote: > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > index 6c55d24..0e2b00a 100644 > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > @@ -470,6 +470,55 @@ static int a5xx_ucode_init(struct msm_gpu *gpu) > return 0; > } > > +static int a5xx_zap_shader_resume(struct msm_gpu *gpu) > +{ > + int ret; > + > + ret = qcom_scm_set_remote_state(0, 13); Is 13 here the PAS_ID, or just a coincidence? And what is state 0? Can we have a define for this? > + if (ret) > + DRM_ERROR("%s: zap-shader resume failed: %d\n", > + gpu->name, ret); > + > + return ret; > +} > + > +static int a5xx_zap_shader_init(struct msm_gpu *gpu) > +{ > + static bool loaded; > + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > + struct a5xx_gpu *a5xx_gpu = to_a5xx_gpu(adreno_gpu); > + struct platform_device *pdev = a5xx_gpu->pdev; > + struct device_node *node; > + int ret; > + > + /* > + * If the zap shader is already loaded into memory we just need to kick > + * the remote processor to reinitialize it > + */ > + if (loaded) > + return a5xx_zap_shader_resume(gpu); > + > + /* Populate the sub-nodes if they haven't already been done */ > + of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); As this is new code, there's little (no) value in doing this stepwise, just squash this with the later commits replacing this. > + > + /* Find the sub-node for the zap shader */ > + node = of_find_node_by_name(pdev->dev.of_node, "qcom,zap-shader"); > + if (!node) { > + DRM_ERROR("%s: qcom,zap-shader not found in device tree\n", > + gpu->name); > + return -ENODEV; > + } > + > + ret = _pil_tz_load_image(of_find_device_by_node(node)); > + if (ret) > + DRM_ERROR("%s: Unable to load the zap shader\n", > + gpu->name); > + > + loaded = !ret; > + > + return ret; > +} > + Regards, Bjorn
On Mon, Apr 17, 2017 at 12:58:07PM -0700, Bjorn Andersson wrote: > On Wed 12 Apr 14:15 PDT 2017, Jordan Crouse wrote: > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > index 6c55d24..0e2b00a 100644 > > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > @@ -470,6 +470,55 @@ static int a5xx_ucode_init(struct msm_gpu *gpu) > > return 0; > > } > > > > +static int a5xx_zap_shader_resume(struct msm_gpu *gpu) > > +{ > > + int ret; > > + > > + ret = qcom_scm_set_remote_state(0, 13); > > Is 13 here the PAS_ID, or just a coincidence? And what is state 0? Can > we have a define for this? I think it is officially a coincidence but probably an informed one. Now that I think about it I seem to remember Andy asking me to add a define for the 13. The state is more confusing - because video is the other consumer of this call and they call 0 to disable and 1 to enable, and GPU calls 0 to enable. I can add a #define but it will be just for GPU - maybe something like REMOTE_STATE_GPU_RESUME? > > + if (ret) > > + DRM_ERROR("%s: zap-shader resume failed: %d\n", > > + gpu->name, ret); > > + > > + return ret; > > +} > > + > > +static int a5xx_zap_shader_init(struct msm_gpu *gpu) > > +{ > > + static bool loaded; > > + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > > + struct a5xx_gpu *a5xx_gpu = to_a5xx_gpu(adreno_gpu); > > + struct platform_device *pdev = a5xx_gpu->pdev; > > + struct device_node *node; > > + int ret; > > + > > + /* > > + * If the zap shader is already loaded into memory we just need to kick > > + * the remote processor to reinitialize it > > + */ > > + if (loaded) > > + return a5xx_zap_shader_resume(gpu); > > + > > + /* Populate the sub-nodes if they haven't already been done */ > > + of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); > > As this is new code, there's little (no) value in doing this stepwise, > just squash this with the later commits replacing this. Yeah, I'm thinking I'll do a squash on the next go around unless Rob objects. Jordan
On Mon 17 Apr 14:51 PDT 2017, Jordan Crouse wrote: > On Mon, Apr 17, 2017 at 12:58:07PM -0700, Bjorn Andersson wrote: > > On Wed 12 Apr 14:15 PDT 2017, Jordan Crouse wrote: > > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > > index 6c55d24..0e2b00a 100644 > > > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > > @@ -470,6 +470,55 @@ static int a5xx_ucode_init(struct msm_gpu *gpu) > > > return 0; > > > } > > > > > > +static int a5xx_zap_shader_resume(struct msm_gpu *gpu) > > > +{ > > > + int ret; > > > + > > > + ret = qcom_scm_set_remote_state(0, 13); > > > > Is 13 here the PAS_ID, or just a coincidence? And what is state 0? Can > > we have a define for this? > > I think it is officially a coincidence but probably an informed one. Now that I > think about it I seem to remember Andy asking me to add a define for the 13. So far I haven't pushed the pas-id defines to the public scm header, but we could do that and until we see a discrepancy just use that define here. > The state is more confusing - because video is the other consumer of this call > and they call 0 to disable and 1 to enable, and GPU calls 0 to enable. I can > add a #define but it will be just for GPU - maybe something like > REMOTE_STATE_GPU_RESUME? > As the states aren't globally defined I think it makes sense to just keep them opaque in the scm-world and have the two(?) states defined locally here in the adreno driver. > > > > + if (ret) > > > + DRM_ERROR("%s: zap-shader resume failed: %d\n", > > > + gpu->name, ret); > > > + > > > + return ret; > > > +} > > > + > > > +static int a5xx_zap_shader_init(struct msm_gpu *gpu) > > > +{ > > > + static bool loaded; > > > + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > > > + struct a5xx_gpu *a5xx_gpu = to_a5xx_gpu(adreno_gpu); > > > + struct platform_device *pdev = a5xx_gpu->pdev; > > > + struct device_node *node; > > > + int ret; > > > + > > > + /* > > > + * If the zap shader is already loaded into memory we just need to kick > > > + * the remote processor to reinitialize it > > > + */ > > > + if (loaded) > > > + return a5xx_zap_shader_resume(gpu); > > > + > > > + /* Populate the sub-nodes if they haven't already been done */ > > > + of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); > > > > As this is new code, there's little (no) value in doing this stepwise, > > just squash this with the later commits replacing this. > > Yeah, I'm thinking I'll do a squash on the next go around unless Rob objects. > Sounds good. Regards, Bjorn
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c index 6c55d24..0e2b00a 100644 --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c @@ -470,6 +470,55 @@ static int a5xx_ucode_init(struct msm_gpu *gpu) return 0; } +static int a5xx_zap_shader_resume(struct msm_gpu *gpu) +{ + int ret; + + ret = qcom_scm_set_remote_state(0, 13); + if (ret) + DRM_ERROR("%s: zap-shader resume failed: %d\n", + gpu->name, ret); + + return ret; +} + +static int a5xx_zap_shader_init(struct msm_gpu *gpu) +{ + static bool loaded; + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); + struct a5xx_gpu *a5xx_gpu = to_a5xx_gpu(adreno_gpu); + struct platform_device *pdev = a5xx_gpu->pdev; + struct device_node *node; + int ret; + + /* + * If the zap shader is already loaded into memory we just need to kick + * the remote processor to reinitialize it + */ + if (loaded) + return a5xx_zap_shader_resume(gpu); + + /* Populate the sub-nodes if they haven't already been done */ + of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); + + /* Find the sub-node for the zap shader */ + node = of_find_node_by_name(pdev->dev.of_node, "qcom,zap-shader"); + if (!node) { + DRM_ERROR("%s: qcom,zap-shader not found in device tree\n", + gpu->name); + return -ENODEV; + } + + ret = _pil_tz_load_image(of_find_device_by_node(node)); + if (ret) + DRM_ERROR("%s: Unable to load the zap shader\n", + gpu->name); + + loaded = !ret; + + return ret; +} + #define A5XX_INT_MASK (A5XX_RBBM_INT_0_MASK_RBBM_AHB_ERROR | \ A5XX_RBBM_INT_0_MASK_RBBM_TRANSFER_TIMEOUT | \ A5XX_RBBM_INT_0_MASK_RBBM_ME_MS_TIMEOUT | \ @@ -654,8 +703,27 @@ static int a5xx_hw_init(struct msm_gpu *gpu) return -EINVAL; } - /* Put the GPU into unsecure mode */ - gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0); + /* + * Try to load a zap shader into the secure world. If successful + * we can use the CP to switch out of secure mode. If not then we + * have no resource but to try to switch ourselves out manually. If we + * guessed wrong then access to the RBBM_SECVID_TRUST_CNTL register will + * be blocked and a permissions violation will soon follow. + */ + ret = a5xx_zap_shader_init(gpu); + if (!ret) { + OUT_PKT7(gpu->rb, CP_SET_SECURE_MODE, 1); + OUT_RING(gpu->rb, 0x00000000); + + gpu->funcs->flush(gpu); + if (!gpu->funcs->idle(gpu)) + return -EINVAL; + } else { + /* Print a warning so if we die, we know why */ + dev_warn_once(gpu->dev->dev, + "Zap shader not enabled - using SECVID_TRUST_CNTL instead\n"); + gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0); + } return 0; }
The A5XX GPU powers on in "secure" mode. In secure mode the GPU can only render to buffers that are marked as secure and inaccessible to the kernel and user through a series of hardware protections. In practice secure mode is used to draw things like a UI on a secure video frame. In order to switch out of secure mode the GPU executes a special shader that clears out the GMEM and other sensitve registers and then writes a register. Because the kernel can't be trusted the shader binary is signed and verified and programmed by the secure world. To do this we need to read the MDT header and the segments from the firmware location and put them in memory and present them for approval. For targets without secure support there is an out: if the secure world doesn't support secure then there are no hardware protections and we can freely write the SECVID_TRUST register from the CPU. We don't have 100% confidence that we can query the secure capabilities at run time but we have enough calls that need to go right to give us some confidence that we're at least doing something useful. Of course if we guess wrong you trigger a permissions violation which usually ends up in a system crash but thats a problem that shows up immediately. Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> --- drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 72 ++++++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-)