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

[John Snow]

On 11/1/19 6:54 AM, Hervé Poussineau wrote:
> 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 <address@hidden>
>> Signed-off-by: Sven Schnelle <address@hidden>
>> ---
>>   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é

Thanks for the look, Hervé!

(Hey, do you want the Floppy device maintainership?)

--js