* Re: [PATCH] drm/i915: Deconstruct struct sgt_dma initialiser
2017-11-06 21:11 ` [PATCH] drm/i915: Deconstruct struct sgt_dma initialiser Chris Wilson
@ 2017-11-07 10:24 ` Tvrtko Ursulin
2017-11-07 11:15 ` Mika Kuoppala
1 sibling, 0 replies; 4+ messages in thread
From: Tvrtko Ursulin @ 2017-11-07 10:24 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Kelly French, Mika Kuoppala
On 06/11/2017 21:11, Chris Wilson wrote:
> gcc-4.4 complains about:
>
> struct sgt_dma iter = {
> .sg = vma->pages->sgl,
> .dma = sg_dma_address(iter.sg),
> .max = iter.dma + iter.sg->length,
> };
>
> drivers/gpu/drm/i915/i915_gem_gtt.c: In function ‘gen8_ppgtt_insert_4lvl’:
> drivers/gpu/drm/i915/i915_gem_gtt.c:938: error: ‘iter.sg’ is used uninitialized in this function
> drivers/gpu/drm/i915/i915_gem_gtt.c:939: error: ‘iter.dma’ is used uninitialized in this function
>
> and worse generates invalid code that triggers a GPF:
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
> IP: gen8_ppgtt_insert_4lvl+0x1b/0x1e0 [i915]
> PGD 0
>
> Oops: 0000 [#1] SMP
> Modules linked in: snd_aloop nf_conntrack_ipv6 nf_defrag_ipv6 nf_log_ipv6 ip6table_filter ip6_tables ctr ccm xt_state nf_log_ipv4
> nf_log_common xt_LOG xt_limit xt_recent xt_owner xt_addrtype iptable_filter ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat
> nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack libcrc32c ip_tables dm_mod vhost_net macvtap macvlan vhost tun kvm_intel kvm
> irqbypass uas usb_storage hid_multitouch btusb btrtl uvcvideo videobuf2_v4l2 videobuf2_core videodev media videobuf2_vmalloc videobuf2_memops
> sg ppdev dell_wmi sparse_keymap mei_wdt sd_mod iTCO_wdt iTCO_vendor_support rtsx_pci_ms memstick rtsx_pci_sdmmc mmc_core dell_smm_hwmon hwmon
> dell_laptop dell_smbios dcdbas joydev input_leds hci_uart btintel btqca btbcm bluetooth parport_pc parport i2c_hid
> intel_lpss_acpi intel_lpss pcspkr wmi int3400_thermal acpi_thermal_rel dell_rbtn mei_me mei snd_hda_codec_hdmi snd_hda_codec_realtek
> snd_hda_codec_generic ahci libahci acpi_pad xhci_pci xhci_hcd snd_hda_intel snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device
> snd_pcm snd_timer snd soundcore int3403_thermal arc4 e1000e ptp pps_core i2c_i801 iwlmvm mac80211 rtsx_pci iwlwifi cfg80211 rfkill
> intel_pch_thermal processor_thermal_device int340x_thermal_zone intel_soc_dts_iosf i915 video fjes
> CPU: 2 PID: 2408 Comm: X Not tainted 4.10.0-rc5+ #1
> Hardware name: Dell Inc. Latitude E7470/0T6HHJ, BIOS 1.11.3 11/09/2016
> task: ffff880219fe4740 task.stack: ffffc90005f98000
> RIP: 0010:gen8_ppgtt_insert_4lvl+0x1b/0x1e0 [i915]
> RSP: 0018:ffffc90005f9b8c8 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: ffff8802167d8000 RCX: 0000000000000001
> RDX: 00000000ffff7000 RSI: ffff880219f94140 RDI: ffff880228444000
> RBP: ffffc90005f9b948 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000080
> R13: 0000000000000001 R14: ffffc90005f9bcd7 R15: ffff88020c9a83c0
> FS: 00007fb53e1ee920(0000) GS:ffff88024dd00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000010 CR3: 000000022ef95000 CR4: 00000000003406e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> ppgtt_bind_vma+0x40/0x50 [i915]
> i915_vma_bind+0xcb/0x1c0 [i915]
> __i915_vma_do_pin+0x6e/0xd0 [i915]
> i915_gem_execbuffer_reserve_vma+0x162/0x1d0 [i915]
> i915_gem_execbuffer_reserve+0x4fc/0x510 [i915]
> ? __kmalloc+0x134/0x250
> ? i915_gem_wait_for_error+0x25/0x100 [i915]
> ? i915_gem_wait_for_error+0x25/0x100 [i915]
> i915_gem_do_execbuffer+0x2df/0xa00 [i915]
> ? drm_malloc_gfp.clone.0+0x42/0x80 [i915]
> ? path_put+0x22/0x30
> ? __check_object_size+0x62/0x1f0
> ? terminate_walk+0x44/0x90
> i915_gem_execbuffer2+0x95/0x1e0 [i915]
> drm_ioctl+0x243/0x490
> ? handle_pte_fault+0x1d7/0x220
> ? i915_gem_do_execbuffer+0xa00/0xa00 [i915]
> ? handle_mm_fault+0x10d/0x2a0
> vfs_ioctl+0x18/0x30
> do_vfs_ioctl+0x14b/0x3f0
> SyS_ioctl+0x92/0xa0
> entry_SYSCALL_64_fastpath+0x1a/0xa9
> RIP: 0033:0x7fb53b4fcb77
> RSP: 002b:00007ffe0c572898 EFLAGS: 00003246 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 00007fb53e17c038 RCX: 00007fb53b4fcb77
> RDX: 00007ffe0c572900 RSI: 0000000040406469 RDI: 000000000000000b
> RBP: 00007fb5376d67e0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000028 R11: 0000000000003246 R12: 0000000000000000
> R13: 0000000000000000 R14: 000055eecb314d00 R15: 000055eecb315460
> Code: 0f 84 5d ff ff ff eb a2 0f 1f 84 00 00 00 00 00 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 58 0f 1f 44 00 00 31 c0 89 4d b0 <4c>
> 8b 60 10 44 8b 70 0c 48 89 d0 4c 8b 2e 48 c1 e8 27 25 ff 01
> RIP: gen8_ppgtt_insert_4lvl+0x1b/0x1e0 [i915] RSP: ffffc90005f9b8c8
> CR2: 0000000000000010
>
> Recent gccs, such as 4.9, 6.3 or 7.2, do not generate the warning nor do
> they explode on use. If we manually create the struct using locals from
> the stack, this should eliminate this issue, and does not alter code
> generation with gcc-7.2.
>
> Fixes: 894ccebee2b0 ("drm/i915: Micro-optimise gen8_ppgtt_insert_entries()")
> Reported-by: Kelly French <kfrench@federalhill.net>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Kelly French <kfrench@federalhill.net>
> Cc: "Mika Kuoppala" <mika.kuoppala@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 25 +++++++++----------------
> 1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 0684d5df97d9..2847a6b41c16 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -958,10 +958,14 @@ static void gen8_ppgtt_clear_4lvl(struct i915_address_space *vm,
> }
> }
>
> -struct sgt_dma {
> +static inline struct sgt_dma {
> struct scatterlist *sg;
> dma_addr_t dma, max;
> -};
> +} sgt_dma(struct i915_vma *vma) {
> + struct scatterlist *sg = vma->pages->sgl;
> + dma_addr_t addr = sg_dma_address(sg);
> + return (struct sgt_dma) { sg, addr, addr + sg->length };
> +}
>
> struct gen8_insert_pte {
> u16 pml4e;
> @@ -1042,11 +1046,7 @@ static void gen8_ppgtt_insert_3lvl(struct i915_address_space *vm,
> u32 unused)
> {
> struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
> - struct sgt_dma iter = {
> - .sg = vma->pages->sgl,
> - .dma = sg_dma_address(iter.sg),
> - .max = iter.dma + iter.sg->length,
> - };
> + struct sgt_dma iter = sgt_dma(vma);
> struct gen8_insert_pte idx = gen8_insert_pte(vma->node.start);
>
> gen8_ppgtt_insert_pte_entries(ppgtt, &ppgtt->pdp, &iter, &idx,
> @@ -1158,11 +1158,7 @@ static void gen8_ppgtt_insert_4lvl(struct i915_address_space *vm,
> u32 unused)
> {
> struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
> - struct sgt_dma iter = {
> - .sg = vma->pages->sgl,
> - .dma = sg_dma_address(iter.sg),
> - .max = iter.dma + iter.sg->length,
> - };
> + struct sgt_dma iter = sgt_dma(vma);
> struct i915_page_directory_pointer **pdps = ppgtt->pml4.pdps;
>
> if (vma->page_sizes.sg > I915_GTT_PAGE_SIZE) {
> @@ -1869,13 +1865,10 @@ static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
> unsigned act_pt = first_entry / GEN6_PTES;
> unsigned act_pte = first_entry % GEN6_PTES;
> const u32 pte_encode = vm->pte_encode(0, cache_level, flags);
> - struct sgt_dma iter;
> + struct sgt_dma iter = sgt_dma(vma);
> gen6_pte_t *vaddr;
>
> vaddr = kmap_atomic_px(ppgtt->pd.page_table[act_pt]);
> - iter.sg = vma->pages->sgl;
> - iter.dma = sg_dma_address(iter.sg);
> - iter.max = iter.dma + iter.sg->length;
> do {
> vaddr[act_pte] = pte_encode | GEN6_PTE_ADDR_ENCODE(iter.dma);
>
>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/i915: Deconstruct struct sgt_dma initialiser
2017-11-06 21:11 ` [PATCH] drm/i915: Deconstruct struct sgt_dma initialiser Chris Wilson
2017-11-07 10:24 ` Tvrtko Ursulin
@ 2017-11-07 11:15 ` Mika Kuoppala
1 sibling, 0 replies; 4+ messages in thread
From: Mika Kuoppala @ 2017-11-07 11:15 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Kelly French
Chris Wilson <chris@chris-wilson.co.uk> writes:
> gcc-4.4 complains about:
>
> struct sgt_dma iter = {
> .sg = vma->pages->sgl,
> .dma = sg_dma_address(iter.sg),
> .max = iter.dma + iter.sg->length,
> };
>
> drivers/gpu/drm/i915/i915_gem_gtt.c: In function ‘gen8_ppgtt_insert_4lvl’:
> drivers/gpu/drm/i915/i915_gem_gtt.c:938: error: ‘iter.sg’ is used uninitialized in this function
> drivers/gpu/drm/i915/i915_gem_gtt.c:939: error: ‘iter.dma’ is used uninitialized in this function
>
> and worse generates invalid code that triggers a GPF:
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
> IP: gen8_ppgtt_insert_4lvl+0x1b/0x1e0 [i915]
> PGD 0
>
> Oops: 0000 [#1] SMP
> Modules linked in: snd_aloop nf_conntrack_ipv6 nf_defrag_ipv6 nf_log_ipv6 ip6table_filter ip6_tables ctr ccm xt_state nf_log_ipv4
> nf_log_common xt_LOG xt_limit xt_recent xt_owner xt_addrtype iptable_filter ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat
> nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack libcrc32c ip_tables dm_mod vhost_net macvtap macvlan vhost tun kvm_intel kvm
> irqbypass uas usb_storage hid_multitouch btusb btrtl uvcvideo videobuf2_v4l2 videobuf2_core videodev media videobuf2_vmalloc videobuf2_memops
> sg ppdev dell_wmi sparse_keymap mei_wdt sd_mod iTCO_wdt iTCO_vendor_support rtsx_pci_ms memstick rtsx_pci_sdmmc mmc_core dell_smm_hwmon hwmon
> dell_laptop dell_smbios dcdbas joydev input_leds hci_uart btintel btqca btbcm bluetooth parport_pc parport i2c_hid
> intel_lpss_acpi intel_lpss pcspkr wmi int3400_thermal acpi_thermal_rel dell_rbtn mei_me mei snd_hda_codec_hdmi snd_hda_codec_realtek
> snd_hda_codec_generic ahci libahci acpi_pad xhci_pci xhci_hcd snd_hda_intel snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device
> snd_pcm snd_timer snd soundcore int3403_thermal arc4 e1000e ptp pps_core i2c_i801 iwlmvm mac80211 rtsx_pci iwlwifi cfg80211 rfkill
> intel_pch_thermal processor_thermal_device int340x_thermal_zone intel_soc_dts_iosf i915 video fjes
> CPU: 2 PID: 2408 Comm: X Not tainted 4.10.0-rc5+ #1
> Hardware name: Dell Inc. Latitude E7470/0T6HHJ, BIOS 1.11.3 11/09/2016
> task: ffff880219fe4740 task.stack: ffffc90005f98000
> RIP: 0010:gen8_ppgtt_insert_4lvl+0x1b/0x1e0 [i915]
> RSP: 0018:ffffc90005f9b8c8 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: ffff8802167d8000 RCX: 0000000000000001
> RDX: 00000000ffff7000 RSI: ffff880219f94140 RDI: ffff880228444000
> RBP: ffffc90005f9b948 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000080
> R13: 0000000000000001 R14: ffffc90005f9bcd7 R15: ffff88020c9a83c0
> FS: 00007fb53e1ee920(0000) GS:ffff88024dd00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000010 CR3: 000000022ef95000 CR4: 00000000003406e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> ppgtt_bind_vma+0x40/0x50 [i915]
> i915_vma_bind+0xcb/0x1c0 [i915]
> __i915_vma_do_pin+0x6e/0xd0 [i915]
> i915_gem_execbuffer_reserve_vma+0x162/0x1d0 [i915]
> i915_gem_execbuffer_reserve+0x4fc/0x510 [i915]
> ? __kmalloc+0x134/0x250
> ? i915_gem_wait_for_error+0x25/0x100 [i915]
> ? i915_gem_wait_for_error+0x25/0x100 [i915]
> i915_gem_do_execbuffer+0x2df/0xa00 [i915]
> ? drm_malloc_gfp.clone.0+0x42/0x80 [i915]
> ? path_put+0x22/0x30
> ? __check_object_size+0x62/0x1f0
> ? terminate_walk+0x44/0x90
> i915_gem_execbuffer2+0x95/0x1e0 [i915]
> drm_ioctl+0x243/0x490
> ? handle_pte_fault+0x1d7/0x220
> ? i915_gem_do_execbuffer+0xa00/0xa00 [i915]
> ? handle_mm_fault+0x10d/0x2a0
> vfs_ioctl+0x18/0x30
> do_vfs_ioctl+0x14b/0x3f0
> SyS_ioctl+0x92/0xa0
> entry_SYSCALL_64_fastpath+0x1a/0xa9
> RIP: 0033:0x7fb53b4fcb77
> RSP: 002b:00007ffe0c572898 EFLAGS: 00003246 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 00007fb53e17c038 RCX: 00007fb53b4fcb77
> RDX: 00007ffe0c572900 RSI: 0000000040406469 RDI: 000000000000000b
> RBP: 00007fb5376d67e0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000028 R11: 0000000000003246 R12: 0000000000000000
> R13: 0000000000000000 R14: 000055eecb314d00 R15: 000055eecb315460
> Code: 0f 84 5d ff ff ff eb a2 0f 1f 84 00 00 00 00 00 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 58 0f 1f 44 00 00 31 c0 89 4d b0 <4c>
> 8b 60 10 44 8b 70 0c 48 89 d0 4c 8b 2e 48 c1 e8 27 25 ff 01
> RIP: gen8_ppgtt_insert_4lvl+0x1b/0x1e0 [i915] RSP: ffffc90005f9b8c8
> CR2: 0000000000000010
>
> Recent gccs, such as 4.9, 6.3 or 7.2, do not generate the warning nor do
> they explode on use. If we manually create the struct using locals from
> the stack, this should eliminate this issue, and does not alter code
> generation with gcc-7.2.
>
> Fixes: 894ccebee2b0 ("drm/i915: Micro-optimise gen8_ppgtt_insert_entries()")
> Reported-by: Kelly French <kfrench@federalhill.net>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Kelly French <kfrench@federalhill.net>
> Cc: "Mika Kuoppala" <mika.kuoppala@intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 25 +++++++++----------------
> 1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 0684d5df97d9..2847a6b41c16 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -958,10 +958,14 @@ static void gen8_ppgtt_clear_4lvl(struct i915_address_space *vm,
> }
> }
>
> -struct sgt_dma {
> +static inline struct sgt_dma {
> struct scatterlist *sg;
> dma_addr_t dma, max;
> -};
> +} sgt_dma(struct i915_vma *vma) {
> + struct scatterlist *sg = vma->pages->sgl;
> + dma_addr_t addr = sg_dma_address(sg);
> + return (struct sgt_dma) { sg, addr, addr + sg->length };
> +}
>
> struct gen8_insert_pte {
> u16 pml4e;
> @@ -1042,11 +1046,7 @@ static void gen8_ppgtt_insert_3lvl(struct i915_address_space *vm,
> u32 unused)
> {
> struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
> - struct sgt_dma iter = {
> - .sg = vma->pages->sgl,
> - .dma = sg_dma_address(iter.sg),
> - .max = iter.dma + iter.sg->length,
> - };
> + struct sgt_dma iter = sgt_dma(vma);
> struct gen8_insert_pte idx = gen8_insert_pte(vma->node.start);
>
> gen8_ppgtt_insert_pte_entries(ppgtt, &ppgtt->pdp, &iter, &idx,
> @@ -1158,11 +1158,7 @@ static void gen8_ppgtt_insert_4lvl(struct i915_address_space *vm,
> u32 unused)
> {
> struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
> - struct sgt_dma iter = {
> - .sg = vma->pages->sgl,
> - .dma = sg_dma_address(iter.sg),
> - .max = iter.dma + iter.sg->length,
> - };
> + struct sgt_dma iter = sgt_dma(vma);
> struct i915_page_directory_pointer **pdps = ppgtt->pml4.pdps;
>
> if (vma->page_sizes.sg > I915_GTT_PAGE_SIZE) {
> @@ -1869,13 +1865,10 @@ static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
> unsigned act_pt = first_entry / GEN6_PTES;
> unsigned act_pte = first_entry % GEN6_PTES;
> const u32 pte_encode = vm->pte_encode(0, cache_level, flags);
> - struct sgt_dma iter;
> + struct sgt_dma iter = sgt_dma(vma);
> gen6_pte_t *vaddr;
>
> vaddr = kmap_atomic_px(ppgtt->pd.page_table[act_pt]);
> - iter.sg = vma->pages->sgl;
> - iter.dma = sg_dma_address(iter.sg);
> - iter.max = iter.dma + iter.sg->length;
> do {
> vaddr[act_pte] = pte_encode | GEN6_PTE_ADDR_ENCODE(iter.dma);
>
> --
> 2.15.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread