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
>>
> 
>