Re: [Qemu-devel] [PATCH] Fix IDE FDC emulation for no media

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 24.04.2012 11:55, schrieb Pavel Hrdina:
>> On 04/24/2012 11:32 AM, Kevin Wolf wrote:
>>> Am 23.04.2012 18:06, schrieb Pavel Hrdina:
>>>> Hi,
>>>> this is the patch to fix incorrect handling of IDE floppy drive controller 
>>>> emulation
>>>> when no media is present. If the guest is booted without a media then the 
>>>> drive
>>>> was not being emulated at all but this patch enables the emulation with no 
>>>> media present.
>>>>
>>>> There was a bug in FDC emulation without media. Driver was not able to 
>>>> recognize that
>>>> there is no media in drive.
>>>>
>>>> This has been tested on both Fedora-16 x86_64 VM and Windows XP VM and the 
>>>> behaviour
>>>> is as expected, i.e. as follows:
>>>>
>>>> Linux guest (Fedora 16 x86_64) tries "mount /dev/fd0" and exit with error
>>>> "mount: /dev/fd0 is not a valid block device" which is the same behavior 
>>>> like
>>>> bare metal with real floppy device (you have to load floppy driver at first
>>>> using e.g. "modprobe floppy" command).
>>>>
>>>> For Windows XP guest the Windows floppy driver is trying to seek the 
>>>> virtual drive
>>>> when you want to open it but driver successfully detect that there is no 
>>>> media in drive
>>>> and then it's asking user to insert floppy media in the drive.
>>>>
>>>> I also tested behavior of this patch if you start guest with "-nodefaults" 
>>>> and both
>>>> Windows and Linux guests detect only FDC but no drive.
>>>>
>>>> Pavel
>>>>
>>>> This patch has been written with help of specifications from:
>>>> http://www.ousob.com/ng/hardware/ngd127.php
>>>> http://www.isdaman.com/alsos/hardware/fdc/floppy.htm
>>>> http://wiki.osdev.org/Floppy_Disk_Controller
>>>>
>>>> Signed-off-by: Pavel Hrdina<address@hidden>
>>>> Signed-off-by: Michal Novotny<address@hidden>
>>> It would be cool to have a qtest case for this. But I think we don't
>>> really have a nice way to talk to the qemu monitor yet, so I'm not
>>> requesting this before the patch can go in.
>>>
>>>> ---
>>>>   hw/fdc.c |   14 ++++++++++----
>>>>   hw/pc.c  |    3 ++-
>>>>   2 files changed, 12 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/fdc.c b/hw/fdc.c
>>>> index a0236b7..6791eff 100644
>>>> --- a/hw/fdc.c
>>>> +++ b/hw/fdc.c
>>>> @@ -179,12 +179,14 @@ static void fd_revalidate(FDrive *drv)
>>>>       FDriveRate rate;
>>>>
>>>>       FLOPPY_DPRINTF("revalidate\n");
>>>> -    if (drv->bs != NULL&&  bdrv_is_inserted(drv->bs)) {
>>>> +    if (drv->bs != NULL) {
>>>>           ro = bdrv_is_read_only(drv->bs);
>>>>           bdrv_get_floppy_geometry_hint(drv->bs,&nb_heads,&max_track,
>>>>                                         &last_sect, 
>>>> drv->drive,&drive,&rate);
>>> I'm not sure how your patch works, but I believe the behaviour of
>>> bdrv_get_floppy_geometry_hint might be one of the keys. If I understand
>>> correctly, it will just return the default geometry, which is one for
>>> 3.5" 1.44 MB floppies, or more precisely:
>>>
>>> { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, },
>>>
>>> Why it makes sense to have a medium geometry when there is no medium I
>>> haven't understood yet, but last_sect/max_track = 0 didn't seem to be
>>> enough for you. Do you know what exactly it is that makes your case work?
>>>
>>> ro has undefined value for a BlockDriverState with no medium, but I
>>> guess it doesn't hurt.
>> This modification is needed for floppy driver in guest to detect floppy 
>> drive.
>
> My question was more about how the floppy drivers in the guest detect
> drives. They can't be relying on the geometry of a medium because no
> medium is inserted. So setting the geometry must have some side effect
> that I'm not aware of.

One way to detect floppies is the CMOS floppy byte, and that way is
broken right now.  Have a look at pc_cmos_init():

    FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
[...]
    /* floppy type */
    if (floppy) {
        fdc_get_bs(fd, floppy);
        for (i = 0; i < 2; i++) {
            if (fd[i] && bdrv_is_inserted(fd[i])) {
                bdrv_get_floppy_geometry_hint(fd[i], &nb_heads, &max_track,
                                              &last_sect, FDRIVE_DRV_NONE,
                                              &fd_type[i], &rate);
            }
        }
    }
    val = (cmos_get_fd_drive_type(fd_type[0]) << 4) |
        cmos_get_fd_drive_type(fd_type[1]);
    rtc_set_memory(s, 0x10, val);

The drive type remains FDRIVE_DRV_NONE, which means "no drive
connected", unless the drive exists *and* has media.

Actually, guessing the drive type from media is problematic.  It should
be a qdev property.

See also https://bugzilla.redhat.com/show_bug.cgi?id=729244

[...]