qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers
@ 2021-04-15 10:23 Philippe Mathieu-Daudé
  2021-04-15 10:23 ` [PATCH 1/4] hw/block/fdc: Replace disabled fprintf() by trace event Philippe Mathieu-Daudé
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-15 10:23 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Mark Cave-Ayland, Richard Henderson,
	Philippe Mathieu-Daudé,
	Max Reitz, Artyom Tarasenko, Paolo Bonzini, Miroslav Rezanina,
	Philippe Mathieu-Daudé,
	Aurelien Jarno

Hi,

The floppy disc controllers pulls in irrelevant devices (sysbus in
an ISA-only machine, ISA bus + isa devices on a sysbus-only machine).

This series clean that by extracting each device in its own file,
adding the corresponding Kconfig symbols: FDC_ISA and FDC_SYSBUS.

Regards,

Phil.

Philippe Mathieu-Daudé (4):
  hw/block/fdc: Replace disabled fprintf() by trace event
  hw/block/fdc: Declare shared prototypes in fdc-internal.h
  hw/block/fdc: Extract ISA floppy controllers to fdc-isa.c
  hw/block/fdc: Extract SysBus floppy controllers to fdc-sysbus.c

 hw/block/fdc-internal.h | 155 ++++++++++
 hw/block/fdc-isa.c      | 313 +++++++++++++++++++++
 hw/block/fdc-sysbus.c   | 252 +++++++++++++++++
 hw/block/fdc.c          | 608 +---------------------------------------
 MAINTAINERS             |   3 +
 hw/block/Kconfig        |   8 +
 hw/block/meson.build    |   2 +
 hw/block/trace-events   |   3 +
 hw/i386/Kconfig         |   2 +-
 hw/isa/Kconfig          |   6 +-
 hw/mips/Kconfig         |   2 +-
 hw/sparc/Kconfig        |   2 +-
 hw/sparc64/Kconfig      |   2 +-
 13 files changed, 751 insertions(+), 607 deletions(-)
 create mode 100644 hw/block/fdc-internal.h
 create mode 100644 hw/block/fdc-isa.c
 create mode 100644 hw/block/fdc-sysbus.c

-- 
2.26.3




^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/4] hw/block/fdc: Replace disabled fprintf() by trace event
  2021-04-15 10:23 [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers Philippe Mathieu-Daudé
@ 2021-04-15 10:23 ` Philippe Mathieu-Daudé
  2021-04-27 15:25   ` John Snow
  2021-04-15 10:23 ` [PATCH 2/4] hw/block/fdc: Declare shared prototypes in fdc-internal.h Philippe Mathieu-Daudé
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-15 10:23 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Mark Cave-Ayland, Richard Henderson,
	Philippe Mathieu-Daudé,
	Max Reitz, Artyom Tarasenko, Paolo Bonzini, Miroslav Rezanina,
	Philippe Mathieu-Daudé,
	Aurelien Jarno

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/fdc.c        | 7 +------
 hw/block/trace-events | 1 +
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a825c2acbae..1d3a0473678 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1242,12 +1242,7 @@ static void fdctrl_external_reset_isa(DeviceState *d)
 
 static void fdctrl_handle_tc(void *opaque, int irq, int level)
 {
-    //FDCtrl *s = opaque;
-
-    if (level) {
-        // XXX
-        FLOPPY_DPRINTF("TC pulsed\n");
-    }
+    trace_fdctrl_tc_pulse(level);
 }
 
 /* Change IRQ state */
diff --git a/hw/block/trace-events b/hw/block/trace-events
index fa12e3a67a7..306989c193c 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -3,6 +3,7 @@
 # fdc.c
 fdc_ioport_read(uint8_t reg, uint8_t value) "read reg 0x%02x val 0x%02x"
 fdc_ioport_write(uint8_t reg, uint8_t value) "write reg 0x%02x val 0x%02x"
+fdctrl_tc_pulse(int level) "TC pulse: %u"
 
 # pflash_cfi01.c
 # pflash_cfi02.c
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/4] hw/block/fdc: Declare shared prototypes in fdc-internal.h
  2021-04-15 10:23 [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers Philippe Mathieu-Daudé
  2021-04-15 10:23 ` [PATCH 1/4] hw/block/fdc: Replace disabled fprintf() by trace event Philippe Mathieu-Daudé
@ 2021-04-15 10:23 ` Philippe Mathieu-Daudé
  2021-04-27 16:53   ` John Snow
  2021-04-15 10:23 ` [PATCH 3/4] hw/block/fdc: Extract ISA floppy controllers to fdc-isa.c Philippe Mathieu-Daudé
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-15 10:23 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Mark Cave-Ayland, Richard Henderson,
	Philippe Mathieu-Daudé,
	Max Reitz, Artyom Tarasenko, Paolo Bonzini, Miroslav Rezanina,
	Philippe Mathieu-Daudé,
	Aurelien Jarno

We want to extract ISA/SysBus code from the generic fdc.c file.
First, declare the prototypes we will access from the new units
into a new local header: "fdc-internal.h".

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/fdc-internal.h | 155 ++++++++++++++++++++++++++++++++++++++++
 hw/block/fdc.c          | 126 +++-----------------------------
 MAINTAINERS             |   1 +
 3 files changed, 164 insertions(+), 118 deletions(-)
 create mode 100644 hw/block/fdc-internal.h

diff --git a/hw/block/fdc-internal.h b/hw/block/fdc-internal.h
new file mode 100644
index 00000000000..ddd41461ff3
--- /dev/null
+++ b/hw/block/fdc-internal.h
@@ -0,0 +1,155 @@
+/*
+ * QEMU Floppy disk emulator (Intel 82078)
+ *
+ * Copyright (c) 2003, 2007 Jocelyn Mayer
+ * Copyright (c) 2008 Hervé Poussineau
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#ifndef HW_BLOCK_FDC_INTERNAL_H
+#define HW_BLOCK_FDC_INTERNAL_H
+
+#include "exec/memory.h"
+#include "exec/ioport.h"
+#include "hw/block/block.h"
+#include "qapi/qapi-types-block.h"
+
+typedef struct FDCtrl FDCtrl;
+
+/* Floppy bus emulation */
+
+typedef struct FloppyBus {
+    BusState bus;
+    FDCtrl *fdc;
+} FloppyBus;
+
+/* Floppy disk drive emulation */
+
+typedef enum FDriveRate {
+    FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
+    FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
+    FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
+    FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
+} FDriveRate;
+
+typedef enum FDriveSize {
+    FDRIVE_SIZE_UNKNOWN,
+    FDRIVE_SIZE_350,
+    FDRIVE_SIZE_525,
+} FDriveSize;
+
+typedef struct FDFormat {
+    FloppyDriveType drive;
+    uint8_t last_sect;
+    uint8_t max_track;
+    uint8_t max_head;
+    FDriveRate rate;
+} FDFormat;
+
+typedef enum FDiskFlags {
+    FDISK_DBL_SIDES  = 0x01,
+} FDiskFlags;
+
+typedef struct FDrive {
+    FDCtrl *fdctrl;
+    BlockBackend *blk;
+    BlockConf *conf;
+    /* Drive status */
+    FloppyDriveType drive;    /* CMOS drive type        */
+    uint8_t perpendicular;    /* 2.88 MB access mode    */
+    /* Position */
+    uint8_t head;
+    uint8_t track;
+    uint8_t sect;
+    /* Media */
+    FloppyDriveType disk;     /* Current disk type      */
+    FDiskFlags flags;
+    uint8_t last_sect;        /* Nb sector per track    */
+    uint8_t max_track;        /* Nb of tracks           */
+    uint16_t bps;             /* Bytes per sector       */
+    uint8_t ro;               /* Is read-only           */
+    uint8_t media_changed;    /* Is media changed       */
+    uint8_t media_rate;       /* Data rate of medium    */
+
+    bool media_validated;     /* Have we validated the media? */
+} FDrive;
+
+struct FDCtrl {
+    MemoryRegion iomem;
+    qemu_irq irq;
+    /* Controller state */
+    QEMUTimer *result_timer;
+    int dma_chann;
+    uint8_t phase;
+    IsaDma *dma;
+    /* Controller's identification */
+    uint8_t version;
+    /* HW */
+    uint8_t sra;
+    uint8_t srb;
+    uint8_t dor;
+    uint8_t dor_vmstate; /* only used as temp during vmstate */
+    uint8_t tdr;
+    uint8_t dsr;
+    uint8_t msr;
+    uint8_t cur_drv;
+    uint8_t status0;
+    uint8_t status1;
+    uint8_t status2;
+    /* Command FIFO */
+    uint8_t *fifo;
+    int32_t fifo_size;
+    uint32_t data_pos;
+    uint32_t data_len;
+    uint8_t data_state;
+    uint8_t data_dir;
+    uint8_t eot; /* last wanted sector */
+    /* States kept only to be returned back */
+    /* precompensation */
+    uint8_t precomp_trk;
+    uint8_t config;
+    uint8_t lock;
+    /* Power down config (also with status regB access mode */
+    uint8_t pwrd;
+    /* Floppy drives */
+    FloppyBus bus;
+    uint8_t num_floppies;
+    FDrive drives[MAX_FD];
+    struct {
+        FloppyDriveType type;
+    } qdev_for_drives[MAX_FD];
+    int reset_sensei;
+    FloppyDriveType fallback; /* type=auto failure fallback */
+    /* Timers state */
+    uint8_t timer0;
+    uint8_t timer1;
+    PortioList portio_list;
+};
+
+extern const FDFormat fd_formats[];
+extern const VMStateDescription vmstate_fdc;
+
+uint32_t fdctrl_read(void *opaque, uint32_t reg);
+void fdctrl_write(void *opaque, uint32_t reg, uint32_t value);
+void fdctrl_reset(FDCtrl *fdctrl, int do_irq);
+void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl, Error **errp);
+
+void fdctrl_init_drives(FloppyBus *bus, DriveInfo **fds);
+
+#endif
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 1d3a0473678..300f39672af 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -48,6 +48,7 @@
 #include "qemu/module.h"
 #include "trace.h"
 #include "qom/object.h"
+#include "fdc-internal.h"
 
 /********************************************************/
 /* debug Floppy devices */
@@ -68,15 +69,8 @@
 #define TYPE_FLOPPY_BUS "floppy-bus"
 OBJECT_DECLARE_SIMPLE_TYPE(FloppyBus, FLOPPY_BUS)
 
-typedef struct FDCtrl FDCtrl;
-typedef struct FDrive FDrive;
 static FDrive *get_drv(FDCtrl *fdctrl, int unit);
 
-struct FloppyBus {
-    BusState bus;
-    FDCtrl *fdc;
-};
-
 static const TypeInfo floppy_bus_info = {
     .name = TYPE_FLOPPY_BUS,
     .parent = TYPE_BUS,
@@ -93,32 +87,11 @@ static void floppy_bus_create(FDCtrl *fdc, FloppyBus *bus, DeviceState *dev)
 /********************************************************/
 /* Floppy drive emulation                               */
 
-typedef enum FDriveRate {
-    FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
-    FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
-    FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
-    FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
-} FDriveRate;
-
-typedef enum FDriveSize {
-    FDRIVE_SIZE_UNKNOWN,
-    FDRIVE_SIZE_350,
-    FDRIVE_SIZE_525,
-} FDriveSize;
-
-typedef struct FDFormat {
-    FloppyDriveType drive;
-    uint8_t last_sect;
-    uint8_t max_track;
-    uint8_t max_head;
-    FDriveRate rate;
-} FDFormat;
-
 /* In many cases, the total sector size of a format is enough to uniquely
  * identify it. However, there are some total sector collisions between
  * formats of different physical size, and these are noted below by
  * highlighting the total sector size for entries with collisions. */
-static const FDFormat fd_formats[] = {
+const FDFormat fd_formats[] = {
     /* First entry is default format */
     /* 1.44 MB 3"1/2 floppy disks */
     { FLOPPY_DRIVE_TYPE_144, 18, 80, 1, FDRIVE_RATE_500K, }, /* 3.5" 2880 */
@@ -186,35 +159,6 @@ static FDriveSize drive_size(FloppyDriveType drive)
 #define FD_SECTOR_SC           2   /* Sector size code */
 #define FD_RESET_SENSEI_COUNT  4   /* Number of sense interrupts on RESET */
 
-/* Floppy disk drive emulation */
-typedef enum FDiskFlags {
-    FDISK_DBL_SIDES  = 0x01,
-} FDiskFlags;
-
-struct FDrive {
-    FDCtrl *fdctrl;
-    BlockBackend *blk;
-    BlockConf *conf;
-    /* Drive status */
-    FloppyDriveType drive;    /* CMOS drive type        */
-    uint8_t perpendicular;    /* 2.88 MB access mode    */
-    /* Position */
-    uint8_t head;
-    uint8_t track;
-    uint8_t sect;
-    /* Media */
-    FloppyDriveType disk;     /* Current disk type      */
-    FDiskFlags flags;
-    uint8_t last_sect;        /* Nb sector per track    */
-    uint8_t max_track;        /* Nb of tracks           */
-    uint16_t bps;             /* Bytes per sector       */
-    uint8_t ro;               /* Is read-only           */
-    uint8_t media_changed;    /* Is media changed       */
-    uint8_t media_rate;       /* Data rate of medium    */
-
-    bool media_validated;     /* Have we validated the media? */
-};
-
 
 static FloppyDriveType get_fallback_drive_type(FDrive *drv);
 
@@ -626,7 +570,6 @@ static const TypeInfo floppy_drive_info = {
 /********************************************************/
 /* Intel 82078 floppy disk controller emulation          */
 
-static void fdctrl_reset(FDCtrl *fdctrl, int do_irq);
 static void fdctrl_to_command_phase(FDCtrl *fdctrl);
 static int fdctrl_transfer_handler (void *opaque, int nchan,
                                     int dma_pos, int dma_len);
@@ -828,58 +771,6 @@ enum {
 #define FD_MULTI_TRACK(state) ((state) & FD_STATE_MULTI)
 #define FD_FORMAT_CMD(state) ((state) & FD_STATE_FORMAT)
 
-struct FDCtrl {
-    MemoryRegion iomem;
-    qemu_irq irq;
-    /* Controller state */
-    QEMUTimer *result_timer;
-    int dma_chann;
-    uint8_t phase;
-    IsaDma *dma;
-    /* Controller's identification */
-    uint8_t version;
-    /* HW */
-    uint8_t sra;
-    uint8_t srb;
-    uint8_t dor;
-    uint8_t dor_vmstate; /* only used as temp during vmstate */
-    uint8_t tdr;
-    uint8_t dsr;
-    uint8_t msr;
-    uint8_t cur_drv;
-    uint8_t status0;
-    uint8_t status1;
-    uint8_t status2;
-    /* Command FIFO */
-    uint8_t *fifo;
-    int32_t fifo_size;
-    uint32_t data_pos;
-    uint32_t data_len;
-    uint8_t data_state;
-    uint8_t data_dir;
-    uint8_t eot; /* last wanted sector */
-    /* States kept only to be returned back */
-    /* precompensation */
-    uint8_t precomp_trk;
-    uint8_t config;
-    uint8_t lock;
-    /* Power down config (also with status regB access mode */
-    uint8_t pwrd;
-    /* Floppy drives */
-    FloppyBus bus;
-    uint8_t num_floppies;
-    FDrive drives[MAX_FD];
-    struct {
-        FloppyDriveType type;
-    } qdev_for_drives[MAX_FD];
-    int reset_sensei;
-    FloppyDriveType fallback; /* type=auto failure fallback */
-    /* Timers state */
-    uint8_t timer0;
-    uint8_t timer1;
-    PortioList portio_list;
-};
-
 static FloppyDriveType get_fallback_drive_type(FDrive *drv)
 {
     return drv->fdctrl->fallback;
@@ -909,7 +800,7 @@ struct FDCtrlISABus {
     int32_t bootindexB;
 };
 
-static uint32_t fdctrl_read (void *opaque, uint32_t reg)
+uint32_t fdctrl_read(void *opaque, uint32_t reg)
 {
     FDCtrl *fdctrl = opaque;
     uint32_t retval;
@@ -946,7 +837,7 @@ static uint32_t fdctrl_read (void *opaque, uint32_t reg)
     return retval;
 }
 
-static void fdctrl_write (void *opaque, uint32_t reg, uint32_t value)
+void fdctrl_write(void *opaque, uint32_t reg, uint32_t value)
 {
     FDCtrl *fdctrl = opaque;
 
@@ -1178,7 +1069,7 @@ static const VMStateDescription vmstate_fdc_phase = {
     }
 };
 
-static const VMStateDescription vmstate_fdc = {
+const VMStateDescription vmstate_fdc = {
     .name = "fdc",
     .version_id = 2,
     .minimum_version_id = 2,
@@ -1268,7 +1159,7 @@ static void fdctrl_raise_irq(FDCtrl *fdctrl)
 }
 
 /* Reset controller */
-static void fdctrl_reset(FDCtrl *fdctrl, int do_irq)
+void fdctrl_reset(FDCtrl *fdctrl, int do_irq)
 {
     int i;
 
@@ -2484,7 +2375,7 @@ static void fdctrl_result_timer(void *opaque)
 
 /* Init functions */
 
-static void fdctrl_init_drives(FloppyBus *bus, DriveInfo **fds)
+void fdctrl_init_drives(FloppyBus *bus, DriveInfo **fds)
 {
     DeviceState *dev;
     int i;
@@ -2542,8 +2433,7 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
     fdctrl_init_drives(&sys->state.bus, fds);
 }
 
-static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl,
-                                  Error **errp)
+void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl, Error **errp)
 {
     int i, j;
     FDrive *drive;
diff --git a/MAINTAINERS b/MAINTAINERS
index 36055f14c59..20996f60e1f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1676,6 +1676,7 @@ M: John Snow <jsnow@redhat.com>
 L: qemu-block@nongnu.org
 S: Supported
 F: hw/block/fdc.c
+F: hw/block/fdc-internal.h
 F: include/hw/block/fdc.h
 F: tests/qtest/fdc-test.c
 T: git https://gitlab.com/jsnow/qemu.git ide
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/4] hw/block/fdc: Extract ISA floppy controllers to fdc-isa.c
  2021-04-15 10:23 [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers Philippe Mathieu-Daudé
  2021-04-15 10:23 ` [PATCH 1/4] hw/block/fdc: Replace disabled fprintf() by trace event Philippe Mathieu-Daudé
  2021-04-15 10:23 ` [PATCH 2/4] hw/block/fdc: Declare shared prototypes in fdc-internal.h Philippe Mathieu-Daudé
@ 2021-04-15 10:23 ` Philippe Mathieu-Daudé
  2021-04-27 17:11   ` John Snow
  2021-04-15 10:23 ` [PATCH 4/4] hw/block/fdc: Extract SysBus floppy controllers to fdc-sysbus.c Philippe Mathieu-Daudé
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-15 10:23 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Mark Cave-Ayland, Richard Henderson,
	Philippe Mathieu-Daudé,
	Max Reitz, Artyom Tarasenko, Paolo Bonzini, Miroslav Rezanina,
	Philippe Mathieu-Daudé,
	Aurelien Jarno

Some machines use floppy controllers via the SysBus interface,
and don't need to pull in all the ISA code.
Extract the ISA specific code to a new unit: fdc-isa.c, and
add a new Kconfig symbol: "FDC_ISA".

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/fdc-isa.c   | 313 +++++++++++++++++++++++++++++++++++++++++++
 hw/block/fdc.c       | 257 -----------------------------------
 MAINTAINERS          |   1 +
 hw/block/Kconfig     |   4 +
 hw/block/meson.build |   1 +
 hw/i386/Kconfig      |   2 +-
 hw/isa/Kconfig       |   6 +-
 hw/sparc64/Kconfig   |   2 +-
 8 files changed, 324 insertions(+), 262 deletions(-)
 create mode 100644 hw/block/fdc-isa.c

diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
new file mode 100644
index 00000000000..97f3f9e5c0a
--- /dev/null
+++ b/hw/block/fdc-isa.c
@@ -0,0 +1,313 @@
+/*
+ * QEMU Floppy disk emulator (Intel 82078)
+ *
+ * Copyright (c) 2003, 2007 Jocelyn Mayer
+ * Copyright (c) 2008 Hervé Poussineau
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+/*
+ * The controller is used in Sun4m systems in a slightly different
+ * way. There are changes in DOR register and DMA is not available.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/block/fdc.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qemu/timer.h"
+#include "hw/acpi/aml-build.h"
+#include "hw/irq.h"
+#include "hw/isa/isa.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
+#include "migration/vmstate.h"
+#include "hw/block/block.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/blockdev.h"
+#include "sysemu/sysemu.h"
+#include "qemu/log.h"
+#include "qemu/main-loop.h"
+#include "qemu/module.h"
+#include "trace.h"
+#include "qom/object.h"
+#include "fdc-internal.h"
+
+OBJECT_DECLARE_SIMPLE_TYPE(FDCtrlISABus, ISA_FDC)
+
+struct FDCtrlISABus {
+    ISADevice parent_obj;
+
+    uint32_t iobase;
+    uint32_t irq;
+    uint32_t dma;
+    struct FDCtrl state;
+    int32_t bootindexA;
+    int32_t bootindexB;
+};
+
+static void fdctrl_external_reset_isa(DeviceState *d)
+{
+    FDCtrlISABus *isa = ISA_FDC(d);
+    FDCtrl *s = &isa->state;
+
+    fdctrl_reset(s, 0);
+}
+
+void isa_fdc_init_drives(ISADevice *fdc, DriveInfo **fds)
+{
+    fdctrl_init_drives(&ISA_FDC(fdc)->state.bus, fds);
+}
+
+static const MemoryRegionPortio fdc_portio_list[] = {
+    { 1, 5, 1, .read = fdctrl_read, .write = fdctrl_write },
+    { 7, 1, 1, .read = fdctrl_read, .write = fdctrl_write },
+    PORTIO_END_OF_LIST(),
+};
+
+static void isabus_fdc_realize(DeviceState *dev, Error **errp)
+{
+    ISADevice *isadev = ISA_DEVICE(dev);
+    FDCtrlISABus *isa = ISA_FDC(dev);
+    FDCtrl *fdctrl = &isa->state;
+    Error *err = NULL;
+
+    isa_register_portio_list(isadev, &fdctrl->portio_list,
+                             isa->iobase, fdc_portio_list, fdctrl,
+                             "fdc");
+
+    isa_init_irq(isadev, &fdctrl->irq, isa->irq);
+    fdctrl->dma_chann = isa->dma;
+    if (fdctrl->dma_chann != -1) {
+        fdctrl->dma = isa_get_dma(isa_bus_from_device(isadev), isa->dma);
+        if (!fdctrl->dma) {
+            error_setg(errp, "ISA controller does not support DMA");
+            return;
+        }
+    }
+
+    qdev_set_legacy_instance_id(dev, isa->iobase, 2);
+
+    fdctrl_realize_common(dev, fdctrl, &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
+}
+
+FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
+{
+    FDCtrlISABus *isa = ISA_FDC(fdc);
+
+    return isa->state.drives[i].drive;
+}
+
+static void isa_fdc_get_drive_max_chs(FloppyDriveType type, uint8_t *maxc,
+                                      uint8_t *maxh, uint8_t *maxs)
+{
+    const FDFormat *fdf;
+
+    *maxc = *maxh = *maxs = 0;
+    for (fdf = fd_formats; fdf->drive != FLOPPY_DRIVE_TYPE_NONE; fdf++) {
+        if (fdf->drive != type) {
+            continue;
+        }
+        if (*maxc < fdf->max_track) {
+            *maxc = fdf->max_track;
+        }
+        if (*maxh < fdf->max_head) {
+            *maxh = fdf->max_head;
+        }
+        if (*maxs < fdf->last_sect) {
+            *maxs = fdf->last_sect;
+        }
+    }
+    (*maxc)--;
+}
+
+static Aml *build_fdinfo_aml(int idx, FloppyDriveType type)
+{
+    Aml *dev, *fdi;
+    uint8_t maxc, maxh, maxs;
+
+    isa_fdc_get_drive_max_chs(type, &maxc, &maxh, &maxs);
+
+    dev = aml_device("FLP%c", 'A' + idx);
+
+    aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
+
+    fdi = aml_package(16);
+    aml_append(fdi, aml_int(idx));  /* Drive Number */
+    aml_append(fdi,
+        aml_int(cmos_get_fd_drive_type(type)));  /* Device Type */
+    /*
+     * the values below are the limits of the drive, and are thus independent
+     * of the inserted media
+     */
+    aml_append(fdi, aml_int(maxc));  /* Maximum Cylinder Number */
+    aml_append(fdi, aml_int(maxs));  /* Maximum Sector Number */
+    aml_append(fdi, aml_int(maxh));  /* Maximum Head Number */
+    /*
+     * SeaBIOS returns the below values for int 0x13 func 0x08 regardless of
+     * the drive type, so shall we
+     */
+    aml_append(fdi, aml_int(0xAF));  /* disk_specify_1 */
+    aml_append(fdi, aml_int(0x02));  /* disk_specify_2 */
+    aml_append(fdi, aml_int(0x25));  /* disk_motor_wait */
+    aml_append(fdi, aml_int(0x02));  /* disk_sector_siz */
+    aml_append(fdi, aml_int(0x12));  /* disk_eot */
+    aml_append(fdi, aml_int(0x1B));  /* disk_rw_gap */
+    aml_append(fdi, aml_int(0xFF));  /* disk_dtl */
+    aml_append(fdi, aml_int(0x6C));  /* disk_formt_gap */
+    aml_append(fdi, aml_int(0xF6));  /* disk_fill */
+    aml_append(fdi, aml_int(0x0F));  /* disk_head_sttl */
+    aml_append(fdi, aml_int(0x08));  /* disk_motor_strt */
+
+    aml_append(dev, aml_name_decl("_FDI", fdi));
+    return dev;
+}
+
+int cmos_get_fd_drive_type(FloppyDriveType fd0)
+{
+    int val;
+
+    switch (fd0) {
+    case FLOPPY_DRIVE_TYPE_144:
+        /* 1.44 Mb 3"5 drive */
+        val = 4;
+        break;
+    case FLOPPY_DRIVE_TYPE_288:
+        /* 2.88 Mb 3"5 drive */
+        val = 5;
+        break;
+    case FLOPPY_DRIVE_TYPE_120:
+        /* 1.2 Mb 5"5 drive */
+        val = 2;
+        break;
+    case FLOPPY_DRIVE_TYPE_NONE:
+    default:
+        val = 0;
+        break;
+    }
+    return val;
+}
+
+static void fdc_isa_build_aml(ISADevice *isadev, Aml *scope)
+{
+    Aml *dev;
+    Aml *crs;
+    int i;
+
+#define ACPI_FDE_MAX_FD 4
+    uint32_t fde_buf[5] = {
+        0, 0, 0, 0,     /* presence of floppy drives #0 - #3 */
+        cpu_to_le32(2)  /* tape presence (2 == never present) */
+    };
+
+    crs = aml_resource_template();
+    aml_append(crs, aml_io(AML_DECODE16, 0x03F2, 0x03F2, 0x00, 0x04));
+    aml_append(crs, aml_io(AML_DECODE16, 0x03F7, 0x03F7, 0x00, 0x01));
+    aml_append(crs, aml_irq_no_flags(6));
+    aml_append(crs,
+        aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));
+
+    dev = aml_device("FDC0");
+    aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
+    aml_append(dev, aml_name_decl("_CRS", crs));
+
+    for (i = 0; i < MIN(MAX_FD, ACPI_FDE_MAX_FD); i++) {
+        FloppyDriveType type = isa_fdc_get_drive_type(isadev, i);
+
+        if (type < FLOPPY_DRIVE_TYPE_NONE) {
+            fde_buf[i] = cpu_to_le32(1);  /* drive present */
+            aml_append(dev, build_fdinfo_aml(i, type));
+        }
+    }
+    aml_append(dev, aml_name_decl("_FDE",
+               aml_buffer(sizeof(fde_buf), (uint8_t *)fde_buf)));
+
+    aml_append(scope, dev);
+}
+
+static const VMStateDescription vmstate_isa_fdc = {
+    .name = "fdc",
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT(state, FDCtrlISABus, 0, vmstate_fdc, FDCtrl),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property isa_fdc_properties[] = {
+    DEFINE_PROP_UINT32("iobase", FDCtrlISABus, iobase, 0x3f0),
+    DEFINE_PROP_UINT32("irq", FDCtrlISABus, irq, 6),
+    DEFINE_PROP_UINT32("dma", FDCtrlISABus, dma, 2),
+    DEFINE_PROP_SIGNED("fdtypeA", FDCtrlISABus, state.qdev_for_drives[0].type,
+                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
+    DEFINE_PROP_SIGNED("fdtypeB", FDCtrlISABus, state.qdev_for_drives[1].type,
+                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
+    DEFINE_PROP_SIGNED("fallback", FDCtrlISABus, state.fallback,
+                        FLOPPY_DRIVE_TYPE_288, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void isabus_fdc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
+
+    dc->realize = isabus_fdc_realize;
+    dc->fw_name = "fdc";
+    dc->reset = fdctrl_external_reset_isa;
+    dc->vmsd = &vmstate_isa_fdc;
+    isa->build_aml = fdc_isa_build_aml;
+    device_class_set_props(dc, isa_fdc_properties);
+    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+}
+
+static void isabus_fdc_instance_init(Object *obj)
+{
+    FDCtrlISABus *isa = ISA_FDC(obj);
+
+    device_add_bootindex_property(obj, &isa->bootindexA,
+                                  "bootindexA", "/floppy@0",
+                                  DEVICE(obj));
+    device_add_bootindex_property(obj, &isa->bootindexB,
+                                  "bootindexB", "/floppy@1",
+                                  DEVICE(obj));
+}
+
+static const TypeInfo isa_fdc_info = {
+    .name          = TYPE_ISA_FDC,
+    .parent        = TYPE_ISA_DEVICE,
+    .instance_size = sizeof(FDCtrlISABus),
+    .class_init    = isabus_fdc_class_init,
+    .instance_init = isabus_fdc_instance_init,
+};
+
+static void isa_fdc_register_types(void)
+{
+    type_register_static(&isa_fdc_info);
+}
+
+type_init(isa_fdc_register_types)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 300f39672af..50567d972ff 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -32,7 +32,6 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
-#include "hw/acpi/aml-build.h"
 #include "hw/irq.h"
 #include "hw/isa/isa.h"
 #include "hw/qdev-properties.h"
@@ -787,19 +786,6 @@ struct FDCtrlSysBus {
     struct FDCtrl state;
 };
 
-OBJECT_DECLARE_SIMPLE_TYPE(FDCtrlISABus, ISA_FDC)
-
-struct FDCtrlISABus {
-    ISADevice parent_obj;
-
-    uint32_t iobase;
-    uint32_t irq;
-    uint32_t dma;
-    struct FDCtrl state;
-    int32_t bootindexA;
-    int32_t bootindexB;
-};
-
 uint32_t fdctrl_read(void *opaque, uint32_t reg)
 {
     FDCtrl *fdctrl = opaque;
@@ -1123,14 +1109,6 @@ static void fdctrl_external_reset_sysbus(DeviceState *d)
     fdctrl_reset(s, 0);
 }
 
-static void fdctrl_external_reset_isa(DeviceState *d)
-{
-    FDCtrlISABus *isa = ISA_FDC(d);
-    FDCtrl *s = &isa->state;
-
-    fdctrl_reset(s, 0);
-}
-
 static void fdctrl_handle_tc(void *opaque, int irq, int level)
 {
     trace_fdctrl_tc_pulse(level);
@@ -2392,11 +2370,6 @@ void fdctrl_init_drives(FloppyBus *bus, DriveInfo **fds)
     }
 }
 
-void isa_fdc_init_drives(ISADevice *fdc, DriveInfo **fds)
-{
-    fdctrl_init_drives(&ISA_FDC(fdc)->state.bus, fds);
-}
-
 void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
                         hwaddr mmio_base, DriveInfo **fds)
 {
@@ -2485,41 +2458,6 @@ void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl, Error **errp)
     }
 }
 
-static const MemoryRegionPortio fdc_portio_list[] = {
-    { 1, 5, 1, .read = fdctrl_read, .write = fdctrl_write },
-    { 7, 1, 1, .read = fdctrl_read, .write = fdctrl_write },
-    PORTIO_END_OF_LIST(),
-};
-
-static void isabus_fdc_realize(DeviceState *dev, Error **errp)
-{
-    ISADevice *isadev = ISA_DEVICE(dev);
-    FDCtrlISABus *isa = ISA_FDC(dev);
-    FDCtrl *fdctrl = &isa->state;
-    Error *err = NULL;
-
-    isa_register_portio_list(isadev, &fdctrl->portio_list,
-                             isa->iobase, fdc_portio_list, fdctrl,
-                             "fdc");
-
-    isa_init_irq(isadev, &fdctrl->irq, isa->irq);
-    fdctrl->dma_chann = isa->dma;
-    if (fdctrl->dma_chann != -1) {
-        fdctrl->dma = isa_get_dma(isa_bus_from_device(isadev), isa->dma);
-        if (!fdctrl->dma) {
-            error_setg(errp, "ISA controller does not support DMA");
-            return;
-        }
-    }
-
-    qdev_set_legacy_instance_id(dev, isa->iobase, 2);
-    fdctrl_realize_common(dev, fdctrl, &err);
-    if (err != NULL) {
-        error_propagate(errp, err);
-        return;
-    }
-}
-
 static void sysbus_fdc_initfn(Object *obj)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
@@ -2567,200 +2505,6 @@ static void sysbus_fdc_common_realize(DeviceState *dev, Error **errp)
     fdctrl_realize_common(dev, fdctrl, errp);
 }
 
-FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
-{
-    FDCtrlISABus *isa = ISA_FDC(fdc);
-
-    return isa->state.drives[i].drive;
-}
-
-static void isa_fdc_get_drive_max_chs(FloppyDriveType type, uint8_t *maxc,
-                                      uint8_t *maxh, uint8_t *maxs)
-{
-    const FDFormat *fdf;
-
-    *maxc = *maxh = *maxs = 0;
-    for (fdf = fd_formats; fdf->drive != FLOPPY_DRIVE_TYPE_NONE; fdf++) {
-        if (fdf->drive != type) {
-            continue;
-        }
-        if (*maxc < fdf->max_track) {
-            *maxc = fdf->max_track;
-        }
-        if (*maxh < fdf->max_head) {
-            *maxh = fdf->max_head;
-        }
-        if (*maxs < fdf->last_sect) {
-            *maxs = fdf->last_sect;
-        }
-    }
-    (*maxc)--;
-}
-
-static Aml *build_fdinfo_aml(int idx, FloppyDriveType type)
-{
-    Aml *dev, *fdi;
-    uint8_t maxc, maxh, maxs;
-
-    isa_fdc_get_drive_max_chs(type, &maxc, &maxh, &maxs);
-
-    dev = aml_device("FLP%c", 'A' + idx);
-
-    aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
-
-    fdi = aml_package(16);
-    aml_append(fdi, aml_int(idx));  /* Drive Number */
-    aml_append(fdi,
-        aml_int(cmos_get_fd_drive_type(type)));  /* Device Type */
-    /*
-     * the values below are the limits of the drive, and are thus independent
-     * of the inserted media
-     */
-    aml_append(fdi, aml_int(maxc));  /* Maximum Cylinder Number */
-    aml_append(fdi, aml_int(maxs));  /* Maximum Sector Number */
-    aml_append(fdi, aml_int(maxh));  /* Maximum Head Number */
-    /*
-     * SeaBIOS returns the below values for int 0x13 func 0x08 regardless of
-     * the drive type, so shall we
-     */
-    aml_append(fdi, aml_int(0xAF));  /* disk_specify_1 */
-    aml_append(fdi, aml_int(0x02));  /* disk_specify_2 */
-    aml_append(fdi, aml_int(0x25));  /* disk_motor_wait */
-    aml_append(fdi, aml_int(0x02));  /* disk_sector_siz */
-    aml_append(fdi, aml_int(0x12));  /* disk_eot */
-    aml_append(fdi, aml_int(0x1B));  /* disk_rw_gap */
-    aml_append(fdi, aml_int(0xFF));  /* disk_dtl */
-    aml_append(fdi, aml_int(0x6C));  /* disk_formt_gap */
-    aml_append(fdi, aml_int(0xF6));  /* disk_fill */
-    aml_append(fdi, aml_int(0x0F));  /* disk_head_sttl */
-    aml_append(fdi, aml_int(0x08));  /* disk_motor_strt */
-
-    aml_append(dev, aml_name_decl("_FDI", fdi));
-    return dev;
-}
-
-int cmos_get_fd_drive_type(FloppyDriveType fd0)
-{
-    int val;
-
-    switch (fd0) {
-    case FLOPPY_DRIVE_TYPE_144:
-        /* 1.44 Mb 3"5 drive */
-        val = 4;
-        break;
-    case FLOPPY_DRIVE_TYPE_288:
-        /* 2.88 Mb 3"5 drive */
-        val = 5;
-        break;
-    case FLOPPY_DRIVE_TYPE_120:
-        /* 1.2 Mb 5"5 drive */
-        val = 2;
-        break;
-    case FLOPPY_DRIVE_TYPE_NONE:
-    default:
-        val = 0;
-        break;
-    }
-    return val;
-}
-
-static void fdc_isa_build_aml(ISADevice *isadev, Aml *scope)
-{
-    Aml *dev;
-    Aml *crs;
-    int i;
-
-#define ACPI_FDE_MAX_FD 4
-    uint32_t fde_buf[5] = {
-        0, 0, 0, 0,     /* presence of floppy drives #0 - #3 */
-        cpu_to_le32(2)  /* tape presence (2 == never present) */
-    };
-
-    crs = aml_resource_template();
-    aml_append(crs, aml_io(AML_DECODE16, 0x03F2, 0x03F2, 0x00, 0x04));
-    aml_append(crs, aml_io(AML_DECODE16, 0x03F7, 0x03F7, 0x00, 0x01));
-    aml_append(crs, aml_irq_no_flags(6));
-    aml_append(crs,
-        aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));
-
-    dev = aml_device("FDC0");
-    aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
-    aml_append(dev, aml_name_decl("_CRS", crs));
-
-    for (i = 0; i < MIN(MAX_FD, ACPI_FDE_MAX_FD); i++) {
-        FloppyDriveType type = isa_fdc_get_drive_type(isadev, i);
-
-        if (type < FLOPPY_DRIVE_TYPE_NONE) {
-            fde_buf[i] = cpu_to_le32(1);  /* drive present */
-            aml_append(dev, build_fdinfo_aml(i, type));
-        }
-    }
-    aml_append(dev, aml_name_decl("_FDE",
-               aml_buffer(sizeof(fde_buf), (uint8_t *)fde_buf)));
-
-    aml_append(scope, dev);
-}
-
-static const VMStateDescription vmstate_isa_fdc ={
-    .name = "fdc",
-    .version_id = 2,
-    .minimum_version_id = 2,
-    .fields = (VMStateField[]) {
-        VMSTATE_STRUCT(state, FDCtrlISABus, 0, vmstate_fdc, FDCtrl),
-        VMSTATE_END_OF_LIST()
-    }
-};
-
-static Property isa_fdc_properties[] = {
-    DEFINE_PROP_UINT32("iobase", FDCtrlISABus, iobase, 0x3f0),
-    DEFINE_PROP_UINT32("irq", FDCtrlISABus, irq, 6),
-    DEFINE_PROP_UINT32("dma", FDCtrlISABus, dma, 2),
-    DEFINE_PROP_SIGNED("fdtypeA", FDCtrlISABus, state.qdev_for_drives[0].type,
-                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
-                        FloppyDriveType),
-    DEFINE_PROP_SIGNED("fdtypeB", FDCtrlISABus, state.qdev_for_drives[1].type,
-                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
-                        FloppyDriveType),
-    DEFINE_PROP_SIGNED("fallback", FDCtrlISABus, state.fallback,
-                        FLOPPY_DRIVE_TYPE_288, qdev_prop_fdc_drive_type,
-                        FloppyDriveType),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void isabus_fdc_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
-
-    dc->realize = isabus_fdc_realize;
-    dc->fw_name = "fdc";
-    dc->reset = fdctrl_external_reset_isa;
-    dc->vmsd = &vmstate_isa_fdc;
-    isa->build_aml = fdc_isa_build_aml;
-    device_class_set_props(dc, isa_fdc_properties);
-    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-}
-
-static void isabus_fdc_instance_init(Object *obj)
-{
-    FDCtrlISABus *isa = ISA_FDC(obj);
-
-    device_add_bootindex_property(obj, &isa->bootindexA,
-                                  "bootindexA", "/floppy@0",
-                                  DEVICE(obj));
-    device_add_bootindex_property(obj, &isa->bootindexB,
-                                  "bootindexB", "/floppy@1",
-                                  DEVICE(obj));
-}
-
-static const TypeInfo isa_fdc_info = {
-    .name          = TYPE_ISA_FDC,
-    .parent        = TYPE_ISA_DEVICE,
-    .instance_size = sizeof(FDCtrlISABus),
-    .class_init    = isabus_fdc_class_init,
-    .instance_init = isabus_fdc_instance_init,
-};
-
 static const VMStateDescription vmstate_sysbus_fdc ={
     .name = "fdc",
     .version_id = 2,
@@ -2844,7 +2588,6 @@ static const TypeInfo sysbus_fdc_type_info = {
 
 static void fdc_register_types(void)
 {
-    type_register_static(&isa_fdc_info);
     type_register_static(&sysbus_fdc_type_info);
     type_register_static(&sysbus_fdc_info);
     type_register_static(&sun4m_fdc_info);
diff --git a/MAINTAINERS b/MAINTAINERS
index 20996f60e1f..e7ed334f586 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1677,6 +1677,7 @@ L: qemu-block@nongnu.org
 S: Supported
 F: hw/block/fdc.c
 F: hw/block/fdc-internal.h
+F: hw/block/fdc-isa.c
 F: include/hw/block/fdc.h
 F: tests/qtest/fdc-test.c
 T: git https://gitlab.com/jsnow/qemu.git ide
diff --git a/hw/block/Kconfig b/hw/block/Kconfig
index 4fcd1521668..0a2c046fa6c 100644
--- a/hw/block/Kconfig
+++ b/hw/block/Kconfig
@@ -1,5 +1,9 @@
 config FDC
     bool
+
+config FDC_ISA
+    bool
+    select FDC
     # FIXME: there is no separate file for the MMIO floppy disk controller, so
     # select ISA_BUS here instead of polluting each board that requires one
     select ISA_BUS
diff --git a/hw/block/meson.build b/hw/block/meson.build
index 5b4a7699f98..f33a665c945 100644
--- a/hw/block/meson.build
+++ b/hw/block/meson.build
@@ -5,6 +5,7 @@
 ))
 softmmu_ss.add(when: 'CONFIG_ECC', if_true: files('ecc.c'))
 softmmu_ss.add(when: 'CONFIG_FDC', if_true: files('fdc.c'))
+softmmu_ss.add(when: 'CONFIG_FDC_ISA', if_true: files('fdc-isa.c'))
 softmmu_ss.add(when: 'CONFIG_NAND', if_true: files('nand.c'))
 softmmu_ss.add(when: 'CONFIG_ONENAND', if_true: files('onenand.c'))
 softmmu_ss.add(when: 'CONFIG_PFLASH_CFI01', if_true: files('pflash_cfi01.c'))
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index 7f91f30877f..bb475648c97 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -23,7 +23,7 @@ config PC
     imply TPM_TIS_ISA
     imply VGA_PCI
     imply VIRTIO_VGA
-    select FDC
+    select FDC_ISA
     select I8259
     select I8254
     select PCKBD
diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index 55e0003ce40..cb1c5e40d2a 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -27,7 +27,7 @@ config PC87312
     select MC146818RTC
     select SERIAL_ISA
     select PARALLEL
-    select FDC
+    select FDC_ISA
     select IDE_ISA
 
 config PIIX3
@@ -46,7 +46,7 @@ config VT82C686
     select ISA_SUPERIO
     select ACPI_SMBUS
     select SERIAL_ISA
-    select FDC
+    select FDC_ISA
     select USB_UHCI
     select APM
 
@@ -55,7 +55,7 @@ config SMC37C669
     select ISA_SUPERIO
     select SERIAL_ISA
     select PARALLEL
-    select FDC
+    select FDC_ISA
 
 config LPC_ICH9
     bool
diff --git a/hw/sparc64/Kconfig b/hw/sparc64/Kconfig
index 980a201bb73..7e557ad17b0 100644
--- a/hw/sparc64/Kconfig
+++ b/hw/sparc64/Kconfig
@@ -6,7 +6,7 @@ config SUN4U
     imply PARALLEL
     select M48T59
     select ISA_BUS
-    select FDC
+    select FDC_ISA
     select SERIAL_ISA
     select PCI_SABRE
     select IDE_CMD646
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/4] hw/block/fdc: Extract SysBus floppy controllers to fdc-sysbus.c
  2021-04-15 10:23 [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  2021-04-15 10:23 ` [PATCH 3/4] hw/block/fdc: Extract ISA floppy controllers to fdc-isa.c Philippe Mathieu-Daudé
@ 2021-04-15 10:23 ` Philippe Mathieu-Daudé
  2021-04-27 17:34   ` John Snow
  2021-04-27 12:54 ` [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers Philippe Mathieu-Daudé
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-15 10:23 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Mark Cave-Ayland, Richard Henderson,
	Philippe Mathieu-Daudé,
	Max Reitz, Artyom Tarasenko, Paolo Bonzini, Miroslav Rezanina,
	Philippe Mathieu-Daudé,
	Aurelien Jarno

Some machines use floppy controllers via the SysBus interface,
and don't need to pull in all the SysBus code.
Extract the SysBus specific code to a new unit: fdc-sysbus.c,
and add a new Kconfig symbol: "FDC_SYSBUS".

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/fdc-sysbus.c | 252 ++++++++++++++++++++++++++++++++++++++++++
 hw/block/fdc.c        | 220 ------------------------------------
 MAINTAINERS           |   1 +
 hw/block/Kconfig      |   4 +
 hw/block/meson.build  |   1 +
 hw/block/trace-events |   2 +
 hw/mips/Kconfig       |   2 +-
 hw/sparc/Kconfig      |   2 +-
 8 files changed, 262 insertions(+), 222 deletions(-)
 create mode 100644 hw/block/fdc-sysbus.c

diff --git a/hw/block/fdc-sysbus.c b/hw/block/fdc-sysbus.c
new file mode 100644
index 00000000000..71755fd6ae4
--- /dev/null
+++ b/hw/block/fdc-sysbus.c
@@ -0,0 +1,252 @@
+/*
+ * QEMU Floppy disk emulator (Intel 82078)
+ *
+ * Copyright (c) 2003, 2007 Jocelyn Mayer
+ * Copyright (c) 2008 Hervé Poussineau
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qom/object.h"
+#include "hw/sysbus.h"
+#include "hw/block/fdc.h"
+#include "migration/vmstate.h"
+#include "fdc-internal.h"
+#include "trace.h"
+
+#define TYPE_SYSBUS_FDC "base-sysbus-fdc"
+typedef struct FDCtrlSysBusClass FDCtrlSysBusClass;
+typedef struct FDCtrlSysBus FDCtrlSysBus;
+DECLARE_OBJ_CHECKERS(FDCtrlSysBus, FDCtrlSysBusClass,
+                     SYSBUS_FDC, TYPE_SYSBUS_FDC)
+
+struct FDCtrlSysBusClass {
+    /*< private >*/
+    SysBusDeviceClass parent_class;
+    /*< public >*/
+
+    bool use_strict_io;
+};
+
+struct FDCtrlSysBus {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
+    struct FDCtrl state;
+};
+
+static uint64_t fdctrl_read_mem(void *opaque, hwaddr reg, unsigned ize)
+{
+    return fdctrl_read(opaque, (uint32_t)reg);
+}
+
+static void fdctrl_write_mem(void *opaque, hwaddr reg,
+                             uint64_t value, unsigned size)
+{
+    fdctrl_write(opaque, (uint32_t)reg, value);
+}
+
+static const MemoryRegionOps fdctrl_mem_ops = {
+    .read = fdctrl_read_mem,
+    .write = fdctrl_write_mem,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static const MemoryRegionOps fdctrl_mem_strict_ops = {
+    .read = fdctrl_read_mem,
+    .write = fdctrl_write_mem,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+static void fdctrl_external_reset_sysbus(DeviceState *d)
+{
+    FDCtrlSysBus *sys = SYSBUS_FDC(d);
+    FDCtrl *s = &sys->state;
+
+    fdctrl_reset(s, 0);
+}
+
+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)
+{
+    FDCtrl *fdctrl;
+    DeviceState *dev;
+    SysBusDevice *sbd;
+    FDCtrlSysBus *sys;
+
+    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);
+    sysbus_mmio_map(sbd, 0, mmio_base);
+
+    fdctrl_init_drives(&sys->state.bus, fds);
+}
+
+void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
+                       DriveInfo **fds, qemu_irq *fdc_tc)
+{
+    DeviceState *dev;
+    FDCtrlSysBus *sys;
+
+    dev = qdev_new("sun-fdtwo");
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sys = SYSBUS_FDC(dev);
+    sysbus_connect_irq(SYS_BUS_DEVICE(sys), 0, irq);
+    sysbus_mmio_map(SYS_BUS_DEVICE(sys), 0, io_base);
+    *fdc_tc = qdev_get_gpio_in(dev, 0);
+
+    fdctrl_init_drives(&sys->state.bus, fds);
+}
+
+static void sysbus_fdc_common_initfn(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+    FDCtrlSysBusClass *sbdc = SYSBUS_FDC_GET_CLASS(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    FDCtrlSysBus *sys = SYSBUS_FDC(obj);
+    FDCtrl *fdctrl = &sys->state;
+
+    fdctrl->dma_chann = -1;
+
+    qdev_set_legacy_instance_id(dev, 0 /* io */, 2); /* FIXME */
+
+    memory_region_init_io(&fdctrl->iomem, obj,
+                          sbdc->use_strict_io ? &fdctrl_mem_strict_ops
+                                              : &fdctrl_mem_ops,
+                          fdctrl, "fdc", 0x08);
+    sysbus_init_mmio(sbd, &fdctrl->iomem);
+
+    sysbus_init_irq(sbd, &fdctrl->irq);
+    qdev_init_gpio_in(dev, fdctrl_handle_tc, 1);
+}
+
+static void sysbus_fdc_common_realize(DeviceState *dev, Error **errp)
+{
+    FDCtrlSysBus *sys = SYSBUS_FDC(dev);
+    FDCtrl *fdctrl = &sys->state;
+
+    fdctrl_realize_common(dev, fdctrl, errp);
+}
+
+static const VMStateDescription vmstate_sysbus_fdc = {
+    .name = "fdc",
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT(state, FDCtrlSysBus, 0, vmstate_fdc, FDCtrl),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property sysbus_fdc_properties[] = {
+    DEFINE_PROP_SIGNED("fdtypeA", FDCtrlSysBus, state.qdev_for_drives[0].type,
+                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
+    DEFINE_PROP_SIGNED("fdtypeB", FDCtrlSysBus, state.qdev_for_drives[1].type,
+                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
+    DEFINE_PROP_SIGNED("fallback", FDCtrlSysBus, state.fallback,
+                        FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void sysbus_fdc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    device_class_set_props(dc, sysbus_fdc_properties);
+    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+}
+
+static const TypeInfo sysbus_fdc_info = {
+    .name          = "sysbus-fdc",
+    .parent        = TYPE_SYSBUS_FDC,
+    .class_init    = sysbus_fdc_class_init,
+};
+
+static void sysbus_fdc_common_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = sysbus_fdc_common_realize;
+    dc->reset = fdctrl_external_reset_sysbus;
+    dc->vmsd = &vmstate_sysbus_fdc;
+}
+
+static const TypeInfo sysbus_fdc_type_info = {
+    .name          = TYPE_SYSBUS_FDC,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(FDCtrlSysBus),
+    .instance_init = sysbus_fdc_common_initfn,
+    .abstract      = true,
+    .class_init    = sysbus_fdc_common_class_init,
+    .class_size    = sizeof(FDCtrlSysBusClass),
+};
+
+static Property sun4m_fdc_properties[] = {
+    DEFINE_PROP_SIGNED("fdtype", FDCtrlSysBus, state.qdev_for_drives[0].type,
+                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
+    DEFINE_PROP_SIGNED("fallback", FDCtrlSysBus, state.fallback,
+                        FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void sun4m_fdc_class_init(ObjectClass *klass, void *data)
+{
+    FDCtrlSysBusClass *sbdc = SYSBUS_FDC_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    sbdc->use_strict_io = true;
+    device_class_set_props(dc, sun4m_fdc_properties);
+    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+}
+
+static const TypeInfo sun4m_fdc_info = {
+    .name          = "sun-fdtwo",
+    .parent        = TYPE_SYSBUS_FDC,
+    .class_init    = sun4m_fdc_class_init,
+};
+
+static void sysbus_fdc_register_types(void)
+{
+    type_register_static(&sun4m_fdc_info);
+    type_register_static(&sysbus_fdc_type_info);
+    type_register_static(&sysbus_fdc_info);
+}
+
+type_init(sysbus_fdc_register_types)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 50567d972ff..64af4d194ce 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -36,7 +36,6 @@
 #include "hw/isa/isa.h"
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
-#include "hw/sysbus.h"
 #include "migration/vmstate.h"
 #include "hw/block/block.h"
 #include "sysemu/block-backend.h"
@@ -775,17 +774,6 @@ static FloppyDriveType get_fallback_drive_type(FDrive *drv)
     return drv->fdctrl->fallback;
 }
 
-#define TYPE_SYSBUS_FDC "base-sysbus-fdc"
-OBJECT_DECLARE_SIMPLE_TYPE(FDCtrlSysBus, SYSBUS_FDC)
-
-struct FDCtrlSysBus {
-    /*< private >*/
-    SysBusDevice parent_obj;
-    /*< public >*/
-
-    struct FDCtrl state;
-};
-
 uint32_t fdctrl_read(void *opaque, uint32_t reg)
 {
     FDCtrl *fdctrl = opaque;
@@ -850,34 +838,6 @@ void fdctrl_write(void *opaque, uint32_t reg, uint32_t value)
     }
 }
 
-static uint64_t fdctrl_read_mem (void *opaque, hwaddr reg,
-                                 unsigned ize)
-{
-    return fdctrl_read(opaque, (uint32_t)reg);
-}
-
-static void fdctrl_write_mem (void *opaque, hwaddr reg,
-                              uint64_t value, unsigned size)
-{
-    fdctrl_write(opaque, (uint32_t)reg, value);
-}
-
-static const MemoryRegionOps fdctrl_mem_ops = {
-    .read = fdctrl_read_mem,
-    .write = fdctrl_write_mem,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-};
-
-static const MemoryRegionOps fdctrl_mem_strict_ops = {
-    .read = fdctrl_read_mem,
-    .write = fdctrl_write_mem,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-    .valid = {
-        .min_access_size = 1,
-        .max_access_size = 1,
-    },
-};
-
 static bool fdrive_media_changed_needed(void *opaque)
 {
     FDrive *drive = opaque;
@@ -1101,19 +1061,6 @@ const VMStateDescription vmstate_fdc = {
     }
 };
 
-static void fdctrl_external_reset_sysbus(DeviceState *d)
-{
-    FDCtrlSysBus *sys = SYSBUS_FDC(d);
-    FDCtrl *s = &sys->state;
-
-    fdctrl_reset(s, 0);
-}
-
-static void fdctrl_handle_tc(void *opaque, int irq, int level)
-{
-    trace_fdctrl_tc_pulse(level);
-}
-
 /* Change IRQ state */
 static void fdctrl_reset_irq(FDCtrl *fdctrl)
 {
@@ -2370,42 +2317,6 @@ void fdctrl_init_drives(FloppyBus *bus, DriveInfo **fds)
     }
 }
 
-void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
-                        hwaddr mmio_base, DriveInfo **fds)
-{
-    FDCtrl *fdctrl;
-    DeviceState *dev;
-    SysBusDevice *sbd;
-    FDCtrlSysBus *sys;
-
-    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);
-    sysbus_mmio_map(sbd, 0, mmio_base);
-
-    fdctrl_init_drives(&sys->state.bus, fds);
-}
-
-void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
-                       DriveInfo **fds, qemu_irq *fdc_tc)
-{
-    DeviceState *dev;
-    FDCtrlSysBus *sys;
-
-    dev = qdev_new("sun-fdtwo");
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-    sys = SYSBUS_FDC(dev);
-    sysbus_connect_irq(SYS_BUS_DEVICE(sys), 0, irq);
-    sysbus_mmio_map(SYS_BUS_DEVICE(sys), 0, io_base);
-    *fdc_tc = qdev_get_gpio_in(dev, 0);
-
-    fdctrl_init_drives(&sys->state.bus, fds);
-}
-
 void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl, Error **errp)
 {
     int i, j;
@@ -2458,139 +2369,8 @@ void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl, Error **errp)
     }
 }
 
-static void sysbus_fdc_initfn(Object *obj)
-{
-    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
-    FDCtrlSysBus *sys = SYSBUS_FDC(obj);
-    FDCtrl *fdctrl = &sys->state;
-
-    fdctrl->dma_chann = -1;
-
-    memory_region_init_io(&fdctrl->iomem, obj, &fdctrl_mem_ops, fdctrl,
-                          "fdc", 0x08);
-    sysbus_init_mmio(sbd, &fdctrl->iomem);
-}
-
-static void sun4m_fdc_initfn(Object *obj)
-{
-    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
-    FDCtrlSysBus *sys = SYSBUS_FDC(obj);
-    FDCtrl *fdctrl = &sys->state;
-
-    fdctrl->dma_chann = -1;
-
-    memory_region_init_io(&fdctrl->iomem, obj, &fdctrl_mem_strict_ops,
-                          fdctrl, "fdctrl", 0x08);
-    sysbus_init_mmio(sbd, &fdctrl->iomem);
-}
-
-static void sysbus_fdc_common_initfn(Object *obj)
-{
-    DeviceState *dev = DEVICE(obj);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-    FDCtrlSysBus *sys = SYSBUS_FDC(obj);
-    FDCtrl *fdctrl = &sys->state;
-
-    qdev_set_legacy_instance_id(dev, 0 /* io */, 2); /* FIXME */
-
-    sysbus_init_irq(sbd, &fdctrl->irq);
-    qdev_init_gpio_in(dev, fdctrl_handle_tc, 1);
-}
-
-static void sysbus_fdc_common_realize(DeviceState *dev, Error **errp)
-{
-    FDCtrlSysBus *sys = SYSBUS_FDC(dev);
-    FDCtrl *fdctrl = &sys->state;
-
-    fdctrl_realize_common(dev, fdctrl, errp);
-}
-
-static const VMStateDescription vmstate_sysbus_fdc ={
-    .name = "fdc",
-    .version_id = 2,
-    .minimum_version_id = 2,
-    .fields = (VMStateField[]) {
-        VMSTATE_STRUCT(state, FDCtrlSysBus, 0, vmstate_fdc, FDCtrl),
-        VMSTATE_END_OF_LIST()
-    }
-};
-
-static Property sysbus_fdc_properties[] = {
-    DEFINE_PROP_SIGNED("fdtypeA", FDCtrlSysBus, state.qdev_for_drives[0].type,
-                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
-                        FloppyDriveType),
-    DEFINE_PROP_SIGNED("fdtypeB", FDCtrlSysBus, state.qdev_for_drives[1].type,
-                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
-                        FloppyDriveType),
-    DEFINE_PROP_SIGNED("fallback", FDCtrlSysBus, state.fallback,
-                        FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
-                        FloppyDriveType),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void sysbus_fdc_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-
-    device_class_set_props(dc, sysbus_fdc_properties);
-    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-}
-
-static const TypeInfo sysbus_fdc_info = {
-    .name          = "sysbus-fdc",
-    .parent        = TYPE_SYSBUS_FDC,
-    .instance_init = sysbus_fdc_initfn,
-    .class_init    = sysbus_fdc_class_init,
-};
-
-static Property sun4m_fdc_properties[] = {
-    DEFINE_PROP_SIGNED("fdtype", FDCtrlSysBus, state.qdev_for_drives[0].type,
-                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
-                        FloppyDriveType),
-    DEFINE_PROP_SIGNED("fallback", FDCtrlSysBus, state.fallback,
-                        FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
-                        FloppyDriveType),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void sun4m_fdc_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-
-    device_class_set_props(dc, sun4m_fdc_properties);
-    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-}
-
-static const TypeInfo sun4m_fdc_info = {
-    .name          = "sun-fdtwo",
-    .parent        = TYPE_SYSBUS_FDC,
-    .instance_init = sun4m_fdc_initfn,
-    .class_init    = sun4m_fdc_class_init,
-};
-
-static void sysbus_fdc_common_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-
-    dc->realize = sysbus_fdc_common_realize;
-    dc->reset = fdctrl_external_reset_sysbus;
-    dc->vmsd = &vmstate_sysbus_fdc;
-}
-
-static const TypeInfo sysbus_fdc_type_info = {
-    .name          = TYPE_SYSBUS_FDC,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(FDCtrlSysBus),
-    .instance_init = sysbus_fdc_common_initfn,
-    .abstract      = true,
-    .class_init    = sysbus_fdc_common_class_init,
-};
-
 static void fdc_register_types(void)
 {
-    type_register_static(&sysbus_fdc_type_info);
-    type_register_static(&sysbus_fdc_info);
-    type_register_static(&sun4m_fdc_info);
     type_register_static(&floppy_bus_info);
     type_register_static(&floppy_drive_info);
 }
diff --git a/MAINTAINERS b/MAINTAINERS
index e7ed334f586..0a908c22103 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1678,6 +1678,7 @@ S: Supported
 F: hw/block/fdc.c
 F: hw/block/fdc-internal.h
 F: hw/block/fdc-isa.c
+F: hw/block/fdc-sysbus.c
 F: include/hw/block/fdc.h
 F: tests/qtest/fdc-test.c
 T: git https://gitlab.com/jsnow/qemu.git ide
diff --git a/hw/block/Kconfig b/hw/block/Kconfig
index 0a2c046fa6c..d50be837666 100644
--- a/hw/block/Kconfig
+++ b/hw/block/Kconfig
@@ -8,6 +8,10 @@ config FDC_ISA
     # select ISA_BUS here instead of polluting each board that requires one
     select ISA_BUS
 
+config FDC_SYSBUS
+    bool
+    select FDC
+
 config SSI_M25P80
     bool
 
diff --git a/hw/block/meson.build b/hw/block/meson.build
index f33a665c945..c3935350485 100644
--- a/hw/block/meson.build
+++ b/hw/block/meson.build
@@ -6,6 +6,7 @@
 softmmu_ss.add(when: 'CONFIG_ECC', if_true: files('ecc.c'))
 softmmu_ss.add(when: 'CONFIG_FDC', if_true: files('fdc.c'))
 softmmu_ss.add(when: 'CONFIG_FDC_ISA', if_true: files('fdc-isa.c'))
+softmmu_ss.add(when: 'CONFIG_FDC_SYSBUS', if_true: files('fdc-sysbus.c'))
 softmmu_ss.add(when: 'CONFIG_NAND', if_true: files('nand.c'))
 softmmu_ss.add(when: 'CONFIG_ONENAND', if_true: files('onenand.c'))
 softmmu_ss.add(when: 'CONFIG_PFLASH_CFI01', if_true: files('pflash_cfi01.c'))
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 306989c193c..266b34393a3 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -3,6 +3,8 @@
 # fdc.c
 fdc_ioport_read(uint8_t reg, uint8_t value) "read reg 0x%02x val 0x%02x"
 fdc_ioport_write(uint8_t reg, uint8_t value) "write reg 0x%02x val 0x%02x"
+
+# fdc-sysbus.c
 fdctrl_tc_pulse(int level) "TC pulse: %u"
 
 # pflash_cfi01.c
diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
index aadd436bf4e..c245e881a2b 100644
--- a/hw/mips/Kconfig
+++ b/hw/mips/Kconfig
@@ -20,7 +20,7 @@ config JAZZ
     select G364FB
     select DP8393X
     select ESP
-    select FDC
+    select FDC_SYSBUS
     select MC146818RTC
     select PCKBD
     select SERIAL
diff --git a/hw/sparc/Kconfig b/hw/sparc/Kconfig
index 8dcb10086fd..79d58beb7a6 100644
--- a/hw/sparc/Kconfig
+++ b/hw/sparc/Kconfig
@@ -8,7 +8,7 @@ config SUN4M
     select UNIMP
     select ESCC
     select ESP
-    select FDC
+    select FDC_SYSBUS
     select SLAVIO
     select LANCE
     select M48T59
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers
  2021-04-15 10:23 [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers Philippe Mathieu-Daudé
                   ` (3 preceding siblings ...)
  2021-04-15 10:23 ` [PATCH 4/4] hw/block/fdc: Extract SysBus floppy controllers to fdc-sysbus.c Philippe Mathieu-Daudé
@ 2021-04-27 12:54 ` Philippe Mathieu-Daudé
  2021-04-27 17:38 ` John Snow
  2021-04-28  7:57 ` Mark Cave-Ayland
  6 siblings, 0 replies; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-27 12:54 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Mark Cave-Ayland, Richard Henderson,
	Max Reitz, Artyom Tarasenko, Paolo Bonzini, Miroslav Rezanina,
	Aurelien Jarno

ping?

On 4/15/21 12:23 PM, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> The floppy disc controllers pulls in irrelevant devices (sysbus in
> an ISA-only machine, ISA bus + isa devices on a sysbus-only machine).
> 
> This series clean that by extracting each device in its own file,
> adding the corresponding Kconfig symbols: FDC_ISA and FDC_SYSBUS.
> 
> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (4):
>   hw/block/fdc: Replace disabled fprintf() by trace event
>   hw/block/fdc: Declare shared prototypes in fdc-internal.h
>   hw/block/fdc: Extract ISA floppy controllers to fdc-isa.c
>   hw/block/fdc: Extract SysBus floppy controllers to fdc-sysbus.c
> 
>  hw/block/fdc-internal.h | 155 ++++++++++
>  hw/block/fdc-isa.c      | 313 +++++++++++++++++++++
>  hw/block/fdc-sysbus.c   | 252 +++++++++++++++++
>  hw/block/fdc.c          | 608 +---------------------------------------
>  MAINTAINERS             |   3 +
>  hw/block/Kconfig        |   8 +
>  hw/block/meson.build    |   2 +
>  hw/block/trace-events   |   3 +
>  hw/i386/Kconfig         |   2 +-
>  hw/isa/Kconfig          |   6 +-
>  hw/mips/Kconfig         |   2 +-
>  hw/sparc/Kconfig        |   2 +-
>  hw/sparc64/Kconfig      |   2 +-
>  13 files changed, 751 insertions(+), 607 deletions(-)
>  create mode 100644 hw/block/fdc-internal.h
>  create mode 100644 hw/block/fdc-isa.c
>  create mode 100644 hw/block/fdc-sysbus.c
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] hw/block/fdc: Replace disabled fprintf() by trace event
  2021-04-15 10:23 ` [PATCH 1/4] hw/block/fdc: Replace disabled fprintf() by trace event Philippe Mathieu-Daudé
@ 2021-04-27 15:25   ` John Snow
  0 siblings, 0 replies; 16+ messages in thread
From: John Snow @ 2021-04-27 15:25 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Mark Cave-Ayland, Richard Henderson,
	Philippe Mathieu-Daudé,
	Max Reitz, Artyom Tarasenko, Paolo Bonzini, Miroslav Rezanina,
	Aurelien Jarno

On 4/15/21 6:23 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   hw/block/fdc.c        | 7 +------
>   hw/block/trace-events | 1 +
>   2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index a825c2acbae..1d3a0473678 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1242,12 +1242,7 @@ static void fdctrl_external_reset_isa(DeviceState *d)
>   
>   static void fdctrl_handle_tc(void *opaque, int irq, int level)
>   {
> -    //FDCtrl *s = opaque;
> -
> -    if (level) {
> -        // XXX
> -        FLOPPY_DPRINTF("TC pulsed\n");
> -    }
> +    trace_fdctrl_tc_pulse(level);
>   }
>   

Do we need this function to fulfill some specific callback signature? 
... Ah, yeah, I guess for qdev_init_gpio_in. OK.

>   /* Change IRQ state */
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index fa12e3a67a7..306989c193c 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -3,6 +3,7 @@
>   # fdc.c
>   fdc_ioport_read(uint8_t reg, uint8_t value) "read reg 0x%02x val 0x%02x"
>   fdc_ioport_write(uint8_t reg, uint8_t value) "write reg 0x%02x val 0x%02x"
> +fdctrl_tc_pulse(int level) "TC pulse: %u"
>   
>   # pflash_cfi01.c
>   # pflash_cfi02.c
> 

Reviewed-by: John Snow <jsnow@redhat.com>



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/4] hw/block/fdc: Declare shared prototypes in fdc-internal.h
  2021-04-15 10:23 ` [PATCH 2/4] hw/block/fdc: Declare shared prototypes in fdc-internal.h Philippe Mathieu-Daudé
@ 2021-04-27 16:53   ` John Snow
  2021-04-27 17:56     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 16+ messages in thread
From: John Snow @ 2021-04-27 16:53 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Mark Cave-Ayland, Richard Henderson,
	Philippe Mathieu-Daudé,
	Max Reitz, Artyom Tarasenko, Paolo Bonzini, Miroslav Rezanina,
	Aurelien Jarno

On 4/15/21 6:23 AM, Philippe Mathieu-Daudé wrote:
> We want to extract ISA/SysBus code from the generic fdc.c file.
> First, declare the prototypes we will access from the new units
> into a new local header: "fdc-internal.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   hw/block/fdc-internal.h | 155 ++++++++++++++++++++++++++++++++++++++++
>   hw/block/fdc.c          | 126 +++-----------------------------
>   MAINTAINERS             |   1 +
>   3 files changed, 164 insertions(+), 118 deletions(-)
>   create mode 100644 hw/block/fdc-internal.h
> 

With our policy of not including osdep.h in headers, it's hard to verify 
that this header is otherwise self-sufficient.


I think the only thing it needs (not in osdep.h) happens to be MAX_FD. I 
added osdep.h just to test:

jsnow@scv ~/s/q/h/block (review)> gcc -I../../include/ -I../../bin/git 
-I/usr/lib64/glib-2.0/include -I/usr/include/glib-2.0 -c -o 
test_header.bin fdc-internal.h
fdc-internal.h:134:19: error: ‘MAX_FD’ undeclared here (not in a function)
   134 |     FDrive drives[MAX_FD];
       |                   ^~~~~~


Should we include the fdc header from the internal one?

> diff --git a/hw/block/fdc-internal.h b/hw/block/fdc-internal.h
> new file mode 100644
> index 00000000000..ddd41461ff3
> --- /dev/null
> +++ b/hw/block/fdc-internal.h
> @@ -0,0 +1,155 @@
> +/*
> + * QEMU Floppy disk emulator (Intel 82078)
> + *
> + * Copyright (c) 2003, 2007 Jocelyn Mayer
> + * Copyright (c) 2008 Hervé Poussineau
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#ifndef HW_BLOCK_FDC_INTERNAL_H
> +#define HW_BLOCK_FDC_INTERNAL_H
> +
> +#include "exec/memory.h"
> +#include "exec/ioport.h"
> +#include "hw/block/block.h"
> +#include "qapi/qapi-types-block.h"
> +
> +typedef struct FDCtrl FDCtrl;
> +
> +/* Floppy bus emulation */
> +
> +typedef struct FloppyBus {
> +    BusState bus;
> +    FDCtrl *fdc;
> +} FloppyBus;
> +
> +/* Floppy disk drive emulation */
> +
> +typedef enum FDriveRate {
> +    FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
> +    FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
> +    FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
> +    FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
> +} FDriveRate;
> +
> +typedef enum FDriveSize {
> +    FDRIVE_SIZE_UNKNOWN,
> +    FDRIVE_SIZE_350,
> +    FDRIVE_SIZE_525,
> +} FDriveSize;
> +
> +typedef struct FDFormat {
> +    FloppyDriveType drive;
> +    uint8_t last_sect;
> +    uint8_t max_track;
> +    uint8_t max_head;
> +    FDriveRate rate;
> +} FDFormat;
> +
> +typedef enum FDiskFlags {
> +    FDISK_DBL_SIDES  = 0x01,
> +} FDiskFlags;
> +
> +typedef struct FDrive {
> +    FDCtrl *fdctrl;
> +    BlockBackend *blk;
> +    BlockConf *conf;
> +    /* Drive status */
> +    FloppyDriveType drive;    /* CMOS drive type        */
> +    uint8_t perpendicular;    /* 2.88 MB access mode    */
> +    /* Position */
> +    uint8_t head;
> +    uint8_t track;
> +    uint8_t sect;
> +    /* Media */
> +    FloppyDriveType disk;     /* Current disk type      */
> +    FDiskFlags flags;
> +    uint8_t last_sect;        /* Nb sector per track    */
> +    uint8_t max_track;        /* Nb of tracks           */
> +    uint16_t bps;             /* Bytes per sector       */
> +    uint8_t ro;               /* Is read-only           */
> +    uint8_t media_changed;    /* Is media changed       */
> +    uint8_t media_rate;       /* Data rate of medium    */
> +
> +    bool media_validated;     /* Have we validated the media? */
> +} FDrive;
> +
> +struct FDCtrl {
> +    MemoryRegion iomem;
> +    qemu_irq irq;
> +    /* Controller state */
> +    QEMUTimer *result_timer;
> +    int dma_chann;
> +    uint8_t phase;
> +    IsaDma *dma;
> +    /* Controller's identification */
> +    uint8_t version;
> +    /* HW */
> +    uint8_t sra;
> +    uint8_t srb;
> +    uint8_t dor;
> +    uint8_t dor_vmstate; /* only used as temp during vmstate */
> +    uint8_t tdr;
> +    uint8_t dsr;
> +    uint8_t msr;
> +    uint8_t cur_drv;
> +    uint8_t status0;
> +    uint8_t status1;
> +    uint8_t status2;
> +    /* Command FIFO */
> +    uint8_t *fifo;
> +    int32_t fifo_size;
> +    uint32_t data_pos;
> +    uint32_t data_len;
> +    uint8_t data_state;
> +    uint8_t data_dir;
> +    uint8_t eot; /* last wanted sector */
> +    /* States kept only to be returned back */
> +    /* precompensation */
> +    uint8_t precomp_trk;
> +    uint8_t config;
> +    uint8_t lock;
> +    /* Power down config (also with status regB access mode */
> +    uint8_t pwrd;
> +    /* Floppy drives */
> +    FloppyBus bus;
> +    uint8_t num_floppies;
> +    FDrive drives[MAX_FD];
> +    struct {
> +        FloppyDriveType type;
> +    } qdev_for_drives[MAX_FD];
> +    int reset_sensei;
> +    FloppyDriveType fallback; /* type=auto failure fallback */
> +    /* Timers state */
> +    uint8_t timer0;
> +    uint8_t timer1;
> +    PortioList portio_list;
> +};
> +
> +extern const FDFormat fd_formats[];
> +extern const VMStateDescription vmstate_fdc;
> +
> +uint32_t fdctrl_read(void *opaque, uint32_t reg);
> +void fdctrl_write(void *opaque, uint32_t reg, uint32_t value);
> +void fdctrl_reset(FDCtrl *fdctrl, int do_irq);
> +void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl, Error **errp);
> +
> +void fdctrl_init_drives(FloppyBus *bus, DriveInfo **fds);
> +
> +#endif
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 1d3a0473678..300f39672af 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -48,6 +48,7 @@
>   #include "qemu/module.h"
>   #include "trace.h"
>   #include "qom/object.h"
> +#include "fdc-internal.h"
>   
>   /********************************************************/
>   /* debug Floppy devices */
> @@ -68,15 +69,8 @@
>   #define TYPE_FLOPPY_BUS "floppy-bus"
>   OBJECT_DECLARE_SIMPLE_TYPE(FloppyBus, FLOPPY_BUS)
>   
> -typedef struct FDCtrl FDCtrl;
> -typedef struct FDrive FDrive;
>   static FDrive *get_drv(FDCtrl *fdctrl, int unit);
>   
> -struct FloppyBus {
> -    BusState bus;
> -    FDCtrl *fdc;
> -};
> -
>   static const TypeInfo floppy_bus_info = {
>       .name = TYPE_FLOPPY_BUS,
>       .parent = TYPE_BUS,
> @@ -93,32 +87,11 @@ static void floppy_bus_create(FDCtrl *fdc, FloppyBus *bus, DeviceState *dev)
>   /********************************************************/
>   /* Floppy drive emulation                               */
>   
> -typedef enum FDriveRate {
> -    FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
> -    FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
> -    FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
> -    FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
> -} FDriveRate;
> -
> -typedef enum FDriveSize {
> -    FDRIVE_SIZE_UNKNOWN,
> -    FDRIVE_SIZE_350,
> -    FDRIVE_SIZE_525,
> -} FDriveSize;
> -
> -typedef struct FDFormat {
> -    FloppyDriveType drive;
> -    uint8_t last_sect;
> -    uint8_t max_track;
> -    uint8_t max_head;
> -    FDriveRate rate;
> -} FDFormat;
> -
>   /* In many cases, the total sector size of a format is enough to uniquely
>    * identify it. However, there are some total sector collisions between
>    * formats of different physical size, and these are noted below by
>    * highlighting the total sector size for entries with collisions. */
> -static const FDFormat fd_formats[] = {
> +const FDFormat fd_formats[] = {
>       /* First entry is default format */
>       /* 1.44 MB 3"1/2 floppy disks */
>       { FLOPPY_DRIVE_TYPE_144, 18, 80, 1, FDRIVE_RATE_500K, }, /* 3.5" 2880 */
> @@ -186,35 +159,6 @@ static FDriveSize drive_size(FloppyDriveType drive)
>   #define FD_SECTOR_SC           2   /* Sector size code */
>   #define FD_RESET_SENSEI_COUNT  4   /* Number of sense interrupts on RESET */
>   
> -/* Floppy disk drive emulation */
> -typedef enum FDiskFlags {
> -    FDISK_DBL_SIDES  = 0x01,
> -} FDiskFlags;
> -
> -struct FDrive {
> -    FDCtrl *fdctrl;
> -    BlockBackend *blk;
> -    BlockConf *conf;
> -    /* Drive status */
> -    FloppyDriveType drive;    /* CMOS drive type        */
> -    uint8_t perpendicular;    /* 2.88 MB access mode    */
> -    /* Position */
> -    uint8_t head;
> -    uint8_t track;
> -    uint8_t sect;
> -    /* Media */
> -    FloppyDriveType disk;     /* Current disk type      */
> -    FDiskFlags flags;
> -    uint8_t last_sect;        /* Nb sector per track    */
> -    uint8_t max_track;        /* Nb of tracks           */
> -    uint16_t bps;             /* Bytes per sector       */
> -    uint8_t ro;               /* Is read-only           */
> -    uint8_t media_changed;    /* Is media changed       */
> -    uint8_t media_rate;       /* Data rate of medium    */
> -
> -    bool media_validated;     /* Have we validated the media? */
> -};
> -
>   
>   static FloppyDriveType get_fallback_drive_type(FDrive *drv);
>   
> @@ -626,7 +570,6 @@ static const TypeInfo floppy_drive_info = {
>   /********************************************************/
>   /* Intel 82078 floppy disk controller emulation          */
>   
> -static void fdctrl_reset(FDCtrl *fdctrl, int do_irq);
>   static void fdctrl_to_command_phase(FDCtrl *fdctrl);
>   static int fdctrl_transfer_handler (void *opaque, int nchan,
>                                       int dma_pos, int dma_len);
> @@ -828,58 +771,6 @@ enum {
>   #define FD_MULTI_TRACK(state) ((state) & FD_STATE_MULTI)
>   #define FD_FORMAT_CMD(state) ((state) & FD_STATE_FORMAT)
>   
> -struct FDCtrl {
> -    MemoryRegion iomem;
> -    qemu_irq irq;
> -    /* Controller state */
> -    QEMUTimer *result_timer;
> -    int dma_chann;
> -    uint8_t phase;
> -    IsaDma *dma;
> -    /* Controller's identification */
> -    uint8_t version;
> -    /* HW */
> -    uint8_t sra;
> -    uint8_t srb;
> -    uint8_t dor;
> -    uint8_t dor_vmstate; /* only used as temp during vmstate */
> -    uint8_t tdr;
> -    uint8_t dsr;
> -    uint8_t msr;
> -    uint8_t cur_drv;
> -    uint8_t status0;
> -    uint8_t status1;
> -    uint8_t status2;
> -    /* Command FIFO */
> -    uint8_t *fifo;
> -    int32_t fifo_size;
> -    uint32_t data_pos;
> -    uint32_t data_len;
> -    uint8_t data_state;
> -    uint8_t data_dir;
> -    uint8_t eot; /* last wanted sector */
> -    /* States kept only to be returned back */
> -    /* precompensation */
> -    uint8_t precomp_trk;
> -    uint8_t config;
> -    uint8_t lock;
> -    /* Power down config (also with status regB access mode */
> -    uint8_t pwrd;
> -    /* Floppy drives */
> -    FloppyBus bus;
> -    uint8_t num_floppies;
> -    FDrive drives[MAX_FD];
> -    struct {
> -        FloppyDriveType type;
> -    } qdev_for_drives[MAX_FD];
> -    int reset_sensei;
> -    FloppyDriveType fallback; /* type=auto failure fallback */
> -    /* Timers state */
> -    uint8_t timer0;
> -    uint8_t timer1;
> -    PortioList portio_list;
> -};
> -
>   static FloppyDriveType get_fallback_drive_type(FDrive *drv)
>   {
>       return drv->fdctrl->fallback;
> @@ -909,7 +800,7 @@ struct FDCtrlISABus {
>       int32_t bootindexB;
>   };
>   
> -static uint32_t fdctrl_read (void *opaque, uint32_t reg)
> +uint32_t fdctrl_read(void *opaque, uint32_t reg)
>   {
>       FDCtrl *fdctrl = opaque;
>       uint32_t retval;
> @@ -946,7 +837,7 @@ static uint32_t fdctrl_read (void *opaque, uint32_t reg)
>       return retval;
>   }
>   
> -static void fdctrl_write (void *opaque, uint32_t reg, uint32_t value)
> +void fdctrl_write(void *opaque, uint32_t reg, uint32_t value)
>   {
>       FDCtrl *fdctrl = opaque;
>   
> @@ -1178,7 +1069,7 @@ static const VMStateDescription vmstate_fdc_phase = {
>       }
>   };
>   
> -static const VMStateDescription vmstate_fdc = {
> +const VMStateDescription vmstate_fdc = {
>       .name = "fdc",
>       .version_id = 2,
>       .minimum_version_id = 2,
> @@ -1268,7 +1159,7 @@ static void fdctrl_raise_irq(FDCtrl *fdctrl)
>   }
>   
>   /* Reset controller */
> -static void fdctrl_reset(FDCtrl *fdctrl, int do_irq)
> +void fdctrl_reset(FDCtrl *fdctrl, int do_irq)
>   {
>       int i;
>   
> @@ -2484,7 +2375,7 @@ static void fdctrl_result_timer(void *opaque)
>   
>   /* Init functions */
>   
> -static void fdctrl_init_drives(FloppyBus *bus, DriveInfo **fds)
> +void fdctrl_init_drives(FloppyBus *bus, DriveInfo **fds)
>   {
>       DeviceState *dev;
>       int i;
> @@ -2542,8 +2433,7 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
>       fdctrl_init_drives(&sys->state.bus, fds);
>   }
>   
> -static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl,
> -                                  Error **errp)
> +void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl, Error **errp)
>   {
>       int i, j;
>       FDrive *drive;
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 36055f14c59..20996f60e1f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1676,6 +1676,7 @@ M: John Snow <jsnow@redhat.com>
>   L: qemu-block@nongnu.org
>   S: Supported
>   F: hw/block/fdc.c
> +F: hw/block/fdc-internal.h
>   F: include/hw/block/fdc.h
>   F: tests/qtest/fdc-test.c
>   T: git https://gitlab.com/jsnow/qemu.git ide
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/4] hw/block/fdc: Extract ISA floppy controllers to fdc-isa.c
  2021-04-15 10:23 ` [PATCH 3/4] hw/block/fdc: Extract ISA floppy controllers to fdc-isa.c Philippe Mathieu-Daudé
@ 2021-04-27 17:11   ` John Snow
  0 siblings, 0 replies; 16+ messages in thread
From: John Snow @ 2021-04-27 17:11 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Mark Cave-Ayland, Richard Henderson,
	Philippe Mathieu-Daudé,
	Max Reitz, Artyom Tarasenko, Paolo Bonzini, Miroslav Rezanina,
	Aurelien Jarno

On 4/15/21 6:23 AM, Philippe Mathieu-Daudé wrote:
> Some machines use floppy controllers via the SysBus interface,
> and don't need to pull in all the ISA code.
> Extract the ISA specific code to a new unit: fdc-isa.c, and
> add a new Kconfig symbol: "FDC_ISA".
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   hw/block/fdc-isa.c   | 313 +++++++++++++++++++++++++++++++++++++++++++
>   hw/block/fdc.c       | 257 -----------------------------------
>   MAINTAINERS          |   1 +
>   hw/block/Kconfig     |   4 +
>   hw/block/meson.build |   1 +
>   hw/i386/Kconfig      |   2 +-
>   hw/isa/Kconfig       |   6 +-
>   hw/sparc64/Kconfig   |   2 +-
>   8 files changed, 324 insertions(+), 262 deletions(-)
>   create mode 100644 hw/block/fdc-isa.c
> 

LGTM but you'll want someone else to review the Kconfig changes.

Reviewed-by: John Snow <jsnow@redhat.com>



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/4] hw/block/fdc: Extract SysBus floppy controllers to fdc-sysbus.c
  2021-04-15 10:23 ` [PATCH 4/4] hw/block/fdc: Extract SysBus floppy controllers to fdc-sysbus.c Philippe Mathieu-Daudé
@ 2021-04-27 17:34   ` John Snow
  2021-04-28 12:30     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 16+ messages in thread
From: John Snow @ 2021-04-27 17:34 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Mark Cave-Ayland, Richard Henderson,
	Philippe Mathieu-Daudé,
	Max Reitz, Artyom Tarasenko, Paolo Bonzini, Miroslav Rezanina,
	Aurelien Jarno

On 4/15/21 6:23 AM, Philippe Mathieu-Daudé wrote:
> Some machines use floppy controllers via the SysBus interface,
> and don't need to pull in all the SysBus code.
> Extract the SysBus specific code to a new unit: fdc-sysbus.c,
> and add a new Kconfig symbol: "FDC_SYSBUS".
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

LGTM, but again you'll want someone to review the Kconfig changes. It 
looks reasonable to me at a glance, but I just don't know what I don't 
know there.

I'm trusting you somewhat that you've audited the proper dependencies 
for each subsystem and that these have been accurately described via 
Kconfig -- my knowledge of non-x86 platforms is a bit meager, so I am 
relying on CI to tell me if this breaks anything I care about.

Would love to get an ACK from Mark Cave-Ayland and Hervé Poussineau if 
possible, but if they're not available to take a quick peek, we'll try 
to get this in closer to the beginning of the dev window to maximize 
problem-finding time.

Reviewed-by: John Snow <jsnow@redhat.com>

> ---
>   hw/block/fdc-sysbus.c | 252 ++++++++++++++++++++++++++++++++++++++++++
>   hw/block/fdc.c        | 220 ------------------------------------
>   MAINTAINERS           |   1 +
>   hw/block/Kconfig      |   4 +
>   hw/block/meson.build  |   1 +
>   hw/block/trace-events |   2 +
>   hw/mips/Kconfig       |   2 +-
>   hw/sparc/Kconfig      |   2 +-
>   8 files changed, 262 insertions(+), 222 deletions(-)
>   create mode 100644 hw/block/fdc-sysbus.c
> 
> diff --git a/hw/block/fdc-sysbus.c b/hw/block/fdc-sysbus.c
> new file mode 100644
> index 00000000000..71755fd6ae4
> --- /dev/null
> +++ b/hw/block/fdc-sysbus.c
> @@ -0,0 +1,252 @@
> +/*
> + * QEMU Floppy disk emulator (Intel 82078)
> + *
> + * Copyright (c) 2003, 2007 Jocelyn Mayer
> + * Copyright (c) 2008 Hervé Poussineau
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qom/object.h"
> +#include "hw/sysbus.h"
> +#include "hw/block/fdc.h"
> +#include "migration/vmstate.h"
> +#include "fdc-internal.h"
> +#include "trace.h"
> +
> +#define TYPE_SYSBUS_FDC "base-sysbus-fdc"
> +typedef struct FDCtrlSysBusClass FDCtrlSysBusClass;
> +typedef struct FDCtrlSysBus FDCtrlSysBus;
> +DECLARE_OBJ_CHECKERS(FDCtrlSysBus, FDCtrlSysBusClass,
> +                     SYSBUS_FDC, TYPE_SYSBUS_FDC)
> +
> +struct FDCtrlSysBusClass {
> +    /*< private >*/
> +    SysBusDeviceClass parent_class;
> +    /*< public >*/
> +
> +    bool use_strict_io;
> +};
> +
> +struct FDCtrlSysBus {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +
> +    struct FDCtrl state;
> +};
> +
> +static uint64_t fdctrl_read_mem(void *opaque, hwaddr reg, unsigned ize)
> +{
> +    return fdctrl_read(opaque, (uint32_t)reg);
> +}
> +
> +static void fdctrl_write_mem(void *opaque, hwaddr reg,
> +                             uint64_t value, unsigned size)
> +{
> +    fdctrl_write(opaque, (uint32_t)reg, value);
> +}
> +
> +static const MemoryRegionOps fdctrl_mem_ops = {
> +    .read = fdctrl_read_mem,
> +    .write = fdctrl_write_mem,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static const MemoryRegionOps fdctrl_mem_strict_ops = {
> +    .read = fdctrl_read_mem,
> +    .write = fdctrl_write_mem,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +static void fdctrl_external_reset_sysbus(DeviceState *d)
> +{
> +    FDCtrlSysBus *sys = SYSBUS_FDC(d);
> +    FDCtrl *s = &sys->state;
> +
> +    fdctrl_reset(s, 0);
> +}
> +
> +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)
> +{
> +    FDCtrl *fdctrl;
> +    DeviceState *dev;
> +    SysBusDevice *sbd;
> +    FDCtrlSysBus *sys;
> +
> +    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);
> +    sysbus_mmio_map(sbd, 0, mmio_base);
> +
> +    fdctrl_init_drives(&sys->state.bus, fds);
> +}
> +
> +void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
> +                       DriveInfo **fds, qemu_irq *fdc_tc)
> +{
> +    DeviceState *dev;
> +    FDCtrlSysBus *sys;
> +
> +    dev = qdev_new("sun-fdtwo");
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    sys = SYSBUS_FDC(dev);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(sys), 0, irq);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(sys), 0, io_base);
> +    *fdc_tc = qdev_get_gpio_in(dev, 0);
> +
> +    fdctrl_init_drives(&sys->state.bus, fds);
> +}
> +
> +static void sysbus_fdc_common_initfn(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    FDCtrlSysBusClass *sbdc = SYSBUS_FDC_GET_CLASS(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    FDCtrlSysBus *sys = SYSBUS_FDC(obj);
> +    FDCtrl *fdctrl = &sys->state;
> +
> +    fdctrl->dma_chann = -1;
> +
> +    qdev_set_legacy_instance_id(dev, 0 /* io */, 2); /* FIXME */
> +
> +    memory_region_init_io(&fdctrl->iomem, obj,
> +                          sbdc->use_strict_io ? &fdctrl_mem_strict_ops
> +                                              : &fdctrl_mem_ops,
> +                          fdctrl, "fdc", 0x08);
> +    sysbus_init_mmio(sbd, &fdctrl->iomem);
> +
> +    sysbus_init_irq(sbd, &fdctrl->irq);
> +    qdev_init_gpio_in(dev, fdctrl_handle_tc, 1);
> +}
> +
> +static void sysbus_fdc_common_realize(DeviceState *dev, Error **errp)
> +{
> +    FDCtrlSysBus *sys = SYSBUS_FDC(dev);
> +    FDCtrl *fdctrl = &sys->state;
> +
> +    fdctrl_realize_common(dev, fdctrl, errp);
> +}
> +
> +static const VMStateDescription vmstate_sysbus_fdc = {
> +    .name = "fdc",
> +    .version_id = 2,
> +    .minimum_version_id = 2,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT(state, FDCtrlSysBus, 0, vmstate_fdc, FDCtrl),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static Property sysbus_fdc_properties[] = {
> +    DEFINE_PROP_SIGNED("fdtypeA", FDCtrlSysBus, state.qdev_for_drives[0].type,
> +                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
> +                        FloppyDriveType),
> +    DEFINE_PROP_SIGNED("fdtypeB", FDCtrlSysBus, state.qdev_for_drives[1].type,
> +                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
> +                        FloppyDriveType),
> +    DEFINE_PROP_SIGNED("fallback", FDCtrlSysBus, state.fallback,
> +                        FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
> +                        FloppyDriveType),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void sysbus_fdc_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    device_class_set_props(dc, sysbus_fdc_properties);
> +    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> +}
> +
> +static const TypeInfo sysbus_fdc_info = {
> +    .name          = "sysbus-fdc",
> +    .parent        = TYPE_SYSBUS_FDC,
> +    .class_init    = sysbus_fdc_class_init,
> +};
> +
> +static void sysbus_fdc_common_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = sysbus_fdc_common_realize;
> +    dc->reset = fdctrl_external_reset_sysbus;
> +    dc->vmsd = &vmstate_sysbus_fdc;
> +}
> +
> +static const TypeInfo sysbus_fdc_type_info = {
> +    .name          = TYPE_SYSBUS_FDC,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(FDCtrlSysBus),
> +    .instance_init = sysbus_fdc_common_initfn,
> +    .abstract      = true,
> +    .class_init    = sysbus_fdc_common_class_init,
> +    .class_size    = sizeof(FDCtrlSysBusClass),
> +};
> +
> +static Property sun4m_fdc_properties[] = {
> +    DEFINE_PROP_SIGNED("fdtype", FDCtrlSysBus, state.qdev_for_drives[0].type,
> +                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
> +                        FloppyDriveType),
> +    DEFINE_PROP_SIGNED("fallback", FDCtrlSysBus, state.fallback,
> +                        FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
> +                        FloppyDriveType),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void sun4m_fdc_class_init(ObjectClass *klass, void *data)
> +{
> +    FDCtrlSysBusClass *sbdc = SYSBUS_FDC_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    sbdc->use_strict_io = true;
> +    device_class_set_props(dc, sun4m_fdc_properties);
> +    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> +}
> +
> +static const TypeInfo sun4m_fdc_info = {
> +    .name          = "sun-fdtwo",
> +    .parent        = TYPE_SYSBUS_FDC,
> +    .class_init    = sun4m_fdc_class_init,
> +};
> +
> +static void sysbus_fdc_register_types(void)
> +{
> +    type_register_static(&sun4m_fdc_info);
> +    type_register_static(&sysbus_fdc_type_info);
> +    type_register_static(&sysbus_fdc_info);
> +}
> +
> +type_init(sysbus_fdc_register_types)
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 50567d972ff..64af4d194ce 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -36,7 +36,6 @@
>   #include "hw/isa/isa.h"
>   #include "hw/qdev-properties.h"
>   #include "hw/qdev-properties-system.h"
> -#include "hw/sysbus.h"
>   #include "migration/vmstate.h"
>   #include "hw/block/block.h"
>   #include "sysemu/block-backend.h"
> @@ -775,17 +774,6 @@ static FloppyDriveType get_fallback_drive_type(FDrive *drv)
>       return drv->fdctrl->fallback;
>   }
>   
> -#define TYPE_SYSBUS_FDC "base-sysbus-fdc"
> -OBJECT_DECLARE_SIMPLE_TYPE(FDCtrlSysBus, SYSBUS_FDC)
> -
> -struct FDCtrlSysBus {
> -    /*< private >*/
> -    SysBusDevice parent_obj;
> -    /*< public >*/
> -
> -    struct FDCtrl state;
> -};
> -
>   uint32_t fdctrl_read(void *opaque, uint32_t reg)
>   {
>       FDCtrl *fdctrl = opaque;
> @@ -850,34 +838,6 @@ void fdctrl_write(void *opaque, uint32_t reg, uint32_t value)
>       }
>   }
>   
> -static uint64_t fdctrl_read_mem (void *opaque, hwaddr reg,
> -                                 unsigned ize)
> -{
> -    return fdctrl_read(opaque, (uint32_t)reg);
> -}
> -
> -static void fdctrl_write_mem (void *opaque, hwaddr reg,
> -                              uint64_t value, unsigned size)
> -{
> -    fdctrl_write(opaque, (uint32_t)reg, value);
> -}
> -
> -static const MemoryRegionOps fdctrl_mem_ops = {
> -    .read = fdctrl_read_mem,
> -    .write = fdctrl_write_mem,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> -};
> -
> -static const MemoryRegionOps fdctrl_mem_strict_ops = {
> -    .read = fdctrl_read_mem,
> -    .write = fdctrl_write_mem,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> -    .valid = {
> -        .min_access_size = 1,
> -        .max_access_size = 1,
> -    },
> -};
> -
>   static bool fdrive_media_changed_needed(void *opaque)
>   {
>       FDrive *drive = opaque;
> @@ -1101,19 +1061,6 @@ const VMStateDescription vmstate_fdc = {
>       }
>   };
>   
> -static void fdctrl_external_reset_sysbus(DeviceState *d)
> -{
> -    FDCtrlSysBus *sys = SYSBUS_FDC(d);
> -    FDCtrl *s = &sys->state;
> -
> -    fdctrl_reset(s, 0);
> -}
> -
> -static void fdctrl_handle_tc(void *opaque, int irq, int level)
> -{
> -    trace_fdctrl_tc_pulse(level);
> -}
> -
>   /* Change IRQ state */
>   static void fdctrl_reset_irq(FDCtrl *fdctrl)
>   {
> @@ -2370,42 +2317,6 @@ void fdctrl_init_drives(FloppyBus *bus, DriveInfo **fds)
>       }
>   }
>   
> -void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
> -                        hwaddr mmio_base, DriveInfo **fds)
> -{
> -    FDCtrl *fdctrl;
> -    DeviceState *dev;
> -    SysBusDevice *sbd;
> -    FDCtrlSysBus *sys;
> -
> -    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);
> -    sysbus_mmio_map(sbd, 0, mmio_base);
> -
> -    fdctrl_init_drives(&sys->state.bus, fds);
> -}
> -
> -void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
> -                       DriveInfo **fds, qemu_irq *fdc_tc)
> -{
> -    DeviceState *dev;
> -    FDCtrlSysBus *sys;
> -
> -    dev = qdev_new("sun-fdtwo");
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> -    sys = SYSBUS_FDC(dev);
> -    sysbus_connect_irq(SYS_BUS_DEVICE(sys), 0, irq);
> -    sysbus_mmio_map(SYS_BUS_DEVICE(sys), 0, io_base);
> -    *fdc_tc = qdev_get_gpio_in(dev, 0);
> -
> -    fdctrl_init_drives(&sys->state.bus, fds);
> -}
> -
>   void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl, Error **errp)
>   {
>       int i, j;
> @@ -2458,139 +2369,8 @@ void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl, Error **errp)
>       }
>   }
>   
> -static void sysbus_fdc_initfn(Object *obj)
> -{
> -    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> -    FDCtrlSysBus *sys = SYSBUS_FDC(obj);
> -    FDCtrl *fdctrl = &sys->state;
> -
> -    fdctrl->dma_chann = -1;
> -
> -    memory_region_init_io(&fdctrl->iomem, obj, &fdctrl_mem_ops, fdctrl,
> -                          "fdc", 0x08);
> -    sysbus_init_mmio(sbd, &fdctrl->iomem);
> -}
> -
> -static void sun4m_fdc_initfn(Object *obj)
> -{
> -    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> -    FDCtrlSysBus *sys = SYSBUS_FDC(obj);
> -    FDCtrl *fdctrl = &sys->state;
> -
> -    fdctrl->dma_chann = -1;
> -
> -    memory_region_init_io(&fdctrl->iomem, obj, &fdctrl_mem_strict_ops,
> -                          fdctrl, "fdctrl", 0x08);
> -    sysbus_init_mmio(sbd, &fdctrl->iomem);
> -}
> -
> -static void sysbus_fdc_common_initfn(Object *obj)
> -{
> -    DeviceState *dev = DEVICE(obj);
> -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> -    FDCtrlSysBus *sys = SYSBUS_FDC(obj);
> -    FDCtrl *fdctrl = &sys->state;
> -
> -    qdev_set_legacy_instance_id(dev, 0 /* io */, 2); /* FIXME */
> -
> -    sysbus_init_irq(sbd, &fdctrl->irq);
> -    qdev_init_gpio_in(dev, fdctrl_handle_tc, 1);
> -}
> -
> -static void sysbus_fdc_common_realize(DeviceState *dev, Error **errp)
> -{
> -    FDCtrlSysBus *sys = SYSBUS_FDC(dev);
> -    FDCtrl *fdctrl = &sys->state;
> -
> -    fdctrl_realize_common(dev, fdctrl, errp);
> -}
> -
> -static const VMStateDescription vmstate_sysbus_fdc ={
> -    .name = "fdc",
> -    .version_id = 2,
> -    .minimum_version_id = 2,
> -    .fields = (VMStateField[]) {
> -        VMSTATE_STRUCT(state, FDCtrlSysBus, 0, vmstate_fdc, FDCtrl),
> -        VMSTATE_END_OF_LIST()
> -    }
> -};
> -
> -static Property sysbus_fdc_properties[] = {
> -    DEFINE_PROP_SIGNED("fdtypeA", FDCtrlSysBus, state.qdev_for_drives[0].type,
> -                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
> -                        FloppyDriveType),
> -    DEFINE_PROP_SIGNED("fdtypeB", FDCtrlSysBus, state.qdev_for_drives[1].type,
> -                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
> -                        FloppyDriveType),
> -    DEFINE_PROP_SIGNED("fallback", FDCtrlSysBus, state.fallback,
> -                        FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
> -                        FloppyDriveType),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
> -static void sysbus_fdc_class_init(ObjectClass *klass, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(klass);
> -
> -    device_class_set_props(dc, sysbus_fdc_properties);
> -    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> -}
> -
> -static const TypeInfo sysbus_fdc_info = {
> -    .name          = "sysbus-fdc",
> -    .parent        = TYPE_SYSBUS_FDC,
> -    .instance_init = sysbus_fdc_initfn,
> -    .class_init    = sysbus_fdc_class_init,
> -};
> -
> -static Property sun4m_fdc_properties[] = {
> -    DEFINE_PROP_SIGNED("fdtype", FDCtrlSysBus, state.qdev_for_drives[0].type,
> -                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
> -                        FloppyDriveType),
> -    DEFINE_PROP_SIGNED("fallback", FDCtrlSysBus, state.fallback,
> -                        FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
> -                        FloppyDriveType),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
> -static void sun4m_fdc_class_init(ObjectClass *klass, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(klass);
> -
> -    device_class_set_props(dc, sun4m_fdc_properties);
> -    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> -}
> -
> -static const TypeInfo sun4m_fdc_info = {
> -    .name          = "sun-fdtwo",
> -    .parent        = TYPE_SYSBUS_FDC,
> -    .instance_init = sun4m_fdc_initfn,
> -    .class_init    = sun4m_fdc_class_init,
> -};
> -
> -static void sysbus_fdc_common_class_init(ObjectClass *klass, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(klass);
> -
> -    dc->realize = sysbus_fdc_common_realize;
> -    dc->reset = fdctrl_external_reset_sysbus;
> -    dc->vmsd = &vmstate_sysbus_fdc;
> -}
> -
> -static const TypeInfo sysbus_fdc_type_info = {
> -    .name          = TYPE_SYSBUS_FDC,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(FDCtrlSysBus),
> -    .instance_init = sysbus_fdc_common_initfn,
> -    .abstract      = true,
> -    .class_init    = sysbus_fdc_common_class_init,
> -};
> -
>   static void fdc_register_types(void)
>   {
> -    type_register_static(&sysbus_fdc_type_info);
> -    type_register_static(&sysbus_fdc_info);
> -    type_register_static(&sun4m_fdc_info);
>       type_register_static(&floppy_bus_info);
>       type_register_static(&floppy_drive_info);
>   }
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e7ed334f586..0a908c22103 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1678,6 +1678,7 @@ S: Supported
>   F: hw/block/fdc.c
>   F: hw/block/fdc-internal.h
>   F: hw/block/fdc-isa.c
> +F: hw/block/fdc-sysbus.c
>   F: include/hw/block/fdc.h
>   F: tests/qtest/fdc-test.c
>   T: git https://gitlab.com/jsnow/qemu.git ide
> diff --git a/hw/block/Kconfig b/hw/block/Kconfig
> index 0a2c046fa6c..d50be837666 100644
> --- a/hw/block/Kconfig
> +++ b/hw/block/Kconfig
> @@ -8,6 +8,10 @@ config FDC_ISA
>       # select ISA_BUS here instead of polluting each board that requires one
>       select ISA_BUS
>   
> +config FDC_SYSBUS
> +    bool
> +    select FDC
> +
>   config SSI_M25P80
>       bool
>   
> diff --git a/hw/block/meson.build b/hw/block/meson.build
> index f33a665c945..c3935350485 100644
> --- a/hw/block/meson.build
> +++ b/hw/block/meson.build
> @@ -6,6 +6,7 @@
>   softmmu_ss.add(when: 'CONFIG_ECC', if_true: files('ecc.c'))
>   softmmu_ss.add(when: 'CONFIG_FDC', if_true: files('fdc.c'))
>   softmmu_ss.add(when: 'CONFIG_FDC_ISA', if_true: files('fdc-isa.c'))
> +softmmu_ss.add(when: 'CONFIG_FDC_SYSBUS', if_true: files('fdc-sysbus.c'))
>   softmmu_ss.add(when: 'CONFIG_NAND', if_true: files('nand.c'))
>   softmmu_ss.add(when: 'CONFIG_ONENAND', if_true: files('onenand.c'))
>   softmmu_ss.add(when: 'CONFIG_PFLASH_CFI01', if_true: files('pflash_cfi01.c'))
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 306989c193c..266b34393a3 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -3,6 +3,8 @@
>   # fdc.c
>   fdc_ioport_read(uint8_t reg, uint8_t value) "read reg 0x%02x val 0x%02x"
>   fdc_ioport_write(uint8_t reg, uint8_t value) "write reg 0x%02x val 0x%02x"
> +
> +# fdc-sysbus.c
>   fdctrl_tc_pulse(int level) "TC pulse: %u"
>   
>   # pflash_cfi01.c
> diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
> index aadd436bf4e..c245e881a2b 100644
> --- a/hw/mips/Kconfig
> +++ b/hw/mips/Kconfig
> @@ -20,7 +20,7 @@ config JAZZ
>       select G364FB
>       select DP8393X
>       select ESP
> -    select FDC
> +    select FDC_SYSBUS
>       select MC146818RTC
>       select PCKBD
>       select SERIAL
> diff --git a/hw/sparc/Kconfig b/hw/sparc/Kconfig
> index 8dcb10086fd..79d58beb7a6 100644
> --- a/hw/sparc/Kconfig
> +++ b/hw/sparc/Kconfig
> @@ -8,7 +8,7 @@ config SUN4M
>       select UNIMP
>       select ESCC
>       select ESP
> -    select FDC
> +    select FDC_SYSBUS
>       select SLAVIO
>       select LANCE
>       select M48T59
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers
  2021-04-15 10:23 [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers Philippe Mathieu-Daudé
                   ` (4 preceding siblings ...)
  2021-04-27 12:54 ` [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers Philippe Mathieu-Daudé
@ 2021-04-27 17:38 ` John Snow
  2021-04-28  7:57 ` Mark Cave-Ayland
  6 siblings, 0 replies; 16+ messages in thread
From: John Snow @ 2021-04-27 17:38 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé,
	qemu-devel, Mark Cave-Ayland, Hervé Poussineau,
	Paolo Bonzini
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Mark Cave-Ayland, Richard Henderson,
	Philippe Mathieu-Daudé,
	Max Reitz, Artyom Tarasenko, Paolo Bonzini, Miroslav Rezanina,
	Aurelien Jarno

On 4/15/21 6:23 AM, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> The floppy disc controllers pulls in irrelevant devices (sysbus in
> an ISA-only machine, ISA bus + isa devices on a sysbus-only machine).
> 
> This series clean that by extracting each device in its own file,
> adding the corresponding Kconfig symbols: FDC_ISA and FDC_SYSBUS.
> 
> Regards,
> 
> Phil.
> 

Lightly reviewed and I'm fine with it overall; but want a quick ~5min 
up-or-down by Hervé and/or Mark (To make sure this doesn't break any 
non-x86 system they may care about), and a quick nod from Paolo for 
KConfig changes would be nice.

I'll be waiting on a reply to my question on patch 01 before staging.

--js

> Philippe Mathieu-Daudé (4):
>    hw/block/fdc: Replace disabled fprintf() by trace event
>    hw/block/fdc: Declare shared prototypes in fdc-internal.h
>    hw/block/fdc: Extract ISA floppy controllers to fdc-isa.c
>    hw/block/fdc: Extract SysBus floppy controllers to fdc-sysbus.c
> 
>   hw/block/fdc-internal.h | 155 ++++++++++
>   hw/block/fdc-isa.c      | 313 +++++++++++++++++++++
>   hw/block/fdc-sysbus.c   | 252 +++++++++++++++++
>   hw/block/fdc.c          | 608 +---------------------------------------
>   MAINTAINERS             |   3 +
>   hw/block/Kconfig        |   8 +
>   hw/block/meson.build    |   2 +
>   hw/block/trace-events   |   3 +
>   hw/i386/Kconfig         |   2 +-
>   hw/isa/Kconfig          |   6 +-
>   hw/mips/Kconfig         |   2 +-
>   hw/sparc/Kconfig        |   2 +-
>   hw/sparc64/Kconfig      |   2 +-
>   13 files changed, 751 insertions(+), 607 deletions(-)
>   create mode 100644 hw/block/fdc-internal.h
>   create mode 100644 hw/block/fdc-isa.c
>   create mode 100644 hw/block/fdc-sysbus.c
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/4] hw/block/fdc: Declare shared prototypes in fdc-internal.h
  2021-04-27 16:53   ` John Snow
@ 2021-04-27 17:56     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-27 17:56 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Mark Cave-Ayland, Richard Henderson,
	Philippe Mathieu-Daudé,
	Max Reitz, Artyom Tarasenko, Paolo Bonzini, Miroslav Rezanina,
	Aurelien Jarno

On 4/27/21 6:53 PM, John Snow wrote:
> On 4/15/21 6:23 AM, Philippe Mathieu-Daudé wrote:
>> We want to extract ISA/SysBus code from the generic fdc.c file.
>> First, declare the prototypes we will access from the new units
>> into a new local header: "fdc-internal.h".
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   hw/block/fdc-internal.h | 155 ++++++++++++++++++++++++++++++++++++++++
>>   hw/block/fdc.c          | 126 +++-----------------------------
>>   MAINTAINERS             |   1 +
>>   3 files changed, 164 insertions(+), 118 deletions(-)
>>   create mode 100644 hw/block/fdc-internal.h
>>
> 
> With our policy of not including osdep.h in headers, it's hard to verify
> that this header is otherwise self-sufficient.
> 
> 
> I think the only thing it needs (not in osdep.h) happens to be MAX_FD. I
> added osdep.h just to test:
> 
> jsnow@scv ~/s/q/h/block (review)> gcc -I../../include/ -I../../bin/git
> -I/usr/lib64/glib-2.0/include -I/usr/include/glib-2.0 -c -o
> test_header.bin fdc-internal.h
> fdc-internal.h:134:19: error: ‘MAX_FD’ undeclared here (not in a function)
>   134 |     FDrive drives[MAX_FD];
>       |                   ^~~~~~
> 
> 
> Should we include the fdc header from the internal one?

Yes, good catch, will do.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers
  2021-04-15 10:23 [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers Philippe Mathieu-Daudé
                   ` (5 preceding siblings ...)
  2021-04-27 17:38 ` John Snow
@ 2021-04-28  7:57 ` Mark Cave-Ayland
  2021-04-28 11:25   ` Philippe Mathieu-Daudé
  6 siblings, 1 reply; 16+ messages in thread
From: Mark Cave-Ayland @ 2021-04-28  7:57 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, John Snow, qemu-devel
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Richard Henderson,
	Philippe Mathieu-Daudé,
	Max Reitz, Aurelien Jarno, Paolo Bonzini, Miroslav Rezanina,
	Artyom Tarasenko

On 15/04/2021 11:23, Philippe Mathieu-Daudé wrote:

> Hi,
> 
> The floppy disc controllers pulls in irrelevant devices (sysbus in
> an ISA-only machine, ISA bus + isa devices on a sysbus-only machine).
> 
> This series clean that by extracting each device in its own file,
> adding the corresponding Kconfig symbols: FDC_ISA and FDC_SYSBUS.
> 
> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (4):
>    hw/block/fdc: Replace disabled fprintf() by trace event
>    hw/block/fdc: Declare shared prototypes in fdc-internal.h
>    hw/block/fdc: Extract ISA floppy controllers to fdc-isa.c
>    hw/block/fdc: Extract SysBus floppy controllers to fdc-sysbus.c
> 
>   hw/block/fdc-internal.h | 155 ++++++++++
>   hw/block/fdc-isa.c      | 313 +++++++++++++++++++++
>   hw/block/fdc-sysbus.c   | 252 +++++++++++++++++
>   hw/block/fdc.c          | 608 +---------------------------------------
>   MAINTAINERS             |   3 +
>   hw/block/Kconfig        |   8 +
>   hw/block/meson.build    |   2 +
>   hw/block/trace-events   |   3 +
>   hw/i386/Kconfig         |   2 +-
>   hw/isa/Kconfig          |   6 +-
>   hw/mips/Kconfig         |   2 +-
>   hw/sparc/Kconfig        |   2 +-
>   hw/sparc64/Kconfig      |   2 +-
>   13 files changed, 751 insertions(+), 607 deletions(-)
>   create mode 100644 hw/block/fdc-internal.h
>   create mode 100644 hw/block/fdc-isa.c
>   create mode 100644 hw/block/fdc-sysbus.c

This basically looks good to me. You may be able to simplify this further by removing 
the global legacy init functions fdctrl_init_sysbus() and sun4m_fdctrl_init(): from 
what I can see fdctrl_init_sysbus() is only used in hw/mips/jazz.c and 
sun4m_fdctrl_init() is only used in hw/sparc/sun4m.c so you might as well inline them 
or move the functions to the relevant files.

Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers
  2021-04-28  7:57 ` Mark Cave-Ayland
@ 2021-04-28 11:25   ` Philippe Mathieu-Daudé
  2021-04-28 12:33     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-28 11:25 UTC (permalink / raw)
  To: Mark Cave-Ayland, John Snow, qemu-devel
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Richard Henderson,
	Philippe Mathieu-Daudé,
	Max Reitz, Aurelien Jarno, Paolo Bonzini, Miroslav Rezanina,
	Artyom Tarasenko

On 4/28/21 9:57 AM, Mark Cave-Ayland wrote:
> On 15/04/2021 11:23, Philippe Mathieu-Daudé wrote:
> 
>> Hi,
>>
>> The floppy disc controllers pulls in irrelevant devices (sysbus in
>> an ISA-only machine, ISA bus + isa devices on a sysbus-only machine).
>>
>> This series clean that by extracting each device in its own file,
>> adding the corresponding Kconfig symbols: FDC_ISA and FDC_SYSBUS.
>>
>> Regards,
>>
>> Phil.
>>
>> Philippe Mathieu-Daudé (4):
>>    hw/block/fdc: Replace disabled fprintf() by trace event
>>    hw/block/fdc: Declare shared prototypes in fdc-internal.h
>>    hw/block/fdc: Extract ISA floppy controllers to fdc-isa.c
>>    hw/block/fdc: Extract SysBus floppy controllers to fdc-sysbus.c
>>
>>   hw/block/fdc-internal.h | 155 ++++++++++
>>   hw/block/fdc-isa.c      | 313 +++++++++++++++++++++
>>   hw/block/fdc-sysbus.c   | 252 +++++++++++++++++
>>   hw/block/fdc.c          | 608 +---------------------------------------
>>   MAINTAINERS             |   3 +
>>   hw/block/Kconfig        |   8 +
>>   hw/block/meson.build    |   2 +
>>   hw/block/trace-events   |   3 +
>>   hw/i386/Kconfig         |   2 +-
>>   hw/isa/Kconfig          |   6 +-
>>   hw/mips/Kconfig         |   2 +-
>>   hw/sparc/Kconfig        |   2 +-
>>   hw/sparc64/Kconfig      |   2 +-
>>   13 files changed, 751 insertions(+), 607 deletions(-)
>>   create mode 100644 hw/block/fdc-internal.h
>>   create mode 100644 hw/block/fdc-isa.c
>>   create mode 100644 hw/block/fdc-sysbus.c
> 
> This basically looks good to me. You may be able to simplify this
> further by removing the global legacy init functions
> fdctrl_init_sysbus() and sun4m_fdctrl_init(): from what I can see
> fdctrl_init_sysbus() is only used in hw/mips/jazz.c and
> sun4m_fdctrl_init() is only used in hw/sparc/sun4m.c so you might as
> well inline them or move the functions to the relevant files.

But both use FDCtrlSysBus which this series makes local to
hw/block/fdc-sysbus.c, and use fdctrl_init_drives() which is declared in
hw/block/fdc-internal.h, and use FloppyBus (also declared there).

Apparently they do that to initialize the fdctrl->dma_chann field...
Which should be a property, but FDCtrl isn't QOM... So not that
easy, it requires more work.

> Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Thanks!

Phil.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/4] hw/block/fdc: Extract SysBus floppy controllers to fdc-sysbus.c
  2021-04-27 17:34   ` John Snow
@ 2021-04-28 12:30     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-28 12:30 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Mark Cave-Ayland, Richard Henderson,
	Philippe Mathieu-Daudé,
	Max Reitz, Artyom Tarasenko, Paolo Bonzini, Miroslav Rezanina,
	Aurelien Jarno

On 4/27/21 7:34 PM, John Snow wrote:
> On 4/15/21 6:23 AM, Philippe Mathieu-Daudé wrote:
>> Some machines use floppy controllers via the SysBus interface,
>> and don't need to pull in all the SysBus code.
>> Extract the SysBus specific code to a new unit: fdc-sysbus.c,
>> and add a new Kconfig symbol: "FDC_SYSBUS".
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> LGTM, but again you'll want someone to review the Kconfig changes. It
> looks reasonable to me at a glance, but I just don't know what I don't
> know there.
> 
> I'm trusting you somewhat that you've audited the proper dependencies
> for each subsystem and that these have been accurately described via
> Kconfig -- my knowledge of non-x86 platforms is a bit meager, so I am
> relying on CI to tell me if this breaks anything I care about.

The way I test these Kconfig changes is configuring with
--without-default-devices, and build/test for each machine doing:

$ echo $MACHINE=y > default-configs/devices/$arch-softmmu.mak

(I overwrite because currently we don't support having a base
config different than the default-configs one).

For example for the SPARCbook machine:

$ echo CONFIG_SUN4M=y > default-configs/devices/sparc-softmmu.mak
$ ninja qemu-system-sparc
$ qemu-system-sparc -M SPARCbook -S

Or for the Jazz machine:

$ echo CONFIG_JAZZ=y > default-configs/devices/mips64el-softmmu.mak
$ echo CONFIG_SEMIHOSTING=y >> default-configs/devices/mips64el-softmmu.mak
$ ninja qemu-system-mips64el
$ ./qemu-system-mips64el -M pica61 -S

(The CONFIG_SEMIHOSTING is a particular kludge pending another series)

But unfortunately there are predating bugs breaking this testing.

I'll add this information in the respin's cover.

> Would love to get an ACK from Mark Cave-Ayland and Hervé Poussineau if
> possible, but if they're not available to take a quick peek, we'll try
> to get this in closer to the beginning of the dev window to maximize
> problem-finding time.

The sun4m maintainer is Artyom.

> Reviewed-by: John Snow <jsnow@redhat.com>

Thanks!

> 
>> ---
>>   hw/block/fdc-sysbus.c | 252 ++++++++++++++++++++++++++++++++++++++++++
>>   hw/block/fdc.c        | 220 ------------------------------------
>>   MAINTAINERS           |   1 +
>>   hw/block/Kconfig      |   4 +
>>   hw/block/meson.build  |   1 +
>>   hw/block/trace-events |   2 +
>>   hw/mips/Kconfig       |   2 +-
>>   hw/sparc/Kconfig      |   2 +-
>>   8 files changed, 262 insertions(+), 222 deletions(-)
>>   create mode 100644 hw/block/fdc-sysbus.c



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers
  2021-04-28 11:25   ` Philippe Mathieu-Daudé
@ 2021-04-28 12:33     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-28 12:33 UTC (permalink / raw)
  To: Mark Cave-Ayland, John Snow, qemu-devel
  Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
	Michael S. Tsirkin, Richard Henderson, Max Reitz, Aurelien Jarno,
	Paolo Bonzini, Miroslav Rezanina, Artyom Tarasenko

On 4/28/21 1:25 PM, Philippe Mathieu-Daudé wrote:
> On 4/28/21 9:57 AM, Mark Cave-Ayland wrote:

>> This basically looks good to me. You may be able to simplify this
>> further by removing the global legacy init functions
>> fdctrl_init_sysbus() and sun4m_fdctrl_init(): from what I can see
>> fdctrl_init_sysbus() is only used in hw/mips/jazz.c and
>> sun4m_fdctrl_init() is only used in hw/sparc/sun4m.c so you might as
>> well inline them or move the functions to the relevant files.
> 
> But both use FDCtrlSysBus which this series makes local to
> hw/block/fdc-sysbus.c, and use fdctrl_init_drives() which is declared in
> hw/block/fdc-internal.h, and use FloppyBus (also declared there).
> 
> Apparently they do that to initialize the fdctrl->dma_chann field...
> Which should be a property, but FDCtrl isn't QOM... So not that
> easy, it requires more work.

Hmm FDCtrl doesn't have to be a QOM object, it is a simple embedded
structure. It should be doable then.



^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-04-28 12:35 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-15 10:23 [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers Philippe Mathieu-Daudé
2021-04-15 10:23 ` [PATCH 1/4] hw/block/fdc: Replace disabled fprintf() by trace event Philippe Mathieu-Daudé
2021-04-27 15:25   ` John Snow
2021-04-15 10:23 ` [PATCH 2/4] hw/block/fdc: Declare shared prototypes in fdc-internal.h Philippe Mathieu-Daudé
2021-04-27 16:53   ` John Snow
2021-04-27 17:56     ` Philippe Mathieu-Daudé
2021-04-15 10:23 ` [PATCH 3/4] hw/block/fdc: Extract ISA floppy controllers to fdc-isa.c Philippe Mathieu-Daudé
2021-04-27 17:11   ` John Snow
2021-04-15 10:23 ` [PATCH 4/4] hw/block/fdc: Extract SysBus floppy controllers to fdc-sysbus.c Philippe Mathieu-Daudé
2021-04-27 17:34   ` John Snow
2021-04-28 12:30     ` Philippe Mathieu-Daudé
2021-04-27 12:54 ` [PATCH 0/4] hw/block/fdc: Allow Kconfig-selecting ISA bus/SysBus floppy controllers Philippe Mathieu-Daudé
2021-04-27 17:38 ` John Snow
2021-04-28  7:57 ` Mark Cave-Ayland
2021-04-28 11:25   ` Philippe Mathieu-Daudé
2021-04-28 12:33     ` Philippe Mathieu-Daudé

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).