[Qemu-devel] [PATCH 0/2] More RELATIVE SEEK fun

[Qemu-devel] [PATCH 0/2] More RELATIVE SEEK fun

[Kevin Wolf]

Hi Pavel,

you said you have a real floppy drive around. Would you mind giving the cases
from patch 2 a try on it? The spec wasn't entirely clear to me, so the values
in the test case depend more on guessing than on knowledge.

I'm pretty sure that qemu is buggy there, though, so if you like to fix first
the test according to real hardware and then qemu to pass the test, that would
be great.

Kevin


Kevin Wolf (2):
  fdc-test: Clean up a bit
  fdc-test: Check RELATIVE SEEK beyond track 0/80

 tests/fdc-test.c |   72 ++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 51 insertions(+), 21 deletions(-)

-- 
1.7.6.5

[Qemu-devel] [PATCH 1/2] fdc-test: Clean up a bit

[Kevin Wolf]

Readability of the test code has suffered as the test case evolved. This
should improve it a bit again.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/fdc-test.c |   36 ++++++++++++++++++++----------------
 1 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index 10d11a4..fa74411 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -93,17 +93,21 @@ static uint8_t floppy_recv(void)
     return inb(FLOPPY_BASE + reg_fifo);
 }
 
-static uint8_t ack_irq(void)
+/* pcn: Present Cylinder Number */
+static void ack_irq(uint8_t *pcn)
 {
     uint8_t ret;
 
     g_assert(get_irq(FLOPPY_IRQ));
     floppy_send(CMD_SENSE_INT);
     floppy_recv();
+
     ret = floppy_recv();
-    g_assert(!get_irq(FLOPPY_IRQ));
+    if (pcn != NULL) {
+        *pcn = ret;
+    }
 
-    return ret;
+    g_assert(!get_irq(FLOPPY_IRQ));
 }
 
 static uint8_t send_read_command(void)
@@ -162,7 +166,7 @@ static uint8_t send_read_command(void)
     return ret;
 }
 
-static void send_step_pulse(int cyl)
+static void send_seek(int cyl)
 {
     int drive = 0;
     int head = 0;
@@ -171,7 +175,7 @@ static void send_step_pulse(int cyl)
     floppy_send(head << 2 | drive);
     g_assert(!get_irq(FLOPPY_IRQ));
     floppy_send(cyl);
-    ack_irq();
+    ack_irq(NULL);
 }
 
 static uint8_t cmos_read(uint8_t reg)
@@ -198,7 +202,7 @@ static void test_no_media_on_start(void)
     assert_bit_set(dir, DSKCHG);
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
-    send_step_pulse(1);
+    send_seek(1);
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
     dir = inb(FLOPPY_BASE + reg_dir);
@@ -229,14 +233,14 @@ static void test_media_change(void)
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
 
-    send_step_pulse(0);
+    send_seek(0);
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
 
     /* Step to next track should clear DSKCHG bit. */
-    send_step_pulse(1);
+    send_seek(1);
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_clear(dir, DSKCHG);
     dir = inb(FLOPPY_BASE + reg_dir);
@@ -252,13 +256,13 @@ static void test_media_change(void)
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
 
-    send_step_pulse(0);
+    send_seek(0);
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
 
-    send_step_pulse(1);
+    send_seek(1);
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
     dir = inb(FLOPPY_BASE + reg_dir);
@@ -292,10 +296,10 @@ static void test_relative_seek(void)
     uint8_t drive = 0;
     uint8_t head = 0;
     uint8_t cyl = 1;
-    uint8_t ret;
+    uint8_t pcn;
 
     /* Send seek to track 0 */
-    send_step_pulse(0);
+    send_seek(0);
 
     /* Send relative seek to increase track by 1 */
     floppy_send(CMD_RELATIVE_SEEK_IN);
@@ -303,8 +307,8 @@ static void test_relative_seek(void)
     g_assert(!get_irq(FLOPPY_IRQ));
     floppy_send(cyl);
 
-    ret = ack_irq();
-    g_assert(ret == 1);
+    ack_irq(&pcn);
+    g_assert(pcn == 1);
 
     /* Send relative seek to decrease track by 1 */
     floppy_send(CMD_RELATIVE_SEEK_OUT);
@@ -312,8 +316,8 @@ static void test_relative_seek(void)
     g_assert(!get_irq(FLOPPY_IRQ));
     floppy_send(cyl);
 
-    ret = ack_irq();
-    g_assert(ret == 0);
+    ack_irq(&pcn);
+    g_assert(pcn == 0);
 }
 
 /* success if no crash or abort */
-- 
1.7.6.5

[Qemu-devel] [PATCH 2/2] fdc-test: Check RELATIVE SEEK beyond track 0/80

[Kevin Wolf]

TODO This needs to be checked against a real drive

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/fdc-test.c |   48 +++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index fa74411..56e745a 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -94,13 +94,17 @@ static uint8_t floppy_recv(void)
 }
 
 /* pcn: Present Cylinder Number */
-static void ack_irq(uint8_t *pcn)
+static void ack_irq(uint8_t *st0, uint8_t *pcn)
 {
     uint8_t ret;
 
     g_assert(get_irq(FLOPPY_IRQ));
     floppy_send(CMD_SENSE_INT);
-    floppy_recv();
+
+    ret = floppy_recv();
+    if (st0 != NULL) {
+        *st0 = ret;
+    }
 
     ret = floppy_recv();
     if (pcn != NULL) {
@@ -175,7 +179,7 @@ static void send_seek(int cyl)
     floppy_send(head << 2 | drive);
     g_assert(!get_irq(FLOPPY_IRQ));
     floppy_send(cyl);
-    ack_irq(NULL);
+    ack_irq(NULL, NULL);
 }
 
 static uint8_t cmos_read(uint8_t reg)
@@ -295,29 +299,51 @@ static void test_relative_seek(void)
 {
     uint8_t drive = 0;
     uint8_t head = 0;
-    uint8_t cyl = 1;
     uint8_t pcn;
+    uint8_t st0;
 
     /* Send seek to track 0 */
     send_seek(0);
 
-    /* Send relative seek to increase track by 1 */
+    /* Send relative seek to increase track by 3 */
     floppy_send(CMD_RELATIVE_SEEK_IN);
     floppy_send(head << 2 | drive);
     g_assert(!get_irq(FLOPPY_IRQ));
-    floppy_send(cyl);
+    floppy_send(3);
 
-    ack_irq(&pcn);
-    g_assert(pcn == 1);
+    ack_irq(&st0, &pcn);
+    g_assert_cmpint(pcn, ==, 3);
+    g_assert_cmpint(st0, ==, 0x20);
 
     /* Send relative seek to decrease track by 1 */
     floppy_send(CMD_RELATIVE_SEEK_OUT);
     floppy_send(head << 2 | drive);
     g_assert(!get_irq(FLOPPY_IRQ));
-    floppy_send(cyl);
+    floppy_send(1);
+
+    ack_irq(&st0, &pcn);
+    g_assert_cmpint(pcn, ==, 2);
+    g_assert_cmpint(st0, ==, 0x20);
+
+    /* Send relative seek to beyond track 0 */
+    floppy_send(CMD_RELATIVE_SEEK_OUT);
+    floppy_send(head << 2 | drive);
+    g_assert(!get_irq(FLOPPY_IRQ));
+    floppy_send(42);
+
+    ack_irq(&st0, &pcn);
+    g_assert_cmpint(pcn, ==, 0);
+    g_assert_cmpint(st0, ==, 0x70);
+
+    /* Send try relative seek to beyond track 80 */
+    floppy_send(CMD_RELATIVE_SEEK_IN);
+    floppy_send(head << 2 | drive);
+    g_assert(!get_irq(FLOPPY_IRQ));
+    floppy_send(200);
 
-    ack_irq(&pcn);
-    g_assert(pcn == 0);
+    ack_irq(&st0, &pcn);
+    g_assert_cmpint(pcn, ==, 79);
+    g_assert_cmpint(st0, ==, 0x50);
 }
 
 /* success if no crash or abort */
-- 
1.7.6.5

Re: [Qemu-devel] [PATCH 2/2] fdc-test: Check RELATIVE SEEK beyond track 0/80

[Pavel Hrdina]

On 07/16/2012 04:36 PM, Kevin Wolf wrote:
> TODO This needs to be checked against a real drive
>
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> ---
>   tests/fdc-test.c |   48 +++++++++++++++++++++++++++++++++++++-----------
>   1 files changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/tests/fdc-test.c b/tests/fdc-test.c
> index fa74411..56e745a 100644
> --- a/tests/fdc-test.c
> +++ b/tests/fdc-test.c
> @@ -94,13 +94,17 @@ static uint8_t floppy_recv(void)
>   }
>   
>   /* pcn: Present Cylinder Number */
> -static void ack_irq(uint8_t *pcn)
> +static void ack_irq(uint8_t *st0, uint8_t *pcn)
>   {
>       uint8_t ret;
>   
>       g_assert(get_irq(FLOPPY_IRQ));
>       floppy_send(CMD_SENSE_INT);
> -    floppy_recv();
> +
> +    ret = floppy_recv();
> +    if (st0 != NULL) {
> +        *st0 = ret;
> +    }
>   
>       ret = floppy_recv();
>       if (pcn != NULL) {
> @@ -175,7 +179,7 @@ static void send_seek(int cyl)
>       floppy_send(head << 2 | drive);
>       g_assert(!get_irq(FLOPPY_IRQ));
>       floppy_send(cyl);
> -    ack_irq(NULL);
> +    ack_irq(NULL, NULL);
>   }
>   
>   static uint8_t cmos_read(uint8_t reg)
> @@ -295,29 +299,51 @@ static void test_relative_seek(void)
>   {
>       uint8_t drive = 0;
>       uint8_t head = 0;
> -    uint8_t cyl = 1;
>       uint8_t pcn;
> +    uint8_t st0;
>   
>       /* Send seek to track 0 */
>       send_seek(0);
>   
> -    /* Send relative seek to increase track by 1 */
> +    /* Send relative seek to increase track by 3 */
>       floppy_send(CMD_RELATIVE_SEEK_IN);
>       floppy_send(head << 2 | drive);
>       g_assert(!get_irq(FLOPPY_IRQ));
> -    floppy_send(cyl);
> +    floppy_send(3);
>   
> -    ack_irq(&pcn);
> -    g_assert(pcn == 1);
> +    ack_irq(&st0, &pcn);
> +    g_assert_cmpint(pcn, ==, 3);
> +    g_assert_cmpint(st0, ==, 0x20);
>   
>       /* Send relative seek to decrease track by 1 */
>       floppy_send(CMD_RELATIVE_SEEK_OUT);
>       floppy_send(head << 2 | drive);
>       g_assert(!get_irq(FLOPPY_IRQ));
> -    floppy_send(cyl);
> +    floppy_send(1);
> +
> +    ack_irq(&st0, &pcn);
> +    g_assert_cmpint(pcn, ==, 2);
> +    g_assert_cmpint(st0, ==, 0x20);
> +
> +    /* Send relative seek to beyond track 0 */
> +    floppy_send(CMD_RELATIVE_SEEK_OUT);
> +    floppy_send(head << 2 | drive);
> +    g_assert(!get_irq(FLOPPY_IRQ));
> +    floppy_send(42);
> +
> +    ack_irq(&st0, &pcn);
> +    g_assert_cmpint(pcn, ==, 0);
> +    g_assert_cmpint(st0, ==, 0x70);
> +
> +    /* Send try relative seek to beyond track 80 */
> +    floppy_send(CMD_RELATIVE_SEEK_IN);
> +    floppy_send(head << 2 | drive);
> +    g_assert(!get_irq(FLOPPY_IRQ));
> +    floppy_send(200);
>   
> -    ack_irq(&pcn);
> -    g_assert(pcn == 0);
> +    ack_irq(&st0, &pcn);
> +    g_assert_cmpint(pcn, ==, 79);
> +    g_assert_cmpint(st0, ==, 0x50);
>   }
>   
>   /* success if no crash or abort */
I tested it on the real floppy and the behavior is more complicated.

Let's say that we start on the track 0 and the floppy media has 80 tracks.

You send the "relative_seek_in" by 1 end the result is
     st0: 0x20
     rcn: 1.
Then you issue the "read_id" command and you get
     st0: 0x00
     st1: 0x00
     pcn: 1.

Then you send the "relative_seek_in" by 100 and the result is
     st0: 0x20
     rcn: 101
but "read_id" returns
     st0: 0x40
     st1: 0x01
     pcn: 0.
This probably means, that you are out of available tracks.

Then you send the "relative_seek_out" by 100 and the result is
     st: 0x70
     rcn: 20
but "read_id" returns
     st0: 0x00
     st1: 0x00
     pcn: 0.
You return reading heads to the track 0, but the "relative_seek_out" 
returns that you are on the relative track 20.

To get the floppy drive fully working, you have to send the "seek" to 
the track 0. If you don't do it, every seek to different track then the 
track 0 will always seek to the track 0 and set rcn to track what you 
want. In this "buggy" state only the "relative_seek" will change the 
real track. The command "read_id" returns the real position of the 
reading heads. For example:

Now you send "seek" to track 2 and you get
     st0: 0x20
     pcn: 2
but if you send "read_id" the result is
     st0: 0x00
     st1: 0x00
     pcn: 0

Pavel

Re: [Qemu-devel] [PATCH 2/2] fdc-test: Check RELATIVE SEEK beyond track 0/80

[Blue Swirl]

On Tue, Jul 17, 2012 at 9:03 AM, Pavel Hrdina <phrdina@redhat.com> wrote:
> On 07/16/2012 04:36 PM, Kevin Wolf wrote:
>>
>> TODO This needs to be checked against a real drive
>>
>> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
>> ---
>>   tests/fdc-test.c |   48 +++++++++++++++++++++++++++++++++++++-----------
>>   1 files changed, 37 insertions(+), 11 deletions(-)
>>
>> diff --git a/tests/fdc-test.c b/tests/fdc-test.c
>> index fa74411..56e745a 100644
>> --- a/tests/fdc-test.c
>> +++ b/tests/fdc-test.c
>> @@ -94,13 +94,17 @@ static uint8_t floppy_recv(void)
>>   }
>>     /* pcn: Present Cylinder Number */
>> -static void ack_irq(uint8_t *pcn)
>> +static void ack_irq(uint8_t *st0, uint8_t *pcn)
>>   {
>>       uint8_t ret;
>>         g_assert(get_irq(FLOPPY_IRQ));
>>       floppy_send(CMD_SENSE_INT);
>> -    floppy_recv();
>> +
>> +    ret = floppy_recv();
>> +    if (st0 != NULL) {
>> +        *st0 = ret;
>> +    }
>>         ret = floppy_recv();
>>       if (pcn != NULL) {
>> @@ -175,7 +179,7 @@ static void send_seek(int cyl)
>>       floppy_send(head << 2 | drive);
>>       g_assert(!get_irq(FLOPPY_IRQ));
>>       floppy_send(cyl);
>> -    ack_irq(NULL);
>> +    ack_irq(NULL, NULL);
>>   }
>>     static uint8_t cmos_read(uint8_t reg)
>> @@ -295,29 +299,51 @@ static void test_relative_seek(void)
>>   {
>>       uint8_t drive = 0;
>>       uint8_t head = 0;
>> -    uint8_t cyl = 1;
>>       uint8_t pcn;
>> +    uint8_t st0;
>>         /* Send seek to track 0 */
>>       send_seek(0);
>>   -    /* Send relative seek to increase track by 1 */
>> +    /* Send relative seek to increase track by 3 */
>>       floppy_send(CMD_RELATIVE_SEEK_IN);
>>       floppy_send(head << 2 | drive);
>>       g_assert(!get_irq(FLOPPY_IRQ));
>> -    floppy_send(cyl);
>> +    floppy_send(3);
>>   -    ack_irq(&pcn);
>> -    g_assert(pcn == 1);
>> +    ack_irq(&st0, &pcn);
>> +    g_assert_cmpint(pcn, ==, 3);
>> +    g_assert_cmpint(st0, ==, 0x20);
>>         /* Send relative seek to decrease track by 1 */
>>       floppy_send(CMD_RELATIVE_SEEK_OUT);
>>       floppy_send(head << 2 | drive);
>>       g_assert(!get_irq(FLOPPY_IRQ));
>> -    floppy_send(cyl);
>> +    floppy_send(1);
>> +
>> +    ack_irq(&st0, &pcn);
>> +    g_assert_cmpint(pcn, ==, 2);
>> +    g_assert_cmpint(st0, ==, 0x20);
>> +
>> +    /* Send relative seek to beyond track 0 */
>> +    floppy_send(CMD_RELATIVE_SEEK_OUT);
>> +    floppy_send(head << 2 | drive);
>> +    g_assert(!get_irq(FLOPPY_IRQ));
>> +    floppy_send(42);
>> +
>> +    ack_irq(&st0, &pcn);
>> +    g_assert_cmpint(pcn, ==, 0);
>> +    g_assert_cmpint(st0, ==, 0x70);
>> +
>> +    /* Send try relative seek to beyond track 80 */
>> +    floppy_send(CMD_RELATIVE_SEEK_IN);
>> +    floppy_send(head << 2 | drive);
>> +    g_assert(!get_irq(FLOPPY_IRQ));
>> +    floppy_send(200);
>>   -    ack_irq(&pcn);
>> -    g_assert(pcn == 0);
>> +    ack_irq(&st0, &pcn);
>> +    g_assert_cmpint(pcn, ==, 79);
>> +    g_assert_cmpint(st0, ==, 0x50);
>>   }
>>     /* success if no crash or abort */
>
> I tested it on the real floppy and the behavior is more complicated.

This reminds me of an idea: it could be interesting to make qtest test
real hardware, for example using a serial port to feed commands and
get responses. The same protocol which libqtest uses could be
interpreted by a boot loader level program or kernel module.

>
> Let's say that we start on the track 0 and the floppy media has 80 tracks.
>
> You send the "relative_seek_in" by 1 end the result is
>     st0: 0x20
>     rcn: 1.
> Then you issue the "read_id" command and you get
>     st0: 0x00
>     st1: 0x00
>     pcn: 1.
>
> Then you send the "relative_seek_in" by 100 and the result is
>     st0: 0x20
>     rcn: 101
> but "read_id" returns
>     st0: 0x40
>     st1: 0x01
>     pcn: 0.
> This probably means, that you are out of available tracks.
>
> Then you send the "relative_seek_out" by 100 and the result is
>     st: 0x70
>     rcn: 20
> but "read_id" returns
>     st0: 0x00
>     st1: 0x00
>     pcn: 0.
> You return reading heads to the track 0, but the "relative_seek_out" returns
> that you are on the relative track 20.
>
> To get the floppy drive fully working, you have to send the "seek" to the
> track 0. If you don't do it, every seek to different track then the track 0
> will always seek to the track 0 and set rcn to track what you want. In this
> "buggy" state only the "relative_seek" will change the real track. The
> command "read_id" returns the real position of the reading heads. For
> example:
>
> Now you send "seek" to track 2 and you get
>     st0: 0x20
>     pcn: 2
> but if you send "read_id" the result is
>     st0: 0x00
>     st1: 0x00
>     pcn: 0
>
> Pavel
>
>

Re: [Qemu-devel] [PATCH 2/2] fdc-test: Check RELATIVE SEEK beyond track 0/80

[Kevin Wolf]

Am 23.07.2012 19:09, schrieb Blue Swirl:
> On Tue, Jul 17, 2012 at 9:03 AM, Pavel Hrdina <phrdina@redhat.com> wrote:
>> I tested it on the real floppy and the behavior is more complicated.
> 
> This reminds me of an idea: it could be interesting to make qtest test
> real hardware, for example using a serial port to feed commands and
> get responses. The same protocol which libqtest uses could be
> interpreted by a boot loader level program or kernel module.

The existing qtest cases wouldn't necessarily work there because they
don't care about timing. Not sure if it's worth the effort writing real
drivers in qtest cases just to be able to run them against real hardware
occasionally.

Kevin

Re: [Qemu-devel] [PATCH 2/2] fdc-test: Check RELATIVE SEEK beyond track 0/80

[Blue Swirl]

On Tue, Jul 24, 2012 at 9:35 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 23.07.2012 19:09, schrieb Blue Swirl:
>> On Tue, Jul 17, 2012 at 9:03 AM, Pavel Hrdina <phrdina@redhat.com> wrote:
>>> I tested it on the real floppy and the behavior is more complicated.
>>
>> This reminds me of an idea: it could be interesting to make qtest test
>> real hardware, for example using a serial port to feed commands and
>> get responses. The same protocol which libqtest uses could be
>> interpreted by a boot loader level program or kernel module.
>
> The existing qtest cases wouldn't necessarily work there because they
> don't care about timing. Not sure if it's worth the effort writing real
> drivers in qtest cases just to be able to run them against real hardware
> occasionally.

It would be nice if the tests paid some attention to timing, though
QEMU does not implement many timing issues either.

The benefit from this setup would be that the test suite would be
validated against real HW. But as the hardware gets older and breaks
down, it will be increasingly difficult to validate the suite in the
future so I also think it's probably not worth the effort.

>
> Kevin