[PULL 0/2] Ide patches

[PULL 0/2] Ide patches

[John Snow]

The following changes since commit 736cf607e40674776d752acc201f565723e86045:

  Update version for v5.0.0-rc0 release (2020-03-24 17:50:00 +0000)

are available in the Git repository at:

  https://github.com/jnsnow/qemu.git tags/ide-pull-request

for you to fetch changes up to 51058b3b3bcbe62506cf191fca1c0d679bb80f2b:

  hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs() (2020-03-24 15:52:16 -0400)

----------------------------------------------------------------
Pull request: IDE

Admittedly the first one is not a crisis fix; but I think it's low-risk to
include for rc1.

The second one is yours, and will shush coverity.

----------------------------------------------------------------

Peter Maydell (1):
  hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

Sven Schnelle (1):
  fdc/i8257: implement verify transfer mode

 include/hw/isa/isa.h |  1 -
 hw/block/fdc.c       | 61 +++++++++++++-------------------------------
 hw/dma/i8257.c       | 20 ++++++++++-----
 hw/ide/sii3112.c     |  8 +++---
 4 files changed, 35 insertions(+), 55 deletions(-)

-- 
2.21.1

[PULL 1/2] fdc/i8257: implement verify transfer mode

[John Snow]

From: Sven Schnelle <svens@stackframe.org>

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>
Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 include/hw/isa/isa.h |  1 -
 hw/block/fdc.c       | 61 +++++++++++++-------------------------------
 hw/dma/i8257.c       | 20 ++++++++++-----
 3 files changed, 31 insertions(+), 51 deletions(-)

diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index e9ac1f1205..59a4d4b50a 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);
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 22e954e0dc..33bc9e2f92 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1714,53 +1714,28 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
     }
     fdctrl->eot = fdctrl->fifo[6];
     if (fdctrl->dor & FD_DOR_DMAEN) {
-        IsaDmaTransferMode dma_mode;
+        /* DMA transfer is enabled. */
         IsaDmaClass *k = ISADMA_GET_CLASS(fdctrl->dma);
-        bool dma_mode_ok;
-        /* DMA transfer are enabled. Check if DMA channel is well programmed */
-        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]) *
+
+        FLOPPY_DPRINTF("direction=%d (%d - %d)\n",
+                       direction, (128 << fdctrl->fifo[5]) *
                        (cur_drv->last_sect - ks + 1), fdctrl->data_len);
-        switch (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;
-        }
-        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 ef15c06d77..1b3435ab58 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;
     device_class_set_props(dc, 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;
-- 
2.21.1

[PULL 2/2] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

[John Snow]

From: Peter Maydell <peter.maydell@linaro.org>

Coverity points out (CID 1421984) that we are leaking the
memory returned by qemu_allocate_irqs(). We can avoid this
leak by switching to using qdev_init_gpio_in(); the base
class finalize will free the irqs that this allocates under
the hood.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
Message-id: 20200323151715.29454-1-peter.maydell@linaro.org
[Maintainer edit: replace `DEVICE(dev)` by `ds` --js]
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/sii3112.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2..d69079c3d9 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp)
 {
     SiI3112PCIState *d = SII3112_PCI(dev);
     PCIIDEState *s = PCI_IDE(dev);
+    DeviceState *ds = DEVICE(dev);
     MemoryRegion *mr;
-    qemu_irq *irq;
     int i;
 
     pci_config_set_interrupt_pin(dev->config, 1);
@@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp)
     memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16);
     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
 
-    irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
+    qdev_init_gpio_in(ds, sii3112_set_irq, 2);
     for (i = 0; i < 2; i++) {
-        ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
-        ide_init2(&s->bus[i], irq[i]);
+        ide_bus_new(&s->bus[i], sizeof(s->bus[i]), ds, i, 1);
+        ide_init2(&s->bus[i], qdev_get_gpio_in(ds, i));
 
         bmdma_init(&s->bus[i], &s->bmdma[i], s);
         s->bmdma[i].bus = &s->bus[i];
-- 
2.21.1

Re: [PULL 0/2] Ide patches

[John Snow]

On 3/24/20 3:55 PM, John Snow wrote:
> The following changes since commit 736cf607e40674776d752acc201f565723e86045:
> 
>   Update version for v5.0.0-rc0 release (2020-03-24 17:50:00 +0000)
> 
> are available in the Git repository at:
> 
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
> 
> for you to fetch changes up to 51058b3b3bcbe62506cf191fca1c0d679bb80f2b:
> 
>   hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs() (2020-03-24 15:52:16 -0400)
> 
> ----------------------------------------------------------------
> Pull request: IDE
> 
> Admittedly the first one is not a crisis fix; but I think it's low-risk to
> include for rc1.
> 
> The second one is yours, and will shush coverity.
> 
> ----------------------------------------------------------------
> 
> Peter Maydell (1):
>   hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()
> 
> Sven Schnelle (1):
>   fdc/i8257: implement verify transfer mode
> 
>  include/hw/isa/isa.h |  1 -
>  hw/block/fdc.c       | 61 +++++++++++++-------------------------------
>  hw/dma/i8257.c       | 20 ++++++++++-----
>  hw/ide/sii3112.c     |  8 +++---
>  4 files changed, 35 insertions(+), 55 deletions(-)
> 

NACK. Mark Cave-Ayland is sending additional fixes.

--js