This is John/Rodrigo's 2 patches with some minor changes, and I added 2 patches.
"drm/i915/uapi: Add query for hwconfig blob" was changed:
* Rename DRM_I915_QUERY_HWCONFIG_TABLE to DRM_I915_QUERY_HWCONFIG_BLOB as requested by Joonas.
* Reword commit message
* I added Acked-by to this patch, but this only applies in the context of this version of the patchset. If my changes are rejected, then please *do not* add my Acked-by to the other series.
In particular, I do not want my Acked-by on the patch if the patch mentions the HWCONFIG format, but is not willing to add that to the actual uAPI.
I also do not want my Acked-by on it if it mentions "consolidation" of this data. Since we are dealing with open source projects (aside from GuC), this doesn't seem appropriate.
"drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item" adds a struct to the uAPI and documents the return value for DRM_I915_QUERY_HWCONFIG_BLOB. (Except, keys / values are still deferred to the PRM.)
"drm/i915/guc: Verify hwconfig blob matches supported format" does the simple verification of the blob to make sure it matches what the uAPI documents.
John Harrison (1): drm/i915/guc: Add fetch of hwconfig table
Jordan Justen (2): drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item drm/i915/guc: Verify hwconfig blob matches supported format
Rodrigo Vivi (1): drm/i915/uapi: Add query for hwconfig blob
drivers/gpu/drm/i915/Makefile | 1 + .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 1 + .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h | 4 + drivers/gpu/drm/i915/gt/uc/intel_guc.h | 3 + .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c | 177 ++++++++++++++++++ .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h | 19 ++ drivers/gpu/drm/i915/gt/uc/intel_uc.c | 6 + drivers/gpu/drm/i915/i915_query.c | 23 +++ include/uapi/drm/i915_drm.h | 25 +++ 9 files changed, 259 insertions(+) create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h
From: John Harrison John.C.Harrison@Intel.com
Implement support for fetching the hardware description table from the GuC. The call is made twice - once without a destination buffer to query the size and then a second time to fill in the buffer.
Note that the table is only available on ADL-P and later platforms.
Cc: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Rodrigo Vivi rodrigo.vivi@intel.com Signed-off-by: John Harrison John.C.Harrison@Intel.com Reviewed-by: Matthew Brost matthew.brost@intel.com --- drivers/gpu/drm/i915/Makefile | 1 + .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 1 + .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h | 4 + drivers/gpu/drm/i915/gt/uc/intel_guc.h | 3 + .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c | 151 ++++++++++++++++++ .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h | 19 +++ drivers/gpu/drm/i915/gt/uc/intel_uc.c | 6 + 7 files changed, 185 insertions(+) create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 451df10e3a36..f6e4a699495e 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -190,6 +190,7 @@ i915-y += gt/uc/intel_uc.o \ gt/uc/intel_guc_rc.o \ gt/uc/intel_guc_slpc.o \ gt/uc/intel_guc_submission.o \ + gt/uc/intel_guc_hwconfig.o \ gt/uc/intel_huc.o \ gt/uc/intel_huc_debugfs.o \ gt/uc/intel_huc_fw.o diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h index 7afdadc7656f..a9a329e53c35 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h @@ -129,6 +129,7 @@ enum intel_guc_action { INTEL_GUC_ACTION_ENGINE_FAILURE_NOTIFICATION = 0x1009, INTEL_GUC_ACTION_SETUP_PC_GUCRC = 0x3004, INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000, + INTEL_GUC_ACTION_GET_HWCONFIG = 0x4100, INTEL_GUC_ACTION_REGISTER_CONTEXT = 0x4502, INTEL_GUC_ACTION_DEREGISTER_CONTEXT = 0x4503, INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505, diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h index c20658ee85a5..8085fb181274 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h @@ -8,6 +8,10 @@
enum intel_guc_response_status { INTEL_GUC_RESPONSE_STATUS_SUCCESS = 0x0, + INTEL_GUC_RESPONSE_NOT_SUPPORTED = 0x20, + INTEL_GUC_RESPONSE_NO_ATTRIBUTE_TABLE = 0x201, + INTEL_GUC_RESPONSE_NO_DECRYPTION_KEY = 0x202, + INTEL_GUC_RESPONSE_DECRYPTION_FAILED = 0x204, INTEL_GUC_RESPONSE_STATUS_GENERIC_FAIL = 0xF000, };
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 697d9d66acef..309bc8d5447b 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -13,6 +13,7 @@ #include "intel_guc_fw.h" #include "intel_guc_fwif.h" #include "intel_guc_ct.h" +#include "intel_guc_hwconfig.h" #include "intel_guc_log.h" #include "intel_guc_reg.h" #include "intel_guc_slpc_types.h" @@ -37,6 +38,8 @@ struct intel_guc { struct intel_guc_ct ct; /** @slpc: sub-structure containing SLPC related data and objects */ struct intel_guc_slpc slpc; + /** @hwconfig: hardware configuration KLV table */ + struct intel_guc_hwconfig hwconfig;
/** @sched_engine: Global engine used to submit requests to GuC */ struct i915_sched_engine *sched_engine; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c new file mode 100644 index 000000000000..ce6088f112d4 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c @@ -0,0 +1,151 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2021 Intel Corporation + */ + +#include "gt/intel_gt.h" +#include "i915_drv.h" +#include "i915_memcpy.h" +#include "intel_guc_hwconfig.h" + +static inline struct intel_guc *hwconfig_to_guc(struct intel_guc_hwconfig *hwconfig) +{ + return container_of(hwconfig, struct intel_guc, hwconfig); +} + +/* + * GuC has a blob containing hardware configuration information (HWConfig). + * This is formatted as a simple and flexible KLV (Key/Length/Value) table. + * + * For example, a minimal version could be: + * enum device_attr { + * ATTR_SOME_VALUE = 0, + * ATTR_SOME_MASK = 1, + * }; + * + * static const u32 hwconfig[] = { + * ATTR_SOME_VALUE, + * 1, // Value Length in DWords + * 8, // Value + * + * ATTR_SOME_MASK, + * 3, + * 0x00FFFFFFFF, 0xFFFFFFFF, 0xFF000000, + * }; + * + * The attribute ids are defined in a hardware spec. + */ + +static int __guc_action_get_hwconfig(struct intel_guc_hwconfig *hwconfig, + u32 ggtt_offset, u32 ggtt_size) +{ + struct intel_guc *guc = hwconfig_to_guc(hwconfig); + u32 action[] = { + INTEL_GUC_ACTION_GET_HWCONFIG, + ggtt_offset, + 0, /* upper 32 bits of address */ + ggtt_size, + }; + int ret; + + ret = intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0); + if (ret == -ENXIO) + return -ENOENT; + + if (!ggtt_size && !ret) + ret = -EINVAL; + + return ret; +} + +static int guc_hwconfig_discover_size(struct intel_guc_hwconfig *hwconfig) +{ + int ret; + + /* Sending a query with too small a table will return the size of the table */ + ret = __guc_action_get_hwconfig(hwconfig, 0, 0); + if (ret < 0) + return ret; + + hwconfig->size = ret; + return 0; +} + +static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig) +{ + struct intel_guc *guc = hwconfig_to_guc(hwconfig); + struct i915_vma *vma; + u32 ggtt_offset; + void *vaddr; + int ret; + + GEM_BUG_ON(!hwconfig->size); + + ret = intel_guc_allocate_and_map_vma(guc, hwconfig->size, &vma, &vaddr); + if (ret) + return ret; + + ggtt_offset = intel_guc_ggtt_offset(guc, vma); + + ret = __guc_action_get_hwconfig(hwconfig, ggtt_offset, hwconfig->size); + if (ret >= 0) + memcpy(hwconfig->ptr, vaddr, hwconfig->size); + + i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP); + + return ret; +} + +static bool has_table(struct drm_i915_private *i915) +{ + if (IS_ALDERLAKE_P(i915)) + return true; + + return false; +} + +/** + * intel_guc_hwconfig_fini - Finalize the HWConfig + * + * Free up the memory allocation holding the table. + */ +void intel_guc_hwconfig_fini(struct intel_guc_hwconfig *hwconfig) +{ + kfree(hwconfig->ptr); + hwconfig->size = 0; + hwconfig->ptr = NULL; +} + +/** + * intel_guc_hwconfig_init - Initialize the HWConfig + * + * Retrieve the HWConfig table from the GuC and save it away in a local memory + * allocation. It can then be queried on demand by other users later on. + */ +int intel_guc_hwconfig_init(struct intel_guc_hwconfig *hwconfig) +{ + struct intel_guc *guc = hwconfig_to_guc(hwconfig); + struct drm_i915_private *i915 = guc_to_gt(guc)->i915; + int ret; + + if (!has_table(i915)) + return 0; + + ret = guc_hwconfig_discover_size(hwconfig); + if (ret) + return ret; + + hwconfig->ptr = kmalloc(hwconfig->size, GFP_KERNEL); + if (!hwconfig->ptr) { + hwconfig->size = 0; + return -ENOMEM; + } + + ret = guc_hwconfig_fill_buffer(hwconfig); + if (ret < 0) { + intel_guc_hwconfig_fini(hwconfig); + return ret; + } + + return 0; +} diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h new file mode 100644 index 000000000000..fdd7f0d6e938 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2021 Intel Corporation + */ + +#ifndef _INTEL_GUC_HWCONFIG_H_ +#define _INTEL_GUC_HWCONFIG_H_ + +#include <linux/types.h> + +struct intel_guc_hwconfig { + u32 size; + void *ptr; +}; + +int intel_guc_hwconfig_init(struct intel_guc_hwconfig *hwconfig); +void intel_guc_hwconfig_fini(struct intel_guc_hwconfig *hwconfig); + +#endif /* _INTEL_GUC_HWCONFIG_H_ */ diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index da199aa6989f..21b82db5d354 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -503,6 +503,10 @@ static int __uc_init_hw(struct intel_uc *uc) if (ret) goto err_log_capture;
+ ret = intel_guc_hwconfig_init(&guc->hwconfig); + if (ret) + drm_err(&i915->drm, "Failed to retrieve hwconfig table: %d\n", ret); + ret = guc_enable_communication(guc); if (ret) goto err_log_capture; @@ -563,6 +567,8 @@ static void __uc_fini_hw(struct intel_uc *uc) if (intel_uc_uses_guc_submission(uc)) intel_guc_submission_disable(guc);
+ intel_guc_hwconfig_fini(&guc->hwconfig); + __uc_sanitize(uc); }
From: Rodrigo Vivi rodrigo.vivi@intel.com
The DRM_I915_QUERY_HWCONFIG_BLOB query item returns a blob of data which it receives from the GuC software. This blob provides some useful data about the hardware for drivers.
Although the blob is not fully documented at this time, the basic format is an array of u32 values. The array is a simple and flexible KLV (Key/Length/Value) formatted table. For example, it could be just: enum device_attr { ATTR_SOME_VALUE = 0, ATTR_SOME_MASK = 1, };
static const u32 hwconfig[] = { ATTR_SOME_VALUE, 1, // Value Length in DWords 8, // Value
ATTR_SOME_MASK, 3, 0x00FFFFFFFF, 0xFFFFFFFF, 0xFF000000, };
The attribute ids and meaning of the values will be documented in the Programmer Reference Manuals when released.
Cc: Tvrtko Ursulin tvrtko.ursulin@linux.intel.com Cc: Kenneth Graunke kenneth.w.graunke@intel.com Cc: Michal Wajdeczko michal.wajdeczko@intel.com Cc: Slawomir Milczarek slawomir.milczarek@intel.com Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com Signed-off-by: Rodrigo Vivi rodrigo.vivi@intel.com Signed-off-by: John Harrison John.C.Harrison@Intel.com Reviewed-by: Matthew Brost matthew.brost@intel.com Acked-by: Jordan Justen jordan.l.justen@intel.com Tested-by: Jordan Justen jordan.l.justen@intel.com --- drivers/gpu/drm/i915/i915_query.c | 23 +++++++++++++++++++++++ include/uapi/drm/i915_drm.h | 1 + 2 files changed, 24 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c index 2dfbc22857a3..195524e9a369 100644 --- a/drivers/gpu/drm/i915/i915_query.c +++ b/drivers/gpu/drm/i915/i915_query.c @@ -479,12 +479,35 @@ static int query_memregion_info(struct drm_i915_private *i915, return total_length; }
+static int query_hwconfig_blob(struct drm_i915_private *i915, + struct drm_i915_query_item *query_item) +{ + struct intel_gt *gt = to_gt(i915); + struct intel_guc_hwconfig *hwconfig = >->uc.guc.hwconfig; + + if (!hwconfig->size || !hwconfig->ptr) + return -ENODEV; + + if (query_item->length == 0) + return hwconfig->size; + + if (query_item->length < hwconfig->size) + return -EINVAL; + + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), + hwconfig->ptr, hwconfig->size)) + return -EFAULT; + + return hwconfig->size; +} + static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv, struct drm_i915_query_item *query_item) = { query_topology_info, query_engine_info, query_perf_config, query_memregion_info, + query_hwconfig_blob, };
int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file) diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 914ebd9290e5..069d2fadfbd9 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -2685,6 +2685,7 @@ struct drm_i915_query_item { #define DRM_I915_QUERY_ENGINE_INFO 2 #define DRM_I915_QUERY_PERF_CONFIG 3 #define DRM_I915_QUERY_MEMORY_REGIONS 4 +#define DRM_I915_QUERY_HWCONFIG_BLOB 5 /* Must be kept compact -- no holes and well documented */
/**
Also, document DRM_I915_QUERY_HWCONFIG_BLOB with this struct.
Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Jordan Justen jordan.l.justen@intel.com --- include/uapi/drm/i915_drm.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 069d2fadfbd9..38b8c11e91f0 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -3276,6 +3276,30 @@ struct drm_i915_gem_create_ext_protected_content { __u32 flags; };
+/** + * DOC: GuC HWCONFIG blob uAPI + * + * The GuC produces a blob with information about the current device. + * i915 reads this blob from GuC and makes it available via this uAPI. + * + * The returned blob is an array of items described by struct + * drm_i915_query_hwconfig_blob_item. The + * drm_i915_query_hwconfig_blob_item length field gives the length of + * the drm_i915_query_hwconfig_blob_item data[] array for the item. + * + * The length of the query data returned by + * DRM_I915_QUERY_HWCONFIG_BLOB will align with the end at the final + * drm_i915_query_hwconfig_blob_item entry. + * + * The meaning of the key field and the data values are documented in + * the Programmer's Reference Manual. + */ +struct drm_i915_query_hwconfig_blob_item { + u32 key; + u32 length; + u32 data[]; +}; + /* ID of the protected content session managed by i915 when PXP is active */ #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
Hi Jordan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next] [also build test ERROR on drm-tip/drm-tip drm-exynos/exynos-drm-next drm/drm-next tegra-drm/drm/tegra/for-next v5.17-rc3 next-20220207] [cannot apply to airlied/drm-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jordan-Justen/GuC-HWCONFIG-with-doc... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: x86_64-randconfig-a013-20220207 (https://download.01.org/0day-ci/archive/20220208/202202080828.bS4NTyCU-lkp@i...) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/7e0cfba7f05cefa8d48ec73782b66b4255a6... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Jordan-Justen/GuC-HWCONFIG-with-documentation/20220208-032950 git checkout 7e0cfba7f05cefa8d48ec73782b66b4255a6b4ff # save the config file to linux build tree mkdir build_dir make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
In file included from <command-line>:32:
./usr/include/drm/i915_drm.h:3298:2: error: unknown type name 'u32'
3298 | u32 key; | ^~~ ./usr/include/drm/i915_drm.h:3299:2: error: unknown type name 'u32' 3299 | u32 length; | ^~~ ./usr/include/drm/i915_drm.h:3300:2: error: unknown type name 'u32' 3300 | u32 data[]; | ^~~
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 07/02/2022 19:28, Jordan Justen wrote:
Also, document DRM_I915_QUERY_HWCONFIG_BLOB with this struct.
Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Jordan Justen jordan.l.justen@intel.com
include/uapi/drm/i915_drm.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 069d2fadfbd9..38b8c11e91f0 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -3276,6 +3276,30 @@ struct drm_i915_gem_create_ext_protected_content { __u32 flags; };
+/**
- DOC: GuC HWCONFIG blob uAPI
- The GuC produces a blob with information about the current device.
- i915 reads this blob from GuC and makes it available via this uAPI.
- The returned blob is an array of items described by struct
- drm_i915_query_hwconfig_blob_item. The
- drm_i915_query_hwconfig_blob_item length field gives the length of
- the drm_i915_query_hwconfig_blob_item data[] array for the item.
- The length of the query data returned by
- DRM_I915_QUERY_HWCONFIG_BLOB will align with the end at the final
- drm_i915_query_hwconfig_blob_item entry.
Align _with_ the end maybe? Or "be equal to the size of all items added together"?
- The meaning of the key field and the data values are documented in
- the Programmer's Reference Manual.
- */
+struct drm_i915_query_hwconfig_blob_item {
- u32 key;
- u32 length;
- u32 data[];
__u32 for uapi headers, just in case you haven't figured out what kernel test robot meant.
Regards,
Tvrtko
+};
- /* ID of the protected content session managed by i915 when PXP is active */ #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
Signed-off-by: Jordan Justen jordan.l.justen@intel.com --- .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c index ce6088f112d4..695ef7a8f519 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c @@ -71,6 +71,26 @@ static int guc_hwconfig_discover_size(struct intel_guc_hwconfig *hwconfig) return 0; }
+static int verify_hwconfig_blob(const struct intel_guc_hwconfig *hwconfig) +{ + if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL) + return -EINVAL; + + struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr; + u32 remaining = (hwconfig->size / 4); + while (remaining > 0) { + if (remaining < 2) + return -EINVAL; + if (pos->length > remaining - 2) + return -EINVAL; + remaining -= 2 + pos->length; + pos = (void *)&pos->data[pos->length]; + } + + DRM_INFO("hwconfig blob format appears valid\n"); + return 0; +} + static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig) { struct intel_guc *guc = hwconfig_to_guc(hwconfig); @@ -91,6 +111,12 @@ static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig) if (ret >= 0) memcpy(hwconfig->ptr, vaddr, hwconfig->size);
+ if (verify_hwconfig_blob(hwconfig)) { + DRM_ERROR("Ignoring invalid hwconfig blob received from " + "GuC!\n"); + return -EINVAL; + } + i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP);
return ret;
Hi Jordan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on drm-tip/drm-tip drm-exynos/exynos-drm-next drm/drm-next tegra-drm/drm/tegra/for-next v5.17-rc3 next-20220207] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jordan-Justen/GuC-HWCONFIG-with-doc... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: i386-randconfig-m021-20220207 (https://download.01.org/0day-ci/archive/20220208/202202080601.vp3JG4Dh-lkp@i...) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/c122b0dea958e76766ce9b4b9f34d2eed16f... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Jordan-Justen/GuC-HWCONFIG-with-documentation/20220208-032950 git checkout c122b0dea958e76766ce9b4b9f34d2eed16fb566 # save the config file to linux build tree mkdir build_dir make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/i915/
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c: In function 'verify_hwconfig_blob':
drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c:79:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
79 | struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr; | ^~~~~~
vim +79 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
73 74 static int verify_hwconfig_blob(const struct intel_guc_hwconfig *hwconfig) 75 { 76 if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL) 77 return -EINVAL; 78
79 struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr;
80 u32 remaining = (hwconfig->size / 4); 81 while (remaining > 0) { 82 if (remaining < 2) 83 return -EINVAL; 84 if (pos->length > remaining - 2) 85 return -EINVAL; 86 remaining -= 2 + pos->length; 87 pos = (void *)&pos->data[pos->length]; 88 } 89 90 DRM_INFO("hwconfig blob format appears valid\n"); 91 return 0; 92 } 93
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Jordan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on drm-tip/drm-tip drm-exynos/exynos-drm-next drm/drm-next tegra-drm/drm/tegra/for-next v5.17-rc3 next-20220207] [cannot apply to airlied/drm-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jordan-Justen/GuC-HWCONFIG-with-doc... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: i386-buildonly-randconfig-r001-20220207 (https://download.01.org/0day-ci/archive/20220208/202202080648.8LUbxyNz-lkp@i...) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0d8850ae2cae85d49bea6ae0799fa41c7202c05c) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/c122b0dea958e76766ce9b4b9f34d2eed16f... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Jordan-Justen/GuC-HWCONFIG-with-documentation/20220208-032950 git checkout c122b0dea958e76766ce9b4b9f34d2eed16fb566 # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/i915/
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c:79:44: warning: mixing declarations and code is a C99 extension [-Wdeclaration-after-statement]
struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr; ^ 1 warning generated.
vim +79 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
73 74 static int verify_hwconfig_blob(const struct intel_guc_hwconfig *hwconfig) 75 { 76 if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL) 77 return -EINVAL; 78
79 struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr;
80 u32 remaining = (hwconfig->size / 4); 81 while (remaining > 0) { 82 if (remaining < 2) 83 return -EINVAL; 84 if (pos->length > remaining - 2) 85 return -EINVAL; 86 remaining -= 2 + pos->length; 87 pos = (void *)&pos->data[pos->length]; 88 } 89 90 DRM_INFO("hwconfig blob format appears valid\n"); 91 return 0; 92 } 93
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Jordan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next] [also build test ERROR on drm-tip/drm-tip drm-exynos/exynos-drm-next drm/drm-next tegra-drm/drm/tegra/for-next v5.17-rc3 next-20220207] [cannot apply to airlied/drm-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jordan-Justen/GuC-HWCONFIG-with-doc... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: i386-randconfig-a016-20220207 (https://download.01.org/0day-ci/archive/20220208/202202080749.RRjxhCB2-lkp@i...) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/c122b0dea958e76766ce9b4b9f34d2eed16f... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Jordan-Justen/GuC-HWCONFIG-with-documentation/20220208-032950 git checkout c122b0dea958e76766ce9b4b9f34d2eed16fb566 # save the config file to linux build tree mkdir build_dir make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/i915/
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c: In function 'verify_hwconfig_blob':
drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c:79:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
79 | struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr; | ^~~~~~ cc1: all warnings being treated as errors
vim +79 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
73 74 static int verify_hwconfig_blob(const struct intel_guc_hwconfig *hwconfig) 75 { 76 if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL) 77 return -EINVAL; 78
79 struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr;
80 u32 remaining = (hwconfig->size / 4); 81 while (remaining > 0) { 82 if (remaining < 2) 83 return -EINVAL; 84 if (pos->length > remaining - 2) 85 return -EINVAL; 86 remaining -= 2 + pos->length; 87 pos = (void *)&pos->data[pos->length]; 88 } 89 90 DRM_INFO("hwconfig blob format appears valid\n"); 91 return 0; 92 } 93
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi,
Commit message please.
Will GuC folks be reviewing this work?
Quick sanity check maybe makes sense, given data is being "sent" to userspace directly, I am just not sure if it is worth having in non-debug builds of i915. Though I will agree not having it in production then largely defeats the purpose so dunno. Effective difference if GuC load fails versus userspace libraries failing to parse hwconfig?
On 07/02/2022 19:28, Jordan Justen wrote:
Signed-off-by: Jordan Justen jordan.l.justen@intel.com
.../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c index ce6088f112d4..695ef7a8f519 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c @@ -71,6 +71,26 @@ static int guc_hwconfig_discover_size(struct intel_guc_hwconfig *hwconfig) return 0; }
+static int verify_hwconfig_blob(const struct intel_guc_hwconfig *hwconfig) +{
- if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL)
return -EINVAL;
So individual item size is minimum one u32, or zero? Document that in patch 3?
- struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr;
kbuild robot told you about mixing declarations and code already. :)
- u32 remaining = (hwconfig->size / 4);
Blank line here and braces not needed.
- while (remaining > 0) {
if (remaining < 2)return -EINVAL;if (pos->length > remaining - 2)return -EINVAL;remaining -= 2 + pos->length;pos = (void *)&pos->data[pos->length];- }
- DRM_INFO("hwconfig blob format appears valid\n");
Probably debug level at most.
- return 0;
+}
- static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig) { struct intel_guc *guc = hwconfig_to_guc(hwconfig);
@@ -91,6 +111,12 @@ static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig) if (ret >= 0) memcpy(hwconfig->ptr, vaddr, hwconfig->size);
- if (verify_hwconfig_blob(hwconfig)) {
Merge under the "ret >= 0" branch above?
DRM_ERROR("Ignoring invalid hwconfig blob received from ""GuC!\n");
Use drm_dbg/drm_err so the log is tied to a device in multi-gpu systems.
Also keep the string on one line as per kernel coding style guide.
Regards,
Tvrtko
return -EINVAL;}
i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP);
return ret;
Tvrtko Ursulin tvrtko.ursulin@linux.intel.com writes:
Will GuC folks be reviewing this work?
I don't know. If you mean the i915 devs interfacing with the GuC, I know John/Rodrigo seemed a bit resistant writing the patches to give userspace this trivial format guarantee on the blob.
So, I hoped they would write the patches (3 & 4 in my series), but now I'm hoping they will at least accept the patches.
Quick sanity check maybe makes sense, given data is being "sent" to userspace directly, I am just not sure if it is worth having in non-debug builds of i915. Though I will agree not having it in production then largely defeats the purpose so dunno.
The check seems fairly trivial, and it seems like i915 should provide whatever guidance/guarantee is possible to userspace. (Thus, do it once per boot even on release builds.) See also, the commit message I added.
Effective difference if GuC load fails versus userspace libraries failing to parse hwconfig?
Lots of potential things to consider. Personally, I think for internal Intel builds, if this check fails, then it ought to cause the GuC to always fail to load, which today means the device will be wedged.
For external builds, I think it should still load the GuC but not send the blob to userspace. This is what should happen with the patches in this series. (I really hope this never happens, which it why I think the internal builds should be so harsh.)
Now ... if i915 ever regains the ability to drive the device without the closed source GuC, well... No reason to go off on unrealistic tangents. :)
Also, later down the road for released products, userspace drivers may choose to bypass the hwconfig to limit the dependence on GuC. That is a related, but different topic.
-Jordan
dri-devel@lists.freedesktop.org
-
Jordan Justen -
kernel test robot -
Tvrtko Ursulin