All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Maydell <peter.maydell@linaro.org>
To: qemu-devel@nongnu.org
Cc: "Aleksandar Rikalo" <aleksandar.rikalo@syrmia.com>,
	qemu-block@nongnu.org,
	"Mark Cave-Ayland" <mark.cave-ayland@ilande.co.uk>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"Hervé Poussineau" <hpoussin@reactos.org>,
	"John Snow" <jsnow@redhat.com>
Subject: [PATCH for-7.1] hw/block/fdc-sysbus: Always mark sysbus floppy controllers as not having DMA
Date: Tue, 12 Apr 2022 17:49:21 +0100	[thread overview]
Message-ID: <20220412164921.1685453-1-peter.maydell@linaro.org> (raw)

The sysbus floppy controllers (devices sysbus-fdc and sun-fdtwo)
don't support DMA.  The core floppy controller code expects this to
be indicated by setting FDCtrl::dma_chann to -1.  This used to be
done in the device instance_init functions sysbus_fdc_initfn() and
sun4m_fdc_initfn(), but in commit 1430759ec3e we refactored this code
and accidentally lost the setting of dma_chann.

For sysbus-fdc this has no ill effects because we were redundantly
also setting dma_chann in fdctrl_init_sysbus(), but for sun-fdtwo
this means that guests which try to enable DMA on the floppy
controller will cause QEMU to crash because FDCtrl::dma is NULL.

Set dma_chann to -1 in the common instance init, and remove the
redundant code in fdctrl_init_sysbus() that is also setting it.

There is a six-year-old FIXME comment in the jazz board code to the
effect that in theory it should support doing DMA via a custom DMA
controller.  If anybody ever chooses to fix that they can do it by
adding support for setting both FDCtrl::dma_chann and FDCtrl::dma.
(A QOM link property 'dma-controller' on the sysbus device which can
be set to an instance of IsaDmaClass is probably the way to go.)

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/958
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/block/fdc.h |  3 +--
 hw/block/fdc-sysbus.c  | 14 +++++++++++---
 hw/mips/jazz.c         |  2 +-
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index 1ecca7cac7f..35248c08379 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -10,8 +10,7 @@
 #define TYPE_ISA_FDC "isa-fdc"
 
 void isa_fdc_init_drives(ISADevice *fdc, DriveInfo **fds);
-void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
-                        hwaddr mmio_base, DriveInfo **fds);
+void fdctrl_init_sysbus(qemu_irq irq, hwaddr mmio_base, DriveInfo **fds);
 void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
                        DriveInfo **fds, qemu_irq *fdc_tc);
 
diff --git a/hw/block/fdc-sysbus.c b/hw/block/fdc-sysbus.c
index 57fc8773f12..6c22c3510da 100644
--- a/hw/block/fdc-sysbus.c
+++ b/hw/block/fdc-sysbus.c
@@ -94,8 +94,7 @@ static void fdctrl_handle_tc(void *opaque, int irq, int level)
     trace_fdctrl_tc_pulse(level);
 }
 
-void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
-                        hwaddr mmio_base, DriveInfo **fds)
+void fdctrl_init_sysbus(qemu_irq irq, hwaddr mmio_base, DriveInfo **fds)
 {
     FDCtrl *fdctrl;
     DeviceState *dev;
@@ -105,7 +104,6 @@ void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
     dev = qdev_new("sysbus-fdc");
     sys = SYSBUS_FDC(dev);
     fdctrl = &sys->state;
-    fdctrl->dma_chann = dma_chann; /* FIXME */
     sbd = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(sbd, &error_fatal);
     sysbus_connect_irq(sbd, 0, irq);
@@ -138,6 +136,16 @@ static void sysbus_fdc_common_instance_init(Object *obj)
     FDCtrlSysBus *sys = SYSBUS_FDC(obj);
     FDCtrl *fdctrl = &sys->state;
 
+    /*
+     * DMA is not currently supported for sysbus floppy controllers.
+     * If we wanted to add support then probably the best approach is
+     * to have a QOM link property 'dma-controller' which the board
+     * code can set to an instance of IsaDmaClass, and an integer
+     * property 'dma-channel', so that we can set fdctrl->dma and
+     * fdctrl->dma_chann accordingly.
+     */
+    fdctrl->dma_chann = -1;
+
     qdev_set_legacy_instance_id(dev, 0 /* io */, 2); /* FIXME */
 
     memory_region_init_io(&fdctrl->iomem, obj,
diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index 44f0d48bfd7..14eaa517435 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -354,7 +354,7 @@ static void mips_jazz_init(MachineState *machine,
         fds[n] = drive_get(IF_FLOPPY, 0, n);
     }
     /* FIXME: we should enable DMA with a custom IsaDma device */
-    fdctrl_init_sysbus(qdev_get_gpio_in(rc4030, 1), -1, 0x80003000, fds);
+    fdctrl_init_sysbus(qdev_get_gpio_in(rc4030, 1), 0x80003000, fds);
 
     /* Real time clock */
     mc146818_rtc_init(isa_bus, 1980, NULL);
-- 
2.25.1



             reply	other threads:[~2022-04-12 16:50 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-12 16:49 Peter Maydell [this message]
2022-04-14 16:33 ` [PATCH for-7.1] hw/block/fdc-sysbus: Always mark sysbus floppy controllers as not having DMA Philippe Mathieu-Daudé
2022-04-14 17:03 ` Peter Maydell
2022-04-18  8:12 ` Mark Cave-Ayland

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220412164921.1685453-1-peter.maydell@linaro.org \
    --to=peter.maydell@linaro.org \
    --cc=aleksandar.rikalo@syrmia.com \
    --cc=f4bug@amsat.org \
    --cc=hpoussin@reactos.org \
    --cc=jsnow@redhat.com \
    --cc=mark.cave-ayland@ilande.co.uk \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.