[Qemu-devel] [PATCH] tests: fix cdrom_pio_impl in ide-test

[Qemu-devel] [PATCH] tests: fix cdrom_pio_impl in ide-test

[Peter Lieven]

The check for the cleared BSY flag has to be performed
before each data transfer and not just before the
first one.

Commit 5f81724d revealed this glitch as the BSY flag
was not set in ATAPI PIO transfers before.

While at it fix the desciptions and add a comment before
the nested for loop that transfers the data.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 tests/ide-test.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index d1014bb..fc1ce52 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -699,24 +699,19 @@ static void cdrom_pio_impl(int nblocks)
     outb(IDE_BASE + reg_lba_middle, BYTE_COUNT_LIMIT & 0xFF);
     outb(IDE_BASE + reg_lba_high, (BYTE_COUNT_LIMIT >> 8 & 0xFF));
     outb(IDE_BASE + reg_command, CMD_PACKET);
-    /* HPD0: Check_Status_A State */
+    /* HP0: Check_Status_A State */
     nsleep(400);
     data = ide_wait_clear(BSY);
-    /* HPD1: Send_Packet State */
+    /* HP1: Send_Packet State */
     assert_bit_set(data, DRQ | DRDY);
     assert_bit_clear(data, ERR | DF | BSY);
 
     /* SCSI CDB (READ10) -- read n*2048 bytes from block 0 */
     send_scsi_cdb_read10(0, nblocks);
 
-    /* HPD3: INTRQ_Wait */
+    /* HP3: INTRQ_Wait */
     ide_wait_intr(IDE_PRIMARY_IRQ);
 
-    /* HPD2: Check_Status_B */
-    data = ide_wait_clear(BSY);
-    assert_bit_set(data, DRQ | DRDY);
-    assert_bit_clear(data, ERR | DF | BSY);
-
     /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes.
      * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 bytes.
      * We allow an odd limit only when the remaining transfer size is
@@ -728,6 +723,11 @@ static void cdrom_pio_impl(int nblocks)
     for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
         size_t offset = i * (limit / 2);
         size_t rem = (rxsize / 2) - offset;
+        /* HP2: Check_Status_B */
+        data = ide_wait_clear(BSY);
+        assert_bit_set(data, DRQ | DRDY);
+        assert_bit_clear(data, ERR | DF | BSY);
+        /* HP4: Transfer_Data */
         for (j = 0; j < MIN((limit / 2), rem); j++) {
             rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
         }
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] tests: fix cdrom_pio_impl in ide-test

[Peter Maydell]

On 20 November 2015 at 14:29, Peter Lieven <pl@kamp.de> wrote:
> The check for the cleared BSY flag has to be performed
> before each data transfer and not just before the
> first one.
>
> Commit 5f81724d revealed this glitch as the BSY flag
> was not set in ATAPI PIO transfers before.
>
> While at it fix the desciptions and add a comment before
> the nested for loop that transfers the data.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>

If the IDE folks can review this I'd like to apply it
direct to master this afternoon so we can tag and roll rc1
today.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] tests: fix cdrom_pio_impl in ide-test

[John Snow]

On 11/20/2015 09:37 AM, Peter Maydell wrote:
> On 20 November 2015 at 14:29, Peter Lieven <pl@kamp.de> wrote:
>> The check for the cleared BSY flag has to be performed
>> before each data transfer and not just before the
>> first one.
>>
>> Commit 5f81724d revealed this glitch as the BSY flag
>> was not set in ATAPI PIO transfers before.
>>
>> While at it fix the desciptions and add a comment before
>> the nested for loop that transfers the data.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
> 
> If the IDE folks can review this I'd like to apply it
> direct to master this afternoon so we can tag and roll rc1
> today.
> 
> thanks
> -- PMM
> 

Please do.

--js

Re: [Qemu-devel] [PATCH] tests: fix cdrom_pio_impl in ide-test

[Kevin Wolf]

Am 20.11.2015 um 15:29 hat Peter Lieven geschrieben:
> The check for the cleared BSY flag has to be performed
> before each data transfer and not just before the
> first one.
> 
> Commit 5f81724d revealed this glitch as the BSY flag
> was not set in ATAPI PIO transfers before.
> 
> While at it fix the desciptions and add a comment before
> the nested for loop that transfers the data.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH] tests: fix cdrom_pio_impl in ide-test

[John Snow]

On 11/20/2015 09:29 AM, Peter Lieven wrote:
> The check for the cleared BSY flag has to be performed
> before each data transfer and not just before the
> first one.
> 
> Commit 5f81724d revealed this glitch as the BSY flag
> was not set in ATAPI PIO transfers before.
> 
> While at it fix the desciptions and add a comment before

Descriptions, if this can be fixed on apply.

> the nested for loop that transfers the data.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  tests/ide-test.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/ide-test.c b/tests/ide-test.c
> index d1014bb..fc1ce52 100644
> --- a/tests/ide-test.c
> +++ b/tests/ide-test.c
> @@ -699,24 +699,19 @@ static void cdrom_pio_impl(int nblocks)
>      outb(IDE_BASE + reg_lba_middle, BYTE_COUNT_LIMIT & 0xFF);
>      outb(IDE_BASE + reg_lba_high, (BYTE_COUNT_LIMIT >> 8 & 0xFF));
>      outb(IDE_BASE + reg_command, CMD_PACKET);
> -    /* HPD0: Check_Status_A State */
> +    /* HP0: Check_Status_A State */
>      nsleep(400);
>      data = ide_wait_clear(BSY);
> -    /* HPD1: Send_Packet State */
> +    /* HP1: Send_Packet State */
>      assert_bit_set(data, DRQ | DRDY);
>      assert_bit_clear(data, ERR | DF | BSY);
>  
>      /* SCSI CDB (READ10) -- read n*2048 bytes from block 0 */
>      send_scsi_cdb_read10(0, nblocks);
>  
> -    /* HPD3: INTRQ_Wait */
> +    /* HP3: INTRQ_Wait */
>      ide_wait_intr(IDE_PRIMARY_IRQ);
>  
> -    /* HPD2: Check_Status_B */
> -    data = ide_wait_clear(BSY);
> -    assert_bit_set(data, DRQ | DRDY);
> -    assert_bit_clear(data, ERR | DF | BSY);
> -
>      /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes.
>       * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 bytes.
>       * We allow an odd limit only when the remaining transfer size is
> @@ -728,6 +723,11 @@ static void cdrom_pio_impl(int nblocks)
>      for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
>          size_t offset = i * (limit / 2);
>          size_t rem = (rxsize / 2) - offset;
> +        /* HP2: Check_Status_B */
> +        data = ide_wait_clear(BSY);
> +        assert_bit_set(data, DRQ | DRDY);
> +        assert_bit_clear(data, ERR | DF | BSY);
> +        /* HP4: Transfer_Data */
>          for (j = 0; j < MIN((limit / 2), rem); j++) {
>              rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
>          }
> 

This looks correct. This will definitely fix the race in the test, since
it was due to a race where we were reading the data when DRQ was not set.

Where I still remain a little confused is the precise flow control that
leads to sending an interrupt where BSY is set and DRQ is clear.

I'd like to investigate that a little more, but for purposes of rc1 and
testing I think this is the right thing to do.

For rc1, however:
Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH] tests: fix cdrom_pio_impl in ide-test

[Peter Maydell]

On 20 November 2015 at 17:12, John Snow <jsnow@redhat.com> wrote:

> This looks correct. This will definitely fix the race in the test, since
> it was due to a race where we were reading the data when DRQ was not set.
>
> Where I still remain a little confused is the precise flow control that
> leads to sending an interrupt where BSY is set and DRQ is clear.
>
> I'd like to investigate that a little more, but for purposes of rc1 and
> testing I think this is the right thing to do.
>
> For rc1, however:
> Reviewed-by: John Snow <jsnow@redhat.com>

Thanks, applied to master (I fixed the commit message typo).

-- PMM