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

2023-05-30 Thread 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 

Reviewed-by: Fei Yang 

> ---
>  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;

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

2023-05-30 Thread 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 

That's correct!

Reviewed-by: Andi Shyti  

Thanks,
Andi