* [PATCH] drm/i915: use static const array for PICK macro
@ 2017-03-20 21:56 ` Arnd Bergmann
0 siblings, 0 replies; 16+ messages in thread
From: Arnd Bergmann @ 2017-03-20 21:56 UTC (permalink / raw)
To: Daniel Vetter, Jani Nikula, David Airlie
Cc: Arnd Bergmann, Mika Kuoppala, Ville Syrjälä,
Chris Wilson, Imre Deak, Ander Conselvan de Oliveira,
Robert Bragg, intel-gfx, dri-devel, linux-kernel
The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
to shrink the i915 kernel module by around 1000 bytes. However, the
downside is a size regression with CONFIG_KASAN, as I found from stack size
warnings with gcc-7.0.1:
before:
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 176 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 224 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
after:
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 1016 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 1960 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
I also checked the module sizes and got with gcc-7.0.1
original:
text data bss dec hex filename
2380830 1155436 4448 3540714 3606ea drivers/gpu/drm/i915/i915-kasan.o
1298054 543692 2884 1844630 1c2596 drivers/gpu/drm/i915/i915-nokasan.o
after ce64645d86ac:
text data bss dec hex filename
2389515 1154476 4448 3548439 362517 drivers/gpu/drm/i915/i915-kasan.o
1299639 543692 2884 1846215 1c2bc7 drivers/gpu/drm/i915/i915-nokasan.o
with this patch:
text data bss dec hex filename
2381275 1163884 4448 3549607 3629a7 drivers/gpu/drm/i915/i915-kasan.o
1296038 543692 2884 1842614 1c1db6 drivers/gpu/drm/i915/i915-nokasan.o
Actually showing a code size growth in .text both with and without kasan,
and my version gets most of it back at the expense of larger .data when
kasan is enabled.
Fixes: ce64645d86ac ("drm/i915: use variadic macros and arrays to choose port/pipe based registers")
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80114
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 04c8f69fcc62..39b53878a188 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -48,7 +48,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
}
-#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
+#define _PICK(__index, ...) ({static const u32 __arr[] = { __VA_ARGS__ }; __arr[__index];})
#define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
#define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))
@@ -2657,10 +2657,10 @@ enum skl_disp_power_wells {
/*
* Clock control & power management
*/
-#define _DPLL_A (dev_priv->info.display_mmio_offset + 0x6014)
-#define _DPLL_B (dev_priv->info.display_mmio_offset + 0x6018)
-#define _CHV_DPLL_C (dev_priv->info.display_mmio_offset + 0x6030)
-#define DPLL(pipe) _MMIO_PIPE3((pipe), _DPLL_A, _DPLL_B, _CHV_DPLL_C)
+#define _DPLL_A 0x6014
+#define _DPLL_B 0x6018
+#define _CHV_DPLL_C 0x6030
+#define DPLL(pipe) _MMIO(dev_priv->info.display_mmio_offset + _PIPE3((pipe), _DPLL_A, _DPLL_B, _CHV_DPLL_C))
#define VGA0 _MMIO(0x6000)
#define VGA1 _MMIO(0x6004)
@@ -2756,10 +2756,10 @@ enum skl_disp_power_wells {
#define SDVO_MULTIPLIER_SHIFT_HIRES 4
#define SDVO_MULTIPLIER_SHIFT_VGA 0
-#define _DPLL_A_MD (dev_priv->info.display_mmio_offset + 0x601c)
-#define _DPLL_B_MD (dev_priv->info.display_mmio_offset + 0x6020)
-#define _CHV_DPLL_C_MD (dev_priv->info.display_mmio_offset + 0x603c)
-#define DPLL_MD(pipe) _MMIO_PIPE3((pipe), _DPLL_A_MD, _DPLL_B_MD, _CHV_DPLL_C_MD)
+#define _DPLL_A_MD 0x601c
+#define _DPLL_B_MD 0x6020
+#define _CHV_DPLL_C_MD 0x603c
+#define DPLL_MD(pipe) _MMIO(dev_priv->info.display_mmio_offset + _PIPE3((pipe), _DPLL_A_MD, _DPLL_B_MD, _CHV_DPLL_C_MD))
/*
* UDI pixel divider, controlling how many pixels are stuffed into a packet.
--
2.9.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH] drm/i915: use static const array for PICK macro
@ 2017-03-20 21:56 ` Arnd Bergmann
0 siblings, 0 replies; 16+ messages in thread
From: Arnd Bergmann @ 2017-03-20 21:56 UTC (permalink / raw)
To: Daniel Vetter, Jani Nikula, David Airlie
Cc: Ander Conselvan de Oliveira, Arnd Bergmann, linux-kernel,
dri-devel, intel-gfx
The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
to shrink the i915 kernel module by around 1000 bytes. However, the
downside is a size regression with CONFIG_KASAN, as I found from stack size
warnings with gcc-7.0.1:
before:
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 176 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 224 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
after:
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 1016 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 1960 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
I also checked the module sizes and got with gcc-7.0.1
original:
text data bss dec hex filename
2380830 1155436 4448 3540714 3606ea drivers/gpu/drm/i915/i915-kasan.o
1298054 543692 2884 1844630 1c2596 drivers/gpu/drm/i915/i915-nokasan.o
after ce64645d86ac:
text data bss dec hex filename
2389515 1154476 4448 3548439 362517 drivers/gpu/drm/i915/i915-kasan.o
1299639 543692 2884 1846215 1c2bc7 drivers/gpu/drm/i915/i915-nokasan.o
with this patch:
text data bss dec hex filename
2381275 1163884 4448 3549607 3629a7 drivers/gpu/drm/i915/i915-kasan.o
1296038 543692 2884 1842614 1c1db6 drivers/gpu/drm/i915/i915-nokasan.o
Actually showing a code size growth in .text both with and without kasan,
and my version gets most of it back at the expense of larger .data when
kasan is enabled.
Fixes: ce64645d86ac ("drm/i915: use variadic macros and arrays to choose port/pipe based registers")
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80114
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 04c8f69fcc62..39b53878a188 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -48,7 +48,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
}
-#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
+#define _PICK(__index, ...) ({static const u32 __arr[] = { __VA_ARGS__ }; __arr[__index];})
#define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
#define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))
@@ -2657,10 +2657,10 @@ enum skl_disp_power_wells {
/*
* Clock control & power management
*/
-#define _DPLL_A (dev_priv->info.display_mmio_offset + 0x6014)
-#define _DPLL_B (dev_priv->info.display_mmio_offset + 0x6018)
-#define _CHV_DPLL_C (dev_priv->info.display_mmio_offset + 0x6030)
-#define DPLL(pipe) _MMIO_PIPE3((pipe), _DPLL_A, _DPLL_B, _CHV_DPLL_C)
+#define _DPLL_A 0x6014
+#define _DPLL_B 0x6018
+#define _CHV_DPLL_C 0x6030
+#define DPLL(pipe) _MMIO(dev_priv->info.display_mmio_offset + _PIPE3((pipe), _DPLL_A, _DPLL_B, _CHV_DPLL_C))
#define VGA0 _MMIO(0x6000)
#define VGA1 _MMIO(0x6004)
@@ -2756,10 +2756,10 @@ enum skl_disp_power_wells {
#define SDVO_MULTIPLIER_SHIFT_HIRES 4
#define SDVO_MULTIPLIER_SHIFT_VGA 0
-#define _DPLL_A_MD (dev_priv->info.display_mmio_offset + 0x601c)
-#define _DPLL_B_MD (dev_priv->info.display_mmio_offset + 0x6020)
-#define _CHV_DPLL_C_MD (dev_priv->info.display_mmio_offset + 0x603c)
-#define DPLL_MD(pipe) _MMIO_PIPE3((pipe), _DPLL_A_MD, _DPLL_B_MD, _CHV_DPLL_C_MD)
+#define _DPLL_A_MD 0x601c
+#define _DPLL_B_MD 0x6020
+#define _CHV_DPLL_C_MD 0x603c
+#define DPLL_MD(pipe) _MMIO(dev_priv->info.display_mmio_offset + _PIPE3((pipe), _DPLL_A_MD, _DPLL_B_MD, _CHV_DPLL_C_MD))
/*
* UDI pixel divider, controlling how many pixels are stuffed into a packet.
--
2.9.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 16+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915: use static const array for PICK macro
2017-03-20 21:56 ` Arnd Bergmann
(?)
@ 2017-03-20 23:28 ` Patchwork
-1 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2017-03-20 23:28 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: use static const array for PICK macro
URL : https://patchwork.freedesktop.org/series/21561/
State : warning
== Summary ==
Series 21561v1 drm/i915: use static const array for PICK macro
https://patchwork.freedesktop.org/api/1.0/series/21561/revisions/1/mbox/
Test gem_exec_suspend:
Subgroup basic-s4-devices:
pass -> DMESG-WARN (fi-snb-2600)
Test kms_cursor_legacy:
Subgroup basic-flip-before-cursor-atomic:
incomplete -> SKIP (fi-byt-j1900)
Test kms_flip:
Subgroup basic-flip-vs-modeset:
notrun -> INCOMPLETE (fi-byt-j1900)
Subgroup basic-flip-vs-wf_vblank:
notrun -> INCOMPLETE (fi-byt-n2820)
Subgroup basic-plain-flip:
incomplete -> PASS (fi-byt-n2820)
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-b:
incomplete -> PASS (fi-snb-2520m)
Subgroup hang-read-crc-pipe-c:
incomplete -> PASS (fi-bsw-n3050)
notrun -> INCOMPLETE (fi-ivb-3520m)
Subgroup read-crc-pipe-c-frame-sequence:
incomplete -> PASS (fi-ivb-3520m)
Test pm_rpm:
Subgroup basic-pci-d3-state:
notrun -> INCOMPLETE (fi-bsw-n3050)
Test prime_self_import:
Subgroup basic-llseek-bad:
notrun -> INCOMPLETE (fi-snb-2520m)
Test prime_vgem:
Subgroup basic-fence-wait-default:
incomplete -> PASS (fi-skl-6770hq)
fi-bdw-5557u total:300 pass:284 dwarn:0 dfail:0 fail:0 skip:16 time: 617s
fi-bsw-n3050 total:300 pass:221 dwarn:0 dfail:0 fail:1 skip:46 time: 0s
fi-byt-j1900 total:239 pass:211 dwarn:0 dfail:0 fail:0 skip:27 time: 0s
fi-byt-n2820 total:300 pass:208 dwarn:0 dfail:0 fail:0 skip:31 time: 0s
fi-hsw-4770 total:300 pass:279 dwarn:0 dfail:0 fail:1 skip:20 time: 660s
fi-ilk-650 total:300 pass:240 dwarn:0 dfail:0 fail:2 skip:58 time: 534s
fi-ivb-3520m total:263 pass:238 dwarn:0 dfail:0 fail:5 skip:19 time: 0s
fi-kbl-7500u total:300 pass:275 dwarn:1 dfail:0 fail:4 skip:20 time: 635s
fi-skl-6260u total:300 pass:283 dwarn:1 dfail:0 fail:4 skip:12 time: 648s
fi-skl-6700k total:300 pass:271 dwarn:5 dfail:0 fail:4 skip:20 time: 664s
fi-skl-6770hq total:300 pass:283 dwarn:1 dfail:0 fail:4 skip:12 time: 687s
fi-snb-2520m total:300 pass:238 dwarn:0 dfail:0 fail:6 skip:35 time: 0s
fi-snb-2600 total:300 pass:258 dwarn:1 dfail:0 fail:5 skip:36 time: 594s
fi-bxt-j4205 failed to connect after reboot
fi-hsw-4770r failed to connect after reboot
fi-skl-6700hq failed to collect. IGT log at Patchwork_4236/fi-skl-6700hq/igt.log
1e78eaf62e1d2fd47cf6fb974876610a5786d352 drm-tip: 2017y-03m-20d-21h-54m-26s UTC integration manifest
a058c30 drm/i915: use static const array for PICK macro
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4236/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: use static const array for PICK macro
2017-03-20 21:56 ` Arnd Bergmann
@ 2017-03-21 8:26 ` Jani Nikula
-1 siblings, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2017-03-21 8:26 UTC (permalink / raw)
To: Arnd Bergmann, Daniel Vetter, David Airlie
Cc: Arnd Bergmann, Mika Kuoppala, Ville Syrjälä,
Chris Wilson, Imre Deak, Ander Conselvan de Oliveira,
Robert Bragg, intel-gfx, dri-devel, linux-kernel
On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
> to shrink the i915 kernel module by around 1000 bytes.
Really, I didn't care one bit about the size shrink, I only cared about
making it easier and less error prone to increase the number of args in
a number of places. Maintainability and correctness were the goals. Just
for the record. ;)
Otherwise, this seems like an acceptable approach to me. Would be
interesting to see what happens with this, and f4c3a88e5f04 ("drm/i915:
Tighten mmio arrays for MIPI_PORT") reverted on top.
BR,
Jani.
> However, the
> downside is a size regression with CONFIG_KASAN, as I found from stack size
> warnings with gcc-7.0.1:
>
> before:
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 176 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 224 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
>
> after:
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 1016 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 1960 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
>
> I also checked the module sizes and got with gcc-7.0.1
>
> original:
> text data bss dec hex filename
> 2380830 1155436 4448 3540714 3606ea drivers/gpu/drm/i915/i915-kasan.o
> 1298054 543692 2884 1844630 1c2596 drivers/gpu/drm/i915/i915-nokasan.o
>
> after ce64645d86ac:
> text data bss dec hex filename
> 2389515 1154476 4448 3548439 362517 drivers/gpu/drm/i915/i915-kasan.o
> 1299639 543692 2884 1846215 1c2bc7 drivers/gpu/drm/i915/i915-nokasan.o
>
> with this patch:
> text data bss dec hex filename
> 2381275 1163884 4448 3549607 3629a7 drivers/gpu/drm/i915/i915-kasan.o
> 1296038 543692 2884 1842614 1c1db6 drivers/gpu/drm/i915/i915-nokasan.o
>
> Actually showing a code size growth in .text both with and without kasan,
> and my version gets most of it back at the expense of larger .data when
> kasan is enabled.
>
> Fixes: ce64645d86ac ("drm/i915: use variadic macros and arrays to choose port/pipe based registers")
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80114
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 04c8f69fcc62..39b53878a188 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -48,7 +48,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
> }
>
> -#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
> +#define _PICK(__index, ...) ({static const u32 __arr[] = { __VA_ARGS__ }; __arr[__index];})
>
> #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
> #define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))
> @@ -2657,10 +2657,10 @@ enum skl_disp_power_wells {
> /*
> * Clock control & power management
> */
> -#define _DPLL_A (dev_priv->info.display_mmio_offset + 0x6014)
> -#define _DPLL_B (dev_priv->info.display_mmio_offset + 0x6018)
> -#define _CHV_DPLL_C (dev_priv->info.display_mmio_offset + 0x6030)
> -#define DPLL(pipe) _MMIO_PIPE3((pipe), _DPLL_A, _DPLL_B, _CHV_DPLL_C)
> +#define _DPLL_A 0x6014
> +#define _DPLL_B 0x6018
> +#define _CHV_DPLL_C 0x6030
> +#define DPLL(pipe) _MMIO(dev_priv->info.display_mmio_offset + _PIPE3((pipe), _DPLL_A, _DPLL_B, _CHV_DPLL_C))
>
> #define VGA0 _MMIO(0x6000)
> #define VGA1 _MMIO(0x6004)
> @@ -2756,10 +2756,10 @@ enum skl_disp_power_wells {
> #define SDVO_MULTIPLIER_SHIFT_HIRES 4
> #define SDVO_MULTIPLIER_SHIFT_VGA 0
>
> -#define _DPLL_A_MD (dev_priv->info.display_mmio_offset + 0x601c)
> -#define _DPLL_B_MD (dev_priv->info.display_mmio_offset + 0x6020)
> -#define _CHV_DPLL_C_MD (dev_priv->info.display_mmio_offset + 0x603c)
> -#define DPLL_MD(pipe) _MMIO_PIPE3((pipe), _DPLL_A_MD, _DPLL_B_MD, _CHV_DPLL_C_MD)
> +#define _DPLL_A_MD 0x601c
> +#define _DPLL_B_MD 0x6020
> +#define _CHV_DPLL_C_MD 0x603c
> +#define DPLL_MD(pipe) _MMIO(dev_priv->info.display_mmio_offset + _PIPE3((pipe), _DPLL_A_MD, _DPLL_B_MD, _CHV_DPLL_C_MD))
>
> /*
> * UDI pixel divider, controlling how many pixels are stuffed into a packet.
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: use static const array for PICK macro
@ 2017-03-21 8:26 ` Jani Nikula
0 siblings, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2017-03-21 8:26 UTC (permalink / raw)
To: Daniel Vetter, David Airlie
Cc: Ander Conselvan de Oliveira, Arnd Bergmann, linux-kernel,
dri-devel, intel-gfx
On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
> to shrink the i915 kernel module by around 1000 bytes.
Really, I didn't care one bit about the size shrink, I only cared about
making it easier and less error prone to increase the number of args in
a number of places. Maintainability and correctness were the goals. Just
for the record. ;)
Otherwise, this seems like an acceptable approach to me. Would be
interesting to see what happens with this, and f4c3a88e5f04 ("drm/i915:
Tighten mmio arrays for MIPI_PORT") reverted on top.
BR,
Jani.
> However, the
> downside is a size regression with CONFIG_KASAN, as I found from stack size
> warnings with gcc-7.0.1:
>
> before:
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 176 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 224 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
>
> after:
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 1016 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 1960 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
>
> I also checked the module sizes and got with gcc-7.0.1
>
> original:
> text data bss dec hex filename
> 2380830 1155436 4448 3540714 3606ea drivers/gpu/drm/i915/i915-kasan.o
> 1298054 543692 2884 1844630 1c2596 drivers/gpu/drm/i915/i915-nokasan.o
>
> after ce64645d86ac:
> text data bss dec hex filename
> 2389515 1154476 4448 3548439 362517 drivers/gpu/drm/i915/i915-kasan.o
> 1299639 543692 2884 1846215 1c2bc7 drivers/gpu/drm/i915/i915-nokasan.o
>
> with this patch:
> text data bss dec hex filename
> 2381275 1163884 4448 3549607 3629a7 drivers/gpu/drm/i915/i915-kasan.o
> 1296038 543692 2884 1842614 1c1db6 drivers/gpu/drm/i915/i915-nokasan.o
>
> Actually showing a code size growth in .text both with and without kasan,
> and my version gets most of it back at the expense of larger .data when
> kasan is enabled.
>
> Fixes: ce64645d86ac ("drm/i915: use variadic macros and arrays to choose port/pipe based registers")
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80114
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 04c8f69fcc62..39b53878a188 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -48,7 +48,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
> }
>
> -#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
> +#define _PICK(__index, ...) ({static const u32 __arr[] = { __VA_ARGS__ }; __arr[__index];})
>
> #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
> #define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))
> @@ -2657,10 +2657,10 @@ enum skl_disp_power_wells {
> /*
> * Clock control & power management
> */
> -#define _DPLL_A (dev_priv->info.display_mmio_offset + 0x6014)
> -#define _DPLL_B (dev_priv->info.display_mmio_offset + 0x6018)
> -#define _CHV_DPLL_C (dev_priv->info.display_mmio_offset + 0x6030)
> -#define DPLL(pipe) _MMIO_PIPE3((pipe), _DPLL_A, _DPLL_B, _CHV_DPLL_C)
> +#define _DPLL_A 0x6014
> +#define _DPLL_B 0x6018
> +#define _CHV_DPLL_C 0x6030
> +#define DPLL(pipe) _MMIO(dev_priv->info.display_mmio_offset + _PIPE3((pipe), _DPLL_A, _DPLL_B, _CHV_DPLL_C))
>
> #define VGA0 _MMIO(0x6000)
> #define VGA1 _MMIO(0x6004)
> @@ -2756,10 +2756,10 @@ enum skl_disp_power_wells {
> #define SDVO_MULTIPLIER_SHIFT_HIRES 4
> #define SDVO_MULTIPLIER_SHIFT_VGA 0
>
> -#define _DPLL_A_MD (dev_priv->info.display_mmio_offset + 0x601c)
> -#define _DPLL_B_MD (dev_priv->info.display_mmio_offset + 0x6020)
> -#define _CHV_DPLL_C_MD (dev_priv->info.display_mmio_offset + 0x603c)
> -#define DPLL_MD(pipe) _MMIO_PIPE3((pipe), _DPLL_A_MD, _DPLL_B_MD, _CHV_DPLL_C_MD)
> +#define _DPLL_A_MD 0x601c
> +#define _DPLL_B_MD 0x6020
> +#define _CHV_DPLL_C_MD 0x603c
> +#define DPLL_MD(pipe) _MMIO(dev_priv->info.display_mmio_offset + _PIPE3((pipe), _DPLL_A_MD, _DPLL_B_MD, _CHV_DPLL_C_MD))
>
> /*
> * UDI pixel divider, controlling how many pixels are stuffed into a packet.
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: use static const array for PICK macro
2017-03-21 8:26 ` Jani Nikula
@ 2017-03-21 8:44 ` Arnd Bergmann
-1 siblings, 0 replies; 16+ messages in thread
From: Arnd Bergmann @ 2017-03-21 8:44 UTC (permalink / raw)
To: Jani Nikula
Cc: Daniel Vetter, David Airlie, Mika Kuoppala,
Ville Syrjälä,
Chris Wilson, Imre Deak, Ander Conselvan de Oliveira,
Robert Bragg, intel-gfx, dri-devel, Linux Kernel Mailing List
On Tue, Mar 21, 2017 at 9:26 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
>> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
>> to shrink the i915 kernel module by around 1000 bytes.
>
> Really, I didn't care one bit about the size shrink, I only cared about
> making it easier and less error prone to increase the number of args in
> a number of places. Maintainability and correctness were the goals. Just
> for the record. ;)
Ok. My only interest here is the warning about possible stack overflow,
though the fact that KASAN considers the array code to be fragile is
an indication that it is perhaps actually dangerous: if we ever run into
a bug that causes the array index to overflow, we might in theory
have a security bug that lets users access arbitrary kernel pointers.
While the risk for that actually happening is very low, the original code
was safer in that regard. My patch on top of yours merely turns a
hypothetical arbitrary stack access into an arbitrary .data access,
and I don't even know which one would be worse.
Arnd
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: use static const array for PICK macro
@ 2017-03-21 8:44 ` Arnd Bergmann
0 siblings, 0 replies; 16+ messages in thread
From: Arnd Bergmann @ 2017-03-21 8:44 UTC (permalink / raw)
To: Jani Nikula
Cc: Ander Conselvan de Oliveira, David Airlie,
Linux Kernel Mailing List, dri-devel, Daniel Vetter, intel-gfx
On Tue, Mar 21, 2017 at 9:26 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
>> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
>> to shrink the i915 kernel module by around 1000 bytes.
>
> Really, I didn't care one bit about the size shrink, I only cared about
> making it easier and less error prone to increase the number of args in
> a number of places. Maintainability and correctness were the goals. Just
> for the record. ;)
Ok. My only interest here is the warning about possible stack overflow,
though the fact that KASAN considers the array code to be fragile is
an indication that it is perhaps actually dangerous: if we ever run into
a bug that causes the array index to overflow, we might in theory
have a security bug that lets users access arbitrary kernel pointers.
While the risk for that actually happening is very low, the original code
was safer in that regard. My patch on top of yours merely turns a
hypothetical arbitrary stack access into an arbitrary .data access,
and I don't even know which one would be worse.
Arnd
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: use static const array for PICK macro
2017-03-21 8:44 ` Arnd Bergmann
@ 2017-03-21 10:33 ` Daniel Vetter
-1 siblings, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2017-03-21 10:33 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jani Nikula, Ander Conselvan de Oliveira, David Airlie,
Linux Kernel Mailing List, dri-devel, Daniel Vetter, intel-gfx
On Tue, Mar 21, 2017 at 09:44:07AM +0100, Arnd Bergmann wrote:
> On Tue, Mar 21, 2017 at 9:26 AM, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
> > On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
> >> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
> >> to shrink the i915 kernel module by around 1000 bytes.
> >
> > Really, I didn't care one bit about the size shrink, I only cared about
> > making it easier and less error prone to increase the number of args in
> > a number of places. Maintainability and correctness were the goals. Just
> > for the record. ;)
>
> Ok. My only interest here is the warning about possible stack overflow,
> though the fact that KASAN considers the array code to be fragile is
> an indication that it is perhaps actually dangerous: if we ever run into
> a bug that causes the array index to overflow, we might in theory
> have a security bug that lets users access arbitrary kernel pointers.
>
> While the risk for that actually happening is very low, the original code
> was safer in that regard. My patch on top of yours merely turns a
> hypothetical arbitrary stack access into an arbitrary .data access,
> and I don't even know which one would be worse.
Even without these arrays, if userspace could control the index we feed
into these you get arbitrary mmio access. Or semi-arbitrary at least.
None of these are bugs we should ever let through, and I think with the
current code design (where the driver constructs structs that contain the
right indizes, and userspace only ever gets to point at these structs
using an idr lookup) none of these are likely to happen.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: use static const array for PICK macro
@ 2017-03-21 10:33 ` Daniel Vetter
0 siblings, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2017-03-21 10:33 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ander Conselvan de Oliveira, David Airlie, intel-gfx,
Linux Kernel Mailing List, dri-devel, Daniel Vetter
On Tue, Mar 21, 2017 at 09:44:07AM +0100, Arnd Bergmann wrote:
> On Tue, Mar 21, 2017 at 9:26 AM, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
> > On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
> >> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
> >> to shrink the i915 kernel module by around 1000 bytes.
> >
> > Really, I didn't care one bit about the size shrink, I only cared about
> > making it easier and less error prone to increase the number of args in
> > a number of places. Maintainability and correctness were the goals. Just
> > for the record. ;)
>
> Ok. My only interest here is the warning about possible stack overflow,
> though the fact that KASAN considers the array code to be fragile is
> an indication that it is perhaps actually dangerous: if we ever run into
> a bug that causes the array index to overflow, we might in theory
> have a security bug that lets users access arbitrary kernel pointers.
>
> While the risk for that actually happening is very low, the original code
> was safer in that regard. My patch on top of yours merely turns a
> hypothetical arbitrary stack access into an arbitrary .data access,
> and I don't even know which one would be worse.
Even without these arrays, if userspace could control the index we feed
into these you get arbitrary mmio access. Or semi-arbitrary at least.
None of these are bugs we should ever let through, and I think with the
current code design (where the driver constructs structs that contain the
right indizes, and userspace only ever gets to point at these structs
using an idr lookup) none of these are likely to happen.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: use static const array for PICK macro
2017-03-21 10:33 ` Daniel Vetter
@ 2017-03-21 11:23 ` Jani Nikula
-1 siblings, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2017-03-21 11:23 UTC (permalink / raw)
To: Daniel Vetter, Arnd Bergmann
Cc: Ander Conselvan de Oliveira, David Airlie,
Linux Kernel Mailing List, dri-devel, Daniel Vetter, intel-gfx
On Tue, 21 Mar 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Mar 21, 2017 at 09:44:07AM +0100, Arnd Bergmann wrote:
>> On Tue, Mar 21, 2017 at 9:26 AM, Jani Nikula
>> <jani.nikula@linux.intel.com> wrote:
>> > On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
>> >> to shrink the i915 kernel module by around 1000 bytes.
>> >
>> > Really, I didn't care one bit about the size shrink, I only cared about
>> > making it easier and less error prone to increase the number of args in
>> > a number of places. Maintainability and correctness were the goals. Just
>> > for the record. ;)
>>
>> Ok. My only interest here is the warning about possible stack overflow,
>> though the fact that KASAN considers the array code to be fragile is
>> an indication that it is perhaps actually dangerous: if we ever run into
>> a bug that causes the array index to overflow, we might in theory
>> have a security bug that lets users access arbitrary kernel pointers.
>>
>> While the risk for that actually happening is very low, the original code
>> was safer in that regard. My patch on top of yours merely turns a
>> hypothetical arbitrary stack access into an arbitrary .data access,
>> and I don't even know which one would be worse.
>
> Even without these arrays, if userspace could control the index we feed
> into these you get arbitrary mmio access. Or semi-arbitrary at least.
>
> None of these are bugs we should ever let through, and I think with the
> current code design (where the driver constructs structs that contain the
> right indizes, and userspace only ever gets to point at these structs
> using an idr lookup) none of these are likely to happen.
That's all true, but I'm curious if explicit checks would help
kasan. Something like:
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 04c8f69fcc62..0ab32a05b5d8 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -48,7 +48,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
}
-#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
+#define _PICK_NARGS(...) ARRAY_SIZE(((const u32 []){ __VA_ARGS__ }))
+#define _PICK(__index, ...) ((__index) >= 0 && (__index) < _PICK_NARGS(__VA_ARGS__) ? ((const u32 []){ __VA_ARGS__ })[__index] : 0)
#define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
#define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))
---
Arnd, can you check that with kasan please? (I don't have gcc 7.) For me
the size diff against current git is
text data bss dec hex filename
-1137236 31211 2948 1171395 11dfc3 drivers/gpu/drm/i915/i915.ko
+1139702 31211 2948 1173861 11e965 drivers/gpu/drm/i915/i915.ko
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: use static const array for PICK macro
@ 2017-03-21 11:23 ` Jani Nikula
0 siblings, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2017-03-21 11:23 UTC (permalink / raw)
To: Daniel Vetter, Arnd Bergmann
Cc: Ander Conselvan de Oliveira, intel-gfx,
Linux Kernel Mailing List, dri-devel, Daniel Vetter
On Tue, 21 Mar 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Mar 21, 2017 at 09:44:07AM +0100, Arnd Bergmann wrote:
>> On Tue, Mar 21, 2017 at 9:26 AM, Jani Nikula
>> <jani.nikula@linux.intel.com> wrote:
>> > On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
>> >> to shrink the i915 kernel module by around 1000 bytes.
>> >
>> > Really, I didn't care one bit about the size shrink, I only cared about
>> > making it easier and less error prone to increase the number of args in
>> > a number of places. Maintainability and correctness were the goals. Just
>> > for the record. ;)
>>
>> Ok. My only interest here is the warning about possible stack overflow,
>> though the fact that KASAN considers the array code to be fragile is
>> an indication that it is perhaps actually dangerous: if we ever run into
>> a bug that causes the array index to overflow, we might in theory
>> have a security bug that lets users access arbitrary kernel pointers.
>>
>> While the risk for that actually happening is very low, the original code
>> was safer in that regard. My patch on top of yours merely turns a
>> hypothetical arbitrary stack access into an arbitrary .data access,
>> and I don't even know which one would be worse.
>
> Even without these arrays, if userspace could control the index we feed
> into these you get arbitrary mmio access. Or semi-arbitrary at least.
>
> None of these are bugs we should ever let through, and I think with the
> current code design (where the driver constructs structs that contain the
> right indizes, and userspace only ever gets to point at these structs
> using an idr lookup) none of these are likely to happen.
That's all true, but I'm curious if explicit checks would help
kasan. Something like:
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 04c8f69fcc62..0ab32a05b5d8 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -48,7 +48,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
}
-#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
+#define _PICK_NARGS(...) ARRAY_SIZE(((const u32 []){ __VA_ARGS__ }))
+#define _PICK(__index, ...) ((__index) >= 0 && (__index) < _PICK_NARGS(__VA_ARGS__) ? ((const u32 []){ __VA_ARGS__ })[__index] : 0)
#define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
#define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))
---
Arnd, can you check that with kasan please? (I don't have gcc 7.) For me
the size diff against current git is
text data bss dec hex filename
-1137236 31211 2948 1171395 11dfc3 drivers/gpu/drm/i915/i915.ko
+1139702 31211 2948 1173861 11e965 drivers/gpu/drm/i915/i915.ko
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915: use static const array for PICK macro
2017-03-20 21:56 ` Arnd Bergmann
` (2 preceding siblings ...)
(?)
@ 2017-03-21 13:16 ` Patchwork
-1 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2017-03-21 13:16 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: use static const array for PICK macro
URL : https://patchwork.freedesktop.org/series/21561/
State : warning
== Summary ==
Series 21561v1 drm/i915: use static const array for PICK macro
https://patchwork.freedesktop.org/api/1.0/series/21561/revisions/1/mbox/
Test gem_exec_flush:
Subgroup basic-batch-kernel-default-uc:
pass -> FAIL (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
Subgroup basic-s4-devices:
dmesg-warn -> PASS (fi-bxt-t5700) fdo#100125
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-legacy:
pass -> DMESG-WARN (fi-byt-n2820)
fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 460s
fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 time: 575s
fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 539s
fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time: 559s
fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 time: 507s
fi-byt-n2820 total:278 pass:246 dwarn:1 dfail:0 fail:0 skip:31 time: 502s
fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 434s
fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 435s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 434s
fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 517s
fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 488s
fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 488s
fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 481s
fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time: 593s
fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time: 481s
fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 519s
fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 549s
fi-snb-2600 total:278 pass:248 dwarn:0 dfail:0 fail:1 skip:29 time: 418s
84b98c6d4f5f6ce0ce17b8fd07629dfc71e7d829 drm-tip: 2017y-03m-21d-11h-08m-23s UTC integration manifest
17c44d8 drm/i915: use static const array for PICK macro
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4245/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: use static const array for PICK macro
2017-03-21 11:23 ` Jani Nikula
@ 2017-03-31 13:37 ` Arnd Bergmann
-1 siblings, 0 replies; 16+ messages in thread
From: Arnd Bergmann @ 2017-03-31 13:37 UTC (permalink / raw)
To: Jani Nikula
Cc: Daniel Vetter, Ander Conselvan de Oliveira, David Airlie,
Linux Kernel Mailing List, dri-devel, Daniel Vetter, intel-gfx
On Tue, Mar 21, 2017 at 12:23 PM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Tue, 21 Mar 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Mar 21, 2017 at 09:44:07AM +0100, Arnd Bergmann wrote:
> Arnd, can you check that with kasan please? (I don't have gcc 7.) For me
> the size diff against current git is
>
> text data bss dec hex filename
> -1137236 31211 2948 1171395 11dfc3 drivers/gpu/drm/i915/i915.ko
> +1139702 31211 2948 1173861 11e965 drivers/gpu/drm/i915/i915.ko
Sorry for the late reply.
I was rather sure that I had done the numbers and replied to you earlier, but
I see no evidence of that, so here it comes again, using gcc-7 and kasan:
text data bss dec hex filename
2623339 511153 12064 3146556 30033c
obj-x86/drivers/gpu/drm/i915/i915-original.o
2634886 511153 12064 3158103 303057
obj-x86/drivers/gpu/drm/i915/i915-linux-next.o
2617989 520561 12064 3150614 301316
obj-x86/drivers/gpu/drm/i915/i915-arndpatch.o
The first one is linux-next with ce64645d86ac ("drm/i915: use variadic macros
and arrays to choose port/pipe based registers") reverted, the second one is
the current version, and the third is with my patch applied on top.
Arnd
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: use static const array for PICK macro
@ 2017-03-31 13:37 ` Arnd Bergmann
0 siblings, 0 replies; 16+ messages in thread
From: Arnd Bergmann @ 2017-03-31 13:37 UTC (permalink / raw)
To: Jani Nikula
Cc: Ander Conselvan de Oliveira, intel-gfx,
Linux Kernel Mailing List, dri-devel, Daniel Vetter
On Tue, Mar 21, 2017 at 12:23 PM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Tue, 21 Mar 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Mar 21, 2017 at 09:44:07AM +0100, Arnd Bergmann wrote:
> Arnd, can you check that with kasan please? (I don't have gcc 7.) For me
> the size diff against current git is
>
> text data bss dec hex filename
> -1137236 31211 2948 1171395 11dfc3 drivers/gpu/drm/i915/i915.ko
> +1139702 31211 2948 1173861 11e965 drivers/gpu/drm/i915/i915.ko
Sorry for the late reply.
I was rather sure that I had done the numbers and replied to you earlier, but
I see no evidence of that, so here it comes again, using gcc-7 and kasan:
text data bss dec hex filename
2623339 511153 12064 3146556 30033c
obj-x86/drivers/gpu/drm/i915/i915-original.o
2634886 511153 12064 3158103 303057
obj-x86/drivers/gpu/drm/i915/i915-linux-next.o
2617989 520561 12064 3150614 301316
obj-x86/drivers/gpu/drm/i915/i915-arndpatch.o
The first one is linux-next with ce64645d86ac ("drm/i915: use variadic macros
and arrays to choose port/pipe based registers") reverted, the second one is
the current version, and the third is with my patch applied on top.
Arnd
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: use static const array for PICK macro
2017-03-21 11:23 ` Jani Nikula
@ 2017-12-11 12:44 ` Arnd Bergmann
-1 siblings, 0 replies; 16+ messages in thread
From: Arnd Bergmann @ 2017-12-11 12:44 UTC (permalink / raw)
To: Jani Nikula
Cc: Daniel Vetter, Ander Conselvan de Oliveira, David Airlie,
Linux Kernel Mailing List, dri-devel, Daniel Vetter,
Intel Graphics
On Tue, Mar 21, 2017 at 12:23 PM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Tue, 21 Mar 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Mar 21, 2017 at 09:44:07AM +0100, Arnd Bergmann wrote:
>>> On Tue, Mar 21, 2017 at 9:26 AM, Jani Nikula
>>> <jani.nikula@linux.intel.com> wrote:
>>> > On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
>>> >> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
>>> >> to shrink the i915 kernel module by around 1000 bytes.
>>> >
>>> > Really, I didn't care one bit about the size shrink, I only cared about
>>> > making it easier and less error prone to increase the number of args in
>>> > a number of places. Maintainability and correctness were the goals. Just
>>> > for the record. ;)
>>>
>>> Ok. My only interest here is the warning about possible stack overflow,
>>> though the fact that KASAN considers the array code to be fragile is
>>> an indication that it is perhaps actually dangerous: if we ever run into
>>> a bug that causes the array index to overflow, we might in theory
>>> have a security bug that lets users access arbitrary kernel pointers.
>>>
>>> While the risk for that actually happening is very low, the original code
>>> was safer in that regard. My patch on top of yours merely turns a
>>> hypothetical arbitrary stack access into an arbitrary .data access,
>>> and I don't even know which one would be worse.
>>
>> Even without these arrays, if userspace could control the index we feed
>> into these you get arbitrary mmio access. Or semi-arbitrary at least.
>>
>> None of these are bugs we should ever let through, and I think with the
>> current code design (where the driver constructs structs that contain the
>> right indizes, and userspace only ever gets to point at these structs
>> using an idr lookup) none of these are likely to happen.
>
> That's all true, but I'm curious if explicit checks would help
> kasan. Something like:
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 04c8f69fcc62..0ab32a05b5d8 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -48,7 +48,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
> }
>
> -#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
> +#define _PICK_NARGS(...) ARRAY_SIZE(((const u32 []){ __VA_ARGS__ }))
> +#define _PICK(__index, ...) ((__index) >= 0 && (__index) < _PICK_NARGS(__VA_ARGS__) ? ((const u32 []){ __VA_ARGS__ })[__index] : 0)
>
> #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
> #define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))
>
> ---
>
> Arnd, can you check that with kasan please? (I don't have gcc 7.) For me
> the size diff against current git is
>
> text data bss dec hex filename
> -1137236 31211 2948 1171395 11dfc3 drivers/gpu/drm/i915/i915.ko
> +1139702 31211 2948 1173861 11e965 drivers/gpu/drm/i915/i915.ko
>
I just revisited my old patch when I ran into the stack size warning once
more, and realized I had not really answered your question earlier.
I compared your version to what is in 4.15-rc3 now, and to my version,
and confirmed that yours produces the largest code size of the three,
and doesn't address the warnings we get, but does cause additional
warnings ("comparison of constant '3' with boolean expression is always
true"), so that won't get us anywhere. Here are the numbers I get
with gcc-8:
text data bss dec hex filename
2500045 486453 6912 2993410 2dad02 i915-kasan-4_14.ko
2488028 497909 6912 2992849 2daad1 i915-kasan-arnd.ko
2508814 486453 6912 3002179 2dcf43 i915-kasan-jani.ko
1639798 63269 4448 1707515 1a0dfb i915-nokasan-4.15.ko
1635284 63269 4448 1703001 19fc59 i915-nokasan-arnd.ko
1648331 63269 4448 1716048 1a2f50 i915-nokasan-jani.ko
I'll resend my old patch with the original description since I can't easily
reproduce it now without your original change, and the code has
changed again in the meantime, so I had to slightly adapt my
patch to still apply.
Arnd
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: use static const array for PICK macro
@ 2017-12-11 12:44 ` Arnd Bergmann
0 siblings, 0 replies; 16+ messages in thread
From: Arnd Bergmann @ 2017-12-11 12:44 UTC (permalink / raw)
To: Jani Nikula
Cc: Ander Conselvan de Oliveira, David Airlie, Intel Graphics,
Linux Kernel Mailing List, dri-devel, Daniel Vetter
On Tue, Mar 21, 2017 at 12:23 PM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Tue, 21 Mar 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Mar 21, 2017 at 09:44:07AM +0100, Arnd Bergmann wrote:
>>> On Tue, Mar 21, 2017 at 9:26 AM, Jani Nikula
>>> <jani.nikula@linux.intel.com> wrote:
>>> > On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
>>> >> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
>>> >> to shrink the i915 kernel module by around 1000 bytes.
>>> >
>>> > Really, I didn't care one bit about the size shrink, I only cared about
>>> > making it easier and less error prone to increase the number of args in
>>> > a number of places. Maintainability and correctness were the goals. Just
>>> > for the record. ;)
>>>
>>> Ok. My only interest here is the warning about possible stack overflow,
>>> though the fact that KASAN considers the array code to be fragile is
>>> an indication that it is perhaps actually dangerous: if we ever run into
>>> a bug that causes the array index to overflow, we might in theory
>>> have a security bug that lets users access arbitrary kernel pointers.
>>>
>>> While the risk for that actually happening is very low, the original code
>>> was safer in that regard. My patch on top of yours merely turns a
>>> hypothetical arbitrary stack access into an arbitrary .data access,
>>> and I don't even know which one would be worse.
>>
>> Even without these arrays, if userspace could control the index we feed
>> into these you get arbitrary mmio access. Or semi-arbitrary at least.
>>
>> None of these are bugs we should ever let through, and I think with the
>> current code design (where the driver constructs structs that contain the
>> right indizes, and userspace only ever gets to point at these structs
>> using an idr lookup) none of these are likely to happen.
>
> That's all true, but I'm curious if explicit checks would help
> kasan. Something like:
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 04c8f69fcc62..0ab32a05b5d8 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -48,7 +48,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
> }
>
> -#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
> +#define _PICK_NARGS(...) ARRAY_SIZE(((const u32 []){ __VA_ARGS__ }))
> +#define _PICK(__index, ...) ((__index) >= 0 && (__index) < _PICK_NARGS(__VA_ARGS__) ? ((const u32 []){ __VA_ARGS__ })[__index] : 0)
>
> #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
> #define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))
>
> ---
>
> Arnd, can you check that with kasan please? (I don't have gcc 7.) For me
> the size diff against current git is
>
> text data bss dec hex filename
> -1137236 31211 2948 1171395 11dfc3 drivers/gpu/drm/i915/i915.ko
> +1139702 31211 2948 1173861 11e965 drivers/gpu/drm/i915/i915.ko
>
I just revisited my old patch when I ran into the stack size warning once
more, and realized I had not really answered your question earlier.
I compared your version to what is in 4.15-rc3 now, and to my version,
and confirmed that yours produces the largest code size of the three,
and doesn't address the warnings we get, but does cause additional
warnings ("comparison of constant '3' with boolean expression is always
true"), so that won't get us anywhere. Here are the numbers I get
with gcc-8:
text data bss dec hex filename
2500045 486453 6912 2993410 2dad02 i915-kasan-4_14.ko
2488028 497909 6912 2992849 2daad1 i915-kasan-arnd.ko
2508814 486453 6912 3002179 2dcf43 i915-kasan-jani.ko
1639798 63269 4448 1707515 1a0dfb i915-nokasan-4.15.ko
1635284 63269 4448 1703001 19fc59 i915-nokasan-arnd.ko
1648331 63269 4448 1716048 1a2f50 i915-nokasan-jani.ko
I'll resend my old patch with the original description since I can't easily
reproduce it now without your original change, and the code has
changed again in the meantime, so I had to slightly adapt my
patch to still apply.
Arnd
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-12-11 12:44 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-20 21:56 [PATCH] drm/i915: use static const array for PICK macro Arnd Bergmann
2017-03-20 21:56 ` Arnd Bergmann
2017-03-20 23:28 ` ✗ Fi.CI.BAT: warning for " Patchwork
2017-03-21 8:26 ` [PATCH] " Jani Nikula
2017-03-21 8:26 ` Jani Nikula
2017-03-21 8:44 ` Arnd Bergmann
2017-03-21 8:44 ` Arnd Bergmann
2017-03-21 10:33 ` [Intel-gfx] " Daniel Vetter
2017-03-21 10:33 ` Daniel Vetter
2017-03-21 11:23 ` [Intel-gfx] " Jani Nikula
2017-03-21 11:23 ` Jani Nikula
2017-03-31 13:37 ` Arnd Bergmann
2017-03-31 13:37 ` Arnd Bergmann
2017-12-11 12:44 ` Arnd Bergmann
2017-12-11 12:44 ` Arnd Bergmann
2017-03-21 13:16 ` ✗ Fi.CI.BAT: warning for " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.