Re: [PATCH] fdc: support READ command with VERIFY DMA mode

[Hervé Poussineau]

Le 29/10/2019 à 12:00, John Snow a écrit :
>
>
> On 10/20/19 2:38 AM, Sven Schnelle wrote:
>> 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.
>>
>
> CC address@hidden, who sometimes submits patches here too.
>
>> Signed-off-by: Sven Schnelle <address@hidden>
>> ---
>>   hw/block/fdc.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index ac5d31e8c1..8a1228df78 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -1733,7 +1733,8 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int 
direction)
>>               dma_mode_ok = (dma_mode == ISADMA_TRANSFER_WRITE);
>>               break;
>>           case FD_DIR_READ:
>> -            dma_mode_ok = (dma_mode == ISADMA_TRANSFER_READ);
>> +            dma_mode_ok = (dma_mode == ISADMA_TRANSFER_READ) ||
>> +                          (dma_mode == ISADMA_TRANSFER_VERIFY);
>
> So we're enabling DMA when the command is an FD_DIR_READ command and the
> dma_mode is VERIFY. Those read commands are:
>
> READ
> READ TRACK
> READ DELETED DATA

OK for this part. However, in an ideal emulation world, the floppy drive 
controller shouldn't know
what is the current DMA mode.
I would remove this whole dma_mode_ok thing, and always assume that operating 
system does a sane thing.
Then, get_transfer_mode() callback and the ISADMA_TRANSFER_* defines can also 
be removed.

>
>>               break;
>>           case FD_DIR_VERIFY:
>>               dma_mode_ok = true;
>> @@ -1835,8 +1836,11 @@ static int fdctrl_transfer_handler (void *opaque, int 
nchan,
>>           switch (fdctrl->data_dir) {
>>           case FD_DIR_READ:
>>               /* READ commands */
>> -            k->write_memory(fdctrl->dma, nchan, fdctrl->fifo + rel_pos,
>> -                            fdctrl->data_pos, len);
>> +            if (k->get_transfer_mode(fdctrl->dma, fdctrl->dma_chann) !=
>> +                ISADMA_TRANSFER_VERIFY) {
>> +                k->write_memory(fdctrl->dma, nchan, fdctrl->fifo + rel_pos,
>> +                        fdctrl->data_pos, len);
>> +            }
>
> Would it horrify you to know I don't know how the VERIFY mode should
> work? It's always nice when you google i8257 to look for information and
> the top page of results are all QEMU patches.
>
> The i8257 spec says this:
>
> (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,
>
> Alright, looks good to me -- my question is if there aren't other
> commands where we want to give this same treatment, but then again...
> we've made it to 2019 without them, so...

It doesn't seem good to me, as it fixes VERIFY mode only for fdc.
Can you try to remove this part, and replace it by the following one (not 
tested) ?

--- a/hw/dma/i8257.c
+++ b/hw/dma/i8257.c
@@ -428,6 +428,11 @@ 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 (r->mode & 0xc0 == 0x00) {
+       /* VERIFY mode, do nothing */
+        return len;
+    }
+
     if (r->mode & 0x20) {
         int i;
         uint8_t *p = buf;

We may also fix i8257_dma_{read,write}_memory to correctly check for mode and 
refuse to
do anything if mode is wrong.

>
>>               break;
>>           case FD_DIR_WRITE:
>>               /* WRITE commands */
>>
>
> Reviewed-by: John Snow <address@hidden>
>
>

Hervé