[Qemu-devel] Re: [PATCH 03/13] ide: Split out BMDMA code from ATA core

[Kevin Wolf <kwolf@redhat.com>]

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 <agraf@suse.de>
> 
> ---
> 
> 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