[PATCH i-g-t v2 7/9] tests/intel/xe_pxp: Test PXP submissions
Teres Alexis, Alan Previn
alan.previn.teres.alexis at intel.com
Tue Jan 28 02:44:49 UTC 2025
i have a couple of comments that are either nits or
out of scope, however i do make one request in __intel_bb_add_object
and wonder if you would be okay or you think the request serves little purpose.
Everything else looks good and i am even okay with a conditional RB if you
agree with that request.
..alan
On Wed, 2025-01-15 at 16:19 -0800, Daniele Ceraolo Spurio wrote:
> The render supports PXP usage via rendercopy. We can use this to test
> that a user is able to correctly encrypt their data. In particular, we
> cover these 2 scenarios:
>
> 1) copy from clear to encrypted - we expect the dest buffer to not match
> the src one due to the encryption.
>
> 2) copy from encrypted to encrypted = we expect the 2 BOs to match since
> they hold the same data encrypted with the same key.
>
> Note that the clear to clear copy is already covered by the
> xe_render_copy test.
alan: nit: perhaps add an additional note that states something like
"~architecturally and naturally, there is no ability to go from
encrypted to clear".
>
> Since the render_copy uses the intel_batchbuffer helpers, those helpers
> have been updated to support vm bind of protected objects.
>
> v2: move igt_require calls inside the subtest
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> ---
> lib/intel_batchbuffer.c | 17 ++-
> tests/intel/xe_pxp.c | 275 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 288 insertions(+), 4 deletions(-)
>
> diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
> index 72bbbf8c6..489e78782 100644
> --- a/lib/intel_batchbuffer.c
> +++ b/lib/intel_batchbuffer.c
> @@ -1333,7 +1333,9 @@ void intel_bb_destroy(struct intel_bb *ibb)
> }
>
> #define XE_OBJ_SIZE(rsvd1) ((rsvd1) & ~(SZ_4K-1))
> -#define XE_OBJ_PAT_IDX(rsvd1) ((rsvd1) & (SZ_4K-1))
> +#define XE_OBJ_PAT_IDX(rsvd1) ((rsvd1) & (0xFF))
> +#define XE_OBJ_PXP_BIT (0x100)
> +#define XE_OBJ_PXP(rsvd1) ((rsvd1) & (XE_OBJ_PXP_BIT))
alan: future follow up: i feel there needs to be some documentation
in igt's version of "struct drm_i915_gem_exec_object2" that describes
how IGT tests are repurposing the objects->rsvd1 variable.
Or at least some documentation here around these macros.
I also see this is the 3rd time we are overloading rsvd1 for values that
igt-user-space is using for its attribute tracking. I honestly feel like
we need to have a user space specific wrapper for objects and batches
and VMs just how it's expected of other user space drivers out there.
However, in any case, this would be a separate series, not in-scope here.
>
> static struct drm_xe_vm_bind_op *xe_alloc_bind_ops(struct intel_bb *ibb,
> uint32_t op, uint32_t flags,
> @@ -1355,6 +1357,9 @@ static struct drm_xe_vm_bind_op *xe_alloc_bind_ops(struct intel_bb *ibb,
>
> ops->op = op;
> ops->flags = flags;
> +
> + if (XE_OBJ_PXP(objects[i]->rsvd1))
> + ops->flags |= DRM_XE_VM_BIND_FLAG_CHECK_PXP;
> ops->obj_offset = 0;
> ops->addr = objects[i]->offset;
> ops->range = XE_OBJ_SIZE(objects[i]->rsvd1);
> @@ -1745,7 +1750,7 @@ static void __remove_from_objects(struct intel_bb *ibb,
> static struct drm_i915_gem_exec_object2 *
> __intel_bb_add_object(struct intel_bb *ibb, uint32_t handle, uint64_t size,
> uint64_t offset, uint64_t alignment, uint8_t pat_index,
> - bool write)
> + bool protected, bool write)
> {
> struct drm_i915_gem_exec_object2 *object;
>
> @@ -1822,6 +1827,7 @@ __intel_bb_add_object(struct intel_bb *ibb, uint32_t handle, uint64_t size,
> object->alignment = alignment;
> object->rsvd1 = size;
> igt_assert(!XE_OBJ_PAT_IDX(object->rsvd1));
> + igt_assert(!XE_OBJ_PXP(object->rsvd1));
>
> if (pat_index == DEFAULT_PAT_INDEX)
> pat_index = intel_get_pat_idx_wb(ibb->fd);
> @@ -1833,6 +1839,9 @@ __intel_bb_add_object(struct intel_bb *ibb, uint32_t handle, uint64_t size,
> * information on xe...
> */
> object->rsvd1 |= pat_index;
alan: im still a bit concerned about the undocumened magic behind how we are
overloading this i915 UAPI variable for xe user-space breadcrumbs tracking.
Thats said, i wonder if we could at least put a build error if pat_index is
not 8-bits wide. That way, in case in future a decision to consumer more bits
for PAT would trigger a build error with comments that explain?
> +
> + if (protected)
> + object->rsvd1 |= XE_OBJ_PXP_BIT;
> }
>
> return object;
> @@ -1845,7 +1854,7 @@ intel_bb_add_object(struct intel_bb *ibb, uint32_t handle, uint64_t size,
> struct drm_i915_gem_exec_object2 *obj = NULL;
>
> obj = __intel_bb_add_object(ibb, handle, size, offset,
> - alignment, DEFAULT_PAT_INDEX, write);
> + alignment, DEFAULT_PAT_INDEX, false, write);
> igt_assert(obj);
>
> return obj;
> @@ -1909,7 +1918,7 @@ __intel_bb_add_intel_buf(struct intel_bb *ibb, struct intel_buf *buf,
>
> obj = __intel_bb_add_object(ibb, buf->handle, intel_buf_bo_size(buf),
> buf->addr.offset, alignment, buf->pat_index,
> - write);
> + buf->is_protected, write);
> igt_assert(obj);
> buf->addr.offset = obj->offset;
>
> diff --git a/tests/intel/xe_pxp.c b/tests/intel/xe_pxp.c
> index 234fa8782..57b82a3d6 100644
> --- a/tests/intel/xe_pxp.c
> +++ b/tests/intel/xe_pxp.c
> @@ -4,6 +4,10 @@
>
alan:snip
> +
> +static void __check_bo_color(int fd, uint32_t bo, uint32_t size, uint32_t color, bool readible)
> +{
> + uint64_t comp;
> + uint64_t *ptr;
> + int i, num_matches = 0;
> +
> + comp = color;
> + comp = comp | (comp << 32);
> +
> + ptr = xe_bo_mmap_ext(fd, bo, size, PROT_READ);
> +
> + igt_assert_eq(size % sizeof(uint64_t), 0);
> +
> + for (i = 0; i < (size / sizeof(uint64_t)); i++)
> + if (ptr[i] == comp)
> + ++num_matches;
> +
> + if (readible)
alan: nit - i think this variable could be a touch more self-explanatory if
it were called "should_match" (or something like that) - though i hadn't checked
if this variable ends up getting overloaded for other uses in future patches,
.. but it's just a nit anyway.
> + igt_assert_eq(num_matches, (size / sizeof(uint64_t)));
> + else
> + igt_assert_eq(num_matches, 0);
> +}
> +
> +static void check_bo_color(int fd, uint32_t bo, uint32_t size, uint8_t color, bool readible)
> +{
> + uint32_t comp;
> +
> + /*
> + * We memset the buffer using a u8 color value. However, this is too
> + * small to ensure the encrypted data does not accidentally match it,
> + * so we scale it up to a bigger size.
> + */
> + comp = color;
> + comp = comp | (comp << 8) | (comp << 16) | (comp << 24);
> +
> + return __check_bo_color(fd, bo, size, comp, readible);
> +}
> +
>
alan:snip
> igt_main
> {
> int xe_fd = -1;
> bool pxp_supported = true;
> + uint32_t devid = 0;
>
> igt_fixture {
> xe_fd = drm_open_driver(DRIVER_XE);
> + devid = intel_get_drm_devid(xe_fd);
> igt_require(xe_has_engine_class(xe_fd, DRM_XE_ENGINE_CLASS_RENDER));
> pxp_supported = is_pxp_hw_supported(xe_fd);
> }
> @@ -195,6 +458,18 @@ igt_main
> test_pxp_queue_creation(xe_fd, pxp_supported);
> }
>
> + igt_subtest_group {
> + igt_describe("Verify protected render operations:");
> + igt_subtest("regular-src-to-pxp-dest-rendercopy") {
> + require_pxp_render(pxp_supported, devid);
> + test_render_regular_src_to_pxp_dest(xe_fd);
> + }
> + igt_subtest("pxp-src-to-pxp-dest-rendercopy") {
> + require_pxp_render(pxp_supported, devid);
> + test_render_pxp_protsrc_to_protdest(xe_fd);
> + }
> + }
> +
> igt_fixture {
> close(xe_fd);
> }
More information about the igt-dev
mailing list