[igt-dev] [PATCH i-g-t] tests/i915: Add simple test for HuC

Fri May 17 19:21:37 UTC 2019

On Fri, 17 May 2019 20:58:33 +0200, Chris Wilson  
<chris at chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2019-05-17 19:47:42)
>> On Fri, 17 May 2019 19:48:55 +0200, Chris Wilson
>> <chris at chris-wilson.co.uk> wrote:
>>
>> > Quoting Michal Wajdeczko (2019-05-17 18:37:53)
>> >> Add simple test to check that HuC firmware is available.
>> >> Use existing I915_GETPARAM and debugfs entry.
>> >>
>> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> >> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> >> Cc: Martin Peres <martin.peres at linux.intel.com>
>> >> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> >> Cc: Tony Ye <tony.ye at intel.com>
>> >> ---
>> >>  tests/Makefile.sources |  3 ++
>> >>  tests/i915/i915_huc.c  | 74  
>> ++++++++++++++++++++++++++++++++++++++++++
>> >>  tests/meson.build      |  1 +
>> >>  3 files changed, 78 insertions(+)
>> >>  create mode 100644 tests/i915/i915_huc.c
>> >>
>> >> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>> >> index 7f921f6c..dfa3fcd3 100644
>> >> --- a/tests/Makefile.sources
>> >> +++ b/tests/Makefile.sources
>> >> @@ -475,6 +475,9 @@ i915_getparams_basic_SOURCES =
>> >> i915/i915_getparams_basic.c
>> >>  TESTS_progs += i915_hangman
>> >>  i915_hangman_SOURCES = i915/i915_hangman.c
>> >>
>> >> +TESTS_progs += i915_huc
>> >> +i915_huc_SOURCES = i915/i915_huc.c
>> >> +
>> >>  TESTS_progs += i915_module_load
>> >>  i915_module_load_SOURCES = i915/i915_module_load.c
>> >>
>> >> diff --git a/tests/i915/i915_huc.c b/tests/i915/i915_huc.c
>> >> new file mode 100644
>> >> index 00000000..10cd5d6f
>> >> --- /dev/null
>> >> +++ b/tests/i915/i915_huc.c
>> >> @@ -0,0 +1,74 @@
>> >> +/* SPDX-License-Identifier: MIT */
>> >> +/*
>> >> + * Copyright © 2019 Intel Corporation
>> >> + */
>> >> +
>> >> +#include "igt.h"
>> >> +#include <fcntl.h>
>> >> +#include <i915_drm.h>
>> >> +#include <sys/ioctl.h>
>> >> +
>> >> +IGT_TEST_DESCRIPTION("Check that HuC firmware is available.");
>> >> +
>> >> +static bool has_guc(int fd)
>> >> +{
>> >> +       uint32_t devid = intel_get_drm_devid(fd);
>> >> +
>> >> +       return IS_SKYLAKE(devid) || IS_BROXTON(devid) ||
>> >> +              IS_KABYLAKE(devid) || IS_COFFEELAKE(devid) ||
>> >> +              IS_GEMINILAKE(devid) || IS_ICELAKE(devid);
>> >> +}
>> >> +
>> >> +static bool has_huc(int fd)
>> >> +{
>> >> +       return has_guc(fd);
>> >> +}
>> >> +
>> >> +static void check_huc_status(int device)
>> >> +{
>> >> +       int loaded = 0;
>> >> +       drm_i915_getparam_t gp = {
>> >> +               .param = I915_PARAM_HUC_STATUS,
>> >> +               .value = &loaded,
>> >> +       };
>> >> +
>> >> +       igt_require(ioctl(device, DRM_IOCTL_I915_GETPARAM, &gp) ==  
>> 0);
>> >> +       igt_assert_eq(loaded, 1);
>> >> +}
>> >> +
>> >> +#define HUC_STATUS_DEBUGFS_FILE "i915_huc_load_status"
>> >> +
>> >> +static void check_huc_info(int device)
>> >> +{
>> >> +       int fd = igt_debugfs_open(device, HUC_STATUS_DEBUGFS_FILE,
>> >> O_RDONLY);
>> >> +       igt_assert_f(fd >= 0, "'%s' debugfs entry not found\n",
>> >> +                    HUC_STATUS_DEBUGFS_FILE);
>> >
>> > Debugfs may not exist...
>> >
>> >> +       close(fd);
>> >> +
>> >> +       igt_assert(igt_debugfs_search(device,  
>> HUC_STATUS_DEBUGFS_FILE,
>> >> +                  "status: fetch SUCCESS"));
>> >> +       igt_assert(igt_debugfs_search(device,  
>> HUC_STATUS_DEBUGFS_FILE,
>> >> +                  "load SUCCESS"));
>> >
>> > and doesn't constitute ABI, so asserts are little ott.
>>
>> btw, IGT code seems to be little inconsistent on that
>
> igt relies on debugfs for sure, (often too much, and we use debugfs as a
> crutch to avoid a real API with stable ABI, imo)
>
> But in terms of an igt determining the expected behaviour of the kernel
> for userspace and defining the ABI, this subtest paints the picture to
> me that HUC_STATUS_DEBUGFS_FILE is defacto ABI. That I want to avoid.
>
> It will certainly be useful to dump the debugfs file at some point, and
> just dumping it into IGT_INFO may be nice as part of the fixture.
>
>> (and based on below discussion, I inclined to drop this subtest)
>>
>> >
>> >> +}
>> >> +
>> >> +igt_main
>> >> +{
>> >> +       int fd;
>> >> +
>> >> +       igt_fixture {
>> >> +               fd = drm_open_driver(DRIVER_INTEL);
>> >> +               igt_require_intel(fd);
>> >> +               igt_skip_on_f(!has_huc(fd),
>> >> +                             "HuC is not available on this
>> >> platform\n");
>> >
>> > Hmm, HuC is optional as it may both be disabled by [default] parameter
>>
>> afaik, this is about to change soon ;)
>
> Apparently they have some blocking bugs to resolve first :-p
>
>> > and lack of fw [a plague on us who use builtin modules]. I think this
>> > should be igt_require(huc_status(fd)) -- skips will be highlighted if  
>> we
>> > regress.
>>
>> I'm not igt expert, so to confirm, maybe subtest should be empty and
>> we should put all require stuff into fixture ?
>>
>> igt_fixture {
>>         igt_require_intel(fd);
>>         igt_require(has_huc(fd));
>>         igt_require(huc_status(fd));
>> }
>>
>> igt_subtest("basic")
>>         igt_success();
>
> Yup. That covers the simplest test we can do, SKIP <->
>
>> > And for the main test we would much rather have something behaviour  
>> that
>> > only the HuC can provide. Is there not any challenge-response we can
>> > send to the gpu for a simple ping? I have no idea what function the  
>> HuC
>> > serves...
>>
>> The goal if this patch was to provide 'simple' test that covers what we
>> expose today (at least GETPARAM is ABI, right?)
>
> Yup. That appears to be used.
>
>> > There's https://bugs.freedesktop.org/show_bug.cgi?id=110617 is we need
>> > an example where loading the HuC breaks previously working video
>> > playback.
>>
>> For above bug, maybe HuC fw binary was corrupted? anyone tried
>> enable_guc=3?
>
> I thought the fw were signed? Wouldn't that prevent execution of a
> corrupted binary? (And if they are not signed, what's stopping us from
> uploading our own...)

I guess we can upload everything, but likely will not be executed unless
signed and verified. And log says that:

[    2.386316] [drm] GuC: Loaded firmware i915/kbl_guc_ver9_39.bin  
(version 9.39)
[    2.438318] [drm:intel_huc_auth] *ERROR* HuC: Firmware not verified  
0x6000
[    2.445235] [drm:intel_huc_auth] *ERROR* HuC: Authentication failed -110

>
> I was hoping maybe it would be an insight as to something we could use
> to determine the HuC exists.

Both authentication check on load and our GETPARAM ABI use exactly the same
register to check if HuC is verified. If we fail to load HuC, today we will
end with -EIO and GuC/HuC reset, so no hope that igt test will provide