From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:41969) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RKt4j-0005yF-6C for qemu-devel@nongnu.org; Mon, 31 Oct 2011 10:42:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RKt4h-0005bm-P5 for qemu-devel@nongnu.org; Mon, 31 Oct 2011 10:42:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55607) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RKt4h-0005bg-FW for qemu-devel@nongnu.org; Mon, 31 Oct 2011 10:42:55 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p9VEgs0E032116 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 31 Oct 2011 10:42:54 -0400 Message-ID: <4EAEB4A9.2000203@redhat.com> Date: Mon, 31 Oct 2011 15:46:01 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <1319811501-6823-1-git-send-email-kwolf@redhat.com> <4EAAD9B4.7040206@redhat.com> In-Reply-To: <4EAAD9B4.7040206@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] dma: Avoid reentrancy in DMA transfer handlers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org Am 28.10.2011 18:35, schrieb Paolo Bonzini: > On 10/28/2011 04:18 PM, Kevin Wolf wrote: >> With the conversion of the block layer to coroutines, bdrv_read/write >> have changed to run a nested event loop that calls qemu_bh_poll. >> Consequently a scheduled BH can be called while a DMA transfer handler >> runs and this means that DMA_run becomes reentrant. >> >> Devices haven't been designed to cope with that, so instead of running a >> nested transfer handler just wait for the next invocation of the BH from the >> main loop. >> >> This fixes some problems with the floppy device. >> >> Signed-off-by: Kevin Wolf >> --- >> hw/dma.c | 9 +++++++++ >> 1 files changed, 9 insertions(+), 0 deletions(-) >> >> diff --git a/hw/dma.c b/hw/dma.c >> index 8a7302a..e8d6341 100644 >> --- a/hw/dma.c >> +++ b/hw/dma.c >> @@ -358,6 +358,13 @@ static void DMA_run (void) >> struct dma_cont *d; >> int icont, ichan; >> int rearm = 0; >> + static int running = 0; >> + >> + if (running) { >> + goto out; >> + } else { >> + running = 1; >> + } >> >> d = dma_controllers; >> >> @@ -374,6 +381,8 @@ static void DMA_run (void) >> } >> } >> >> +out: >> + running = 0; >> if (rearm) >> qemu_bh_schedule_idle(dma_bh); >> } > > Hmm, I think you should set rearm = 1 to ensure the BH is run when > ultimately you leave the sync read. Sorry for not spotting this before. I was about to agree, but in fact adding a rearm = 1; line leads to crashes, whereas in the version I posted it just works. So it looks like something is wrong with doing it, even though it seemed to make perfect sense at the first sight. Kevin