[PATCH 1/2] [v3] nouveau: add command-line GSP-RM registry support

[Timur Tabi]

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.