Re: [PATCH v20 net-next 7/9] octeontx2-af: npc: Support for custom KPU profile from filesystem

[Ratheesh Kannoth]

On 2026-06-09 at 09:34:51, Ratheesh Kannoth (rkannoth@xxxxxxxxxxx) wrote:
> Flashing updated firmware on deployed devices is cumbersome. Provide a
> mechanism to load a custom KPU (Key Parse Unit) profile directly from
> the filesystem at module load time.
>
> When the rvu_af module is loaded with the kpu_profile parameter, the
> specified profile is read from /lib/firmware/kpu and programmed into
> the KPU registers. Add npc_kpu_profile_cam2 for the extended cam format
> used by filesystem-loaded profiles and support ptype/ptype_mask in
> npc_config_kpucam when profile->from_fs is set.
>
> Usage:
>   1. Copy the KPU profile file to /lib/firmware/kpu.
>   2. Build OCTEONTX2_AF as a module.
>   3. Load: insmod rvu_af.ko kpu_profile=<profile_name>
>
> Signed-off-by: Ratheesh Kannoth <rkannoth@xxxxxxxxxxx>

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260609040453.711932-1-rkannoth%40marvell.com says:

>  	cam0.dp2_data = ~kpucam->dp2 & kpucam->dp2_mask;
>>
>> +	if (profile->from_fs) {
>> +		u8 ptype = kpucam2->ptype;
>> +		u8 pmask = kpucam2->ptype_mask;
>> +
>> +		*val |= FIELD_PREP(GENMASK_ULL(57, 56), ptype & pmask);
>> +		*mask |= FIELD_PREP(GENMASK_ULL(57, 56), ~ptype & pmask);
>> +	}
>Bits [57:56] of this register fall inside the field that struct
>npc_kpu_cam currently declares as reserved:
>    struct npc_kpu_cam {
>    #if defined(__BIG_ENDIAN_BITFIELD)
>        u64 rsvd_63_56     : 8;
>        ...
>    #else
>        ...
>        u64 rsvd_63_56     : 8;
>    #endif
>    }
>The new code repurposes two of those reserved bits, gated only by the
>software flag profile->from_fs.  Is there a hardware capability
>indicator (chip revision, NPC_AF_CONST bit, etc.) that should also be
>consulted before driving these bits?  On silicon that does not
>implement the extended cam format, what behaviour do bits [57:56]
>exhibit?
>Should struct npc_kpu_cam be updated so the reserved field reflects
>the new ptype usage rather than leaving the kernel's hardware
>description and the runtime behaviour disagreeing?
Will update kernel's hw description as a patch to "net-next" once this
series is merged.

>> +		profile->kpu[kpu].cam_entries2 = entries;
>> +		profile->kpu[kpu].action_entries2 = entries;
>> +		ret = npc_alloc_kpu_cam2_n_action2(rvu, kpu, entries);
>> +		if (ret) {
>> +			dev_warn(rvu->dev,
>> +				 "profile entry allocation failed for kpu=%d for %d entries\n",
>> +				 kpu, entries);
>> +			return -EINVAL;
>> +		}
>Are cam_entries2 and action_entries2 being assigned in the wrong
>order with the allocation?  When npc_alloc_kpu_cam2_n_action2() fails
>(for example after cam2 succeeded but action2 returned -ENOMEM), the
>function returns -EINVAL but profile->kpu[kpu].cam_entries2 and
>action_entries2 stay at the entries value while cam2 / action2 may
>be partly NULL.
>profile->kpu aliases the file-scope npc_kpu_profiles[] array (set in
>npc_prepare_default_kpu()), so the inconsistent state persists in
>the static array even after the caller falls back to default.  The
>fallback path in npc_load_kpu_profile() calls npc_prepare_default_kpu()
>again, but that helper does not clear cam_entries2, action_entries2,
>cam2 or action2.
>Could the counts be set only after successful allocation, or zeroed
>on the error return?
cam_entries2 and action_entries2 are used only in case of loading from fs. So
it wont cause any issues.

> +static int npc_load_kpu_profile_from_fs(struct rvu *rvu)
>> +{
>> +	struct npc_kpu_profile_adapter *profile = &rvu->kpu;
>> +	const char *kpu_profile = rvu->kpu_pfl_name;
>> +	const struct firmware *fw = NULL;
>> +	int ret, fw_kpus = 0;
>> +	char path[512] = "kpu/";
>> +
>> +	if (strlen(kpu_profile) > sizeof(path) - strlen("kpu/") - 1) {
>> +		dev_err(rvu->dev, "kpu profile name is too big\n");
>> +		return -ENOSPC;
>> +	}
>> +
>> +	strcat(path, kpu_profile);
>> +
>> +	if (request_firmware_direct(&fw, path, rvu->dev))
>> +		return -ENOENT;
>> +
>> +	dev_info(rvu->dev, "Loading KPU profile from filesystem: %s\n",
>> +		 path);
>> +
>> +	rvu->kpu_fwdata = fw->data;
>> +	rvu->kpu_fwdata_sz = fw->size;
>> +
>> +	ret = npc_apply_custom_kpu(rvu, profile, true, &fw_kpus);
>> +	release_firmware(fw);
>> +	rvu->kpu_fwdata = NULL;
>> +
>> +	if (ret) {
>> +		rvu->kpu_fwdata_sz = 0;
>> +		dev_err(rvu->dev,
>> +			"Loading KPU profile from filesystem failed\n");
>> +		return ret;
>> +	}
>[Medium, Low]
>Is the change of the firmware search path a backwards-compatibility
>regression for existing users?
This was modified as part of review comment from Simon.

>On a separate note, on the success path this function leaves
>rvu->kpu_fwdata == NULL but rvu->kpu_fwdata_sz unchanged at fw->size
>(only the error path resets the size to 0).  rvu_npc_freemem() does
>kfree(rvu->kpu_fwdata) for the non-mmapped case, which assumes that
>field is either NULL or kmalloc'd memory; here rvu->kpu_fwdata is
>briefly aliased to fw->data (not kmalloc'd) before being NULLed.
>Should kpu_fwdata_sz be reset to 0 on success too, and should the
>firmware bytes be copied into kmalloc storage as the old code did to
>keep the pointer/size pair consistent?

This was intentional change. rvu->kpu_fwdata_sz is used later so we kept it
as such.

>> +				     npc_get_ikpu_nth_entry(rvu, idx),
>> +				     0, idx, true);
>>
>>  	/* Program KPU CAM and Action profiles */
>>  	num_kpus = rvu->kpu.kpus;
>> @@ -2068,6 +2348,11 @@ static void npc_parser_profile_init(struct rvu *rvu, int blkaddr)
>>
>>  	for (idx = 0; idx < num_kpus; idx++)
>>  		npc_program_kpu_profile(rvu, blkaddr, idx, &rvu->kpu.kpu[idx]);
>> +
>> +	if (profile->from_fs) {
>> +		rvu_write64(rvu, blkaddr, NPC_AF_PKINDX_TYPE(54), 0x03);
>> +		rvu_write64(rvu, blkaddr, NPC_AF_PKINDX_TYPE(58), 0x03);
>> +	}
>>  }
>Just above, the same function clamps PKIND iteration with an explicit
>comment about hardware capability:
>    /* First program IKPU profile i.e PKIND configs.
>     * Check HW max count to avoid configuring junk or
>     * writing to unsupported CSR addresses.
>     */
>    num_pkinds = rvu->kpu.pkinds;
>    num_pkinds = min_t(int, hw->npc_pkinds, num_pkinds);
>Do these new writes to NPC_AF_PKINDX_TYPE(54) and
>NPC_AF_PKINDX_TYPE(58) bypass that safeguard?  hw->npc_pkinds is read
>from NPC_AF_CONST1 at runtime and stored as u8, so 54/58 are not
>statically guaranteed to be valid on all silicon variants where this
>non-cn20k path runs.
>Could these be guarded with a check against hw->npc_pkinds, or
>expressed via a symbolic constant tied to a chip-revision check?
>Also, the values 54, 58, and 0x03 appear here without commentary or
>mention in the commit log.  Could a brief comment or named constant
>explain what these PKIND indices represent and what 0x03 selects, so
>the kernel-firmware contract is visible to readers?
This is very low level hw bits (similar to npc_profile.h)