[PATCH i-g-t v5 09/17] tests/xe_exec_sip: Introduce invalid instruction tests
Manszewski, Christoph
christoph.manszewski at intel.com
Fri Aug 30 12:30:16 UTC 2024
Hi Zbigniew,
On 30.08.2024 13:54, Zbigniew Kempczyński wrote:
> On Thu, Aug 29, 2024 at 04:45:39PM +0200, Christoph Manszewski wrote:
>> From: Andrzej Hajda <andrzej.hajda at intel.com>
>>
>> Xe2 and earlier gens are able to handle very limited set of invalid
>> instructions - only illegal and undefined opcodes, other errors in
>> instruction can cause undefined behavior.
>> Illegal/undefined opcode results in:
>> - setting illegal opcode status bit - cr0.1[28],
>> - calling SIP if illegal opcode bit is enabled - cr0.1[12].
>> cr0.1[12] can be enabled directly from the thread or by thread dispatcher
>> from Interface Descriptor Data provided to COMPUTE_WALKER instruction.
>>
>> Implemented cases:
>> - check if SIP is not called when exception is not enabled,
>> - check if SIP is called when exception is enabled from EU thread,
>> - check if SIP is called when exception is enabled from COMPUTE_WALKER
>>
>> Signed-off-by: Andrzej Hajda <andrzej.hajda at intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
>> Cc: Dominik Grzegorzek <dominik.grzegorzek at intel.com>
>> ---
>> tests/intel/xe_exec_sip.c | 118 ++++++++++++++++++++++++++++++++++----
>> 1 file changed, 108 insertions(+), 10 deletions(-)
>>
>> diff --git a/tests/intel/xe_exec_sip.c b/tests/intel/xe_exec_sip.c
>> index 3b46f5618..5384979a6 100644
>> --- a/tests/intel/xe_exec_sip.c
>> +++ b/tests/intel/xe_exec_sip.c
>> @@ -30,12 +30,25 @@
>> #define COLOR_C4 0xc4
>>
>> #define SHADER_CANARY 0x01010101
>> +#define SIP_CANARY 0x02020202
>>
>> enum shader_type {
>> SHADER_HANG,
>> + SHADER_INV_INSTR_DISABLED,
>> + SHADER_INV_INSTR_THREAD_ENABLED,
>> + SHADER_INV_INSTR_WALKER_ENABLED,
>> SHADER_WRITE,
>> };
>>
>> +enum sip_type {
>> + SIP_INV_INSTR,
>> + SIP_NULL,
>> +};
>> +
>> +/* Control Register cr0.1 bits for exception handling */
>> +#define ILLEGAL_OPCODE_ENABLE BIT(12)
>> +#define ILLEGAL_OPCODE_STATUS BIT(28)
>> +
>> static struct intel_buf *
>> create_fill_buf(int fd, int width, int height, uint8_t color)
>> {
>> @@ -58,8 +71,12 @@ create_fill_buf(int fd, int width, int height, uint8_t color)
>> static struct gpgpu_shader *get_shader(int fd, const int shadertype)
>> {
>> static struct gpgpu_shader *shader;
>> + uint32_t bad;
>>
>> shader = gpgpu_shader_create(fd);
>> + if (shadertype == SHADER_INV_INSTR_WALKER_ENABLED)
>> + shader->illegal_opcode_exception_enable = true;
>> +
>> gpgpu_shader__write_dword(shader, SHADER_CANARY, 0);
>>
>> switch (shadertype) {
>> @@ -70,19 +87,59 @@ static struct gpgpu_shader *get_shader(int fd, const int shadertype)
>> break;
>> case SHADER_WRITE:
>> break;
>> + case SHADER_INV_INSTR_THREAD_ENABLED:
>> + gpgpu_shader__set_exception(shader, ILLEGAL_OPCODE_ENABLE);
>> + __attribute__ ((fallthrough));
>
> Just be curious - on which gcc you're observing this warning without
> fallthrough attribute? clang explicitely requires -Wimplicit-fallthrough
> to enable this warning.
IIRC it was a checkpatch warning/check
>
>> + case SHADER_INV_INSTR_DISABLED:
>> + case SHADER_INV_INSTR_WALKER_ENABLED:
>> + bad = (shadertype == SHADER_INV_INSTR_DISABLED) ? ILLEGAL_OPCODE_ENABLE : 0;
>> + gpgpu_shader__write_on_exception(shader, 1, 0, ILLEGAL_OPCODE_ENABLE, bad);
>> + gpgpu_shader__nop(shader);
>> + gpgpu_shader__nop(shader);
>> + /* modify second nop, set only opcode bits[6:0] */
>> + shader->instr[shader->size / 4 - 1][0] = 0x7f;
>
> This pattern 'shader->size / 4 - 1' deserves own macro or inline
> like last_instr(shader) or similar.
Ok
>
>> + /* SIP should clear exception bit */
>> + bad = ILLEGAL_OPCODE_STATUS;
>> + gpgpu_shader__write_on_exception(shader, 2, 0, ILLEGAL_OPCODE_STATUS, bad);
>> + break;
>> }
>>
>> gpgpu_shader__eot(shader);
>
> I would add one blank line before return on all functions in which
> return is glued with the code.
This is not code introduced in this patch nor even in this series.
>
>> return shader;
>> }
>>
>> -static uint32_t gpgpu_shader(int fd, struct intel_bb *ibb, const int shadertype,
>> +static struct gpgpu_shader *get_sip(int fd, const int siptype, unsigned int y_offset)
>> +{
>> + static struct gpgpu_shader *sip;
>> +
>> + if (siptype == SIP_NULL)
>> + return NULL;
>> +
>> + sip = gpgpu_shader_create(fd);
>> + gpgpu_shader__write_dword(sip, SIP_CANARY, y_offset);
>> +
>> + switch (siptype) {
>> + case SIP_INV_INSTR:
>> + gpgpu_shader__write_on_exception(sip, 1, y_offset, ILLEGAL_OPCODE_STATUS, 0);
>> + break;
>> + }
>> +
>> + gpgpu_shader__end_system_routine(sip, false);
>
> Ditto.
Sure.
>
>> + return sip;
>> +}
>> +
>> +static uint32_t gpgpu_shader(int fd, struct intel_bb *ibb, const int shadertype, const int siptype,
>> unsigned int threads, unsigned int width, unsigned int height)
>> {
>> struct intel_buf *buf = create_fill_buf(fd, width, height, COLOR_C4);
>> + struct gpgpu_shader *sip = get_sip(fd, siptype, height / 2);
>> struct gpgpu_shader *shader = get_shader(fd, shadertype);
>>
>> - gpgpu_shader_exec(ibb, buf, 1, threads, shader, NULL, 0, 0);
>> + gpgpu_shader_exec(ibb, buf, 1, threads, shader, sip, 0, 0);
>> +
>> + if (sip)
>> + gpgpu_shader_destroy(sip);
>> +
>> gpgpu_shader_destroy(shader);
>
> Ditto. >
>> return buf->handle;
>> }
>> @@ -98,10 +155,10 @@ static void check_fill_buf(uint8_t *ptr, const int width, const int x,
>> }
>>
>> static void check_buf(int fd, uint32_t handle, int width, int height,
>> - uint8_t poison_c)
>> + int shadertype, int siptype, uint8_t poison_c)
>> {
>> unsigned int sz = ALIGN(width * height, 4096);
>> - int thread_count = 0;
>> + int thread_count = 0, sip_count = 0;
>> uint32_t *ptr;
>> int i, j;
>>
>> @@ -119,7 +176,27 @@ static void check_buf(int fd, uint32_t handle, int width, int height,
>> i = 0;
>> }
>>
>> + for (i = 0, j = height / 2; j < height; ++j) {
>> + if (ptr[j * width / 4] == SIP_CANARY) {
>> + ++sip_count;
>> + i = 4;
>> + }
>> +
>> + for (; i < width; i++)
>> + check_fill_buf((uint8_t *)ptr, width, i, j, poison_c);
>> +
>> + i = 0;
>> + }
>> +
>> igt_assert(thread_count);
>> + if (shadertype == SHADER_INV_INSTR_DISABLED)
>> + igt_assert(!sip_count);
>> + else if (siptype == SIP_INV_INSTR && shadertype != SHADER_INV_INSTR_DISABLED)
>> + igt_assert_f(thread_count == sip_count,
>> + "Thread and SIP count mismatch, %d != %d\n",
>> + thread_count, sip_count);
>> + else
>> + igt_assert(sip_count == 0);
>>
>> munmap(ptr, sz);
>> }
>> @@ -143,8 +220,20 @@ xe_sysfs_get_job_timeout_ms(int fd, struct drm_xe_engine_class_instance *eci)
>> *
>> * SUBTEST: sanity-after-timeout
>> * Description: check basic shader execution after job timeout
>> + *
>> + * SUBTEST: invalidinstr-disabled
>> + * Description: Verify that we don't enter SIP after running into an invalid
>> + * instruction when exception is not enabled.
>> + *
>> + * SUBTEST: invalidinstr-thread-enabled
>> + * Description: Verify that we enter SIP after running into an invalid instruction
>> + * when exception is enabled from thread.
>> + *
>> + * SUBTEST: invalidinstr-walker-enabled
>> + * Description: Verify that we enter SIP after running into an invalid instruction
>> + * when exception is enabled from COMPUTE_WALKER.
>> */
>> -static void test_sip(int shader, struct drm_xe_engine_class_instance *eci, uint32_t flags)
>> +static void test_sip(int shader, int sip, struct drm_xe_engine_class_instance *eci, uint32_t flags)
>> {
>> unsigned int threads = 512;
>> unsigned int height = max_t(threads, HEIGHT, threads * 2);
>> @@ -171,12 +260,12 @@ static void test_sip(int shader, struct drm_xe_engine_class_instance *eci, uint3
>
> Likely I missed this in previous patch review, shader should be
> enum, not int.
True, nice catch - thanks.
Christoph
>
> --
> Zbigniew
>
>> ibb = intel_bb_create_with_context(fd, exec_queue_id, vm_id, NULL, 4096);
>>
>> igt_nsec_elapsed(&ts);
>> - handle = gpgpu_shader(fd, ibb, shader, threads, width, height);
>> + handle = gpgpu_shader(fd, ibb, shader, sip, threads, width, height);
>>
>> intel_bb_sync(ibb);
>> igt_assert_lt_u64(igt_nsec_elapsed(&ts), timeout);
>>
>> - check_buf(fd, handle, width, height, COLOR_C4);
>> + check_buf(fd, handle, width, height, shader, sip, COLOR_C4);
>>
>> gem_close(fd, handle);
>> intel_bb_destroy(ibb);
>> @@ -204,17 +293,26 @@ igt_main
>> fd = drm_open_driver(DRIVER_XE);
>>
>> test_render_and_compute("sanity", fd, eci)
>> - test_sip(SHADER_WRITE, eci, 0);
>> + test_sip(SHADER_WRITE, SIP_NULL, eci, 0);
>>
>> test_render_and_compute("sanity-after-timeout", fd, eci) {
>> - test_sip(SHADER_HANG, eci, 0);
>> + test_sip(SHADER_HANG, SIP_NULL, eci, 0);
>>
>> xe_for_each_engine(fd, eci)
>> if (eci->engine_class == DRM_XE_ENGINE_CLASS_RENDER ||
>> eci->engine_class == DRM_XE_ENGINE_CLASS_COMPUTE)
>> - test_sip(SHADER_WRITE, eci, 0);
>> + test_sip(SHADER_WRITE, SIP_NULL, eci, 0);
>> }
>>
>> + test_render_and_compute("invalidinstr-disabled", fd, eci)
>> + test_sip(SHADER_INV_INSTR_DISABLED, SIP_INV_INSTR, eci, 0);
>> +
>> + test_render_and_compute("invalidinstr-thread-enabled", fd, eci)
>> + test_sip(SHADER_INV_INSTR_THREAD_ENABLED, SIP_INV_INSTR, eci, 0);
>> +
>> + test_render_and_compute("invalidinstr-walker-enabled", fd, eci)
>> + test_sip(SHADER_INV_INSTR_WALKER_ENABLED, SIP_INV_INSTR, eci, 0);
>> +
>> igt_fixture
>> drm_close_driver(fd);
>> }
>> --
>> 2.34.1
>>
More information about the igt-dev
mailing list