<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="#0563C1" vlink="#954F72" style="word-wrap:break-word"> <div class="WordSection1"> [Public]<o:p></o:p> <o:p> </o:p> Thanks for the review, comments in line.<o:p></o:p> <o:p> </o:p> <div> <o:p> </o:p> <o:p> </o:p> Thanks,<o:p></o:p> Candice<o:p></o:p> </div> <o:p> </o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Wang, Yang(Kevin) <KevinYang.Wang@amd.com> Sent: Monday, April 18, 2022 11:50 AM To: Li, Candice <Candice.Li@amd.com>; amd-gfx@lists.freedesktop.org Cc: Clements, John <John.Clements@amd.com> Subject: Re: [PATCH 2/2] drm/amdgpu: Add debugfs TA load/unload/invoke support<o:p></o:p> </div> </div> <o:p> </o:p> <o:p> </o:p> <div> <div> <o:p> </o:p> </div> <div class="MsoNormal" align="center" style="text-align:center"> <hr size="2" width="98%" align="center"> </div> <div id="divRplyFwdMsg"> From: amd-gfx <<a href="mailto:amd-gfx-bounces@lists.freedesktop.org">amd-gfx-bounces@lists.freedesktop.org</a>> on behalf of Candice Li <<a href="mailto:candice.li@amd.com">candice.li@amd.com</a>> Sent: Monday, April 18, 2022 11:09 AM To: <a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> <<a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a>> Cc: Li, Candice <<a href="mailto:Candice.Li@amd.com">Candice.Li@amd.com</a>>; Clements, John <<a href="mailto:John.Clements@amd.com">John.Clements@amd.com</a>> Subject: [PATCH 2/2] drm/amdgpu: Add debugfs TA load/unload/invoke support <o:p></o:p> <div> <o:p></o:p> </div> </div> <div> <div> Add debugfs support to load/unload/invoke TA in runtime. Signed-off-by: John Clements <<a href="mailto:john.clements@amd.com">john.clements@amd.com</a>> Signed-off-by: Candice Li <<a href="mailto:candice.li@amd.com">candice.li@amd.com</a>> --- drivers/gpu/drm/amd/amdgpu/Makefile | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 2 + drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.c | 312 ++++++++++++++++++++ drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.h | 30 ++ 4 files changed, 345 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.c create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.h diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile index 7d7af43a258f83..b525f9be9326f4 100644 --- a/drivers/gpu/drm/amd/amdgpu/Makefile +++ b/drivers/gpu/drm/amd/amdgpu/Makefile @@ -58,7 +58,7 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \ amdgpu_vm_sdma.o amdgpu_discovery.o amdgpu_ras_eeprom.o amdgpu_nbio.o \ amdgpu_umc.o smu_v11_0_i2c.o amdgpu_fru_eeprom.o amdgpu_rap.o \ amdgpu_fw_attestation.o amdgpu_securedisplay.o \ - amdgpu_eeprom.o amdgpu_mca.o + amdgpu_eeprom.o amdgpu_mca.o amdgpu_psp_ta.o amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c index 13e4d8f9b87449..eedb12f6b8a32d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c @@ -38,6 +38,7 @@ #include "amdgpu_umr.h" #include "amdgpu_reset.h" +#include "amdgpu_psp_ta.h" #if defined(CONFIG_DEBUG_FS) @@ -1767,6 +1768,7 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev) DRM_ERROR("registering register debugfs failed (%d).\n", r); amdgpu_debugfs_firmware_init(adev); + amdgpu_ta_if_debugfs_init(adev); #if defined(CONFIG_DRM_AMD_DC) if (amdgpu_device_has_dc_support(adev)) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.c new file mode 100644 index 00000000000000..916bf3f6fce0d4 --- /dev/null +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.c @@ -0,0 +1,312 @@ +/* + * Copyright 2022 Advanced Micro Devices, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + * + */ + +#include "amdgpu.h" +#include "amdgpu_psp_ta.h" + +static const char *TA_IF_FS_NAME = "ta_if"; + +struct dentry *dir; +struct dentry *ta_load_debugfs_dentry; +struct dentry *ta_unload_debugfs_dentry; +struct dentry *ta_invoke_debugfs_dentry;<o:p></o:p> </div> <div> <o:p> </o:p> </div> <div> [kevin]:<o:p></o:p> </div> <div> <o:p> </o:p> </div> <div> make above variable as static is better for this case, but it seems it is not used in this patch? <o:p></o:p> </div> <div> if so, you will get a "defined but not used" error, when turn on build option " -Werror=unused-variable".<o:p></o:p> </div> <div> <o:p> </o:p> </div> Please don't mind this prompt. <o:p></o:p> <div> <o:p> </o:p> </div> <div> Best Regards,<o:p></o:p> </div> <div> Kevin<o:p></o:p> <o:p> </o:p> [Candice] Thanks for the review. These variables are used in my patch. <o:p></o:p> Will update ta_load/unload/invoke_debugfs_dentry to static in v2, but dentry dir will be used by amdgpu_ta_if_debugfs_remove, so it should be not.<o:p></o:p> + +static ssize_t ta_if_load_debugfs_read(struct file *fp, char *buf, size_t len, loff_t *off); +static ssize_t ta_if_unload_debugfs_read(struct file *fp, char *buf, size_t len, loff_t *off); +static ssize_t ta_if_invoke_debugfs_read(struct file *fp, char *buf, size_t len, loff_t *off); + + +static uint32_t get_bin_version(const uint8_t *bin) +{ + const struct common_firmware_header *hdr = + (const struct common_firmware_header *)bin; + + return hdr->ucode_version; +} + +static uint32_t get_shared_buf_size(uint32_t shared_buf_len) +{ + return (shared_buf_len % PAGE_SIZE) ? + (shared_buf_len/PAGE_SIZE + 1) * PAGE_SIZE : + shared_buf_len;<o:p></o:p> </div> <div> [kevin]:<o:p></o:p> </div> <div> <ol start="1" type="1"> <li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level1 lfo3"> the above code can be replaced with "ALIGN" macro..<o:p></o:p></li><li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level1 lfo3"> <o:p> </o:p></li><li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level1 lfo3"> #define ALIGN(x, a)>---->-------__ALIGN_KERNEL((x), (a)<o:p></o:p></li></ol> [Candice] Thanks for the suggestion, I will replace it with PAGE_ALIGN macro directly in v2.<o:p></o:p> <ol start="2" type="1"> <li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l3 level1 lfo6"> do you forget to add some lock resource to protect/cover multi process case?<o:p></o:p></li><li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l3 level1 lfo6"> e.g: process#1 exec load_ta() and process#2 exec unload_ta()<o:p></o:p></li></ol> <div> [Candice] We already have psp->mutex there for psp cmd submission, do we still need another one?<o:p></o:p> <o:p> </o:p> </div> </div> <div> and why not use write operation instead of read (load/unload/invoke) ? this seems more reasonable.<o:p></o:p> <o:p> </o:p> [Candice] The debugfs write doesn't allow the kernel to copy data back to the user, so with debugfs read, we can copy data from the user, then copy data back to the user.<o:p></o:p> <o:p> </o:p> </div> <div> <div> <o:p> </o:p> </div> </div> <div> Best Regards,<o:p></o:p> </div> <div> Kevin<o:p></o:p> </div> <div> <o:p> </o:p> <o:p> </o:p> <o:p> </o:p> </div> <div> +} + +static void prep_ta_mem_context(struct psp_context *psp, + struct ta_context *context, + uint8_t *shared_buf, + uint32_t shared_buf_len) +{ + context->mem_context.shared_mem_size = get_shared_buf_size(shared_buf_len); + psp_ta_init_shared_buf(psp, &context->mem_context); + + memcpy((void *)context->mem_context.shared_buf, shared_buf, shared_buf_len); +} + +static bool is_ta_type_valid(enum ta_type_id ta_type) +{ + bool ret = false; + + switch (ta_type) { + case TA_TYPE_RAS: + ret = true; + break; + default: + break; + } + + return ret; +} + +static const struct file_operations ta_load_debugfs_fops = { + .read = ta_if_load_debugfs_read, + .llseek = default_llseek, + .owner = THIS_MODULE +}; + +static const struct file_operations ta_unload_debugfs_fops = { + .read = ta_if_unload_debugfs_read, + .llseek = default_llseek, + .owner = THIS_MODULE +}; + +static const struct file_operations ta_invoke_debugfs_fops = { + .read = ta_if_invoke_debugfs_read, + .llseek = default_llseek, + .owner = THIS_MODULE +}; + + +/** + * DOC: AMDGPU TA debugfs interfaces + * + * Three debugfs interfaces can be opened by a program to + * load/invoke/unload TA, + * + * - /sys/kernel/debug/dri/<N>/ta_if/ta_load + * - /sys/kernel/debug/dri/<N>/ta_if/ta_invoke + * - /sys/kernel/debug/dri/<N>/ta_if/ta_unload + * + * How to use the interfaces in a program? + * + * A program needs to provide transmit buffer to the interfaces + * and will receive buffer from the interfaces below, + * + * - For TA load debugfs interface: + * Transmit buffer: + * - TA type (4bytes) + * - TA bin length (4bytes) + * - TA bin + * Receive buffer: + * - TA ID (4bytes) + * + * - For TA invoke debugfs interface: + * Transmit buffer: + * - TA ID (4bytes) + * - TA CMD ID (4bytes) + * - TA shard buf length (4bytes) + * - TA shared buf + * Receive buffer: + * - TA shared buf + * + * - For TA unload debugfs interface: + * Transmit buffer: + * - TA ID (4bytes) + */ + +static ssize_t ta_if_load_debugfs_read(struct file *fp, char *buf, size_t len, loff_t *off) +{ + uint32_t ta_type = 0; + uint32_t ta_bin_len = 0; + uint8_t *ta_bin = NULL; + uint32_t copy_pos = 0; + int ret = 0; + + struct amdgpu_device *adev = (struct amdgpu_device *)file_inode(fp)->i_private; + struct psp_context *psp = &adev->psp; + struct ta_context context = {0}; + + if ((fp == NULL) || (buf == NULL)) + return -EINVAL; + + ret = copy_from_user((void *)&ta_type, &buf[copy_pos], sizeof(uint32_t)); + if (ret || (!is_ta_type_valid(ta_type))) + return -EINVAL; + + copy_pos += sizeof(uint32_t); + + ret = copy_from_user((void *)&ta_bin_len, &buf[copy_pos], sizeof(uint32_t)); + if (ret) + return -EINVAL; + + copy_pos += sizeof(uint32_t); + + ta_bin = kzalloc(ta_bin_len, GFP_KERNEL); + if (!ta_bin) + ret = -ENOMEM; + ret = copy_from_user((void *)ta_bin, &buf[copy_pos], ta_bin_len); + if (ret) + goto err_free_bin; + + ret = psp_ras_terminate(psp); + if (ret) { + dev_err(adev->dev, "Failed to unload embedded RAS TA\n"); + goto err_free_bin; + } + + context.ta_type = ta_type; + context.ta_load_type = GFX_CMD_ID_LOAD_TA; + context.bin_desc.fw_version = get_bin_version(ta_bin); + context.bin_desc.size_bytes = ta_bin_len; + context.bin_desc.start_addr = ta_bin; + + ret = psp_ta_load(psp, &context); + + if (ret || context.resp_status) { + dev_err(adev->dev, "TA load via debugfs failed (%d) status %d\n", + ret, context.resp_status); + goto err_free_bin; + } + + context.initialized = true; + ret = copy_to_user((char *)buf, (void *)&context.session_id, sizeof(uint32_t)); + +err_free_bin: + kfree(ta_bin); + + return ret; +} + +static ssize_t ta_if_unload_debugfs_read(struct file *fp, char *buf, size_t len, loff_t *off) +{ + uint32_t ta_id = 0; + int ret = 0; + + struct amdgpu_device *adev = (struct amdgpu_device *)file_inode(fp)->i_private; + struct psp_context *psp = &adev->psp; + struct ta_context context = {0}; + + if ((fp == NULL) || (buf == NULL)) + return -1; + + ret = copy_from_user((void *)&ta_id, buf, sizeof(uint32_t)); + if (ret) + return -EINVAL; + + context.session_id = ta_id; + + ret = psp_ta_unload(psp, &context); + if (!ret) + context.initialized = false; + + return ret; +} +<o:p></o:p> </div> <div> +static ssize_t ta_if_invoke_debugfs_read(struct file *fp, char *buf, size_t len, loff_t *off)<o:p></o:p> </div> <div> +{ + uint32_t ta_id = 0; + uint32_t cmd_id = 0; + uint32_t shared_buf_len = 0; + uint8_t *shared_buf = NULL; + uint32_t copy_pos = 0; + int ret = 0; + + struct amdgpu_device *adev = (struct amdgpu_device *)file_inode(fp)->i_private; + struct psp_context *psp = &adev->psp; + struct ta_context context = {0}; + + if ((fp == NULL) || (buf == NULL)) + return -EINVAL;<o:p></o:p> </div> <div> <o:p> </o:p> </div> <div> [kevin]:<o:p></o:p> </div> <div> “if ((fp == NULL) ..."<o:p></o:p> </div> <div> The linux kernel will automatically create struct file instance when you open the file, even if you do not provide a callback function (in fops)<o:p></o:p> </div> <div> <div> <o:p> </o:p> </div> </div> <div> [kevin]:<o:p></o:p> </div> <div> <o:p> </o:p> </div> <div> you should add some check here to avoid that the user buf is overflow.<o:p></o:p> </div> e.g: If the length of the user buf is less than that required, an error should be returned directly <o:p></o:p> <o:p> </o:p> [Candice] Will remove the fp check in v2. Current check should be enough, copy_from_user will not access the buf if there is no more length.<o:p></o:p> <div> <o:p> </o:p> </div> <div> + + ret = copy_from_user((void *)&ta_id, &buf[copy_pos], sizeof(uint32_t)); + if (ret) + return -EINVAL; + copy_pos += sizeof(uint32_t); + + ret = copy_from_user((void *)&cmd_id, &buf[copy_pos], sizeof(uint32_t)); + if (ret) + return -EINVAL; + copy_pos += sizeof(uint32_t); + + ret = copy_from_user((void *)&shared_buf_len, &buf[copy_pos], sizeof(uint32_t)); + if (ret) + return -EINVAL; + copy_pos += sizeof(uint32_t); + + shared_buf = kzalloc(shared_buf_len, GFP_KERNEL); + if (!shared_buf) + ret = -ENOMEM; + ret = copy_from_user((void *)shared_buf, &buf[copy_pos], shared_buf_len); + if (ret) + goto err_free_shared_buf; + + context.session_id = ta_id; + + prep_ta_mem_context(psp, &context, shared_buf, shared_buf_len); + + ret = psp_ta_invoke_indirect(psp, cmd_id, &context); + + if (ret || context.resp_status) { + dev_err(adev->dev, "TA invoke via debugfs failed (%d) status %d\n", + ret, context.resp_status); + goto err_free_ta_shared_buf; + } + + ret = copy_to_user((char *)buf, context.mem_context.shared_buf, shared_buf_len); + +err_free_ta_shared_buf: + psp_ta_free_shared_buf(&context.mem_context); + +err_free_shared_buf: + kfree(shared_buf); + + return ret; +} + +static struct dentry *amdgpu_ta_if_debugfs_create(struct amdgpu_device *adev) +{ + struct drm_minor *minor = adev_to_drm(adev)->primary; + + dir = debugfs_create_dir(TA_IF_FS_NAME, minor->debugfs_root); + + ta_load_debugfs_dentry = debugfs_create_file("ta_load", 0400, dir, adev, + &ta_load_debugfs_fops); + + ta_unload_debugfs_dentry = debugfs_create_file("ta_unload", 0400, dir, + adev, &ta_unload_debugfs_fops); + + ta_invoke_debugfs_dentry = debugfs_create_file("ta_invoke", 0400, dir, + adev, &ta_invoke_debugfs_fops); + return dir; +} + +void amdgpu_ta_if_debugfs_init(struct amdgpu_device *adev) +{ +#if defined(CONFIG_DEBUG_FS) + dir = amdgpu_ta_if_debugfs_create(adev); +#endif +} + +void amdgpu_ta_if_debugfs_remove(void) +{ + debugfs_remove_recursive(dir); +} diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.h new file mode 100644 index 00000000000000..883f89d57616d0 --- /dev/null +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.h @@ -0,0 +1,30 @@ +/* + * Copyright 2022 Advanced Micro Devices, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + * + */ + +#ifndef __AMDGPU_PSP_TA_H__ +#define __AMDGPU_PSP_TA_H__ + +void amdgpu_ta_if_debugfs_init(struct amdgpu_device *adev); +void amdgpu_ta_if_debugfs_remove(void); + +#endif -- 2.17.1<o:p></o:p> </div> </div> </div> </div> </body> </html>