Re: [PATCH] [v5] nouveau: add command-line GSP-RM registry support

[Timur Tabi <ttabi@xxxxxxxxxx>]

On Wed, 2024-04-10 at 13:30 +0200, Danilo Krummrich wrote:

> On Tue, Apr 09, 2024 at 06:15:52PM -0500, 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 <ttabi@xxxxxxxxxx>
> > ---
> > v5:
> > Add REGISTRY_MAX_KEY_LENGTH
> > registry_list_entry.key is now char[64] instead of char *
> > use strnlen instead of strlen
> > removed some debug printks
> > fixed most checkpatch complaints
> > rebased to drm-fixes
>
> This patch seems to be based on drm-misc-fixes instead. For this patch the
> correct target branch would be drm-misc-next though.

Ok, but then you need to pick up Kees' patch "nouveau/gsp: Avoid addressing beyond end of rpc->entries" because I expect my patch to be applied on top of it.  That's why I chose that branch.

> You can, additionally, use '--base' when running git format-patch. This will
> include the hash of the base commit.

I will take drm-misc-next, add Kees' patch, and rebase onto that.  I'll use --base but I'm not sure whether it will print something useful at this point.

> > +struct registry_list_entry {
> > +	struct list_head head;
> > +	enum registry_type type;
> > +	size_t klen;
> > +	size_t vlen;
> > +	char key[REGISTRY_MAX_KEY_LENGTH] __counted_by(klen);
>
> Now that this is an array, we should remove the __counted_by() annotation.

Ok.

> > +	u32 dword;				/* TYPE_DWORD */
> > +	u8 binary[] __counted_by(vlen);	/* TYPE_BINARY or TYPE_STRING */
>
> NIT: Can't we put these two into a union?

Sure.

> > +static int add_registry(struct nvkm_gsp *gsp, const char *key,
> > +			enum registry_type type, const void *data, size_t length)
> > +{
> > +	struct registry_list_entry *reg;
> > +	size_t nlen = strnlen(key, REGISTRY_MAX_KEY_LENGTH) + 1;
>
> Guess the only reason for strnlen() here is that you want to stop counting once
> you exceed REGISTRY_MAX_KEY_LENGTH anyway, right?

Exactly.  

> > +	size_t vlen; /* value length, non-zero if binary or string */
> > +
> > +	if (nlen > REGISTRY_MAX_KEY_LENGTH)
> > +		return -EFBIG;
>
> Still prefer EINVAL, EFBIG doesn't really apply here.

I knew I was forgetting something.

> > +	vlen = (type == REGISTRY_TABLE_ENTRY_TYPE_DWORD) ? 0 : length;
> > +
> > +	reg = kmalloc(sizeof(*reg) + vlen, GFP_KERNEL);
> > +	if (!reg)
> > +		return -ENOMEM;
> > +
> > +	switch (type) {
> > +	case REGISTRY_TABLE_ENTRY_TYPE_DWORD:
> > +		reg->dword = *(const u32 *)(data);
> > +		break;
> > +	case REGISTRY_TABLE_ENTRY_TYPE_BINARY:
> > +	case REGISTRY_TABLE_ENTRY_TYPE_STRING:
> > +		memcpy(reg->binary, data, length);
>
> Prefer to use vlen here...

I'll change it, but I think 'length' is more correct, since it's supposed to be the actual size of the data, whereas 'vlen' is just used to determine the size of the structure.  Especially since ...

> > +		break;
> > +	default:
> > +		nvkm_error(&gsp->subdev, "unrecognized registry type %u for '%s'\n",
> > +			   type, key);
> > +		kfree(reg);
> > +		return -EINVAL;
> > +	}
> > +
> > +	memcpy(reg->key, key, nlen);
> > +	reg->klen = nlen;
> > +	reg->vlen = length;
>
> ...and here.

... it can't be vlen here because the value has to be '4' for dwords, and 'vlen' is 0 for dwords.