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

[Kevin Wolf]

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.

>>>
>>>   /********************************************************/
>>> @@ -937,6 +940,9 @@ static int fdctrl_media_changed(FDrive *drv)
>>>
>>>       if (!drv->bs)
>>>           return 0;
>>> +    /* This is needed for driver to detect there is no media in drive */
>>> +    if (!bdrv_is_inserted(drv->bs))
>>> +        return 1;
>> In which case is this required to detect that there is no media? After
>> eject? If so, why isn't the code in fdctrl_change_cb() enough?
>>
>> Or do you in fact need it for the initial state?
> As i wrote to Stefan,
> You can look here, http://www.isdaman.com/alsos/hardware/fdc/floppy.htm 
> , for specification of DIR register. Bit7 is there as CHAN and in this 
> bit is saved information whether media is changed or not. This bit is 
> set to true while there is no media. And floppy driver is checking this 
> bit to detect media change or media missing.
> 
> And this is needed for all cases if there is no media in drive. Code in 
> fdctrl_change_cb() is needed only for detect that there is no media when 
> you try to mount it (linux guest) or open it (windows guest).

Hm. There seems to be more wrong that just this. I think the main
problem is that we clear the bit after reading it out once, whereas it
seems to stay set in reality. It would only be cleared once some command
(not sure which are possible) succeeds with a newly inserted disk.

Unfortunately I couldn't really find any appropriate documentation for
the bit. The FDC spec says "DSKCHG monitors the pin of the same name and
reflects the opposite value seen on the disk cable", but I couldn't find
any description on how floppy drives set it on the cable.

Does anyone have some pointer to a spec for this?

Kevin