[PATCH 0/2] drm/i915/gt: Fix recent kCFI violations

[PATCH 0/2] drm/i915/gt: Fix recent kCFI violations

[Nathan Chancellor]

Hi all,

This series fixes a few clang kernel Control Flow Integrity (kCFI)
violations that appear after commit 9275277d5324 ("drm/i915: use
pat_index instead of cache_level"). They were found between run time
testing on real hardware and compile time testing with
-Wincompatible-function-pointer-types-strict (which is not yet enabled
for the kernel but I build with it locally to catch new instances).

If there are any problems or questions, please let me know.

---
Nathan Chancellor (2):
      drm/i915/gt: Fix second parameter type of pre-gen8 pte_encode callbacks
      drm/i915/gt: Fix parameter in gmch_ggtt_insert_{entries,page}()

 drivers/gpu/drm/i915/gt/intel_ggtt.c      | 26 +++++++++++++-------------
 drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c |  8 ++++----
 2 files changed, 17 insertions(+), 17 deletions(-)
---
base-commit: 08264f85c5c05ecc38d409c84d48cfb00ccd3bc4
change-id: 20230530-i915-gt-cache_level-wincompatible-function-pointer-types-strict-32a5c65249a5

Best regards,
-- 
Nathan Chancellor <nathan@kernel.org>

[PATCH 1/2] drm/i915/gt: Fix second parameter type of pre-gen8 pte_encode callbacks

[Nathan Chancellor]

When booting a kernel compiled with CONFIG_CFI_CLANG (kCFI), there is a
CFI failure in ggtt_probe_common() when trying to call hsw_pte_encode()
via an indirect call:

  [    5.030027] CFI failure at ggtt_probe_common+0xd1/0x130 [i915] (target: hsw_pte_encode+0x0/0x30 [i915]; expected type: 0xf5c1d0fc)

With kCFI, indirect calls are validated against their expected type
versus actual type and failures occur when the two types do not match.

clang's -Wincompatible-function-pointer-types-strict can catch this at
compile time but it is not enabled for the kernel yet:

  drivers/gpu/drm/i915/gt/intel_ggtt.c:1155:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t,
  enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
                  ggtt->vm.pte_encode = iris_pte_encode;
                                      ^ ~~~~~~~~~~~~~~~
  drivers/gpu/drm/i915/gt/intel_ggtt.c:1157:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t,
  enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
                  ggtt->vm.pte_encode = hsw_pte_encode;
                                      ^ ~~~~~~~~~~~~~~
  drivers/gpu/drm/i915/gt/intel_ggtt.c:1159:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t,
  enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
                  ggtt->vm.pte_encode = byt_pte_encode;
                                      ^ ~~~~~~~~~~~~~~
  drivers/gpu/drm/i915/gt/intel_ggtt.c:1161:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t,
  enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
                  ggtt->vm.pte_encode = ivb_pte_encode;
                                      ^ ~~~~~~~~~~~~~~
  drivers/gpu/drm/i915/gt/intel_ggtt.c:1163:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t,
  enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
                  ggtt->vm.pte_encode = snb_pte_encode;
                                      ^ ~~~~~~~~~~~~~~
  5 errors generated.

In this case, the pre-gen8 pte_encode functions have a second parameter
type of 'enum i915_cache_level' whereas the function pointer prototype
in 'struct i915_address_space' expects a second parameter type of
'unsigned int'.

Update the second parameter of the callbacks and the comment above them
noting that these statements are still valid, which matches other
functions and files, to clear up the kCFI failures at run time.

Fixes: 9275277d5324 ("drm/i915: use pat_index instead of cache_level")
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 drivers/gpu/drm/i915/gt/intel_ggtt.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index 2a7942fac798..122197737ef2 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -1015,16 +1015,16 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
 
 /*
  * For pre-gen8 platforms pat_index is the same as enum i915_cache_level,
- * so these PTE encode functions are left with using cache_level.
+ * so the switch-case statements in these PTE encode functions are still valid.
  * See translation table LEGACY_CACHELEVEL.
  */
 static u64 snb_pte_encode(dma_addr_t addr,
-			  enum i915_cache_level level,
+			  unsigned int pat_index,
 			  u32 flags)
 {
 	gen6_pte_t pte = GEN6_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID;
 
-	switch (level) {
+	switch (pat_index) {
 	case I915_CACHE_L3_LLC:
 	case I915_CACHE_LLC:
 		pte |= GEN6_PTE_CACHE_LLC;
@@ -1033,19 +1033,19 @@ static u64 snb_pte_encode(dma_addr_t addr,
 		pte |= GEN6_PTE_UNCACHED;
 		break;
 	default:
-		MISSING_CASE(level);
+		MISSING_CASE(pat_index);
 	}
 
 	return pte;
 }
 
 static u64 ivb_pte_encode(dma_addr_t addr,
-			  enum i915_cache_level level,
+			  unsigned int pat_index,
 			  u32 flags)
 {
 	gen6_pte_t pte = GEN6_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID;
 
-	switch (level) {
+	switch (pat_index) {
 	case I915_CACHE_L3_LLC:
 		pte |= GEN7_PTE_CACHE_L3_LLC;
 		break;
@@ -1056,14 +1056,14 @@ static u64 ivb_pte_encode(dma_addr_t addr,
 		pte |= GEN6_PTE_UNCACHED;
 		break;
 	default:
-		MISSING_CASE(level);
+		MISSING_CASE(pat_index);
 	}
 
 	return pte;
 }
 
 static u64 byt_pte_encode(dma_addr_t addr,
-			  enum i915_cache_level level,
+			  unsigned int pat_index,
 			  u32 flags)
 {
 	gen6_pte_t pte = GEN6_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID;
@@ -1071,31 +1071,31 @@ static u64 byt_pte_encode(dma_addr_t addr,
 	if (!(flags & PTE_READ_ONLY))
 		pte |= BYT_PTE_WRITEABLE;
 
-	if (level != I915_CACHE_NONE)
+	if (pat_index != I915_CACHE_NONE)
 		pte |= BYT_PTE_SNOOPED_BY_CPU_CACHES;
 
 	return pte;
 }
 
 static u64 hsw_pte_encode(dma_addr_t addr,
-			  enum i915_cache_level level,
+			  unsigned int pat_index,
 			  u32 flags)
 {
 	gen6_pte_t pte = HSW_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID;
 
-	if (level != I915_CACHE_NONE)
+	if (pat_index != I915_CACHE_NONE)
 		pte |= HSW_WB_LLC_AGE3;
 
 	return pte;
 }
 
 static u64 iris_pte_encode(dma_addr_t addr,
-			   enum i915_cache_level level,
+			   unsigned int pat_index,
 			   u32 flags)
 {
 	gen6_pte_t pte = HSW_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID;
 
-	switch (level) {
+	switch (pat_index) {
 	case I915_CACHE_NONE:
 		break;
 	case I915_CACHE_WT:

-- 
2.40.1

Re: [PATCH 1/2] drm/i915/gt: Fix second parameter type of pre-gen8 pte_encode callbacks

[Andi Shyti]

Hi Nathan,

On Tue, May 30, 2023 at 11:24:38AM -0700, Nathan Chancellor wrote:
> When booting a kernel compiled with CONFIG_CFI_CLANG (kCFI), there is a
> CFI failure in ggtt_probe_common() when trying to call hsw_pte_encode()
> via an indirect call:
> 
>   [    5.030027] CFI failure at ggtt_probe_common+0xd1/0x130 [i915] (target: hsw_pte_encode+0x0/0x30 [i915]; expected type: 0xf5c1d0fc)
> 
> With kCFI, indirect calls are validated against their expected type
> versus actual type and failures occur when the two types do not match.
> 
> clang's -Wincompatible-function-pointer-types-strict can catch this at
> compile time but it is not enabled for the kernel yet:
> 
>   drivers/gpu/drm/i915/gt/intel_ggtt.c:1155:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t,
>   enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
>                   ggtt->vm.pte_encode = iris_pte_encode;
>                                       ^ ~~~~~~~~~~~~~~~
>   drivers/gpu/drm/i915/gt/intel_ggtt.c:1157:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t,
>   enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
>                   ggtt->vm.pte_encode = hsw_pte_encode;
>                                       ^ ~~~~~~~~~~~~~~
>   drivers/gpu/drm/i915/gt/intel_ggtt.c:1159:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t,
>   enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
>                   ggtt->vm.pte_encode = byt_pte_encode;
>                                       ^ ~~~~~~~~~~~~~~
>   drivers/gpu/drm/i915/gt/intel_ggtt.c:1161:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t,
>   enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
>                   ggtt->vm.pte_encode = ivb_pte_encode;
>                                       ^ ~~~~~~~~~~~~~~
>   drivers/gpu/drm/i915/gt/intel_ggtt.c:1163:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t,
>   enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
>                   ggtt->vm.pte_encode = snb_pte_encode;
>                                       ^ ~~~~~~~~~~~~~~
>   5 errors generated.
> 
> In this case, the pre-gen8 pte_encode functions have a second parameter
> type of 'enum i915_cache_level' whereas the function pointer prototype
> in 'struct i915_address_space' expects a second parameter type of
> 'unsigned int'.
> 
> Update the second parameter of the callbacks and the comment above them
> noting that these statements are still valid, which matches other
> functions and files, to clear up the kCFI failures at run time.
> 
> Fixes: 9275277d5324 ("drm/i915: use pat_index instead of cache_level")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

That's correct!

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> 

Thanks,
Andi

RE: [PATCH 1/2] drm/i915/gt: Fix second parameter type of pre-gen8 pte_encode callbacks

[Yang, Fei]

> Subject: [PATCH 1/2] drm/i915/gt: Fix second parameter type of pre-gen8 pte_encode callbacks
>
> When booting a kernel compiled with CONFIG_CFI_CLANG (kCFI), there is a CFI failure in ggtt_probe_common() when trying to call hsw_pte_encode() via an indirect call:
>
>   [    5.030027] CFI failure at ggtt_probe_common+0xd1/0x130 [i915] (target: hsw_pte_encode+0x0/0x30 [i915]; expected type: 0xf5c1d0fc)
>
> With kCFI, indirect calls are validated against their expected type versus actual type and failures occur when the two types do not match.
>
> clang's -Wincompatible-function-pointer-types-strict can catch this at compile time but it is not enabled for the kernel yet:
>
>   drivers/gpu/drm/i915/gt/intel_ggtt.c:1155:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t,
>   enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
>                   ggtt->vm.pte_encode = iris_pte_encode;
>                                       ^ ~~~~~~~~~~~~~~~
>   drivers/gpu/drm/i915/gt/intel_ggtt.c:1157:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t,
>   enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
>                   ggtt->vm.pte_encode = hsw_pte_encode;
>                                       ^ ~~~~~~~~~~~~~~
>   drivers/gpu/drm/i915/gt/intel_ggtt.c:1159:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t,
>   enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
>                   ggtt->vm.pte_encode = byt_pte_encode;
>                                       ^ ~~~~~~~~~~~~~~
>   drivers/gpu/drm/i915/gt/intel_ggtt.c:1161:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t,
>   enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
>                   ggtt->vm.pte_encode = ivb_pte_encode;
>                                       ^ ~~~~~~~~~~~~~~
>   drivers/gpu/drm/i915/gt/intel_ggtt.c:1163:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t,
>   enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
>                   ggtt->vm.pte_encode = snb_pte_encode;
>                                       ^ ~~~~~~~~~~~~~~
>   5 errors generated.
>
> In this case, the pre-gen8 pte_encode functions have a second parameter type of 'enum i915_cache_level' whereas the function pointer prototype in 'struct i915_address_space' expects a second parameter type of 'unsigned int'.
>
> Update the second parameter of the callbacks and the comment above them noting that these statements are still valid, which matches other functions and files, to clear up the kCFI failures at run time.
>
> Fixes: 9275277d5324 ("drm/i915: use pat_index instead of cache_level")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

Reviewed-by: Fei Yang <fei.yang@intel.com>

> ---
>  drivers/gpu/drm/i915/gt/intel_ggtt.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index 2a7942fac798..122197737ef2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -1015,16 +1015,16 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
>
>  /*
>   * For pre-gen8 platforms pat_index is the same as enum i915_cache_level,
> - * so these PTE encode functions are left with using cache_level.
> + * so the switch-case statements in these PTE encode functions are still valid.
>   * See translation table LEGACY_CACHELEVEL.
>   */
>  static u64 snb_pte_encode(dma_addr_t addr,
> -                       enum i915_cache_level level,
> +                       unsigned int pat_index,
>                         u32 flags)
>  {
>       gen6_pte_t pte = GEN6_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID;
>
> -     switch (level) {
> +     switch (pat_index) {
>       case I915_CACHE_L3_LLC:
>       case I915_CACHE_LLC:
>               pte |= GEN6_PTE_CACHE_LLC;
> @@ -1033,19 +1033,19 @@ static u64 snb_pte_encode(dma_addr_t addr,
>               pte |= GEN6_PTE_UNCACHED;
>               break;
>       default:
> -             MISSING_CASE(level);
> +             MISSING_CASE(pat_index);
>       }
>
>       return pte;
>  }
>
>  static u64 ivb_pte_encode(dma_addr_t addr,
> -                       enum i915_cache_level level,
> +                       unsigned int pat_index,
>                         u32 flags)
>  {
>       gen6_pte_t pte = GEN6_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID;
>
> -     switch (level) {
> +     switch (pat_index) {
>       case I915_CACHE_L3_LLC:
>               pte |= GEN7_PTE_CACHE_L3_LLC;
>               break;
> @@ -1056,14 +1056,14 @@ static u64 ivb_pte_encode(dma_addr_t addr,
>               pte |= GEN6_PTE_UNCACHED;
>               break;
>       default:
> -             MISSING_CASE(level);
> +             MISSING_CASE(pat_index);
>       }
>
>       return pte;
>  }
>
>  static u64 byt_pte_encode(dma_addr_t addr,
> -                       enum i915_cache_level level,
> +                       unsigned int pat_index,
>                         u32 flags)
>  {
>       gen6_pte_t pte = GEN6_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID; @@ -1071,31 +1071,31 @@ static u64 byt_pte_encode(dma_addr_t addr,
>       if (!(flags & PTE_READ_ONLY))
>               pte |= BYT_PTE_WRITEABLE;
>
> -     if (level != I915_CACHE_NONE)
> +     if (pat_index != I915_CACHE_NONE)
>               pte |= BYT_PTE_SNOOPED_BY_CPU_CACHES;
>
>       return pte;
>  }
>
>  static u64 hsw_pte_encode(dma_addr_t addr,
> -                       enum i915_cache_level level,
> +                       unsigned int pat_index,
>                         u32 flags)
>  {
>       gen6_pte_t pte = HSW_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID;
>
> -     if (level != I915_CACHE_NONE)
> +     if (pat_index != I915_CACHE_NONE)
>               pte |= HSW_WB_LLC_AGE3;
>
>       return pte;
>  }
>
>  static u64 iris_pte_encode(dma_addr_t addr,
> -                        enum i915_cache_level level,
> +                        unsigned int pat_index,
>                          u32 flags)
>  {
>       gen6_pte_t pte = HSW_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID;
>
> -     switch (level) {
> +     switch (pat_index) {
>       case I915_CACHE_NONE:
>               break;
>       case I915_CACHE_WT:
>
> --
> 2.40.1

[PATCH 2/2] drm/i915/gt: Fix parameter in gmch_ggtt_insert_{entries,page}()

[Nathan Chancellor]

When building with clang's -Wincompatible-function-pointer-types-strict,
the following warnings occur:

  drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c:102:23: error: incompatible function pointer types assigning to 'void (*)(struct i915_address_space *, dma_addr_t, u64, unsigned int, u32)' (aka 'void (*)(struct i915_address_space *, unsigned int, unsigned long long, unsigned int, unsigned int)') from 'void (struct i915_address_space *, dma_addr_t, u64, enum i915_cache_level, u32)' (aka 'void (struct i915_address_space *, unsigned int, unsigned long long, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
          ggtt->vm.insert_page = gmch_ggtt_insert_page;
                               ^ ~~~~~~~~~~~~~~~~~~~~~
  drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c:103:26: error: incompatible function pointer types assigning to 'void (*)(struct i915_address_space *, struct i915_vma_resource *, unsigned int, u32)' (aka 'void (*)(struct i915_address_space *, struct i915_vma_resource *, unsigned int, unsigned int)') from 'void (struct i915_address_space *, struct i915_vma_resource *, enum i915_cache_level, u32)' (aka 'void (struct i915_address_space *, struct i915_vma_resource *, enum i915_cache_level, unsigned int)') [-Werror, -Wincompatible-function-pointer-types-strict]
          ggtt->vm.insert_entries = gmch_ggtt_insert_entries;
                                  ^ ~~~~~~~~~~~~~~~~~~~~~~~~
  2 errors generated.

The warning is pointing out that while 'enum i915_cache_level' and
'unsigned int' are ABI compatible, these indirect calls will fail
clang's kernel Control Flow Integrity (kCFI) checks, as the callback's
signature does not exactly match the prototype's signature.

To fix this, replace the cache_level parameter with pat_index, as was
done in other places within i915 where there is no difference between
cache_level and pat_index on certain generations.

Fixes: 9275277d5324 ("drm/i915: use pat_index instead of cache_level")
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c b/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c
index d6a74ae2527b..866c416afb73 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c
@@ -18,10 +18,10 @@
 static void gmch_ggtt_insert_page(struct i915_address_space *vm,
 				  dma_addr_t addr,
 				  u64 offset,
-				  enum i915_cache_level cache_level,
+				  unsigned int pat_index,
 				  u32 unused)
 {
-	unsigned int flags = (cache_level == I915_CACHE_NONE) ?
+	unsigned int flags = (pat_index == I915_CACHE_NONE) ?
 		AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
 
 	intel_gmch_gtt_insert_page(addr, offset >> PAGE_SHIFT, flags);
@@ -29,10 +29,10 @@ static void gmch_ggtt_insert_page(struct i915_address_space *vm,
 
 static void gmch_ggtt_insert_entries(struct i915_address_space *vm,
 				     struct i915_vma_resource *vma_res,
-				     enum i915_cache_level cache_level,
+				     unsigned int pat_index,
 				     u32 unused)
 {
-	unsigned int flags = (cache_level == I915_CACHE_NONE) ?
+	unsigned int flags = (pat_index == I915_CACHE_NONE) ?
 		AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
 
 	intel_gmch_gtt_insert_sg_entries(vma_res->bi.pages, vma_res->start >> PAGE_SHIFT,

-- 
2.40.1

Re: [PATCH 2/2] drm/i915/gt: Fix parameter in gmch_ggtt_insert_{entries,page}()

[Andi Shyti]

Hi Nathan,

On Tue, May 30, 2023 at 11:24:39AM -0700, Nathan Chancellor wrote:
> When building with clang's -Wincompatible-function-pointer-types-strict,
> the following warnings occur:
> 
>   drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c:102:23: error: incompatible function pointer types assigning to 'void (*)(struct i915_address_space *, dma_addr_t, u64, unsigned int, u32)' (aka 'void (*)(struct i915_address_space *, unsigned int, unsigned long long, unsigned int, unsigned int)') from 'void (struct i915_address_space *, dma_addr_t, u64, enum i915_cache_level, u32)' (aka 'void (struct i915_address_space *, unsigned int, unsigned long long, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
>           ggtt->vm.insert_page = gmch_ggtt_insert_page;
>                                ^ ~~~~~~~~~~~~~~~~~~~~~
>   drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c:103:26: error: incompatible function pointer types assigning to 'void (*)(struct i915_address_space *, struct i915_vma_resource *, unsigned int, u32)' (aka 'void (*)(struct i915_address_space *, struct i915_vma_resource *, unsigned int, unsigned int)') from 'void (struct i915_address_space *, struct i915_vma_resource *, enum i915_cache_level, u32)' (aka 'void (struct i915_address_space *, struct i915_vma_resource *, enum i915_cache_level, unsigned int)') [-Werror, -Wincompatible-function-pointer-types-strict]
>           ggtt->vm.insert_entries = gmch_ggtt_insert_entries;
>                                   ^ ~~~~~~~~~~~~~~~~~~~~~~~~
>   2 errors generated.
> 
> The warning is pointing out that while 'enum i915_cache_level' and
> 'unsigned int' are ABI compatible, these indirect calls will fail
> clang's kernel Control Flow Integrity (kCFI) checks, as the callback's
> signature does not exactly match the prototype's signature.
> 
> To fix this, replace the cache_level parameter with pat_index, as was
> done in other places within i915 where there is no difference between
> cache_level and pat_index on certain generations.
> 
> Fixes: 9275277d5324 ("drm/i915: use pat_index instead of cache_level")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

same clang issue as before, I'm OK with this patch, from my side:

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> 

Thanks,
Andi

RE: [PATCH 2/2] drm/i915/gt: Fix parameter in gmch_ggtt_insert_{entries,page}()

[Yang, Fei]

> Subject: [PATCH 2/2] drm/i915/gt: Fix parameter in gmch_ggtt_insert_{entries,page}()
>
> When building with clang's -Wincompatible-function-pointer-types-strict,
> the following warnings occur:
>
>   drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c:102:23: error: incompatible function pointer types assigning to 'void (*)(struct i915_address_space *, dma_addr_t, u64, unsigned int, u32)' (aka 'void (*)(struct i915_address_space *, unsigned int, unsigned long long, unsigned int, unsigned int)') from 'void (struct i915_address_space *, dma_addr_t, u64, enum i915_cache_level, u32)' (aka 'void (struct i915_address_space *, unsigned int, unsigned long long, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict]
>           ggtt->vm.insert_page = gmch_ggtt_insert_page;
>                                ^ ~~~~~~~~~~~~~~~~~~~~~
>   drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c:103:26: error: incompatible function pointer types assigning to 'void (*)(struct i915_address_space *, struct i915_vma_resource *, unsigned int, u32)' (aka 'void (*)(struct i915_address_space *, struct i915_vma_resource *, unsigned int, unsigned int)') from 'void (struct i915_address_space *, struct i915_vma_resource *, enum i915_cache_level, u32)' (aka 'void (struct i915_address_space *, struct i915_vma_resource *, enum i915_cache_level, unsigned int)') [-Werror, -Wincompatible-function-pointer-types-strict]
>           ggtt->vm.insert_entries = gmch_ggtt_insert_entries;
>                                   ^ ~~~~~~~~~~~~~~~~~~~~~~~~
>   2 errors generated.
>
> The warning is pointing out that while 'enum i915_cache_level' and 'unsigned int' are ABI compatible, these indirect calls will fail clang's kernel Control Flow Integrity (kCFI) checks, as the callback's signature does not exactly match the prototype's signature.
>
> To fix this, replace the cache_level parameter with pat_index, as was done in other places within i915 where there is no difference between cache_level and pat_index on certain generations.
>
> Fixes: 9275277d5324 ("drm/i915: use pat_index instead of cache_level")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

Reviewed-by: Fei Yang <fei.yang@intel.com>

> ---
>  drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c b/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c
> index d6a74ae2527b..866c416afb73 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c
> @@ -18,10 +18,10 @@
>  static void gmch_ggtt_insert_page(struct i915_address_space *vm,
>                                 dma_addr_t addr,
>                                 u64 offset,
> -                               enum i915_cache_level cache_level,
> +                               unsigned int pat_index,
>                                 u32 unused)
>  {
> -     unsigned int flags = (cache_level == I915_CACHE_NONE) ?
> +     unsigned int flags = (pat_index == I915_CACHE_NONE) ?
>               AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
>
>       intel_gmch_gtt_insert_page(addr, offset >> PAGE_SHIFT, flags); @@ -29,10 +29,10 @@ static void gmch_ggtt_insert_page(struct i915_address_space *vm,
>
>  static void gmch_ggtt_insert_entries(struct i915_address_space *vm,
>                                    struct i915_vma_resource *vma_res,
> -                                  enum i915_cache_level cache_level,
> +                                  unsigned int pat_index,
>                                    u32 unused)
>  {
> -     unsigned int flags = (cache_level == I915_CACHE_NONE) ?
> +     unsigned int flags = (pat_index == I915_CACHE_NONE) ?
>               AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
>
>       intel_gmch_gtt_insert_sg_entries(vma_res->bi.pages, vma_res->start >> PAGE_SHIFT,
>
> --
> 2.40.1

Re: [PATCH 0/2] drm/i915/gt: Fix recent kCFI violations

[Andi Shyti]

Hi Nathan,

On Tue, May 30, 2023 at 11:24:37AM -0700, Nathan Chancellor wrote:
> Hi all,
> 
> This series fixes a few clang kernel Control Flow Integrity (kCFI)
> violations that appear after commit 9275277d5324 ("drm/i915: use
> pat_index instead of cache_level"). They were found between run time
> testing on real hardware and compile time testing with
> -Wincompatible-function-pointer-types-strict (which is not yet enabled
> for the kernel but I build with it locally to catch new instances).
> 
> If there are any problems or questions, please let me know.
> 
> ---
> Nathan Chancellor (2):
>       drm/i915/gt: Fix second parameter type of pre-gen8 pte_encode callbacks
>       drm/i915/gt: Fix parameter in gmch_ggtt_insert_{entries,page}()

pushed in drm-intel-gt-next.

Thank you,
Andi