From patchwork Tue Oct 15 21:45:52 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pavel Roskin X-Patchwork-Id: 3050741 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 199F39F2B7 for ; Wed, 16 Oct 2013 07:07:05 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D8B0C2045B for ; Wed, 16 Oct 2013 07:07:03 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 79BF320456 for ; Wed, 16 Oct 2013 07:07:02 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 68901E78A2 for ; Wed, 16 Oct 2013 00:07:02 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from c60.cesmail.net (c60.cesmail.net [216.154.195.49]) by gabe.freedesktop.org (Postfix) with ESMTP id DBCD9E5D73 for ; Tue, 15 Oct 2013 14:45:57 -0700 (PDT) Received: from unknown (HELO smtprelay1.cesmail.net) ([192.168.1.111]) by c60.cesmail.net with ESMTP; 15 Oct 2013 17:45:56 -0400 Received: from IRBT4585 (206.83.81.178.ptr.us.xo.net [206.83.81.178]) by smtprelay1.cesmail.net (Postfix) with ESMTPSA id A98853496C; Tue, 15 Oct 2013 17:47:05 -0400 (EDT) Date: Tue, 15 Oct 2013 17:45:52 -0400 From: Pavel Roskin To: Chris Wilson Subject: Re: i915 driver fails on i686 userspace + x86_64 kernel Message-ID: <20131015174552.3c68055b@IRBT4585> In-Reply-To: <20131015205908.GI29463@nuc-i3427.alporthouse.com> References: <20131011223725.azyntmy4mc4wsgsg-cebfxv@webmail.spamcop.net> <20131012204437.GK8349@nuc-i3427.alporthouse.com> <20131013004010.53zcuugudc4g4s4o-cebfxv@webmail.spamcop.net> <20131013140004.GB6816@nuc-i3427.alporthouse.com> <20131015111224.3bc5eae7@IRBT4585> <20131015152350.GH29463@nuc-i3427.alporthouse.com> <20131015140050.0496aedd@IRBT4585> <20131015205908.GI29463@nuc-i3427.alporthouse.com> X-Mailer: Claws Mail 3.8.0 (GTK+ 2.24.10; i686-pc-linux-gnu) Mime-Version: 1.0 X-Mailman-Approved-At: Tue, 15 Oct 2013 18:52:06 -0700 Cc: dri-devel@lists.freedesktop.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, 15 Oct 2013 21:59:08 +0100 Chris Wilson wrote: > Yikes, that implies we have a size mismatch with the kernel - ideally > we construct the struct to have the same size when compiled with 32 > or 64 bits. > > Please try: > > commit a63b4d5a0766a7e98efeff8dd520c58e9a1bea88 > Author: Chris Wilson > Date: Tue Oct 15 21:53:16 2013 +0100 > > sna: Expand packed KMS structs for 64-bit alignment It helps. Xorg starts. However, I feel uneasy about that patch. I tried to debug the problem, and it looked not as a data corruption, but as something opposite. The kernel writes correct data to the userspace, but the userspace gets the old value. My theory is that the compat code incorrectly restores some data on the stack that should not be restored. It would happen to the value that is the closest to the stack pointer. gcc orders variables on the stack on its own, so additional arrays would only protect the enc structure only if gcc would place them below enc. The attached patch makes ioctl calls with enc and the 4 members of the encoderid array. encoderid[0] keeps the original value of 0x12345678. Other calls return the right value. The kernel can be changed to show the pointer address and read back the value with get_user. The value and the pointer address are correct. I tried Linux 3.8.12 and it made no difference. I also recompiled the kernel and xf86-video-intel with gcc 4.4.7 (the Ubuntu default is 4.6.3), and it also made no difference. It's hard to believe that there is a bug in the stack saving code that went unnoticed for so long, but I cannot think of anything else that would explain the results I'm observing. If the compiler gets the size of a certain structure wrong, why would the compat call restore any data on stack on return? I have an impression that your patch complicates things too much without addressing the actual problem. Protect array From: Pavel Roskin --- src/sna/sna_display.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/sna/sna_display.c b/src/sna/sna_display.c index 28151ab..27e429c 100644 --- a/src/sna/sna_display.c +++ b/src/sna/sna_display.c @@ -2588,7 +2588,8 @@ sna_output_init(ScrnInfoPtr scrn, struct sna_mode *mode, int num) struct sna *sna = to_sna(scrn); xf86OutputPtr output; struct drm_mode_get_connector conn; - struct drm_mode_get_encoder enc; + int encoderid[4]; + static struct drm_mode_get_encoder enc; struct drm_mode_modeinfo dummy; struct sna_output *sna_output; const char *output_name; @@ -2601,17 +2602,38 @@ sna_output_init(ScrnInfoPtr scrn, struct sna_mode *mode, int num) VG_CLEAR(conn); VG_CLEAR(enc); + DBG(("%s: &encoderid=%p\n", __FUNCTION__, &encoderid)); + DBG(("%s: &enc.encoder_id=%p\n", __FUNCTION__, &enc.encoder_id)); + conn.connector_id = mode->kmode->connectors[num]; conn.count_props = 0; conn.count_modes = 1; /* skip detect */ conn.modes_ptr = (uintptr_t)&dummy; conn.count_encoders = 1; conn.encoders_ptr = (uintptr_t)&enc.encoder_id; + enc.encoder_id = 0x12345678; if (drmIoctl(sna->kgem.fd, DRM_IOCTL_MODE_GETCONNECTOR, &conn)) { DBG(("%s: GETCONNECTOR failed, ret=%d\n", __FUNCTION__, errno)); return false; } + DBG(("%s: enc.encoder_id=%#x\n", __FUNCTION__, enc.encoder_id)); + + for (i = 0; i < 4; i++) { + conn.connector_id = mode->kmode->connectors[num]; + conn.count_props = 0; + conn.count_modes = 1; /* skip detect */ + conn.modes_ptr = (uintptr_t)&dummy; + conn.count_encoders = 1; + conn.encoders_ptr = (uintptr_t)&encoderid[i]; + encoderid[i] = 0x12345678; + + if (drmIoctl(sna->kgem.fd, DRM_IOCTL_MODE_GETCONNECTOR, &conn)) { + DBG(("%s: GETCONNECTOR failed, ret=%d\n", __FUNCTION__, errno)); + return false; + } + DBG(("%s: encoderid[%d]=%#x\n", __FUNCTION__, i, encoderid[i])); + } if (conn.count_encoders != 1) { DBG(("%s: unexpected number [%d] of encoders attached\n",