| Message ID | dd85b8dc8c3f336d950f2ade95e24d3bd5f62103.1335197193.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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);
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
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
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.
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
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); >
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
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
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
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
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.
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
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 [...]
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.
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
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);