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

[Kevin Wolf]

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.

> -        if (nb_heads != 0 && max_track != 0 && last_sect != 0) {
> -            FLOPPY_DPRINTF("User defined disk (%d %d %d)",
> +        if (!bdrv_is_inserted(drv->bs)) {
> +            FLOPPY_DPRINTF("No media in drive\n");
> +        } else if (nb_heads != 0 && max_track != 0 && last_sect != 0) {
> +            FLOPPY_DPRINTF("User defined disk (%d %d %d)\n",
>                             nb_heads - 1, max_track, last_sect);
>          } else {
>              FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n", nb_heads,
> @@ -201,11 +203,12 @@ static void fd_revalidate(FDrive *drv)
>          drv->drive = drive;
>          drv->media_rate = rate;
>      } else {
> -        FLOPPY_DPRINTF("No disk in drive\n");
> +        FLOPPY_DPRINTF("Drive disabled\n");
>          drv->last_sect = 0;
>          drv->max_track = 0;
>          drv->flags &= ~FDISK_DBL_SIDES;
>      }
> +
>  }
>  
>  /********************************************************/
> @@ -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?

Kevin