From patchwork Thu Aug 13 22:03:28 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Cave-Ayland X-Patchwork-Id: 507209 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 91C47140325 for ; Fri, 14 Aug 2015 08:04:17 +1000 (AEST) Received: from localhost ([::1]:44229 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZQ0bf-0007NQ-2o for incoming@patchwork.ozlabs.org; Thu, 13 Aug 2015 18:04:15 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33199) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZQ0bL-00076m-9k for qemu-devel@nongnu.org; Thu, 13 Aug 2015 18:03:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZQ0bG-0002wB-Kr for qemu-devel@nongnu.org; Thu, 13 Aug 2015 18:03:55 -0400 Received: from s16892447.onlinehome-server.info ([82.165.15.123]:46299) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZQ0bG-0002qW-AW for qemu-devel@nongnu.org; Thu, 13 Aug 2015 18:03:50 -0400 Received: from [2.219.48.20] (helo=[192.168.1.86]) by s16892447.onlinehome-server.info with esmtpsa (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1ZQ0b4-000716-1Q; Thu, 13 Aug 2015 23:03:41 +0100 Message-ID: <55CD1430.5060107@ilande.co.uk> Date: Thu, 13 Aug 2015 23:03:28 +0100 From: Mark Cave-Ayland User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.8.0 MIME-Version: 1.0 To: Aurelien Jarno References: <1438448070-32552-1-git-send-email-mark.cave-ayland@ilande.co.uk> <20150801183337.GA10456@aurel32.net> In-Reply-To: <20150801183337.GA10456@aurel32.net> X-SA-Exim-Connect-IP: 2.219.48.20 X-SA-Exim-Mail-From: mark.cave-ayland@ilande.co.uk X-SA-Exim-Version: 4.2.1 (built Sun, 08 Jan 2012 02:45:44 +0000) X-SA-Exim-Scanned: No (on s16892447.onlinehome-server.info); Unknown failure X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 82.165.15.123 Cc: jsnow@redhat.com, agraf@suse.de, qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [PATCHv2] macio: handle non-block ATAPI DMA transfers the same as block DMA transfers X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On 01/08/15 19:33, Aurelien Jarno wrote: > On 2015-08-01 17:54, Mark Cave-Ayland wrote: >> The existing code incorrectly changes the dma_active flag when a non-block >> transfer has completed leading to a hang on newer versions of Linux because the >> IDE and DMA engines deadlock waiting for each other. >> >> Instead copy the buffer directly to RAM, set the remaining transfer size to 0 and >> then invoke the ATAPI callback manually once again to correctly finish the >> transfer in an identical manner to a block transfer. >> >> Signed-off-by: Mark Cave-Ayland >> --- > > Thanks for the patch, it improves things here. I don't get messages > anymore, and I get less messages when mounting the CD-ROM, though I > still get one: > > [ 307.258463] pata-macio 0.00021000:ata-4: timeout flushing DMA > [ 307.262856] ata2.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x0 > [ 307.262919] ata2.00: BMDMA stat 0x6 > [ 307.263048] sr 1:0:0:0: CDB: Get configuration: 46 00 00 28 00 00 00 00 10 00 > [ 307.263289] ata2.00: cmd a0/01:00:00:10:00/00:00:00:00:00/a0 tag 0 dma 16400 in > [ 307.263297] res 41/50:03:00:10:00/00:00:00:00:00/a0 Emask 0x20 (host bus error) > [ 307.263407] ata2.00: status: { DRDY ERR } > [ 307.271251] ata2.00: configured for MWDMA2 > [ 307.271824] ata2: EH complete > > The CD-ROM is fully functional though. Apologies for the delay in getting back to you on this one, however I finally found some time today to try and work out what is happening. Attached is a v3 patch that works here for your lenny and wheezy standard images - please can you test and confirm it works for you? Debugging is enabled just in case anything unusual shows up during your testing. I've made a couple of minor tweaks to v2: firstly I don't call pmac_ide_atapi_transfer_cb() to go around again, since I was seeing the dbdma_end() function being called more than once when lba == -1. Secondly, I've moved the DBDMA flush out of the RUN status check section so it is called unconditionally, and removed the second version which was actually causing the problem above by not clearing the FLUSH bit upon write. FWIW with this patch applied I still see an error upon mount with your squeeze image but I believe that this may be a bug in that particular version of the Linux kernel. With MACIO and DBDMA logging enabled I see the following when trying to mount the CDROM: ------------ IDE transfer buffer_size: 14 buffer_index: 0 lba: ffffffff size: 14 ------------------------- DBDMA: DBDMA_run_bh DBDMA: writel 0x0000000000000d00 <= 0x80008000 DBDMA: channel 0x1a reg 0x0 DBDMA: status 0x00008400 DBDMA: readl 0x0000000000000d00 => 0x80008000 DBDMA: channel 0x1a reg 0x0 DBDMA: DBDMA_run_bh DBDMA: channel_run dbdma_cmd 0x252c328 req_count 0x0020 command 0x3000 phy_addr 0x0670e000 cmd_dep 0x00000000 res_count 0x0000 xfer_status 0x0000 DBDMA: start_input DBDMA: addr 0x670e000 key 0x0 pmac_ide_atapi_transfer_cb DBDMA: dbdma_end DBDMA: conditional_wait DBDMA: dbdma_cmdptr_save 0x05ef2000 DBDMA: xfer_status 0x00008400 res_count 0x0000 DBDMA: conditional_interrupt DBDMA: conditional_branch DBDMA: dbdma_cmdptr_load 0x05ef2010 DBDMA: channel_run dbdma_cmd 0x252c328 req_count 0x0000 command 0x7000 phy_addr 0x00000000 cmd_dep 0x00000000 res_count 0x0000 xfer_status 0x0000 DBDMA: readl 0x0000000000000d04 => 0x00008000 DBDMA: channel 0x1a reg 0x1 DBDMA: readl 0x0000000000000d04 => 0x00008000 DBDMA: channel 0x1a reg 0x1 DBDMA: writel 0x0000000000000d00 <= 0x98000000 DBDMA: channel 0x1a reg 0x0 DBDMA: status 0x00000000 DBDMA: writel 0x0000000000000d00 <= 0xf8000000 DBDMA: channel 0x1a reg 0x0 DBDMA: status 0x00000000 DBDMA: readl 0x0000000000000d04 => 0x00000000 DBDMA: channel 0x1a reg 0x1 DBDMA: writel 0x0000000000000d0c <= 0x05ef2000 DBDMA: channel 0x1a reg 0x3 DBDMA: dbdma_cmdptr_load 0x05ef2000 ------------ IDE transfer buffer_size: 1000 buffer_index: 0 lba: 21260 size: 1000 ------------------------- ..etc.. Tracing through the kernel source I can see that the DMA read error is being caused by pmac_ide_dma_end() returning a non-zero value: static int pmac_ide_dma_end (ide_drive_t *drive) { ide_hwif_t *hwif = drive->hwif; pmac_ide_hwif_t *pmif = (pmac_ide_hwif_t *)dev_get_drvdata(hwif->gendev.parent); volatile struct dbdma_regs __iomem *dma = pmif->dma_regs; u32 dstat; dstat = readl(&dma->status); writel(((RUN|WAKE|DEAD) << 16), &dma->control); /* verify good dma status. we don't check for ACTIVE beeing 0. We should... * in theory, but with ATAPI decices doing buffer underruns, that would * cause us to disable DMA, which isn't what we want */ return (dstat & (RUN|DEAD)) != RUN; } Right at the very end of the CDROM code the problem was being caused by the very last request (very similar to the initial request above) that looked like this: ------------ IDE transfer buffer_size: 14 buffer_index: 0 lba: ffffffff size: 14 ------------------------- DBDMA: writel 0x0000000000000d00 <= 0x80008000 DBDMA: channel 0x1a reg 0x0 DBDMA: status 0x00008400 DBDMA: readl 0x0000000000000d00 => 0x80008000 DBDMA: channel 0x1a reg 0x0 DBDMA: DBDMA_run_bh DBDMA: channel_run dbdma_cmd 0x252c328 req_count 0x0020 command 0x3000 phy_addr 0x05e10000 cmd_dep 0x00000000 res_count 0x0000 xfer_status 0x0000 DBDMA: start_input DBDMA: addr 0x5e10000 key 0x0 pmac_ide_atapi_transfer_cb DBDMA: dbdma_end DBDMA: conditional_wait DBDMA: dbdma_cmdptr_save 0x05ef2000 DBDMA: xfer_status 0x00008400 res_count 0x0000 DBDMA: conditional_interrupt DBDMA: conditional_branch DBDMA: dbdma_cmdptr_load 0x05ef2010 DBDMA: channel_run dbdma_cmd 0x252c328 req_count 0x0000 command 0x7000 phy_addr 0x00000000 cmd_dep 0x00000000 res_count 0x0000 xfer_status 0x0000 DBDMA: readl 0x0000000000000d04 => 0x00008000 DBDMA: channel 0x1a reg 0x1 DBDMA: readl 0x0000000000000d04 => 0x00008000 DBDMA: channel 0x1a reg 0x1 DBDMA: writel 0x0000000000000d00 <= 0x98000000 DBDMA: channel 0x1a reg 0x0 DBDMA: status 0x00000000 DBDMA: writel 0x0000000000000d00 <= 0xf8000000 DBDMA: channel 0x1a reg 0x0 DBDMA: status 0x00000000 DBDMA: readl 0x0000000000000d04 => 0x00000000 DBDMA: channel 0x1a reg 0x1 DBDMA: writel 0x0000000000000d0c <= 0x05ef2000 DBDMA: channel 0x1a reg 0x3 DBDMA: dbdma_cmdptr_load 0x05ef2000 DBDMA: readl 0x0000000000000d04 => 0x00000000 DBDMA: channel 0x1a reg 0x1 DBDMA: readl 0x0000000000000d04 => 0x00000000 ^^^^^^^^^^ DBDMA: channel 0x1a reg 0x1 DBDMA: writel 0x0000000000000d00 <= 0x98000000 DBDMA: channel 0x1a reg 0x0 DBDMA: status 0x00000000 DBDMA: writel 0x0000000000000b00 <= 0xf8000000 DBDMA: channel 0x16 reg 0x0 DBDMA: status 0x00000000 DBDMA: readl 0x0000000000000b04 => 0x00000000 DBDMA: channel 0x16 reg 0x1 DBDMA: writel 0x0000000000000b0c <= 0x05eea000 DBDMA: channel 0x16 reg 0x3 DBDMA: dbdma_cmdptr_load 0x05eea000 For some reason pmac_ide_dma_end() was being called without an active DMA request and so the RUN bit was clear on entry which was causing the error flag to be set. Given that this doesn't occur on lenny and wheezy, I'm inclined to believe that this is a bug in the squeeze kernel. Anyhow if this survives your testing then I'll repost as 2 separate patches with a CC to qemu-stable so that this also gets picked up in the next 2.4 release. ATB, Mark. Reviewed-by: Aurelien Jarno diff --git a/hw/ide/macio.c b/hw/ide/macio.c index 66ac2ba..1d29fb7 100644 --- a/hw/ide/macio.c +++ b/hw/ide/macio.c @@ -31,7 +31,7 @@ #include /* debug MACIO */ -// #define DEBUG_MACIO + #define DEBUG_MACIO #ifdef DEBUG_MACIO static const int debug_macio = 1; @@ -274,8 +274,11 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) /* Non-block ATAPI transfer - just copy to RAM */ s->io_buffer_size = MIN(s->io_buffer_size, io->len); cpu_physical_memory_write(io->addr, s->io_buffer, s->io_buffer_size); + + /* End of IDE transfer */ + s->io_buffer_size = 0; + io->len = 0; ide_atapi_cmd_ok(s); - m->dma_active = false; goto done; } diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c index b25e851..6a59dcd 100644 --- a/hw/misc/macio/mac_dbdma.c +++ b/hw/misc/macio/mac_dbdma.c @@ -42,7 +42,7 @@ #include "qemu/main-loop.h" /* debug DBDMA */ -//#define DEBUG_DBDMA +#define DEBUG_DBDMA #ifdef DEBUG_DBDMA #define DBDMA_DPRINTF(fmt, ...) \ @@ -590,12 +590,13 @@ dbdma_control_write(DBDMA_channel *ch) if ((ch->regs[DBDMA_STATUS] & RUN) && !(status & RUN)) { /* RUN is cleared */ status &= ~(ACTIVE|DEAD); - if ((status & FLUSH) && ch->flush) { - ch->flush(&ch->io); - status &= ~FLUSH; - } } + if ((status & FLUSH) && ch->flush) { + ch->flush(&ch->io); + status &= ~FLUSH; + } + DBDMA_DPRINTF(" status 0x%08x\n", status); ch->regs[DBDMA_STATUS] = status; @@ -603,9 +604,6 @@ dbdma_control_write(DBDMA_channel *ch) if (status & ACTIVE) { DBDMA_kick(dbdma_from_ch(ch)); } - if ((status & FLUSH) && ch->flush) { - ch->flush(&ch->io); - } } static void dbdma_write(void *opaque, hwaddr addr,