[PATCH-for-5.2 0/7] hw/block/fdc: Cleanups trying to make sense of the floppy controllers

[PATCH-for-5.2 0/7] hw/block/fdc: Cleanups trying to make sense of the floppy controllers

[Philippe Mathieu-Daudé]

Trivial patches while trying to make sense of the floppy
controllers code.

Philippe Mathieu-Daudé (7):
  hw/block/fdc: Let sector count be unsigned
  hw/block/fdc: Let sector offset be unsigned
  hw/block/fdc: Use warn_report() instead of debug FLOPPY_DPRINTF()
    calls
  hw/block/fdc: Convert debug FLOPPY_DPRINTF() to trace events
  hw/block/fdc: Drop pointless FLOPPY_DPRINTF() call
  hw/block/fdc: Use more descriptive TypeInfo names
  hw/block/fdc: Add ASCII art schema of QOM relations

 hw/block/fdc.c        | 87 +++++++++++++++++++++++++++----------------
 hw/block/trace-events |  3 ++
 2 files changed, 57 insertions(+), 33 deletions(-)

-- 
2.21.3

[PATCH-for-5.2 1/7] hw/block/fdc: Let sector count be unsigned

[Philippe Mathieu-Daudé]

Sectors count can not be negative, make it unsigned.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/block/fdc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index e9ed3eef45..2cec7568c1 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -245,14 +245,14 @@ static void fd_init(FDrive *drv)
 
 #define NUM_SIDES(drv) ((drv)->flags & FDISK_DBL_SIDES ? 2 : 1)
 
-static int fd_sector_calc(uint8_t head, uint8_t track, uint8_t sect,
-                          uint8_t last_sect, uint8_t num_sides)
+static uint32_t fd_sector_calc(uint8_t head, uint8_t track, uint8_t sect,
+                               uint8_t last_sect, uint8_t num_sides)
 {
     return (((track * num_sides) + head) * last_sect) + sect - 1;
 }
 
 /* Returns current position, in sectors, for given drive */
-static int fd_sector(FDrive *drv)
+static uint32_t fd_sector(FDrive *drv)
 {
     return fd_sector_calc(drv->head, drv->track, drv->sect, drv->last_sect,
                           NUM_SIDES(drv));
-- 
2.21.3

[PATCH-for-5.2 2/7] hw/block/fdc: Let sector offset be unsigned

[Philippe Mathieu-Daudé]

Sector offset can not be negative, make it unsigned.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/block/fdc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 2cec7568c1..c91ed7ee2d 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -259,7 +259,7 @@ static uint32_t fd_sector(FDrive *drv)
 }
 
 /* Returns current position, in bytes, for given drive */
-static int fd_offset(FDrive *drv)
+static uint64_t fd_offset(FDrive *drv)
 {
     g_assert(fd_sector(drv) < INT_MAX >> BDRV_SECTOR_BITS);
     return fd_sector(drv) << BDRV_SECTOR_BITS;
-- 
2.21.3

[PATCH-for-5.2 3/7] hw/block/fdc: Use warn_report() instead of debug FLOPPY_DPRINTF() calls

[Philippe Mathieu-Daudé]

Use warn_report() instead of debug FLOPPY_DPRINTF() calls.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/block/fdc.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index c91ed7ee2d..ee45ec0b27 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -395,12 +395,10 @@ static int pick_geometry(FDrive *drv)
     if (match == -1) {
         if (size_match != -1) {
             parse = &fd_formats[size_match];
-            FLOPPY_DPRINTF("User requested floppy drive type '%s', "
-                           "but inserted medium appears to be a "
-                           "%"PRId64" sector '%s' type\n",
-                           FloppyDriveType_str(drv->drive),
-                           nb_sectors,
-                           FloppyDriveType_str(parse->drive));
+            warn_report("User requested floppy drive type '%s', but inserted "
+                        "medium appears to be a %"PRId64" sector '%s' type",
+                        FloppyDriveType_str(drv->drive), nb_sectors,
+                        FloppyDriveType_str(parse->drive));
         }
         assert(type_match != -1 && "misconfigured fd_format");
         match = type_match;
@@ -1805,8 +1803,8 @@ static int fdctrl_transfer_handler (void *opaque, int nchan,
             /* READ & SCAN commands and realign to a sector for WRITE */
             if (blk_pread(cur_drv->blk, fd_offset(cur_drv),
                           fdctrl->fifo, BDRV_SECTOR_SIZE) < 0) {
-                FLOPPY_DPRINTF("Floppy: error getting sector %d\n",
-                               fd_sector(cur_drv));
+                warn_report("Floppy: error getting sector %" PRIu32,
+                            fd_sector(cur_drv));
                 /* Sure, image size is too small... */
                 memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
             }
@@ -1833,8 +1831,8 @@ static int fdctrl_transfer_handler (void *opaque, int nchan,
                            fdctrl->data_pos, len);
             if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv),
                            fdctrl->fifo, BDRV_SECTOR_SIZE, 0) < 0) {
-                FLOPPY_DPRINTF("error writing sector %d\n",
-                               fd_sector(cur_drv));
+                warn_report("error writing sector %" PRIu32,
+                            fd_sector(cur_drv));
                 fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
                 goto transfer_error;
             }
@@ -1911,8 +1909,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
         if (pos == 0) {
             if (fdctrl->data_pos != 0)
                 if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
-                    FLOPPY_DPRINTF("error seeking to next sector %d\n",
-                                   fd_sector(cur_drv));
+                    warn_report("error seeking to next sector %" PRIu32,
+                                fd_sector(cur_drv));
                     return 0;
                 }
             if (blk_pread(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
@@ -1997,7 +1995,7 @@ static void fdctrl_format_sector(FDCtrl *fdctrl)
     if (cur_drv->blk == NULL ||
         blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
                    BDRV_SECTOR_SIZE, 0) < 0) {
-        FLOPPY_DPRINTF("error formatting sector %d\n", fd_sector(cur_drv));
+        warn_report("error formatting sector %" PRIu32, fd_sector(cur_drv));
         fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
     } else {
         if (cur_drv->sect == cur_drv->last_sect) {
@@ -2421,13 +2419,13 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
             cur_drv = get_cur_drv(fdctrl);
             if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
                            BDRV_SECTOR_SIZE, 0) < 0) {
-                FLOPPY_DPRINTF("error writing sector %d\n",
-                               fd_sector(cur_drv));
+                warn_report("error writing sector %" PRIu32,
+                            fd_sector(cur_drv));
                 break;
             }
             if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
-                FLOPPY_DPRINTF("error seeking to next sector %d\n",
-                               fd_sector(cur_drv));
+                warn_report("error seeking to next sector %" PRIu32,
+                            fd_sector(cur_drv));
                 break;
             }
         }
-- 
2.21.3

[PATCH-for-5.2 4/7] hw/block/fdc: Convert debug FLOPPY_DPRINTF() to trace events

[Philippe Mathieu-Daudé]

Convert debug FLOPPY_DPRINTF() to trace events.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/block/fdc.c        | 6 +++---
 hw/block/trace-events | 3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index ee45ec0b27..f9f3f3c079 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -326,7 +326,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
 /* Set drive back to track 0 */
 static void fd_recalibrate(FDrive *drv)
 {
-    FLOPPY_DPRINTF("recalibrate\n");
+    trace_floppy_recalibrate();
     fd_seek(drv, 0, 0, 1, 1);
 }
 
@@ -438,7 +438,7 @@ static void fd_revalidate(FDrive *drv)
 {
     int rc;
 
-    FLOPPY_DPRINTF("revalidate\n");
+    trace_floppy_revalidate();
     if (drv->blk != NULL) {
         drv->ro = blk_is_read_only(drv->blk);
         if (!blk_is_inserted(drv->blk)) {
@@ -1283,7 +1283,7 @@ static void fdctrl_reset(FDCtrl *fdctrl, int do_irq)
 {
     int i;
 
-    FLOPPY_DPRINTF("reset controller\n");
+    trace_fdc_reset();
     fdctrl_reset_irq(fdctrl);
     /* Initialise controller */
     fdctrl->sra = 0;
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 958fcc5508..9f7caf9b17 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -1,8 +1,11 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
 # fdc.c
+fdc_reset(void) ""
 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"
+floppy_recalibrate(void) ""
+floppy_revalidate(void) ""
 
 # pflash_cfi02.c
 # pflash_cfi01.c
-- 
2.21.3

[PATCH-for-5.2 5/7] hw/block/fdc: Drop pointless FLOPPY_DPRINTF() call

[Philippe Mathieu-Daudé]

Remove not very helpful debug call.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/block/fdc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index f9f3f3c079..278220ed29 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2636,7 +2636,6 @@ static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl,
         }
     }
 
-    FLOPPY_DPRINTF("init controller\n");
     fdctrl->fifo = qemu_memalign(512, FD_SECTOR_LEN);
     memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
     fdctrl->fifo_size = 512;
-- 
2.21.3

[PATCH-for-5.2 6/7] hw/block/fdc: Use more descriptive TypeInfo names

[Philippe Mathieu-Daudé]

Better name TypeInfo structures:

- ISA bus
- Common floppy controller
- Intel 82078 floppy controller
- SUN floppy controller

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/block/fdc.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 278220ed29..6944b06e4b 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2930,7 +2930,7 @@ static void isabus_fdc_instance_init(Object *obj)
                                   DEVICE(obj));
 }
 
-static const TypeInfo isa_fdc_info = {
+static const TypeInfo isabus_fdc_info = {
     .name          = TYPE_ISA_FDC,
     .parent        = TYPE_ISA_DEVICE,
     .instance_size = sizeof(FDCtrlISABus),
@@ -2971,7 +2971,7 @@ static void sysbus_fdc_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
 
-static const TypeInfo sysbus_fdc_info = {
+static const TypeInfo sysbus_fdc_i82078_info = {
     .name          = "sysbus-fdc",
     .parent        = TYPE_SYSBUS_FDC,
     .instance_init = sysbus_fdc_initfn,
@@ -2997,7 +2997,7 @@ static void sun4m_fdc_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
 
-static const TypeInfo sun4m_fdc_info = {
+static const TypeInfo sysbus_fdc_sun4m_info = {
     .name          = "SUNW,fdtwo",
     .parent        = TYPE_SYSBUS_FDC,
     .instance_init = sun4m_fdc_initfn,
@@ -3013,7 +3013,7 @@ static void sysbus_fdc_common_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_sysbus_fdc;
 }
 
-static const TypeInfo sysbus_fdc_type_info = {
+static const TypeInfo sysbus_fdc_common_info = {
     .name          = TYPE_SYSBUS_FDC,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(FDCtrlSysBus),
@@ -3024,10 +3024,12 @@ 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);
+    type_register_static(&isabus_fdc_info);
+
+    type_register_static(&sysbus_fdc_common_info);
+    type_register_static(&sysbus_fdc_i82078_info);
+    type_register_static(&sysbus_fdc_sun4m_info);
+
     type_register_static(&floppy_bus_info);
     type_register_static(&floppy_drive_info);
 }
-- 
2.21.3

[PATCH-for-5.2 7/7] hw/block/fdc: Add ASCII art schema of QOM relations

[Philippe Mathieu-Daudé]

Without knowing the QEMU history, it is hard to relate QEMU objects
with the hardware datasheet.

For example, one naively expects:

* a floppy disk is plugged / unplugged on the bus

  Wrong! QEMU floppy disks always sit on the bus. The block drives
  are plugged / unplugged on the disks, and the disks magically
  re-adapt their proprieties to match the block drive.

* a floppy controller has a fixed number of disks pluggable on the bus

  Wrong! QEMU floppy controllers have as much slots as the number of
  floppy drive provided when a machine is created. Then the ACPI table
  are generated and the number of slots can not be modified. So if you
  expect a dual slot controller being created with slot A and B, if
  the machine is created with a single drive attached, the controller
  will only have slot A created, and you will never be able to plug
  drive B without risking a mismatch in the ACPI tables.

* a floppy controller supporting 4 disks uses 2 buses

  Wrong! QEMU uses a single bus to plug the 4 disks.

As all these false assumptions are not obvious (we don't plug a disk,
we plug a block drive into a disk, etc...), start documenting the QOM
relationships with a simple ASCII schema.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/block/fdc.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 6944b06e4b..b109f37050 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -47,6 +47,28 @@
 #include "qemu/module.h"
 #include "trace.h"
 
+/*
+ * QOM relationship:
+ * =================
+ *
+ *                  +-------------------+
+ *                  |                   |
+ * isa/sysbus  <--->|                   |
+ *                  |                   |
+ *  irq/dma    <----|        fdc        |
+ *                  |
+ *      clk    ---->|                   |        +-+------+-+    +-+------+-+
+ *                  |                   |        | | blk  | |    | | blk  | |
+ *                  +--------+----------+        | |      | |    | |      | |
+ *                           |                   | +------+ |    | +------+ |
+ *                           |                   |          |    |          |
+ *                           |                   |  floppy  |    |  floppy  |
+ *                           |                   +----+-----+    +----+-----+
+ *                           |   floppy-bus           |               |
+ *                           +------------------------v---------------v---
+ *
+ */
+
 /********************************************************/
 /* debug Floppy devices */
 
-- 
2.21.3

Re: [PATCH-for-5.2 7/7] hw/block/fdc: Add ASCII art schema of QOM relations

[Kevin Wolf]

Am 06.08.2020 um 10:08 hat Philippe Mathieu-Daudé geschrieben:
> Without knowing the QEMU history, it is hard to relate QEMU objects
> with the hardware datasheet.
> 
> For example, one naively expects:
> 
> * a floppy disk is plugged / unplugged on the bus
> 
>   Wrong! QEMU floppy disks always sit on the bus. The block drives
>   are plugged / unplugged on the disks, and the disks magically
>   re-adapt their proprieties to match the block drive.

This is because what sits on the bus is not a floppy disk, but a floppy
drive. FloppyDrive is also what the type is called.

The disk is represented by the BlockDriverState (the actual image file)
that is inserted in the BlockBackend (which is logically part of the
drive).

> * a floppy controller has a fixed number of disks pluggable on the bus
> 
>   Wrong! QEMU floppy controllers have as much slots as the number of
>   floppy drive provided when a machine is created. Then the ACPI table
>   are generated and the number of slots can not be modified. So if you
>   expect a dual slot controller being created with slot A and B, if
>   the machine is created with a single drive attached, the controller
>   will only have slot A created, and you will never be able to plug
>   drive B without risking a mismatch in the ACPI tables.

Hm... I guess hotplugging floppy drives might actually have worked,
though I have never tried it on real hardware. I'm pretty sure it wasn't
an official feature, though, and ACPI tables certainly won't magically
change if you do this because (apart from polling, I guess) software has
no way to detect that you tinkered with the floppy cable. :-)

> * a floppy controller supporting 4 disks uses 2 buses
> 
>   Wrong! QEMU uses a single bus to plug the 4 disks.

But we don't even emulate floppy controllers that can have more than two
floppy drives:

    $ x86_64-softmmu/qemu-system-x86_64 -device floppy -device floppy -device floppy
    qemu-system-x86_64: -device floppy: Can't create floppy unit 2, bus supports only 2 units

This is checked in floppy_drive_realize(), so it applies to all
variants of the controller.

If you want more floppy drives, you have to create a second controller
(with a different iobase). Though I don't think I actually got this
working when I tried. I wasn't sure if the problem was the emulation or
the guest OSes (or SeaBIOS actually for DOS).

> As all these false assumptions are not obvious (we don't plug a disk,
> we plug a block drive into a disk, etc...), start documenting the QOM
> relationships with a simple ASCII schema.

Maybe be more specific to have: "floppy (drive)" and "blk (disk)".
Because the ASCII schema is actually true, though you seem to have
misunderstood what each item in it is supposed to represent.

Actually "blk (disk)" is not 100% accurate either because the drive
always has a BlockBackend present. It's really the BlockDriverState
inserted into the BlockBackend that is the disk.

In summary, to be honest, I believe since its qdevification, floppy is
one of the block devices that is modelled the best on the QOM + block
backend level. Only SCSI might be comparable, but IDE, virtio-blk and
usb-storage are a mess in comparison.

Kevin

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/block/fdc.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 6944b06e4b..b109f37050 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -47,6 +47,28 @@
>  #include "qemu/module.h"
>  #include "trace.h"
>  
> +/*
> + * QOM relationship:
> + * =================
> + *
> + *                  +-------------------+
> + *                  |                   |
> + * isa/sysbus  <--->|                   |
> + *                  |                   |
> + *  irq/dma    <----|        fdc        |
> + *                  |
> + *      clk    ---->|                   |        +-+------+-+    +-+------+-+
> + *                  |                   |        | | blk  | |    | | blk  | |
> + *                  +--------+----------+        | |      | |    | |      | |
> + *                           |                   | +------+ |    | +------+ |
> + *                           |                   |          |    |          |
> + *                           |                   |  floppy  |    |  floppy  |
> + *                           |                   +----+-----+    +----+-----+
> + *                           |   floppy-bus           |               |
> + *                           +------------------------v---------------v---
> + *
> + */
> +
>  /********************************************************/
>  /* debug Floppy devices */
>  
> -- 
> 2.21.3
> 

Re: [PATCH-for-5.2 7/7] hw/block/fdc: Add ASCII art schema of QOM relations

[Philippe Mathieu-Daudé]

On 8/6/20 10:57 AM, Kevin Wolf wrote:
> Am 06.08.2020 um 10:08 hat Philippe Mathieu-Daudé geschrieben:
>> Without knowing the QEMU history, it is hard to relate QEMU objects
>> with the hardware datasheet.
>>
>> For example, one naively expects:
>>
>> * a floppy disk is plugged / unplugged on the bus
>>
>>   Wrong! QEMU floppy disks always sit on the bus. The block drives
>>   are plugged / unplugged on the disks, and the disks magically
>>   re-adapt their proprieties to match the block drive.
> 
> This is because what sits on the bus is not a floppy disk, but a floppy
> drive. FloppyDrive is also what the type is called.
> 
> The disk is represented by the BlockDriverState (the actual image file)
> that is inserted in the BlockBackend (which is logically part of the
> drive).
> 
>> * a floppy controller has a fixed number of disks pluggable on the bus
>>
>>   Wrong! QEMU floppy controllers have as much slots as the number of
>>   floppy drive provided when a machine is created. Then the ACPI table
>>   are generated and the number of slots can not be modified. So if you
>>   expect a dual slot controller being created with slot A and B, if
>>   the machine is created with a single drive attached, the controller
>>   will only have slot A created, and you will never be able to plug
>>   drive B without risking a mismatch in the ACPI tables.
> 
> Hm... I guess hotplugging floppy drives might actually have worked,
> though I have never tried it on real hardware. I'm pretty sure it wasn't
> an official feature, though, and ACPI tables certainly won't magically
> change if you do this because (apart from polling, I guess) software has
> no way to detect that you tinkered with the floppy cable. :-)
> 
>> * a floppy controller supporting 4 disks uses 2 buses
>>
>>   Wrong! QEMU uses a single bus to plug the 4 disks.
> 
> But we don't even emulate floppy controllers that can have more than two
> floppy drives:
> 
>     $ x86_64-softmmu/qemu-system-x86_64 -device floppy -device floppy -device floppy
>     qemu-system-x86_64: -device floppy: Can't create floppy unit 2, bus supports only 2 units

This comment is for developers, the warning is for user.

It comes from:

    if (dev->unit >= MAX_FD) {
        error_setg(errp, "Can't create floppy unit %d, bus supports "
                   "only %d units", dev->unit, MAX_FD);
        return;
    }

But you can compile QEMU with MAX_FD=4:

static FDrive *get_drv(FDCtrl *fdctrl, int unit)
{
    switch (unit) {
        case 0: return drv0(fdctrl);
        case 1: return drv1(fdctrl);
#if MAX_FD == 4
        case 2: return drv2(fdctrl);
        case 3: return drv3(fdctrl);
#endif
        default: return NULL;
    }
}

ACPI also handles 4 slots:

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);
}


> 
> This is checked in floppy_drive_realize(), so it applies to all
> variants of the controller.
> 
> If you want more floppy drives, you have to create a second controller
> (with a different iobase). Though I don't think I actually got this
> working when I tried. I wasn't sure if the problem was the emulation or
> the guest OSes (or SeaBIOS actually for DOS).
> 
>> As all these false assumptions are not obvious (we don't plug a disk,
>> we plug a block drive into a disk, etc...), start documenting the QOM
>> relationships with a simple ASCII schema.
> 
> Maybe be more specific to have: "floppy (drive)" and "blk (disk)".
> Because the ASCII schema is actually true, though you seem to have
> misunderstood what each item in it is supposed to represent.
> 
> Actually "blk (disk)" is not 100% accurate either because the drive
> always has a BlockBackend present. It's really the BlockDriverState
> inserted into the BlockBackend that is the disk.
> 
> In summary, to be honest, I believe since its qdevification, floppy is
> one of the block devices that is modelled the best on the QOM + block
> backend level. Only SCSI might be comparable, but IDE, virtio-blk and
> usb-storage are a mess in comparison.

I'm sorry I didn't want to criticize the model or hurt you, I just want
to note the differences between how the controller is described in the
Intel 82078 datasheet and how the QEMU model works. Maybe I'm wrong
assuming there would be a 1:1 match.

I'll repost with the name updated in the schema and removing my
assumptions from the commit description that appears as simple
critics.

> 
> Kevin
> 
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/block/fdc.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index 6944b06e4b..b109f37050 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -47,6 +47,28 @@
>>  #include "qemu/module.h"
>>  #include "trace.h"
>>  
>> +/*
>> + * QOM relationship:
>> + * =================
>> + *
>> + *                  +-------------------+
>> + *                  |                   |
>> + * isa/sysbus  <--->|                   |
>> + *                  |                   |
>> + *  irq/dma    <----|        fdc        |
>> + *                  |
>> + *      clk    ---->|                   |        +-+------+-+    +-+------+-+
>> + *                  |                   |        | | blk  | |    | | blk  | |
>> + *                  +--------+----------+        | |      | |    | |      | |
>> + *                           |                   | +------+ |    | +------+ |
>> + *                           |                   |          |    |          |
>> + *                           |                   |  floppy  |    |  floppy  |
>> + *                           |                   +----+-----+    +----+-----+
>> + *                           |   floppy-bus           |               |
>> + *                           +------------------------v---------------v---
>> + *
>> + */
>> +
>>  /********************************************************/
>>  /* debug Floppy devices */
>>  
>> -- 
>> 2.21.3
>>
> 
> 

Re: [PATCH-for-5.2 7/7] hw/block/fdc: Add ASCII art schema of QOM relations

[Kevin Wolf]

Am 06.08.2020 um 12:33 hat Philippe Mathieu-Daudé geschrieben:
> On 8/6/20 10:57 AM, Kevin Wolf wrote:
> > Am 06.08.2020 um 10:08 hat Philippe Mathieu-Daudé geschrieben:
> >> Without knowing the QEMU history, it is hard to relate QEMU objects
> >> with the hardware datasheet.
> >>
> >> For example, one naively expects:
> >>
> >> * a floppy disk is plugged / unplugged on the bus
> >>
> >>   Wrong! QEMU floppy disks always sit on the bus. The block drives
> >>   are plugged / unplugged on the disks, and the disks magically
> >>   re-adapt their proprieties to match the block drive.
> > 
> > This is because what sits on the bus is not a floppy disk, but a floppy
> > drive. FloppyDrive is also what the type is called.
> > 
> > The disk is represented by the BlockDriverState (the actual image file)
> > that is inserted in the BlockBackend (which is logically part of the
> > drive).
> > 
> >> * a floppy controller has a fixed number of disks pluggable on the bus
> >>
> >>   Wrong! QEMU floppy controllers have as much slots as the number of
> >>   floppy drive provided when a machine is created. Then the ACPI table
> >>   are generated and the number of slots can not be modified. So if you
> >>   expect a dual slot controller being created with slot A and B, if
> >>   the machine is created with a single drive attached, the controller
> >>   will only have slot A created, and you will never be able to plug
> >>   drive B without risking a mismatch in the ACPI tables.
> > 
> > Hm... I guess hotplugging floppy drives might actually have worked,
> > though I have never tried it on real hardware. I'm pretty sure it wasn't
> > an official feature, though, and ACPI tables certainly won't magically
> > change if you do this because (apart from polling, I guess) software has
> > no way to detect that you tinkered with the floppy cable. :-)
> > 
> >> * a floppy controller supporting 4 disks uses 2 buses
> >>
> >>   Wrong! QEMU uses a single bus to plug the 4 disks.
> > 
> > But we don't even emulate floppy controllers that can have more than two
> > floppy drives:
> > 
> >     $ x86_64-softmmu/qemu-system-x86_64 -device floppy -device floppy -device floppy
> >     qemu-system-x86_64: -device floppy: Can't create floppy unit 2, bus supports only 2 units
> 
> This comment is for developers, the warning is for user.
> 
> It comes from:
> 
>     if (dev->unit >= MAX_FD) {
>         error_setg(errp, "Can't create floppy unit %d, bus supports "
>                    "only %d units", dev->unit, MAX_FD);
>         return;
>     }
> 
> But you can compile QEMU with MAX_FD=4:
> [...]

Ah, I wasn't aware that this was supposed to be changed.

I don't even have the spec here for a floppy controller that supports
four drives. My copy has "reserved" for those bits that apparently refer
to the additional drives (if the QEMU code is right).

> > This is checked in floppy_drive_realize(), so it applies to all
> > variants of the controller.
> > 
> > If you want more floppy drives, you have to create a second controller
> > (with a different iobase). Though I don't think I actually got this
> > working when I tried. I wasn't sure if the problem was the emulation or
> > the guest OSes (or SeaBIOS actually for DOS).
> > 
> >> As all these false assumptions are not obvious (we don't plug a disk,
> >> we plug a block drive into a disk, etc...), start documenting the QOM
> >> relationships with a simple ASCII schema.
> > 
> > Maybe be more specific to have: "floppy (drive)" and "blk (disk)".
> > Because the ASCII schema is actually true, though you seem to have
> > misunderstood what each item in it is supposed to represent.
> > 
> > Actually "blk (disk)" is not 100% accurate either because the drive
> > always has a BlockBackend present. It's really the BlockDriverState
> > inserted into the BlockBackend that is the disk.
> > 
> > In summary, to be honest, I believe since its qdevification, floppy is
> > one of the block devices that is modelled the best on the QOM + block
> > backend level. Only SCSI might be comparable, but IDE, virtio-blk and
> > usb-storage are a mess in comparison.
> 
> I'm sorry I didn't want to criticize the model or hurt you, I just want
> to note the differences between how the controller is described in the
> Intel 82078 datasheet and how the QEMU model works. Maybe I'm wrong
> assuming there would be a 1:1 match.

Sorry for not being clear. This isn't about criticism at all. I just
wanted to suggest that if you're trying to find out how to model new
devices, looking at floppy is probably a better start than looking at
most other block devices.

I don't know the floppy emulation well enough to say much about the
MAX_FD == 4 case, but the basic separation into controller and drive,
and that removable media are not represented in qdev, but just as block
nodes inserted into the BlockBackend of the drive, feels like the right
approach.

> I'll repost with the name updated in the schema and removing my
> assumptions from the commit description that appears as simple
> critics.

I think the most important part to fix in the commit message would be
the confusion between drives and disks.

Kevin