[Qemu-devel] [PATCH 0/2] fdc: fix media handling

[Qemu-devel] [PATCH 0/2] fdc: fix media handling

[Pavel Hrdina]

This patch series fixes handling of FDC when you don't have media inserted.
Guest should see floppy drive if you start guest without media and should
detect that there is no media in drive.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>

Pavel Hrdina (2):
  fdc: floppy drive should be visible after start without media
  fdc: fix media detection

 hw/fdc.c |    8 ++------
 hw/pc.c  |    2 +-
 2 files changed, 3 insertions(+), 7 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH 1/2] fdc: floppy drive should be visible after start without media

[Pavel Hrdina]

If you start guest with floppy drive but without media inserted,
the guest now can see floppy drive.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 hw/pc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 4d34a33..967c17a 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -382,7 +382,7 @@ 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 (fd[i]) {
                 bdrv_get_floppy_geometry_hint(fd[i], &nb_heads, &max_track,
                                               &last_sect, FDRIVE_DRV_NONE,
                                               &fd_type[i], &rate);
-- 
1.7.7.6

[Qemu-devel] [PATCH 2/2] fdc: fix media detection

[Pavel Hrdina]

We have to set up 'media_changed' after guest start so floppy driver
could detect that there is no media in drive. For this purpose we call
'fdctrl_change_cb' instead of 'fd_revalidate' in 'fdctrl_connect_drives'.
'fd_revalidate' is called inside 'fdctrl_change_cb'.

In 'fdctrl_handle_seek' we always set current track because we don't care
if there is media inserted or not.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 hw/fdc.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index cb4cd25..337b35a 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1617,11 +1617,7 @@ static void fdctrl_handle_seek(FDCtrl *fdctrl, int direction)
     /* The seek command just sends step pulses to the drive and doesn't care if
      * there is a medium inserted of if it's banging the head against the drive.
      */
-    if (fdctrl->fifo[2] > cur_drv->max_track) {
-        cur_drv->track = cur_drv->max_track;
-    } else {
-        cur_drv->track = fdctrl->fifo[2];
-    }
+    cur_drv->track = fdctrl->fifo[2];
     /* Raise Interrupt */
     fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
 }
@@ -1878,7 +1874,7 @@ static int fdctrl_connect_drives(FDCtrl *fdctrl)
         }
 
         fd_init(drive);
-        fd_revalidate(drive);
+        fdctrl_change_cb(drive, 0);
         if (drive->bs) {
             bdrv_set_dev_ops(drive->bs, &fdctrl_block_ops, drive);
         }
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 2/2] fdc: fix media detection

[Kevin Wolf]

Am 22.05.2012 12:59, schrieb Pavel Hrdina:
> We have to set up 'media_changed' after guest start so floppy driver
> could detect that there is no media in drive. For this purpose we call
> 'fdctrl_change_cb' instead of 'fd_revalidate' in 'fdctrl_connect_drives'.
> 'fd_revalidate' is called inside 'fdctrl_change_cb'.
> 
> In 'fdctrl_handle_seek' we always set current track because we don't care
> if there is media inserted or not.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>

Can you please add a qtest case that shows the problems that you're
fixing in this series?

> diff --git a/hw/fdc.c b/hw/fdc.c
> index cb4cd25..337b35a 100644
> --- a/hw/fdc.c
> +++ b/hw/fdc.c
> @@ -1617,11 +1617,7 @@ static void fdctrl_handle_seek(FDCtrl *fdctrl, int direction)
>      /* The seek command just sends step pulses to the drive and doesn't care if
>       * there is a medium inserted of if it's banging the head against the drive.
>       */
> -    if (fdctrl->fifo[2] > cur_drv->max_track) {
> -        cur_drv->track = cur_drv->max_track;
> -    } else {
> -        cur_drv->track = fdctrl->fifo[2];
> -    }
> +    cur_drv->track = fdctrl->fifo[2];

Why is it okay to have cur_drv->track point outside the floppy? Won't it
mess up future calculations? Not all other places check it again
cur_drv->max_track.

Kevin

Re: [Qemu-devel] [PATCH 2/2] fdc: fix media detection

[Pavel Hrdina]

On 05/22/2012 02:01 PM, Kevin Wolf wrote:
> Am 22.05.2012 12:59, schrieb Pavel Hrdina:
>> We have to set up 'media_changed' after guest start so floppy driver
>> could detect that there is no media in drive. For this purpose we call
>> 'fdctrl_change_cb' instead of 'fd_revalidate' in 'fdctrl_connect_drives'.
>> 'fd_revalidate' is called inside 'fdctrl_change_cb'.
>>
>> In 'fdctrl_handle_seek' we always set current track because we don't care
>> if there is media inserted or not.
>>
>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
> Can you please add a qtest case that shows the problems that you're
> fixing in this series?
I'm new to qemu. By "add a qtest case" you mean update tests/fdc-test.c ?
>> diff --git a/hw/fdc.c b/hw/fdc.c
>> index cb4cd25..337b35a 100644
>> --- a/hw/fdc.c
>> +++ b/hw/fdc.c
>> @@ -1617,11 +1617,7 @@ static void fdctrl_handle_seek(FDCtrl *fdctrl, int direction)
>>       /* The seek command just sends step pulses to the drive and doesn't care if
>>        * there is a medium inserted of if it's banging the head against the drive.
>>        */
>> -    if (fdctrl->fifo[2]>  cur_drv->max_track) {
>> -        cur_drv->track = cur_drv->max_track;
>> -    } else {
>> -        cur_drv->track = fdctrl->fifo[2];
>> -    }
>> +    cur_drv->track = fdctrl->fifo[2];
> Why is it okay to have cur_drv->track point outside the floppy? Won't it
> mess up future calculations? Not all other places check it again
> cur_drv->max_track.
>
> Kevin
Well, you are right. Than we have to set 'max_track' even if there is no 
media. I tested this on bare-metal without media and where floppy driver 
ask to seek to specific track, it ends good and return specific track 
position as actual.
I'll rewrite this behavior and send patch v2.

Re: [Qemu-devel] [PATCH 2/2] fdc: fix media detection

[Kevin Wolf]

Am 22.05.2012 15:28, schrieb Pavel Hrdina:
> On 05/22/2012 02:01 PM, Kevin Wolf wrote:
>> Am 22.05.2012 12:59, schrieb Pavel Hrdina:
>>> We have to set up 'media_changed' after guest start so floppy driver
>>> could detect that there is no media in drive. For this purpose we call
>>> 'fdctrl_change_cb' instead of 'fd_revalidate' in 'fdctrl_connect_drives'.
>>> 'fd_revalidate' is called inside 'fdctrl_change_cb'.
>>>
>>> In 'fdctrl_handle_seek' we always set current track because we don't care
>>> if there is media inserted or not.
>>>
>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>> Can you please add a qtest case that shows the problems that you're
>> fixing in this series?
> I'm new to qemu. By "add a qtest case" you mean update tests/fdc-test.c ?

Sorry, I should have been more specific. Yes, that's what I mean.

>>> diff --git a/hw/fdc.c b/hw/fdc.c
>>> index cb4cd25..337b35a 100644
>>> --- a/hw/fdc.c
>>> +++ b/hw/fdc.c
>>> @@ -1617,11 +1617,7 @@ static void fdctrl_handle_seek(FDCtrl *fdctrl, int direction)
>>>       /* The seek command just sends step pulses to the drive and doesn't care if
>>>        * there is a medium inserted of if it's banging the head against the drive.
>>>        */
>>> -    if (fdctrl->fifo[2]>  cur_drv->max_track) {
>>> -        cur_drv->track = cur_drv->max_track;
>>> -    } else {
>>> -        cur_drv->track = fdctrl->fifo[2];
>>> -    }
>>> +    cur_drv->track = fdctrl->fifo[2];
>> Why is it okay to have cur_drv->track point outside the floppy? Won't it
>> mess up future calculations? Not all other places check it again
>> cur_drv->max_track.
>>
>> Kevin
> Well, you are right. Than we have to set 'max_track' even if there is no 
> media. I tested this on bare-metal without media and where floppy driver 
> ask to seek to specific track, it ends good and return specific track 
> position as actual.
> I'll rewrite this behavior and send patch v2.

You mean max_track = 0 isn't a good value to work with? How can a real
drive position the head correctly when it doesn't have a media (and
therefore doesn't know its geometry)?

But if you have a good default value for max_track that we should use
when no medium is present, go ahead.

Kevin

Re: [Qemu-devel] [PATCH 2/2] fdc: fix media detection

[Pavel Hrdina]

On 05/22/2012 03:42 PM, Kevin Wolf wrote:
> Am 22.05.2012 15:28, schrieb Pavel Hrdina:
>> On 05/22/2012 02:01 PM, Kevin Wolf wrote:
>>> Am 22.05.2012 12:59, schrieb Pavel Hrdina:
>>>> We have to set up 'media_changed' after guest start so floppy driver
>>>> could detect that there is no media in drive. For this purpose we call
>>>> 'fdctrl_change_cb' instead of 'fd_revalidate' in 'fdctrl_connect_drives'.
>>>> 'fd_revalidate' is called inside 'fdctrl_change_cb'.
>>>>
>>>> In 'fdctrl_handle_seek' we always set current track because we don't care
>>>> if there is media inserted or not.
>>>>
>>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>>> Can you please add a qtest case that shows the problems that you're
>>> fixing in this series?
>> I'm new to qemu. By "add a qtest case" you mean update tests/fdc-test.c ?
> Sorry, I should have been more specific. Yes, that's what I mean.
>
>>>> diff --git a/hw/fdc.c b/hw/fdc.c
>>>> index cb4cd25..337b35a 100644
>>>> --- a/hw/fdc.c
>>>> +++ b/hw/fdc.c
>>>> @@ -1617,11 +1617,7 @@ static void fdctrl_handle_seek(FDCtrl *fdctrl, int direction)
>>>>        /* The seek command just sends step pulses to the drive and doesn't care if
>>>>         * there is a medium inserted of if it's banging the head against the drive.
>>>>         */
>>>> -    if (fdctrl->fifo[2]>   cur_drv->max_track) {
>>>> -        cur_drv->track = cur_drv->max_track;
>>>> -    } else {
>>>> -        cur_drv->track = fdctrl->fifo[2];
>>>> -    }
>>>> +    cur_drv->track = fdctrl->fifo[2];
>>> Why is it okay to have cur_drv->track point outside the floppy? Won't it
>>> mess up future calculations? Not all other places check it again
>>> cur_drv->max_track.
>>>
>>> Kevin
>> Well, you are right. Than we have to set 'max_track' even if there is no
>> media. I tested this on bare-metal without media and where floppy driver
>> ask to seek to specific track, it ends good and return specific track
>> position as actual.
>> I'll rewrite this behavior and send patch v2.
> You mean max_track = 0 isn't a good value to work with? How can a real
> drive position the head correctly when it doesn't have a media (and
> therefore doesn't know its geometry)?
>
> But if you have a good default value for max_track that we should use
> when no medium is present, go ahead.
>
> Kevin
When you try mount floppy in linux while there is no media then a floppy 
driver tries to seek on track 1. Virtual guest always get actual track 
position 0 so tries to seek again and stuck in loop which cause kernel 
panic. On bare-metal floppy driver gets actual track position 1 and stop 
seeking.

I thing, that a real drive ignore geometry and just seeks to specific 
position.

Pavel