Patchwork Fix IDE FDC emulation for no media

login
register
mail settings
Submitter Pavel Hrdina
Date April 23, 2012, 4:06 p.m.
Message ID <dd85b8dc8c3f336d950f2ade95e24d3bd5f62103.1335197193.git.phrdina@redhat.com>
Download mbox | patch
Permalink /patch/154552/
State New
Headers show

Comments

Pavel Hrdina - April 23, 2012, 4:06 p.m.
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 <phrdina@redhat.com>
Signed-off-by: Michal Novotny <minovotn@redhat.com>
---
 hw/fdc.c |   14 ++++++++++----
 hw/pc.c  |    3 ++-
 2 files changed, 12 insertions(+), 5 deletions(-)
Paolo Bonzini - April 24, 2012, 6:55 a.m.
Il 23/04/2012 18:06, Pavel Hrdina ha scritto:
> 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 <phrdina@redhat.com>
> Signed-off-by: Michal Novotny <minovotn@redhat.com>
> ---
>  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);
> -        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;
>      if (drv->media_changed) {
>          drv->media_changed = 0;
>          ret = 1;
> diff --git a/hw/pc.c b/hw/pc.c
> index 1f5aacb..29a604b 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -382,7 +382,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>      if (floppy) {
>          fdc_get_bs(fd, floppy);
>          for (i = 0; i < 2; i++) {
> -            if (fd[i] && bdrv_is_inserted(fd[i])) {
> +            /* If there is no media in a drive, we still have the drive present */
> +            if (fd[i]) {
>                  bdrv_get_floppy_geometry_hint(fd[i], &nb_heads, &max_track,
>                                                &last_sect, FDRIVE_DRV_NONE,
>                                                &fd_type[i], &rate);

Strictly speaking this final hunk should not be necessary: the fd_type
is by default FDRIVE_DRV_NONE and it is correct if there is no medium in
the drive.  However, it does not hurt.

The rest of the patch looks good.  It's strictly a bugfix and doesn't
change the hardware exposed to the guest (only the media), so
I think this does not require a compatibility property.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo
Pavel Hrdina - April 24, 2012, 7:39 a.m.
On 04/24/2012 08:55 AM, Paolo Bonzini wrote:
> Il 23/04/2012 18:06, Pavel Hrdina ha scritto:
>> 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<phrdina@redhat.com>
>> Signed-off-by: Michal Novotny<minovotn@redhat.com>
>> ---
>>   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);
>> -        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;
>>       if (drv->media_changed) {
>>           drv->media_changed = 0;
>>           ret = 1;
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 1f5aacb..29a604b 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -382,7 +382,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>       if (floppy) {
>>           fdc_get_bs(fd, floppy);
>>           for (i = 0; i<  2; i++) {
>> -            if (fd[i]&&  bdrv_is_inserted(fd[i])) {
>> +            /* If there is no media in a drive, we still have the drive present */
>> +            if (fd[i]) {
>>                   bdrv_get_floppy_geometry_hint(fd[i],&nb_heads,&max_track,
>>                                                 &last_sect, FDRIVE_DRV_NONE,
>>                                                 &fd_type[i],&rate);
> Strictly speaking this final hunk should not be necessary: the fd_type
> is by default FDRIVE_DRV_NONE and it is correct if there is no medium in
> the drive.  However, it does not hurt.
>
> The rest of the patch looks good.  It's strictly a bugfix and doesn't
> change the hardware exposed to the guest (only the media), so
> I think this does not require a compatibility property.
>
> Reviewed-by: Paolo Bonzini<pbonzini@redhat.com>
>
> Paolo
>
Hi,
there is bug that you cannot see any floppy drive in guest if you use 
defaults or set up a floppy drive without media. On bare-metal you can 
see floppy drive even without media.

For qemu you can check that there is floppy drive present by using qemu 
monitor and "info block". After you inserted media to the floppy drive 
on running guest, you still cannot see any floppy drive in guest.

This patch will set floppy drive to default values if you start guest 
with floppy drive without media and after you insert proper media to 
floppy drive you can access it from guest. And also this patch fixing 
"no media in drive" detection.

Pavel
Andreas Färber - April 24, 2012, 7:48 a.m.
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 <phrdina@redhat.com>
> Signed-off-by: Michal Novotny <minovotn@redhat.com>
> ---

Please see http://wiki.qemu.org/Contribute/SubmitAPatch for hints on
writing a commit message for a patch: Subject should have a prefix, such
as "fdc: Fix emulation for no media" and the body should not contain
personal letter-style contents such as "Hi, this is the patch". Any such
personal comments can go under the --- line where they are not committed
to git history.

Cc'ing floppy author and block maintainer.

Andreas

>  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);
> -        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;
>      if (drv->media_changed) {
>          drv->media_changed = 0;
>          ret = 1;
> diff --git a/hw/pc.c b/hw/pc.c
> index 1f5aacb..29a604b 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -382,7 +382,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>      if (floppy) {
>          fdc_get_bs(fd, floppy);
>          for (i = 0; i < 2; i++) {
> -            if (fd[i] && bdrv_is_inserted(fd[i])) {
> +            /* If there is no media in a drive, we still have the drive present */
> +            if (fd[i]) {
>                  bdrv_get_floppy_geometry_hint(fd[i], &nb_heads, &max_track,
>                                                &last_sect, FDRIVE_DRV_NONE,
>                                                &fd_type[i], &rate);
Stefan Hajnoczi - April 24, 2012, 9:06 a.m.
On Mon, Apr 23, 2012 at 5:06 PM, Pavel Hrdina <phrdina@redhat.com> wrote:
> Hi,
> this is the patch to fix incorrect handling of IDE floppy drive controller emulation

s/IDE//

It's unrelated to IDE.

> @@ -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;
>     if (drv->media_changed) {
>         drv->media_changed = 0;
>         ret = 1;

Why isn't the BlockDevOps.change_media_cb() mechanism enough to report
disk changes correctly?

Stefan
Kevin Wolf - April 24, 2012, 9:32 a.m.
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 <phrdina@redhat.com>
> Signed-off-by: Michal Novotny <minovotn@redhat.com>

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
Pavel Hrdina - April 24, 2012, 9:46 a.m.
On 04/24/2012 11:06 AM, Stefan Hajnoczi wrote:
> On Mon, Apr 23, 2012 at 5:06 PM, Pavel Hrdina<phrdina@redhat.com>  wrote:
>> Hi,
>> this is the patch to fix incorrect handling of IDE floppy drive controller emulation
> s/IDE//
>
> It's unrelated to IDE.
>
>> @@ -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;
>>      if (drv->media_changed) {
>>          drv->media_changed = 0;
>>          ret = 1;
> Why isn't the BlockDevOps.change_media_cb() mechanism enough to report
> disk changes correctly?
>
> 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.
Pavel Hrdina - April 24, 2012, 9:55 a.m.
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<phrdina@redhat.com>
>> Signed-off-by: Michal Novotny<minovotn@redhat.com>
> 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.

>> -        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;
>>       }
>> +
>>   }
This code is needed to set up default drive geometry so guest can detect 
floppy drive controller.
>>
>>   /********************************************************/
>> @@ -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).
>
> Kevin
Pavel Hrdina - April 24, 2012, 9:57 a.m.
On 04/24/2012 09:48 AM, Andreas Färber 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<phrdina@redhat.com>
>> Signed-off-by: Michal Novotny<minovotn@redhat.com>
>> ---
> Please see http://wiki.qemu.org/Contribute/SubmitAPatch for hints on
> writing a commit message for a patch: Subject should have a prefix, such
> as "fdc: Fix emulation for no media" and the body should not contain
> personal letter-style contents such as "Hi, this is the patch". Any such
> personal comments can go under the --- line where they are not committed
> to git history.
>
> Cc'ing floppy author and block maintainer.
>
> Andreas
Thanks for correction, I'll send v2 patch with right commit message.

Pavel
>>   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);
>> -        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;
>>       if (drv->media_changed) {
>>           drv->media_changed = 0;
>>           ret = 1;
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 1f5aacb..29a604b 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -382,7 +382,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>       if (floppy) {
>>           fdc_get_bs(fd, floppy);
>>           for (i = 0; i<  2; i++) {
>> -            if (fd[i]&&  bdrv_is_inserted(fd[i])) {
>> +            /* If there is no media in a drive, we still have the drive present */
>> +            if (fd[i]) {
>>                   bdrv_get_floppy_geometry_hint(fd[i],&nb_heads,&max_track,
>>                                                 &last_sect, FDRIVE_DRV_NONE,
>>                                                 &fd_type[i],&rate);
>
Kevin Wolf - April 24, 2012, 10:23 a.m.
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<phrdina@redhat.com>
>>> Signed-off-by: Michal Novotny<minovotn@redhat.com>
>> 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
Pavel Hrdina - April 24, 2012, 10:28 a.m.
On 04/24/2012 12:23 PM, Kevin Wolf wrote:
> 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<phrdina@redhat.com>
>>>> Signed-off-by: Michal Novotny<minovotn@redhat.com>
>>> 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.
Well, this is good question, I'll investigate little bit more about this 
and probably send new version of this patch.
>>>>    /********************************************************/
>>>> @@ -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?
This is proper behaviour, because driver will read DIR register to 
detect if there was media change. If it was, driver will run whatever he 
was doing with saved state, that there was media changed and check DIR 
again. If there is still bit7 setted to true, driver will decide that 
there is really no media in drive and end with this "error".
>
> Kevin
Kevin Wolf - April 24, 2012, 10:46 a.m.
Am 24.04.2012 12:28, schrieb Pavel Hrdina:
> On 04/24/2012 12:23 PM, Kevin Wolf wrote:
>> 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<phrdina@redhat.com>
>>>>> Signed-off-by: Michal Novotny<minovotn@redhat.com>
>>>> 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.
> Well, this is good question, I'll investigate little bit more about this 
> and probably send new version of this patch.

Ok, thanks. I think it's important to understand _why_ a patch works,
not just _that_ it works (for a specific case).

>>>>>    /********************************************************/
>>>>> @@ -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?
> This is proper behaviour, because driver will read DIR register to 
> detect if there was media change. If it was, driver will run whatever he 
> was doing with saved state, that there was media changed and check DIR 
> again. If there is still bit7 setted to true, driver will decide that 
> there is really no media in drive and end with this "error".

So I was pointed to a spec that actually says something about DSKCHG:

"This signal is active at power-on and latched inactive when a diskette
is present, the drive is selected and a step pulse occurs. This signal
goes active when the diskette is removed from the drive. The presence of
a diskette is determined by a sensor."

I guess we should change the logic of our emulation to match this.

Kevin
Stefan Hajnoczi - April 24, 2012, 11:38 a.m.
On Tue, Apr 24, 2012 at 10:46 AM, Pavel Hrdina <phrdina@redhat.com> wrote:
>
> On 04/24/2012 11:06 AM, Stefan Hajnoczi wrote:
>
> On Mon, Apr 23, 2012 at 5:06 PM, Pavel Hrdina <phrdina@redhat.com> wrote:
>
> Hi,
> this is the patch to fix incorrect handling of IDE floppy drive controller
> emulation
>
> s/IDE//
>
> It's unrelated to IDE.
>
> @@ -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;
>     if (drv->media_changed) {
>         drv->media_changed = 0;
>         ret = 1;
>
> Why isn't the BlockDevOps.change_media_cb() mechanism enough to report
> disk changes correctly?
>
> 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.

I can't find anything from your link that says the bit is set while
there is no media.

"Disk Change: 1 = disk changed since last command, 0 = disk not changed"

The "82078 44 PIN CHMOS SINGLE-CHIP FLOPPY DISK CONTROLLER" datasheet
(http://wiki.qemu.org/images/f/f0/29047403.pdf) doesn't give details
but suggests what you are saying.  It says "DSKCHG monitors the pin of
the same name and
reflects the opposite value seen on the disk cable".  So perhaps this
is really a level-triggered "is the drive empty?" bit.

This is still pretty unclear to me.  Also, if you need to implement
the level-triggered behavior, then can we remove/simplify/clean up the
existing media change code in hw/fdc.c?

Stefan
Pavel Hrdina - April 24, 2012, 12:05 p.m.
On 04/24/2012 01:38 PM, Stefan Hajnoczi wrote:
> On Tue, Apr 24, 2012 at 10:46 AM, Pavel Hrdina<phrdina@redhat.com>  wrote:
>> On 04/24/2012 11:06 AM, Stefan Hajnoczi wrote:
>>
>> On Mon, Apr 23, 2012 at 5:06 PM, Pavel Hrdina<phrdina@redhat.com>  wrote:
>>
>> Hi,
>> this is the patch to fix incorrect handling of IDE floppy drive controller
>> emulation
>>
>> s/IDE//
>>
>> It's unrelated to IDE.
>>
>> @@ -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;
>>      if (drv->media_changed) {
>>          drv->media_changed = 0;
>>          ret = 1;
>>
>> Why isn't the BlockDevOps.change_media_cb() mechanism enough to report
>> disk changes correctly?
>>
>> 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.
> I can't find anything from your link that says the bit is set while
> there is no media.
>
> "Disk Change: 1 = disk changed since last command, 0 = disk not changed"
>
> The "82078 44 PIN CHMOS SINGLE-CHIP FLOPPY DISK CONTROLLER" datasheet
> (http://wiki.qemu.org/images/f/f0/29047403.pdf) doesn't give details
> but suggests what you are saying.  It says "DSKCHG monitors the pin of
> the same name and
> reflects the opposite value seen on the disk cable".  So perhaps this
> is really a level-triggered "is the drive empty?" bit.
>
> This is still pretty unclear to me.  Also, if you need to implement
> the level-triggered behavior, then can we remove/simplify/clean up the
> existing media change code in hw/fdc.c?
>
> Stefan
I didn't find anything about that too, but I checked this behaviour on 
real floppy drive and it returns always this bit setted up without 
media. It could be probably good to rewrite media change code.
Hervé Poussineau - April 24, 2012, 7:19 p.m.
Hi,

Pavel Hrdina a écrit :
> On 04/24/2012 09:48 AM, Andreas Färber 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.
>>>
 >>> [...]

I just sent a patch to simplify media change handling [1].
Can you try to see if it fixes your problem?

Regards

Hervé

[1] http://article.gmane.org/gmane.comp.emulators.qemu/148187
Markus Armbruster - April 26, 2012, 2:58 p.m.
Kevin Wolf <kwolf@redhat.com> 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<phrdina@redhat.com>
>>>> Signed-off-by: Michal Novotny<minovotn@redhat.com>
>>> 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

[...]
Markus Armbruster - April 26, 2012, 3:13 p.m.
Pavel Hrdina <phrdina@redhat.com> writes:

> 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

As Andreas already pointed out, your commit message needs work.  Please
limit line length to 70 characters.

> 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 <phrdina@redhat.com>
> Signed-off-by: Michal Novotny <minovotn@redhat.com>
> ---
>  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);
> -        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;
>      if (drv->media_changed) {
>          drv->media_changed = 0;
>          ret = 1;
> diff --git a/hw/pc.c b/hw/pc.c
> index 1f5aacb..29a604b 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -382,7 +382,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>      if (floppy) {
>          fdc_get_bs(fd, floppy);
>          for (i = 0; i < 2; i++) {
> -            if (fd[i] && bdrv_is_inserted(fd[i])) {
> +            /* If there is no media in a drive, we still have the drive present */

This comment explains why you remove bdrv_is_inserted().  It makes no
sense once its gone.

> +            if (fd[i]) {
>                  bdrv_get_floppy_geometry_hint(fd[i], &nb_heads, &max_track,
>                                                &last_sect, FDRIVE_DRV_NONE,
>                                                &fd_type[i], &rate);

This fixes the CMOS floppy initialization bug I described elsewhere in
the thread, because bdrv_get_floppy_geometry_hint() picks FDRIVE_DRV_144
when its bs argument is empty.

Are you sure the patches to fdc.c are necessary to fix the bug you're
seeing?  To be honest, I doubt it.
Pavel Hrdina - April 29, 2012, 10:01 a.m.
On 04/26/2012 05:13 PM, Markus Armbruster wrote:
> Pavel Hrdina<phrdina@redhat.com>  writes:
>
>> 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
> As Andreas already pointed out, your commit message needs work.  Please
> limit line length to 70 characters.
>
>> 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<phrdina@redhat.com>
>> Signed-off-by: Michal Novotny<minovotn@redhat.com>
>> ---
>>   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);
>> -        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;
>>       if (drv->media_changed) {
>>           drv->media_changed = 0;
>>           ret = 1;
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 1f5aacb..29a604b 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -382,7 +382,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>       if (floppy) {
>>           fdc_get_bs(fd, floppy);
>>           for (i = 0; i<  2; i++) {
>> -            if (fd[i]&&  bdrv_is_inserted(fd[i])) {
>> +            /* If there is no media in a drive, we still have the drive present */
> This comment explains why you remove bdrv_is_inserted().  It makes no
> sense once its gone.
>
>> +            if (fd[i]) {
>>                   bdrv_get_floppy_geometry_hint(fd[i],&nb_heads,&max_track,
>>                                                 &last_sect, FDRIVE_DRV_NONE,
>>                                                 &fd_type[i],&rate);
> This fixes the CMOS floppy initialization bug I described elsewhere in
> the thread, because bdrv_get_floppy_geometry_hint() picks FDRIVE_DRV_144
> when its bs argument is empty.
>
> Are you sure the patches to fdc.c are necessary to fix the bug you're
> seeing?  To be honest, I doubt it.
Hi Markus,

yes, you are right that this is only needed to fix that bug and fixes to 
fdc.c in this patch isn't necessary to fix this bug.

But I also need to fix media detection behaviour in fdc.c otherwise 
windows guest will print error message while opening floppy drive and 
kernel guest ends with kernel panic while mounting floppy drive.

I have already started rewriting this patch.

Pavel

Patch

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);
-        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;
     if (drv->media_changed) {
         drv->media_changed = 0;
         ret = 1;
diff --git a/hw/pc.c b/hw/pc.c
index 1f5aacb..29a604b 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -382,7 +382,8 @@  void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
     if (floppy) {
         fdc_get_bs(fd, floppy);
         for (i = 0; i < 2; i++) {
-            if (fd[i] && bdrv_is_inserted(fd[i])) {
+            /* If there is no media in a drive, we still have the drive present */
+            if (fd[i]) {
                 bdrv_get_floppy_geometry_hint(fd[i], &nb_heads, &max_track,
                                               &last_sect, FDRIVE_DRV_NONE,
                                               &fd_type[i], &rate);