[Qemu-devel] [PATCH v2] fdc: fix media change detection for windows

[Qemu-devel] [PATCH v2] fdc: fix media change detection for windows

[Pavel Hrdina]

The Windows uses 'READ' command at the start of an instalation
without checking the 'dir' register. We have to abort the transfer
with an abnormal termination if there is no media in the drive.

We have to also check the 'media_change' bit in the 'fd_seek'. This
internal seek clears the 'media_change' bit, too, if there is
a media inserted.

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

diff --git a/hw/fdc.c b/hw/fdc.c
index 30d34e3..2d6dd30 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -127,8 +127,13 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
     uint32_t sector;
     int ret;
 
+    if (drv->bs != NULL && bdrv_is_inserted(drv->bs)) {
+        drv->media_changed = 0;
+    }
+
     if (track > drv->max_track ||
-        (head != 0 && (drv->flags & FDISK_DBL_SIDES) == 0)) {
+        (head != 0 && (drv->flags & FDISK_DBL_SIDES) == 0) ||
+        drv->media_changed) {
         FLOPPY_DPRINTF("try to read %d %02x %02x (max=%d %d %02x %02x)\n",
                        head, track, sect, 1,
                        (drv->flags & FDISK_DBL_SIDES) == 0 ? 0 : 1,
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH v2] fdc: fix media change detection for windows

[Pavel Hrdina]

Sorry, I forget the qtest. I'll create it and send it again.

Pavel

On 06/05/2012 05:46 PM, Pavel Hrdina wrote:
> The Windows uses 'READ' command at the start of an instalation
> without checking the 'dir' register. We have to abort the transfer
> with an abnormal termination if there is no media in the drive.
>
> We have to also check the 'media_change' bit in the 'fd_seek'. This
> internal seek clears the 'media_change' bit, too, if there is
> a media inserted.
>
> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
> ---
>   hw/fdc.c |    7 ++++++-
>   1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/hw/fdc.c b/hw/fdc.c
> index 30d34e3..2d6dd30 100644
> --- a/hw/fdc.c
> +++ b/hw/fdc.c
> @@ -127,8 +127,13 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
>       uint32_t sector;
>       int ret;
>
> +    if (drv->bs != NULL&&  bdrv_is_inserted(drv->bs)) {
> +        drv->media_changed = 0;
> +    }
> +
>       if (track>  drv->max_track ||
> -        (head != 0&&  (drv->flags&  FDISK_DBL_SIDES) == 0)) {
> +        (head != 0&&  (drv->flags&  FDISK_DBL_SIDES) == 0) ||
> +        drv->media_changed) {
>           FLOPPY_DPRINTF("try to read %d %02x %02x (max=%d %d %02x %02x)\n",
>                          head, track, sect, 1,
>                          (drv->flags&  FDISK_DBL_SIDES) == 0 ? 0 : 1,

Re: [Qemu-devel] [PATCH v2] fdc: fix media change detection for windows

[Kevin Wolf]

Am 05.06.2012 17:50, schrieb Pavel Hrdina:
> Sorry, I forget the qtest. I'll create it and send it again.
> 
> Pavel
> 
> On 06/05/2012 05:46 PM, Pavel Hrdina wrote:
>> The Windows uses 'READ' command at the start of an instalation
>> without checking the 'dir' register. We have to abort the transfer
>> with an abnormal termination if there is no media in the drive.
>>
>> We have to also check the 'media_change' bit in the 'fd_seek'. This
>> internal seek clears the 'media_change' bit, too, if there is
>> a media inserted.
>>
>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>> ---
>>   hw/fdc.c |    7 ++++++-
>>   1 files changed, 6 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/fdc.c b/hw/fdc.c
>> index 30d34e3..2d6dd30 100644
>> --- a/hw/fdc.c
>> +++ b/hw/fdc.c
>> @@ -127,8 +127,13 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
>>       uint32_t sector;
>>       int ret;
>>
>> +    if (drv->bs != NULL&&  bdrv_is_inserted(drv->bs)) {
>> +        drv->media_changed = 0;
>> +    }
>> +
>>       if (track>  drv->max_track ||
>> -        (head != 0&&  (drv->flags&  FDISK_DBL_SIDES) == 0)) {
>> +        (head != 0&&  (drv->flags&  FDISK_DBL_SIDES) == 0) ||
>> +        drv->media_changed) {

Why not directly use bdrv_is_inserted() here?

Fiddling around with media_changed feels rather hacky and is strictly
speaking incorrect because a step pulse is not guaranteed to happen in
the following code. The floppy code doesn't get it quite right today
anyway, but a hack like this would contribute to the problem.


>>           FLOPPY_DPRINTF("try to read %d %02x %02x (max=%d %d %02x %02x)\n",
>>                          head, track, sect, 1,
>>                          (drv->flags&  FDISK_DBL_SIDES) == 0 ? 0 : 1,

Kevin

Re: [Qemu-devel] [PATCH v2] fdc: fix media change detection for windows

[Pavel Hrdina]

On 06/05/2012 05:56 PM, Kevin Wolf wrote:
> Am 05.06.2012 17:50, schrieb Pavel Hrdina:
>> Sorry, I forget the qtest. I'll create it and send it again.
>>
>> Pavel
>>
>> On 06/05/2012 05:46 PM, Pavel Hrdina wrote:
>>> The Windows uses 'READ' command at the start of an instalation
>>> without checking the 'dir' register. We have to abort the transfer
>>> with an abnormal termination if there is no media in the drive.
>>>
>>> We have to also check the 'media_change' bit in the 'fd_seek'. This
>>> internal seek clears the 'media_change' bit, too, if there is
>>> a media inserted.
>>>
>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>>> ---
>>>    hw/fdc.c |    7 ++++++-
>>>    1 files changed, 6 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/hw/fdc.c b/hw/fdc.c
>>> index 30d34e3..2d6dd30 100644
>>> --- a/hw/fdc.c
>>> +++ b/hw/fdc.c
>>> @@ -127,8 +127,13 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
>>>        uint32_t sector;
>>>        int ret;
>>>
>>> +    if (drv->bs != NULL&&   bdrv_is_inserted(drv->bs)) {
>>> +        drv->media_changed = 0;
>>> +    }
>>> +
>>>        if (track>   drv->max_track ||
>>> -        (head != 0&&   (drv->flags&   FDISK_DBL_SIDES) == 0)) {
>>> +        (head != 0&&   (drv->flags&   FDISK_DBL_SIDES) == 0) ||
>>> +        drv->media_changed) {
> Why not directly use bdrv_is_inserted() here?
>
> Fiddling around with media_changed feels rather hacky and is strictly
> speaking incorrect because a step pulse is not guaranteed to happen in
> the following code. The floppy code doesn't get it quite right today
> anyway, but a hack like this would contribute to the problem.
>
>
>>>            FLOPPY_DPRINTF("try to read %d %02x %02x (max=%d %d %02x %02x)\n",
>>>                           head, track, sect, 1,
>>>                           (drv->flags&   FDISK_DBL_SIDES) == 0 ? 0 : 1,
> Kevin
>
I'll move the code into the end of 'fd_seek' function, where a step is 
guaranteed. Internal seek should also reset the 'media_changed' bit if 
there is a media in the drive. I'll create the qtest tomorrow then I'll 
send another version of this patch.

Pavel

Re: [Qemu-devel] [PATCH v2] fdc: fix media change detection for windows

[Kevin Wolf]

Am 05.06.2012 18:34, schrieb Pavel Hrdina:
> On 06/05/2012 05:56 PM, Kevin Wolf wrote:
>> Am 05.06.2012 17:50, schrieb Pavel Hrdina:
>>> Sorry, I forget the qtest. I'll create it and send it again.
>>>
>>> Pavel
>>>
>>> On 06/05/2012 05:46 PM, Pavel Hrdina wrote:
>>>> The Windows uses 'READ' command at the start of an instalation
>>>> without checking the 'dir' register. We have to abort the transfer
>>>> with an abnormal termination if there is no media in the drive.
>>>>
>>>> We have to also check the 'media_change' bit in the 'fd_seek'. This
>>>> internal seek clears the 'media_change' bit, too, if there is
>>>> a media inserted.
>>>>
>>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>>>> ---
>>>>    hw/fdc.c |    7 ++++++-
>>>>    1 files changed, 6 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/hw/fdc.c b/hw/fdc.c
>>>> index 30d34e3..2d6dd30 100644
>>>> --- a/hw/fdc.c
>>>> +++ b/hw/fdc.c
>>>> @@ -127,8 +127,13 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
>>>>        uint32_t sector;
>>>>        int ret;
>>>>
>>>> +    if (drv->bs != NULL&&   bdrv_is_inserted(drv->bs)) {
>>>> +        drv->media_changed = 0;
>>>> +    }
>>>> +
>>>>        if (track>   drv->max_track ||
>>>> -        (head != 0&&   (drv->flags&   FDISK_DBL_SIDES) == 0)) {
>>>> +        (head != 0&&   (drv->flags&   FDISK_DBL_SIDES) == 0) ||
>>>> +        drv->media_changed) {
>> Why not directly use bdrv_is_inserted() here?
>>
>> Fiddling around with media_changed feels rather hacky and is strictly
>> speaking incorrect because a step pulse is not guaranteed to happen in
>> the following code. The floppy code doesn't get it quite right today
>> anyway, but a hack like this would contribute to the problem.
>>
>>
>>>>            FLOPPY_DPRINTF("try to read %d %02x %02x (max=%d %d %02x %02x)\n",
>>>>                           head, track, sect, 1,
>>>>                           (drv->flags&   FDISK_DBL_SIDES) == 0 ? 0 : 1,
>> Kevin
>>
> I'll move the code into the end of 'fd_seek' function, where a step is 
> guaranteed. Internal seek should also reset the 'media_changed' bit if 
> there is a media in the drive. I'll create the qtest tomorrow then I'll 
> send another version of this patch.

My point is that the success of a seek has nothing to do with the DSKCHG
signal. Even if you move the code around, DSKCHG may start to coincide
with the condition you really want to check, but it's logically still
not what should be checked. What's wrong with !bdrv_is_inserted() in the
if condition?

If something is wrong with the media_changed handling, then it's a
separate bug that should be dealt with in a separate patch.

Kevin

Re: [Qemu-devel] [PATCH v2] fdc: fix media change detection for windows

[Pavel Hrdina]

[-- Attachment #1: Type: text/plain, Size: 3410 bytes --]



On 06/06/2012 12:35 PM, Kevin Wolf wrote:
> Am 05.06.2012 18:34, schrieb Pavel Hrdina:
>> On 06/05/2012 05:56 PM, Kevin Wolf wrote:
>>> Am 05.06.2012 17:50, schrieb Pavel Hrdina:
>>>> Sorry, I forget the qtest. I'll create it and send it again.
>>>>
>>>> Pavel
>>>>
>>>> On 06/05/2012 05:46 PM, Pavel Hrdina wrote:
>>>>> The Windows uses 'READ' command at the start of an instalation
>>>>> without checking the 'dir' register. We have to abort the transfer
>>>>> with an abnormal termination if there is no media in the drive.
>>>>>
>>>>> We have to also check the 'media_change' bit in the 'fd_seek'. This
>>>>> internal seek clears the 'media_change' bit, too, if there is
>>>>> a media inserted.
>>>>>
>>>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>>>>> ---
>>>>>     hw/fdc.c |    7 ++++++-
>>>>>     1 files changed, 6 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/hw/fdc.c b/hw/fdc.c
>>>>> index 30d34e3..2d6dd30 100644
>>>>> --- a/hw/fdc.c
>>>>> +++ b/hw/fdc.c
>>>>> @@ -127,8 +127,13 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
>>>>>         uint32_t sector;
>>>>>         int ret;
>>>>>
>>>>> +    if (drv->bs != NULL&&    bdrv_is_inserted(drv->bs)) {
>>>>> +        drv->media_changed = 0;
>>>>> +    }
>>>>> +
>>>>>         if (track>    drv->max_track ||
>>>>> -        (head != 0&&    (drv->flags&    FDISK_DBL_SIDES) == 0)) {
>>>>> +        (head != 0&&    (drv->flags&    FDISK_DBL_SIDES) == 0) ||
>>>>> +        drv->media_changed) {
>>> Why not directly use bdrv_is_inserted() here?
>>>
>>> Fiddling around with media_changed feels rather hacky and is strictly
>>> speaking incorrect because a step pulse is not guaranteed to happen in
>>> the following code. The floppy code doesn't get it quite right today
>>> anyway, but a hack like this would contribute to the problem.
>>>
>>>
>>>>>             FLOPPY_DPRINTF("try to read %d %02x %02x (max=%d %d %02x %02x)\n",
>>>>>                            head, track, sect, 1,
>>>>>                            (drv->flags&    FDISK_DBL_SIDES) == 0 ? 0 : 1,
>>> Kevin
>>>
>> I'll move the code into the end of 'fd_seek' function, where a step is
>> guaranteed. Internal seek should also reset the 'media_changed' bit if
>> there is a media in the drive. I'll create the qtest tomorrow then I'll
>> send another version of this patch.
> My point is that the success of a seek has nothing to do with the DSKCHG
> signal. Even if you move the code around, DSKCHG may start to coincide
> with the condition you really want to check, but it's logically still
> not what should be checked. What's wrong with !bdrv_is_inserted() in the
> if condition?
>
> If something is wrong with the media_changed handling, then it's a
> separate bug that should be dealt with in a separate patch.
>
> Kevin
I don't think that is true. Please read this 
http://wiki.osdev.org/Floppy_Disk_Controller#DIR_register.2C_Disk_Change_bit 
and http://wiki.osdev.org/Floppy_Disk_Controller#Seek . Directly it has 
nothing to do with DSKCHG bit, but indirectly seek to different cylinder 
fails if there is no media in drive on real FDC. Only successful seek to 
different cylinder clears the DSKCHG bit.

That's why I do it this way, because in our FDC emulation we clears the 
DSKCHG bit or not also if you seek to the same cylinder. The function 
'fd_seek' stands for implied seek which also clears the DSKCHG bit.

Pavel

[-- Attachment #2: Type: text/html, Size: 4654 bytes --]

Re: [Qemu-devel] [PATCH v2] fdc: fix media change detection for windows

[Kevin Wolf]

Am 06.06.2012 15:43, schrieb Pavel Hrdina:
>>> I'll move the code into the end of 'fd_seek' function, where a step is 
>>> guaranteed. Internal seek should also reset the 'media_changed' bit if 
>>> there is a media in the drive. I'll create the qtest tomorrow then I'll 
>>> send another version of this patch.
>> My point is that the success of a seek has nothing to do with the DSKCHG
>> signal. Even if you move the code around, DSKCHG may start to coincide
>> with the condition you really want to check, but it's logically still
>> not what should be checked. What's wrong with !bdrv_is_inserted() in the
>> if condition?
>>
>> If something is wrong with the media_changed handling, then it's a
>> separate bug that should be dealt with in a separate patch.
>>
>> Kevin
> I don't think that is true. Please read this
> http://wiki.osdev.org/Floppy_Disk_Controller#DIR_register.2C_Disk_Change_bit
> and http://wiki.osdev.org/Floppy_Disk_Controller#Seek . Directly it has
> nothing to do with DSKCHG bit, but indirectly seek to different cylinder
> fails if there is no media in drive on real FDC. Only successful seek to
> different cylinder clears the DSKCHG bit.

If you have the choice between a direct way (bdrv_is_inserted) and an
indirect one (DSKCHG), you should always use the directly related one.
It makes the intention of the code clearer and is less likely to break,
because you actually said what you want and not just something that you
believe happens at the same time.

So your explanation confirms for me that the check should be whether a
medium is in the drive, i.e. bdrv_is_inserted().

> That's why I do it this way, because in our FDC emulation we clears the
> DSKCHG bit or not also if you seek to the same cylinder. The function
> 'fd_seek' stands for implied seek which also clears the DSKCHG bit.

That the DSKCHG bit is cleared even if the head doesn't move is a bug in
our code. I accepted Hervé's patches because they were a big
improvement, but they don't implement quite correct behaviour yet.
Relying on buggy behaviour is obviously wrong.

Anyway, I'm not arguing against clearing DSKCHG in this function. I just
say that it addresses an unrelated problem and should be posted in a
different patch. And even then the updated DSKCHG isn't directly related
to the check you're introducing in this patch, so it still would have to
stay bdrv_is_inserted() there.

Kevin