Re: [PATCH 1/2] [v3] nouveau: add command-line GSP-RM registry support
Hi Timur, kernel test robot noticed the following build warnings: [auto build test WARNING on drm-intel/for-linux-next-fixes] [also build test WARNING on drm-tip/drm-tip linus/master v6.8-rc4 next-20240213] [cannot apply to drm-misc/drm-misc-next drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-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#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Timur-Tabi/nouveau-add-command-line-GSP-RM-registry-support/20240213-051852 base: git://anongit.freedesktop.org/drm-intel for-linux-next-fixes patch link: https://lore.kernel.org/r/20240212211548.1094496-2-ttabi%40nvidia.com patch subject: [PATCH 1/2] [v3] nouveau: add command-line GSP-RM registry support config: parisc-defconfig (https://download.01.org/0day-ci/archive/20240214/[email protected]/config) compiler: hppa-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240214/[email protected]/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/ All warnings (new ones prefixed by >>): drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c: In function 'r535_gsp_rpc_set_registry': >> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1317:24: warning: variable >> 'length' set but not used [-Wunused-but-set-variable] 1317 | size_t length; |^~ -- >> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1246: warning: cannot >> understand function prototype: 'const struct nv_gsp_registry_entries >> r535_registry_entries[] = ' drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1673: warning: Function parameter or struct member 'priv' not described in 'r535_gsp_msg_run_cpu_sequencer' drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1673: warning: Function parameter or struct member 'fn' not described in 'r535_gsp_msg_run_cpu_sequencer' drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1673: warning: Function parameter or struct member 'repv' not described in 'r535_gsp_msg_run_cpu_sequencer' drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1673: warning: Function parameter or struct member 'repc' not described in 'r535_gsp_msg_run_cpu_sequencer' drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:2046: warning: Function parameter or struct member 'gsp' not described in 'r535_gsp_libos_init' drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:2186: warning: Function parameter or struct member 'gsp' not described in 'nvkm_gsp_radix3_sg' drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:2186: warning: Function parameter or struct member 'sgt' not described in 'nvkm_gsp_radix3_sg' drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:2186: warning: Function parameter or struct member 'size' not described in 'nvkm_gsp_radix3_sg' drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:2186: warning: Function parameter or struct member 'rx3' not described in 'nvkm_gsp_radix3_sg' vim +/length +1317 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c 1235 1236 /** 1237 * r535_registry_entries - required registry entries for GSP-RM 1238 * 1239 * This array lists registry entries that are required for GSP-RM to 1240 * function correctly. 1241 * 1242 * RMSecBusResetEnable - enables PCI secondary bus reset 1243 * RMForcePcieConfigSave - forces GSP-RM to preserve PCI configuration 1244 * registers on any PCI reset. 1245 */ > 1246 static const struct nv_gsp_registry_entries r535_registry_entries[] = { 1247 { "RMSecBusResetEnable", 1 }, 1248 { "RMForcePcieConfigSave", 1 }, 1249 }; 1250 #define NV_GSP_REG_NUM_ENTRIES ARRAY_SIZE(r535_registry_entries) 1251 1252 /** 1253 * strip - strips all characters in 'reject' from 's' 1254 * @s: string to strip 1255 * @reject: string of characters to remove 1256 * 1257 * 's' is modified. 1258 * 1259 * Returns the length of the new string. 1260 */ 1261 static size_t strip(char *s, const char *reject) 1262 { 1263 char *p = s, *p2 = s; 1264 size_t length = 0; 1265 char c; 1266 1267 do { 1268 while ((c = *p2) && strchr(reject, c)) 1269 p2++; 1270 1271 *p++ = c = *p2++; 1272 length++; 1273 } while (c); 1274 1275 return length; 1276 } 1277 1278 /** 1279 * r535_gsp_rpc_set_registry - build registry RPC and call GSP-RM 1280 * @gsp: gsp pointer 1281 * 1282 * The GSP-RM registry is a set of
Re: [PATCH 1/2] [v3] nouveau: add command-line GSP-RM registry support
On Tue, 2024-02-13 at 16:43 +0100, Danilo Krummrich wrote:
> > +struct registry_list_entry {
> > + struct list_head list;
>
> Nit: 'head' or 'entry' might be more suitable.
Will fix in v4.
>
> > + size_t name_len;
> > + u32 type;
>
> I prefer to represent type as enum or use a define for '1 = integer' instead.
> This also gets you rid of the need to explicitly explain it's meaning in the
> documentation of add_registry().
Will fix in v4.
>
> > + u32 data;
> > + u32 length;
> > + char name[];
> > +};
> > +
> > +/**
> > + * add_registry -- adds a registry entry
> > + * @gsp: gsp pointer
> > + * @name: name of the registry key
> > + * @type: type of data (1 = integer)
> > + * @data: value
> > + * @length: size of data, in bytes
> > + *
> > + * Adds a registry key/value pair to the registry database.
> > + *
> > + * Currently, only 32-bit integers (type == 1, length == 4) are supported.
>
> What if we pass something else? This function doesn't seem to ensure nothing
> else is accepted. Although I recognize that it's only used by
> add_registry_num()
> enforcing this limitation by it's function signature.
GSP-RM technically supports two other types: binary data and strings. For
example, apparently it's possible to override the VBIOS itself with a
registry key. However, I'm not familiar with any actual non-numeric
registry keys that the user would set.
I can add support for binary data and strings.
>
> > + *
> > + * This function collects the registry information in a linked list. After
> > + * all registry keys have been added, build_registry() is used to create
> > the
> > + * RPC data structure.
> > + *
> > + * registry_rpc_size is a running total of the size of all registry keys.
> > + * It's used to avoid an O(n) calculation of the size when the RPC is
> > built.
> > + *
> > + * Returns 0 on success, or negative error code on error.
> > + */
> > +static int add_registry(struct nvkm_gsp *gsp, const char *name, u32 type,
> > u32 data, u32 length)
> > +{
> > + struct registry_list_entry *reg;
> > + size_t nlen = strlen(name) + 1;
> > +
> > + /* Set an arbitrary limit to avoid problems with broken command lines */
> > + if (strlen(name) > 64)
>
> Could re-use nlen for this check.
Will fix in v4.
>
> > + return -EFBIG;
>
> This error code doesn't seem to apply here, prefer EINVAL.
You don't like creative usage of error codes?
> > + while ((start = strsep(&next, ";"))) {
> > + long value;
> > +
> > + equal = strchr(start, '=');
> > + if (!equal || (equal == start) || !isdigit(equal[1])) {
> > + nvkm_error(&gsp->subdev,
> > + "ignoring invalid registry string
> > '%s'\n", start);
> > + continue;
> > + }
> >
> > - rpc->numEntries = NV_GSP_REG_NUM_ENTRIES;
> > + ret = kstrtol(equal + 1, 0, &value);
> > + if (ret) {
> > + nvkm_error(&gsp->subdev,
> > + "ignoring invalid registry value in
> > '%s'\n", start);
> > + continue;
> > + }
>
> At a first glance, the string parsing looks correct to me. Did you test with
> invalid
> strings as well?
Yes. It would be nice if there were a regex parser in the kernel, but I
think I did a good job testing and rejecting strings.
> > - str_offset = offsetof(typeof(*rpc), entries[NV_GSP_REG_NUM_ENTRIES]);
> > - strings = (char *)&rpc->entries[NV_GSP_REG_NUM_ENTRIES];
> > - for (i = 0; i < NV_GSP_REG_NUM_ENTRIES; i++) {
> > - int name_len = strlen(r535_registry_entries[i].name) + 1;
> > -
> > - rpc->entries[i].nameOffset = str_offset;
> > - rpc->entries[i].type = 1;
> > - rpc->entries[i].data = r535_registry_entries[i].value;
> > - rpc->entries[i].length = 4;
> > - memcpy(strings, r535_registry_entries[i].name, name_len);
> > - strings += name_len;
> > - str_offset += name_len;
> > + /* Truncate the key=value string to just key */
> > + *equal = 0;
>
> What's the purpose for that?
Take for example, this parameter passed to Nouveau:
NVreg_RegistryDwords="RM1457588=1;TEST=53"
The strsep() call points 'next' to "RM1457588=1", replacing the ; with \0.
'equal' then points to the '='. kstrtol() converts the "1" to 1. So all
that's left is extracting the "RM1457588" into its own string. That's what
"*equal = 0" does.
Re: [PATCH 1/2] [v3] nouveau: add command-line GSP-RM registry support
On 2/12/24 22:15, Timur Tabi wrote:
Add the NVreg_RegistryDwords command line parameter, which allows
specifying additional registry keys to be sent to GSP-RM. This
allows additional configuration, debugging, and experimentation
with GSP-RM, which uses these keys to alter its behavior.
Note that these keys are passed as-is to GSP-RM, and Nouveau does
not parse them. This is in contrast to the Nvidia driver, which may
parse some of the keys to configure some functionality in concert with
GSP-RM. Therefore, any keys which also require action by the driver
may not function correctly when passed by Nouveau. Caveat emptor.
The name and format of NVreg_RegistryDwords is the same as used by
the Nvidia driver, to maintain compatibility.
Signed-off-by: Timur Tabi
---
v3: rebased to drm-next
.../gpu/drm/nouveau/include/nvkm/subdev/gsp.h | 6 +
.../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c| 279 --
2 files changed, 261 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
index 6f5d376d8fcc..3fbc57b16a05 100644
--- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
+++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
@@ -211,6 +211,12 @@ struct nvkm_gsp {
struct mutex mutex;;
struct idr idr;
} client_id;
+
+ /* A linked list of registry items. The registry RPC will be built from
it. */
+ struct list_head registry_list;
+
+ /* The size of the registry RPC */
+ size_t registry_rpc_size;
};
static inline bool
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
index 1c46e3f919be..86b62c7e1229 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
@@ -54,6 +54,8 @@
#include
#include
+#include
+#include
#define GSP_MSG_MIN_SIZE GSP_PAGE_SIZE
#define GSP_MSG_MAX_SIZE GSP_PAGE_MIN_SIZE * 16
@@ -1082,51 +1084,280 @@ r535_gsp_rpc_unloading_guest_driver(struct nvkm_gsp
*gsp, bool suspend)
return nvkm_gsp_rpc_wr(gsp, rpc, true);
}
+struct registry_list_entry {
+ struct list_head list;
Nit: 'head' or 'entry' might be more suitable.
+ size_t name_len;
+ u32 type;
I prefer to represent type as enum or use a define for '1 = integer' instead.
This also gets you rid of the need to explicitly explain it's meaning in the
documentation of add_registry().
+ u32 data;
+ u32 length;
+ char name[];
+};
+
+/**
+ * add_registry -- adds a registry entry
+ * @gsp: gsp pointer
+ * @name: name of the registry key
+ * @type: type of data (1 = integer)
+ * @data: value
+ * @length: size of data, in bytes
+ *
+ * Adds a registry key/value pair to the registry database.
+ *
+ * Currently, only 32-bit integers (type == 1, length == 4) are supported.
What if we pass something else? This function doesn't seem to ensure nothing
else is accepted. Although I recognize that it's only used by add_registry_num()
enforcing this limitation by it's function signature.
+ *
+ * This function collects the registry information in a linked list. After
+ * all registry keys have been added, build_registry() is used to create the
+ * RPC data structure.
+ *
+ * registry_rpc_size is a running total of the size of all registry keys.
+ * It's used to avoid an O(n) calculation of the size when the RPC is built.
+ *
+ * Returns 0 on success, or negative error code on error.
+ */
+static int add_registry(struct nvkm_gsp *gsp, const char *name, u32 type, u32
data, u32 length)
+{
+ struct registry_list_entry *reg;
+ size_t nlen = strlen(name) + 1;
+
+ /* Set an arbitrary limit to avoid problems with broken command lines */
+ if (strlen(name) > 64)
Could re-use nlen for this check.
+ return -EFBIG;
This error code doesn't seem to apply here, prefer EINVAL.
+
+ reg = kmalloc(sizeof(struct registry_list_entry) + nlen, GFP_KERNEL);
+ if (!reg)
+ return -ENOMEM;
+
+ memcpy(reg->name, name, nlen);
+ reg->name_len = nlen;
+ reg->type = type;
+ reg->data = data;
+ reg->length = length;
+
+ nvkm_debug(&gsp->subdev, "adding GSP-RM registry '%s=%u'\n", name,
data);
+ list_add_tail(®->list, &gsp->registry_list);
+ gsp->registry_rpc_size += sizeof(PACKED_REGISTRY_ENTRY) + nlen;
+
+ return 0;
+}
+
+static int add_registry_num(struct nvkm_gsp *gsp, const char *name, u32 value)
+{
+ return add_registry(gsp, name, 1, value, sizeof(u32));
+}
+
+/**
+ * build_registry -- create the registry RPC data
+ * @gsp: gsp pointer
+ * @registry: pointer to the RPC payload to fill
+ *
+ * After all registry key/value pairs have been added, call this function to
+ * build the RPC.
+ *
+ * The registry RPC looks like this:
+ *
+ * +-+
+ * |NvU32 size; |
+ * |
