From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=49944 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PQfeL-0001aj-0o for qemu-devel@nongnu.org; Thu, 09 Dec 2010 07:31:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PQfeH-0005Kr-D4 for qemu-devel@nongnu.org; Thu, 09 Dec 2010 07:31:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:61772) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PQfeH-0005KV-5x for qemu-devel@nongnu.org; Thu, 09 Dec 2010 07:31:01 -0500 Message-ID: <4D00CC38.6010807@redhat.com> Date: Thu, 09 Dec 2010 13:31:52 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <1291810400-11309-1-git-send-email-agraf@suse.de> <1291810400-11309-4-git-send-email-agraf@suse.de> In-Reply-To: <1291810400-11309-4-git-send-email-agraf@suse.de> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 03/13] ide: Split out BMDMA code from ATA core List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Joerg Roedel , Paul Brook , QEMU-devel Developers , Blue Swirl , Gerd Hoffmann , Stefan Hajnoczi , tj@kernel.org, Roland Elek , Sebastian Herbszt Am 08.12.2010 13:13, schrieb Alexander Graf: > The ATA core is currently heavily intertwined with BMDMA code. Let's loosen > that a bit, so we can happily replace the DMA backend with different > implementations. > > Signed-off-by: Alexander Graf > > --- > > v7 -> v8: > > - rewrite as DMA ops > --- > hw/ide/cmd646.c | 6 +- > hw/ide/core.c | 322 ++++++++++++----------------------------------------- > hw/ide/internal.h | 53 +++++++-- > hw/ide/pci.c | 278 +++++++++++++++++++++++++++++++++++++++++++++- > hw/ide/pci.h | 1 + > hw/ide/piix.c | 6 +- > hw/ide/via.c | 6 +- > 7 files changed, 399 insertions(+), 273 deletions(-) > @@ -367,6 +369,17 @@ typedef enum { IDE_HD, IDE_CD, IDE_CFATA } IDEDriveKind; > > typedef void EndTransferFunc(IDEState *); > > + > +typedef void TransferStartFunc(IDEState *, > + uint8_t *, > + int, > + EndTransferFunc *); > +typedef void IRQSetFunc(IDEBus *); These two typedefs are unused. > +typedef void DMAStartFunc(void *, IDEState *, BlockDriverCompletionFunc *); > +typedef int DMAFunc(void *); > +typedef int DMAIntFunc(void *, int); > +typedef void DMARestartFunc(void *, int, int); > + > /* NOTE: IDEState represents in fact one drive */ > struct IDEState { > IDEBus *bus; > @@ -443,12 +456,33 @@ struct IDEState { > uint8_t *smart_selftest_data; > }; > > +struct IDEDMAOps { > + DMAFunc *start_irq; > + DMAStartFunc *start_dma; > + DMAFunc *start_transfer; > + DMAIntFunc *prepare_buf; > + DMAIntFunc *rw_buf; > + DMAIntFunc *set_unit; > + DMAIntFunc *set_status; > + DMAFunc *set_inactive; > + DMARestartFunc *restart_cb; > + DMAFunc *reset; > +}; > + > +struct IDEDMA { > + struct IDEDMAOps const *ops; Why hiding the const somewhere in the middle? > + void *opaque; > + struct iovec iov; > + QEMUIOVector qiov; > + BlockDriverAIOCB *aiocb; > +}; I'm wondering if this interface where you pass a void* to all DMA functions is really optimal. You completely lose type safety this way. Maybe we should use inheritance like in other places in qemu and implement BMDMAState with IDEDMA as its "base class"? This would mean that we need to make IDEBus.dma a pointer rather than embedding the structure, but it's probably worth the changes. > +static int bmdma_set_status(void *opaque, int status) > +{ > + BMDMAState *bm = opaque; > + bm->status |= status; The name of this function is misleading. You're just setting a flag, not setting a new value for the whole status register. Kevin