Re: [PATCH v6 7/7] drm/xe/tests: Add KUnit tests for VF provisioning error handling
Michal Wajdeczko <[email protected]>
| Newsgroups | org.freedesktop.lists.intel-xe |
|---|---|
| Message-ID | <[email protected]> |
On 8/12/2026 2:43 PM, Satyanarayana K V P wrote: > VF relies on the PF to provide a valid hardware configuration via GuC > KLV responses. In unlikely event of PF malfunction or misconfiguration, > a VF may receive incomplete, zero, or out-of-range values for its > submission contexts, doorbells, VRAM or GGTT assignment. > > Add KUnit test cases that use the xe_guc_mmio_send_recv() stub to inject > bad KLV responses and verify that VF can survive without crashing for > the invalid configuration data received. > > Signed-off-by: Satyanarayana K V P <[email protected]> > Cc: Michal Wajdeczko <[email protected]> > --- > V5 -> V6: > - Fixed review comments (Michal W). > > V4 -> V5: > - Added alignment tests for vram, GGTT base and size (Michal W). > - Separated stubs for ctx, db, vram and GGTT (Michal W). > - Added new test cases for xe_guc_mmio_send_recv() (Michal W). > > V3 -> V4: > - Changed stub function from guc_action_query_single_klv32() and > guc_action_query_single_klv64() to xe_guc_mmio_send_recv() (Michal W). > - Fixed review comments (Michal W). > > V2 -> V3: > - Renamed the test names. (Michal W). > - Fixed review comments (Michal W). > > V1 -> V2: > - Renamed the test file (Michal W). > - Fixed review comments (Michal W). > --- > .../gpu/drm/xe/tests/xe_gt_sriov_vf_kunit.c | 484 ++++++++++++++++++ > drivers/gpu/drm/xe/xe_gt_sriov_vf.c | 4 + > 2 files changed, 488 insertions(+) > create mode 100644 drivers/gpu/drm/xe/tests/xe_gt_sriov_vf_kunit.c > > 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 000000000000..8cb7f3cebb31 > --- /dev/null > +++ b/drivers/gpu/drm/xe/tests/xe_gt_sriov_vf_kunit.c > @@ -0,0 +1,484 @@ > +// SPDX-License-Identifier: GPL-2.0 AND MIT > +/* > + * Copyright © 2026 Intel Corporation > + */ > + > +#include <kunit/static_stub.h> > +#include <kunit/test.h> > + > +#include "regs/xe_guc_regs.h" > +#include "xe_device.h" > +#include "xe_kunit_helpers.h" > +#include "xe_pci_test.h" > +#include "xe_guc.h" > + > +#define TEST_VRAM SZ_8G > +#define TEST_GGTT_SIZE SZ_2G > +#define TEST_CTXS 1024 > +#define TEST_DBS 128 > +#define TEST_GGTT_START 0xa0a00000ull > + > +struct config { > + u32 orig; > + u32 query; > + int expected_ret; > + int ret; > + const char *name; > +} vf_ctx_testcase[] = { > + { > + .orig = TEST_CTXS, .query = TEST_CTXS, > + .expected_ret = 0, .name = "same", > + }, > + { > + .orig = 0, .query = 0, > + .expected_ret = -ENODATA, .name = "none", > + }, > + { > + .orig = 0, .query = GUC_ID_MAX + 1, > + .expected_ret = -EPROTO, .name = "overflow", > + }, > + { > + .orig = TEST_CTXS, .query = 0, > + .expected_ret = -EREMCHG, .name = "lost", > + }, > + { > + .orig = TEST_CTXS, .query = TEST_CTXS / 2, > + .expected_ret = -EREMCHG, .name = "reduced", > + }, > + { > + .orig = TEST_CTXS, .query = TEST_CTXS * 2, > + .expected_ret = -EREMCHG, .name = "increased", > + }, > + { > + .expected_ret = -EPROTO, .ret = -EPROTO, > + .name = "proto_error", > + }, > + { > + .expected_ret = -EIO, .ret = -EIO, > + .name = "IO error", > + }, > +}, vf_db_testcase[] = { > + { > + .orig = TEST_DBS, .query = TEST_DBS, > + .expected_ret = 0, .name = "same", > + }, > + { > + .orig = 0, .query = 0, > + .expected_ret = 0, .name = "none", > + }, > + { > + .orig = TEST_DBS, .query = GUC_NUM_DOORBELLS + 1, > + .expected_ret = -EPROTO, .name = "overflow", > + }, > + { > + .orig = TEST_DBS, .query = 0, > + .expected_ret = -EREMCHG, .name = "lost", > + }, > + { > + .orig = TEST_DBS, .query = TEST_DBS / 2, > + .expected_ret = -EREMCHG, .name = "reduced", > + }, > + { > + .orig = TEST_DBS, .query = GUC_NUM_DOORBELLS - 1, > + .expected_ret = -EREMCHG, .name = "increased", > + }, > + { > + .expected_ret = -EPROTO, .ret = -EPROTO, > + .name = "proto_error" > + }, > + { > + .expected_ret = -EIO, .ret = -EIO, > + .name = "IO error", > + }, > +}; > + > +struct config_vram { > + u64 orig; > + u64 query; > + int expected_ret; > + int ret; > + const char *name; > +} vf_vram_testcase[] = { > + { > + .orig = TEST_VRAM, .query = TEST_VRAM, > + .expected_ret = 0, .name = "same", > + }, > + { > + .orig = 0, .query = 0, > + .expected_ret = -ENODATA, .name = "none", > + }, > + { > + .orig = TEST_VRAM / 2, .query = TEST_VRAM + SZ_1G, > + .expected_ret = -EREMCHG, .name = "overflow", > + }, > + { > + .orig = TEST_VRAM / 2, .query = 0, > + .expected_ret = -EREMCHG, .name = "lost", > + }, > + { > + .orig = TEST_VRAM / 2, .query = TEST_VRAM / 4, > + .expected_ret = -EREMCHG, .name = "reduced", > + }, > + { > + .orig = TEST_VRAM, .query = TEST_VRAM * 2, > + .expected_ret = -EREMCHG, .name = "increased", > + }, > + { > + .orig = TEST_VRAM - SZ_1M, .query = TEST_VRAM - SZ_1M, > + .expected_ret = -EINVAL, .name = "unaligned", > + }, > + { > + .expected_ret = -EPROTO, .ret = -EPROTO, > + .name = "proto_error" > + }, > + { > + .expected_ret = -EIO, .ret = -EIO, > + .name = "IO error", > + }, > +}; > + > +struct config_ggtt { > + u64 start_orig; > + u64 start_query; > + u64 size_orig; > + u64 size_query; > + int expected_ret; > + int ret; > + int flags; > + const char *name; > +} vf_ggtt_testcase[] = { static const ? as a bonus you likely will not need to initialize some members to 0 > + { > + .start_orig = TEST_GGTT_START, .start_query = TEST_GGTT_START, > + .size_orig = TEST_GGTT_SIZE, .size_query = TEST_GGTT_SIZE, > + .expected_ret = 0, .flags = 0, > + .name = "same", > + }, > + { > + .start_orig = 0, .start_query = 0, > + .size_orig = 0, .size_query = 0, > + .expected_ret = -ENODATA, .flags = 0, > + .name = "none", > + }, we need all those negative test cases also for the initial query (orig = 0) > + { > + .start_orig = TEST_GGTT_START, .start_query = TEST_GGTT_START, > + .size_orig = TEST_GGTT_SIZE, .size_query = TEST_GGTT_SIZE + SZ_1G, > + .expected_ret = -EREMCHG, .flags = 0, > + .name = "overflow", > + }, > + { > + .start_orig = TEST_GGTT_START, .start_query = TEST_GGTT_START, > + .size_orig = TEST_GGTT_SIZE, .size_query = 0, > + .expected_ret = -ENODATA, .flags = 0, > + .name = "lost", > + }, > + { > + .start_orig = TEST_GGTT_START, .start_query = TEST_GGTT_START, > + .size_orig = TEST_GGTT_SIZE, .size_query = TEST_GGTT_SIZE - SZ_1M, > + .expected_ret = -EREMCHG, .flags = 0, > + .name = "reduced", > + }, > + { > + .start_orig = TEST_GGTT_START, .start_query = TEST_GGTT_START, > + .size_orig = TEST_GGTT_SIZE, .size_query = TEST_GGTT_SIZE + SZ_1M, > + .expected_ret = -EREMCHG, .flags = 0, > + .name = "increased", > + }, > + { > + .start_orig = TEST_GGTT_START, .start_query = TEST_GGTT_START, > + .size_orig = TEST_GGTT_SIZE - SZ_2K, .size_query = TEST_GGTT_SIZE - SZ_2K, it shouldn't be possible to have unaligned original GGTT size (size_orig) > + .expected_ret = -EINVAL, .flags = 0, > + .name = "unaligned_size_4K", > + > + }, > + { > + .start_orig = TEST_GGTT_START, .start_query = TEST_GGTT_START, > + .size_orig = TEST_GGTT_SIZE - SZ_8K, .size_query = TEST_GGTT_SIZE - SZ_8K, > + .expected_ret = -EINVAL, .flags = XE_VRAM_FLAGS_NEED64K, > + .name = "unaligned_size_64K", > + }, ditto > + { > + .start_orig = TEST_GGTT_START - SZ_2K, .start_query = TEST_GGTT_START - SZ_2K, > + .size_orig = TEST_GGTT_SIZE, .size_query = TEST_GGTT_SIZE, > + .expected_ret = -EINVAL, .flags = 0, > + .name = "unaligned_base_4k", > + }, same here, we should never store invalid GGTT start > + { > + .start_orig = TEST_GGTT_START - SZ_8K, .start_query = TEST_GGTT_START - SZ_8K, > + .size_orig = TEST_GGTT_SIZE, .size_query = TEST_GGTT_SIZE, > + .expected_ret = -EINVAL, .flags = XE_VRAM_FLAGS_NEED64K, > + .name = "unaligned_base_64k", > + }, > + { > + .expected_ret = -EPROTO, .ret = -EPROTO, > + .name = "proto_error" > + }, > + { > + .expected_ret = -EIO, .ret = -EIO, > + .name = "IO error", > + }, > +}; > + > +struct config_mmio { > + u32 response_length; > + u32 response_value; > + int expected_ret; > + int ret; > + const char *name; > +} guc_mmio_resp_testcase[] = { > + { > + .response_length = sizeof(u32) / sizeof(u32), > + .response_value = TEST_CTXS, .expected_ret = 0, .name = "same", > + }, > + { > + .response_length = 0, .response_value = 0, > + .expected_ret = -ENODATA, .name = "none", > + }, > + { > + .response_length = sizeof(u64) / sizeof(u32), > + .response_value = TEST_CTXS, .expected_ret = -EOVERFLOW, > + .name = "overflow", > + }, > + { > + .response_length = sizeof(u32) / sizeof(u32), > + .response_value = 0, .expected_ret = -EREMCHG, > + .name = "lost", > + }, > + { > + .response_length = 0xffff, .response_value = 0xffff, > + .expected_ret = -EOVERFLOW, .name = "invalid", > + }, > + { > + .expected_ret = -EPROTO, .ret = -EPROTO, > + .name = "proto_error", > + }, > + { > + .expected_ret = -EIO, .ret = -EIO, > + .name = "IO error", > + }, > +}; > + > +KUNIT_ARRAY_PARAM_DESC(ctx_testcase, vf_ctx_testcase, name); > +KUNIT_ARRAY_PARAM_DESC(db_testcase, vf_db_testcase, name); > +KUNIT_ARRAY_PARAM_DESC(vram_testcase, vf_vram_testcase, name); > +KUNIT_ARRAY_PARAM_DESC(ggtt_testcase, vf_ggtt_testcase, name); > +KUNIT_ARRAY_PARAM_DESC(guc_mmio_resp, guc_mmio_resp_testcase, name); hmm, I'm wondering if maybe keeping config struct + KNIT_ARRAY helper closer to the actual test case code will be easier to view/maintain ? > + > +static int xe_guc_mmio_send_recv_stub_ctx(struct xe_guc *guc, const u32 *request, > + u32 len, u32 *response_buf) > +{ > + struct kunit *test = kunit_get_current_test(); > + const struct config *c = test->param_value; > + > + 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, > + FIELD_GET(VF2GUC_QUERY_SINGLE_KLV_REQUEST_MSG_1_KEY, request[1]), > + GUC_KLV_VF_CFG_NUM_CONTEXTS_KEY); > + > + response_buf[0] = sizeof(u32) / sizeof(u32); > + response_buf[1] = c->query; > + > + return 0; > +} > + > +static int xe_guc_mmio_send_recv_stub_db(struct xe_guc *guc, const u32 *request, > + u32 len, u32 *response_buf) > +{ > + struct kunit *test = kunit_get_current_test(); > + const struct config *c = test->param_value; > + > + 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, > + FIELD_GET(VF2GUC_QUERY_SINGLE_KLV_REQUEST_MSG_1_KEY, request[1]), > + GUC_KLV_VF_CFG_NUM_DOORBELLS_KEY); > + > + response_buf[0] = sizeof(u32) / sizeof(u32); > + response_buf[1] = c->query; > + > + return 0; > +} > + > +static int xe_guc_mmio_send_recv_stub_vram(struct xe_guc *guc, const u32 *request, > + u32 len, u32 *response_buf) > +{ > + struct kunit *test = kunit_get_current_test(); > + const struct config_vram *c = test->param_value; > + > + 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, > + FIELD_GET(VF2GUC_QUERY_SINGLE_KLV_REQUEST_MSG_1_KEY, request[1]), > + GUC_KLV_VF_CFG_LMEM_SIZE_KEY); > + > + response_buf[0] = sizeof(u64) / sizeof(u32); > + response_buf[1] = lower_32_bits(c->query); > + response_buf[2] = upper_32_bits(c->query); > + > + return 0; > +} > + > +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); > + 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] = sizeof(u64) / sizeof(u32); > + if (key == GUC_KLV_VF_CFG_GGTT_START_KEY) { > + response_buf[1] = lower_32_bits(c->start_query); > + response_buf[2] = upper_32_bits(c->start_query); > + } else { else if (key == START) > + response_buf[1] = lower_32_bits(c->size_query); > + response_buf[2] = upper_32_bits(c->size_query); > + } else KUNIT_FAIL > + > + return 0; > +} > + > +static int xe_guc_mmio_send_recv_stub(struct xe_guc *guc, const u32 *request, > + u32 len, u32 *response_buf) > +{ > + struct kunit *test = kunit_get_current_test(); > + const struct config_mmio *c = test->param_value; > + > + 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); > + /** > + * Let us take help of vf_get_ctxs_cfg() function to test responses > + * from Guc. > + */ do we really need this comment? > + KUNIT_ASSERT_EQ(test, > + FIELD_GET(VF2GUC_QUERY_SINGLE_KLV_REQUEST_MSG_1_KEY, request[1]), > + GUC_KLV_VF_CFG_NUM_CONTEXTS_KEY); > + > + response_buf[0] = c->response_length; > + response_buf[1] = c->response_value; > + > + return 0; > +} > + > +static void test_ctxs(struct kunit *test) > +{ > + struct xe_gt *gt = test->priv; > + const struct config *c = test->param_value; > + > + gt->sriov.vf.self_config.num_ctxs = c->orig; > + kunit_activate_static_stub(test, xe_guc_mmio_send_recv, > + xe_guc_mmio_send_recv_stub_ctx); > + KUNIT_EXPECT_EQ(test, vf_get_ctxs_cfg(gt), c->expected_ret); > +} > + > +static void test_dbs(struct kunit *test) > +{ > + struct xe_gt *gt = test->priv; > + const struct config *c = test->param_value; > + > + gt->sriov.vf.self_config.num_dbs = c->orig; > + kunit_activate_static_stub(test, xe_guc_mmio_send_recv, > + xe_guc_mmio_send_recv_stub_db); > + KUNIT_EXPECT_EQ(test, vf_get_dbs_cfg(gt), c->expected_ret); > +} > + > +static void test_vram(struct kunit *test) > +{ > + struct xe_gt *gt = test->priv; > + const struct config_vram *c = test->param_value; > + > + gt->tile->sriov.vf.self_config.lmem_size = c->orig; > + kunit_activate_static_stub(test, xe_guc_mmio_send_recv, > + xe_guc_mmio_send_recv_stub_vram); > + KUNIT_EXPECT_EQ(test, vf_get_lmem_info(gt), c->expected_ret); > +} > + > +static void test_ggtt(struct kunit *test) > +{ > + const struct config_ggtt *c = test->param_value; > + struct xe_gt *gt = test->priv; > + struct xe_device *xe = gt_to_xe(gt); > + > + gt->tile->sriov.vf.self_config.ggtt_base = c->start_orig; > + gt->tile->sriov.vf.self_config.ggtt_size = c->size_orig; > + > + xe->info.vram_flags = c->flags; hmm, if flags are only used here, then we should name it c->vram_flags but OTOH maybe it is overkill to test for 64K mis-alignment if we already have test for 4K and the code under test is the same? > + > + kunit_activate_static_stub(test, xe_guc_mmio_send_recv, > + xe_guc_mmio_send_recv_stub_ggtt); > + KUNIT_EXPECT_EQ(test, vf_get_ggtt_info(gt), c->expected_ret); > +} > + > +static void test_guc_mmio(struct kunit *test) > +{ > + const struct config_mmio *c = test->param_value; > + struct xe_gt *gt = test->priv; > + > + gt->sriov.vf.self_config.num_ctxs = TEST_CTXS; > + kunit_activate_static_stub(test, xe_guc_mmio_send_recv, > + xe_guc_mmio_send_recv_stub); > + KUNIT_EXPECT_EQ(test, vf_get_ctxs_cfg(gt), c->expected_ret); > +} > + > +static int vf_gt_config_test_init(struct kunit *test) > +{ > + struct xe_pci_fake_data fake = { > + .sriov_mode = XE_SRIOV_MODE_VF, > + .platform = XE_BATTLEMAGE, /* any random DGFX platform with SR-IOV */ > + .subplatform = XE_SUBPLATFORM_NONE, > + .graphics_verx100 = 2001, > + }; > + struct xe_device *xe; > + struct xe_gt *gt; > + > + test->priv = &fake; > + xe_kunit_helper_xe_device_test_init(test); > + > + xe = test->priv; > + KUNIT_ASSERT_TRUE(test, IS_SRIOV_VF(xe)); > + KUNIT_ASSERT_TRUE(test, IS_DGFX(xe)); > + > + gt = xe_root_mmio_gt(xe); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gt); > + test->priv = gt; > + > + return 0; > +} > + > +static struct kunit_case vf_gt_config_test_cases[] = { > + KUNIT_CASE_PARAM(test_ctxs, ctx_testcase_gen_params), > + KUNIT_CASE_PARAM(test_dbs, db_testcase_gen_params), > + KUNIT_CASE_PARAM(test_vram, vram_testcase_gen_params), > + KUNIT_CASE_PARAM(test_ggtt, ggtt_testcase_gen_params), > + KUNIT_CASE_PARAM(test_guc_mmio, guc_mmio_resp_gen_params), > + {} > +}; > + > +static struct kunit_suite vf_gt_config_suite = { > + .name = "vf_gt_config", > + .test_cases = vf_gt_config_test_cases, > + .init = vf_gt_config_test_init, > +}; > + > +kunit_test_suite(vf_gt_config_suite); > diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c > index be932ebe6a8d..d6b631cd4dfc 100644 > --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c > +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c > @@ -1684,3 +1684,7 @@ int xe_gt_sriov_vf_wait_valid_ggtt(struct xe_gt *gt) > > return atomic_read(>->sriov.vf.migration.fixups_complete_count); > } > + > +#if IS_BUILTIN(CONFIG_DRM_XE_KUNIT_TEST) > +#include "tests/xe_gt_sriov_vf_kunit.c" > +#endif