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 <hpous...@reactos.org>
>> Signed-off-by: Sven Schnelle <sv...@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é
Thanks for the look, Hervé!
(Hey, do you want the Floppy device maintainership?)
--js
