From 9e58f172700a3fc56fd6c97d033efa8ade51c9ce Mon Sep 17 00:00:00 2001 From: Sven Schnelle Date: Fri, 1 Nov 2019 17:55:13 +0100 Subject: [PATCH 1/5] fdc/i8257: implement verify transfer mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Sven Schnelle Reviewed-by: Hervé Poussineau --- hw/block/fdc.c | 61 +++++++++++++------------------------------- hw/dma/i8257.c | 20 ++++++++++----- include/hw/isa/isa.h | 1 - 3 files changed, 31 insertions(+), 51 deletions(-) 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; 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); From d6ef883d9d79f0f2f84df4c5e4d0d6c5eda79237 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 23 Mar 2020 15:17:15 +0000 Subject: [PATCH 2/5] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: John Snow Tested-by: BALATON Zoltan Message-id: 20200323151715.29454-1-peter.maydell@linaro.org [Maintainer edit: replace `DEVICE(dev)` by `ds` --js] Signed-off-by: John Snow --- 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]; From b93858756982ec76a36d4140765efad5ce0fbd5f Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Tue, 24 Mar 2020 21:05:17 +0000 Subject: [PATCH 3/5] via-ide: don't use PCI level for legacy IRQs The PCI level calculation was accidentally left in when rebasing from a previous patchset. Since both IRQs are driven separately, the value being passed into the IRQ handler should be used directly. Signed-off-by: Mark Cave-Ayland Message-id: 20200324210519.2974-2-mark.cave-ayland@ilande.co.uk Signed-off-by: John Snow --- hw/ide/via.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/ide/via.c b/hw/ide/via.c index 8de4945cc1..2a55b7fbc6 100644 --- a/hw/ide/via.c +++ b/hw/ide/via.c @@ -112,7 +112,6 @@ static void via_ide_set_irq(void *opaque, int n, int level) d->config[0x70 + n * 8] &= ~0x80; } - level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80); qemu_set_irq(isa_get_irq(NULL, 14 + n), level); } From 627a445ae0f0942a4cb95097204c7879722ce263 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Tue, 24 Mar 2020 21:05:18 +0000 Subject: [PATCH 4/5] via-ide: use qdev gpio rather than qemu_allocate_irqs() This prevents the memory from qemu_allocate_irqs() from being leaked which can in some cases be spotted by Coverity (CID 1421984). Signed-off-by: Mark Cave-Ayland Message-id: 20200324210519.2974-3-mark.cave-ayland@ilande.co.uk Signed-off-by: John Snow --- hw/ide/via.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/hw/ide/via.c b/hw/ide/via.c index 2a55b7fbc6..be09912b33 100644 --- a/hw/ide/via.c +++ b/hw/ide/via.c @@ -160,6 +160,7 @@ static void via_ide_reset(DeviceState *dev) static void via_ide_realize(PCIDevice *dev, Error **errp) { PCIIDEState *d = PCI_IDE(dev); + DeviceState *ds = DEVICE(dev); uint8_t *pci_conf = dev->config; int i; @@ -187,9 +188,10 @@ static void via_ide_realize(PCIDevice *dev, Error **errp) bmdma_setup_bar(d); pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); + qdev_init_gpio_in(ds, via_ide_set_irq, 2); for (i = 0; i < 2; i++) { - ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2); - ide_init2(&d->bus[i], qemu_allocate_irq(via_ide_set_irq, d, i)); + ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, 2); + ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i)); bmdma_init(&d->bus[i], &d->bmdma[i], d); d->bmdma[i].bus = &d->bus[i]; From cbf4c9ac9c000f7caf1bfee031041b62d2b000c8 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Tue, 24 Mar 2020 21:05:19 +0000 Subject: [PATCH 5/5] cmd646-ide: use qdev gpio rather than qemu_allocate_irqs() This prevents the memory from qemu_allocate_irqs() from being leaked which can in some cases be spotted by Coverity (CID 1421984). Signed-off-by: Mark Cave-Ayland Message-id: 20200324210519.2974-4-mark.cave-ayland@ilande.co.uk Signed-off-by: John Snow --- hw/ide/cmd646.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c index 699f25824d..c254631485 100644 --- a/hw/ide/cmd646.c +++ b/hw/ide/cmd646.c @@ -249,8 +249,8 @@ static void cmd646_pci_config_write(PCIDevice *d, uint32_t addr, uint32_t val, static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) { PCIIDEState *d = PCI_IDE(dev); + DeviceState *ds = DEVICE(dev); uint8_t *pci_conf = dev->config; - qemu_irq *irq; int i; pci_conf[PCI_CLASS_PROG] = 0x8f; @@ -291,16 +291,15 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) /* TODO: RST# value should be 0 */ pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1 - irq = qemu_allocate_irqs(cmd646_set_irq, d, 2); + qdev_init_gpio_in(ds, cmd646_set_irq, 2); for (i = 0; i < 2; i++) { - ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(dev), i, 2); - ide_init2(&d->bus[i], irq[i]); + ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, 2); + ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i)); bmdma_init(&d->bus[i], &d->bmdma[i], d); d->bmdma[i].bus = &d->bus[i]; ide_register_restart_cb(&d->bus[i]); } - g_free(irq); } static void pci_cmd646_ide_exitfn(PCIDevice *dev)