Re: [PATCH v2] fdc/i8257: implement verify transfer mode

["Hervé Poussineau" <hpoussin@reactos.org>]

Le 30/10/2019 à 09:28, Sven Schnelle a écrit :
> While working on the Tulip driver i tried to write some Teledisk images to
> a floppy image which didn't work. Turned out that Teledisk checks the written
> data by issuing a READ command to the FDC but running the DMA controller
> in VERIFY mode. As we ignored the DMA request in that case, the DMA transfer
> never finished, and Teledisk reported an error.
> 
> The i8257 spec says about verify transfers:
> 
> 3) DMA verify, which does not actually involve the transfer of data. When an
> 8257 channel is in the DMA verify mode, it will respond the same as described
> for transfer operations, except that no memory or I/O read/write control signals
> will be generated.
> 
> Hervé proposed to remove all the dma_mode_ok stuff from fdc to have a more
> clear boundary between DMA and FDC, so this patch also does that.
> 
> Suggested-by: Hervé Poussineau <hpoussin@reactos.org>
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>   hw/block/fdc.c       | 39 +++++++++++++++------------------------
>   hw/dma/i8257.c       | 20 +++++++++++++-------
>   include/hw/isa/isa.h |  1 -
>   3 files changed, 28 insertions(+), 32 deletions(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index ac5d31e8c1..18fd22bfb7 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1716,9 +1716,8 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
>       if (fdctrl->dor & FD_DOR_DMAEN) {
>           IsaDmaTransferMode dma_mode;

You need to remove this dma_mode variable because you don't set it anymore.

>           IsaDmaClass *k = ISADMA_GET_CLASS(fdctrl->dma);
> -        bool dma_mode_ok;
> +
>           /* DMA transfer are enabled. Check if DMA channel is well programmed */

Second part of this comment should be removed.

> -        dma_mode = k->get_transfer_mode(fdctrl->dma, fdctrl->dma_chann);
>           FLOPPY_DPRINTF("dma_mode=%d direction=%d (%d - %d)\n",
>                          dma_mode, direction,
>                          (128 << fdctrl->fifo[5]) *

You need to remove dma_mode variable from printf statement, as you removed the variable.

> @@ -1727,40 +1726,32 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
>           case FD_DIR_SCANE:
>           case FD_DIR_SCANL:
>           case FD_DIR_SCANH:
> -            dma_mode_ok = (dma_mode == ISADMA_TRANSFER_VERIFY);
>               break;
>           case FD_DIR_WRITE:
> -            dma_mode_ok = (dma_mode == ISADMA_TRANSFER_WRITE);
>               break;
>           case FD_DIR_READ:
> -            dma_mode_ok = (dma_mode == ISADMA_TRANSFER_READ);
>               break;
>           case FD_DIR_VERIFY:
> -            dma_mode_ok = true;
>               break;
>           default:
> -            dma_mode_ok = false;
>               break;
>           }

Now that you have removed the dma_mode_ok instructions, you have a switch where all cases do nothing.
Please completly remove the switch statement.

> -        if (dma_mode_ok) {
> -            /* No access is allowed until DMA transfer has completed */
> -            fdctrl->msr &= ~FD_MSR_RQM;
> -            if (direction != FD_DIR_VERIFY) {
> -                /* Now, we just have to wait for the DMA controller to
> -                 * recall us...
> -                 */
> -                k->hold_DREQ(fdctrl->dma, fdctrl->dma_chann);
> -                k->schedule(fdctrl->dma);
> -            } else {
> -                /* Start transfer */
> -                fdctrl_transfer_handler(fdctrl, fdctrl->dma_chann, 0,
> -                                        fdctrl->data_len);
> -            }
> -            return;
> +
> +        /* No access is allowed until DMA transfer has completed */
> +        fdctrl->msr &= ~FD_MSR_RQM;
> +        if (direction != FD_DIR_VERIFY) {
> +            /*
> +             * Now, we just have to wait for the DMA controller to
> +             * recall us...
> +             */
> +            k->hold_DREQ(fdctrl->dma, fdctrl->dma_chann);
> +            k->schedule(fdctrl->dma);
>           } else {
> -            FLOPPY_DPRINTF("bad dma_mode=%d direction=%d\n", dma_mode,
> -                           direction);
> +            /* Start transfer */
> +            fdctrl_transfer_handler(fdctrl, fdctrl->dma_chann, 0,
> +                    fdctrl->data_len);
>           }
> +        return;
>       }
>       FLOPPY_DPRINTF("start non-DMA transfer\n");
>       fdctrl->msr |= FD_MSR_NONDMA | FD_MSR_RQM;
> diff --git a/hw/dma/i8257.c b/hw/dma/i8257.c
> index 792f617eb4..85dad3d391 100644
> --- a/hw/dma/i8257.c
> +++ b/hw/dma/i8257.c
> @@ -292,12 +292,6 @@ static uint64_t i8257_read_cont(void *opaque, hwaddr nport, unsigned size)
>       return val;
>   }
>   
> -static IsaDmaTransferMode i8257_dma_get_transfer_mode(IsaDma *obj, int nchan)
> -{
> -    I8257State *d = I8257(obj);
> -    return (d->regs[nchan & 3].mode >> 2) & 3;
> -}
> -
>   static bool i8257_dma_has_autoinitialization(IsaDma *obj, int nchan)
>   {
>       I8257State *d = I8257(obj);
> @@ -400,6 +394,11 @@ static void i8257_dma_register_channel(IsaDma *obj, int nchan,
>       r->opaque = opaque;
>   }
>   
> +static bool i8257_is_verify_transfer(I8257Regs *r)
> +{
> +    return (r->mode & 0x0c) == 0;
> +}
> +
>   static int i8257_dma_read_memory(IsaDma *obj, int nchan, void *buf, int pos,
>                                    int len)
>   {
> @@ -407,6 +406,10 @@ static int i8257_dma_read_memory(IsaDma *obj, int nchan, void *buf, int pos,
>       I8257Regs *r = &d->regs[nchan & 3];
>       hwaddr addr = ((r->pageh & 0x7f) << 24) | (r->page << 16) | r->now[ADDR];
>   
> +    if (i8257_is_verify_transfer(r)) {
> +        return len;
> +    }
> +
>       if (r->mode & 0x20) {
>           int i;
>           uint8_t *p = buf;
> @@ -431,6 +434,10 @@ static int i8257_dma_write_memory(IsaDma *obj, int nchan, void *buf, int pos,
>       I8257Regs *r = &s->regs[nchan & 3];
>       hwaddr addr = ((r->pageh & 0x7f) << 24) | (r->page << 16) | r->now[ADDR];
>   
> +    if (i8257_is_verify_transfer(r)) {
> +        return len;
> +    }
> +
>       if (r->mode & 0x20) {
>           int i;
>           uint8_t *p = buf;
> @@ -597,7 +604,6 @@ static void i8257_class_init(ObjectClass *klass, void *data)
>       dc->vmsd = &vmstate_i8257;
>       dc->props = i8257_properties;
>   
> -    idc->get_transfer_mode = i8257_dma_get_transfer_mode;
>       idc->has_autoinitialization = i8257_dma_has_autoinitialization;
>       idc->read_memory = i8257_dma_read_memory;
>       idc->write_memory = i8257_dma_write_memory;
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index 018ada4f6f..f516e253c5 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -56,7 +56,6 @@ typedef int (*IsaDmaTransferHandler)(void *opaque, int nchan, int pos,
>   typedef struct IsaDmaClass {
>       InterfaceClass parent;
>   
> -    IsaDmaTransferMode (*get_transfer_mode)(IsaDma *obj, int nchan);
>       bool (*has_autoinitialization)(IsaDma *obj, int nchan);
>       int (*read_memory)(IsaDma *obj, int nchan, void *buf, int pos, int len);
>       int (*write_memory)(IsaDma *obj, int nchan, void *buf, int pos, int len);
> 

Otherwise, the i8257.c parts look good. This might fix some other devices (except fdc) which might use VERIFY mode.

Hervé