Re: [PATCH v9 10/10] drm/xe/tests: Add KUnit tests for VF provisioning error handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] xe/tests: incorrect ABI macros used for KLV response length in stubs
--

commit c0ab3b2485a7858871113ee1bceb9e8d26f059a2
Author: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>

drm/xe/tests: Add KUnit tests for VF provisioning error handling

VF relies on the PF to provide a valid hardware configuration via GuC
KLV responses. This commit adds KUnit test cases that use the
xe_guc_mmio_send_recv() stub to inject bad KLV responses to verify the
VF error handling.

> diff --git a/drivers/gpu/drm/xe/tests/xe_gt_sriov_vf_kunit.c b/drivers/gpu/drm/xe/tests/xe_gt_sriov_vf_kunit.c
> new file mode 100644
> index 0000000000000..5202ef804240c
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/tests/xe_gt_sriov_vf_kunit.c
> @@ -0,0 +1,666 @@

[ ... ]

> +static inline void fill_resp_buff(u32 *response, u64 query, size_t type)
> +{
> +	if (type == sizeof(u32)) {
> +		response[0] = sizeof(u32) / sizeof(u32);
> +		response[1] = query;
> +	} else if (type == sizeof(u64)) {
> +		response[0] = sizeof(u64) / sizeof(u32);

[Severity: Low]
Does this code bypass the ABI bitfield macros? Direct integer assignment
circumvents the VF2GUC_QUERY_SINGLE_KLV_RESPONSE_MSG_0_LENGTH and MBZ
macros for constructing the response header, which might make the test
fragile to future ABI changes.

> +		response[1] = lower_32_bits(query);
> +		response[2] = upper_32_bits(query);
> +	}
> +}

[ ... ]

> +static int xe_guc_mmio_send_recv_stub_ggtt(struct xe_guc *guc, const u32 *request,
> +					   u32 len, u32 *response_buf)
> +{
> +	struct kunit *test = kunit_get_current_test();
> +	const struct config_ggtt *c = test->param_value;
> +	u32 key;
> +
> +	if (c->ret)
> +		return c->ret;
> +
> +	KUNIT_ASSERT_EQ(test, FIELD_GET(GUC_HXG_REQUEST_MSG_0_ACTION, request[0]),
> +			GUC_ACTION_VF2GUC_QUERY_SINGLE_KLV);
> +	KUNIT_ASSERT_EQ(test, len, VF2GUC_QUERY_SINGLE_KLV_REQUEST_MSG_LEN);
> +	key = FIELD_GET(VF2GUC_QUERY_SINGLE_KLV_REQUEST_MSG_1_KEY, request[1]);
> +	KUNIT_ASSERT_TRUE(test,
> +			  key == GUC_KLV_VF_CFG_GGTT_START_KEY ||
> +			  key == GUC_KLV_VF_CFG_GGTT_SIZE_KEY);
> +
> +	response_buf[0] = FIELD_PREP(GUC_HXG_RESPONSE_MSG_0_DATA0,
> +				     sizeof(u64) / sizeof(u32));

[Severity: Low]
Does this code use the correct macro for the KLV length field? It uses
GUC_HXG_RESPONSE_MSG_0_DATA0, which appears to belong to a different message
type (HXG AUX data) instead of VF2GUC_QUERY_SINGLE_KLV_RESPONSE_MSG_0_LENGTH.
While it happens to work, it seems to bypass the ABI definition.

> +	if (key == GUC_KLV_VF_CFG_GGTT_START_KEY) {
> +		response_buf[1] = lower_32_bits(c->query.start);
> +		response_buf[2] = upper_32_bits(c->query.start);
> +	} else if (key == GUC_KLV_VF_CFG_GGTT_SIZE_KEY) {
> +		response_buf[1] = lower_32_bits(c->query.size);
> +		response_buf[2] = upper_32_bits(c->query.size);
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918100533.567225-12-satyanarayana.k.v.p@intel.com?part=10