Re: [Qemu-block] [Qemu-devel] [PATCH 3/4] ide-test: add cdrom pio test

[Markus Armbruster]

John Snow <address@hidden> writes:

> Add a simple read test for ATAPI devices,
> using the PIO mechanism.
>
> Signed-off-by: John Snow <address@hidden>
> ---
>  tests/ide-test.c | 144 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 144 insertions(+)
>
> diff --git a/tests/ide-test.c b/tests/ide-test.c
> index 4a07e3a..90524e3 100644
> --- a/tests/ide-test.c
> +++ b/tests/ide-test.c
> @@ -45,6 +45,12 @@
>  #define IDE_BASE 0x1f0
>  #define IDE_PRIMARY_IRQ 14
>  
> +#define ATAPI_BLOCK_SIZE 2048
> +
> +/* How many bytes to receive via ATAPI PIO at one time.
> + * Must be less than 0xFFFF. */
> +#define BYTE_COUNT_LIMIT 5120
> +
>  enum {
>      reg_data        = 0x0,
>      reg_nsectors    = 0x2,
> @@ -80,6 +86,7 @@ enum {
>      CMD_WRITE_DMA   = 0xca,
>      CMD_FLUSH_CACHE = 0xe7,
>      CMD_IDENTIFY    = 0xec,
> +    CMD_PACKET      = 0xa0,
>  
>      CMDF_ABORT      = 0x100,
>      CMDF_NO_BM      = 0x200,
> @@ -585,6 +592,140 @@ static void test_isa_retry_flush(const char *machine)
>      test_retry_flush("isapc");
>  }
>  
> +typedef struct Read10CDB {
> +    uint8_t opcode;
> +    uint8_t flags;
> +    uint32_t lba;
> +    uint8_t reserved;
> +    uint16_t nblocks;
> +    uint8_t control;
> +    uint16_t padding;
> +} __attribute__((__packed__)) Read10CDB;
> +
> +static void send_scsi_cdb_read10(uint32_t lba, uint16_t nblocks)
> +{
> +    Read10CDB pkt = { .padding = 0 };
> +    int i;
> +
> +    /* Construct SCSI CDB packet */
> +    pkt.opcode = 0x28;
> +    pkt.lba = cpu_to_be32(lba);
> +    pkt.nblocks = cpu_to_be16(nblocks);
> +
> +    /* Send Packet */
> +    for (i = 0; i < sizeof(Read10CDB)/2; i++) {
> +        outw(IDE_BASE + reg_data, ((uint16_t *)&pkt)[i]);

Requires pkt to be suitable aligned.  It is.

> +    }
> +}
> +
> +static void nsleep(int64_t nsecs)
> +{
> +    const struct timespec val = { .tv_nsec = nsecs };
> +    nanosleep(&val, NULL);
> +    clock_set(nsecs);
> +}
> +
> +static uint8_t ide_wait_clear(uint8_t flag)
> +{
> +    int i;
> +    uint8_t data;
> +
> +    /* Wait with a 5 second timeout */
> +    for (i = 0; i <= 12500000; i++) {
> +        data = inb(IDE_BASE + reg_status);
> +        if (!(data & flag)) {
> +            return data;
> +        }
> +        nsleep(400);
> +    }
> +    g_assert_not_reached();
> +    return 0xff;

Unreachable code, not needed as long as g_assert_not_reached() is
properly annotated noreturn.

> +}
> +
> +static void cdrom_pio_impl(int nblocks)
> +{
> +    FILE *fh;
> +    size_t patt_len = ATAPI_BLOCK_SIZE * MAX(16, nblocks);
> +    char *pattern = g_malloc(patt_len);
> +    size_t rxsize = ATAPI_BLOCK_SIZE * nblocks;
> +    char *rx = g_malloc0(rxsize);
> +    int i, j;
> +    uint8_t data;
> +    uint16_t limit;
> +
> +    /* Prepopulate the CDROM with an interesting pattern */
> +    generate_pattern(pattern, patt_len, ATAPI_BLOCK_SIZE);
> +    fh = fopen(tmp_path, "w+");
> +    fwrite(pattern, ATAPI_BLOCK_SIZE, MAX(16, nblocks), fh);

I guess I would've avoided repeating MAX(16, nblocks), but it'll do.

> +    fclose(fh);
> +
> +    ide_test_start(
> +      "-drive file=%s,if=ide,media=cdrom,cache=writeback,format=raw", 
> tmp_path);

Legacy syntax.  Okay.

> +    qtest_irq_intercept_in(global_qtest, "ioapic");
> +
> +    /* PACKET command on device 0 */
> +    outb(IDE_BASE + reg_device, 0);
> +    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);

Ignorant question: why no reg_lba_low?

> +    /* HPD0: Check_Status_A State */
> +    nsleep(400);
> +    data = ide_wait_clear(BSY);

Ignorant question: why do you need to wait 400ns before you wait?

> +    /* HPD1: 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 */
> +    i = 0;
> +    do {
> +        data = get_irq(IDE_PRIMARY_IRQ);
> +        nsleep(400);
> +        i++;
> +        g_assert_cmpint(i, <=, 12500000);
> +    } while (!data);

Similar to ide_wait_clear().  Why do you need to nsleep() after
get_irq() returned non-zero?

> +
> +    /* 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
> +     * less than BYTE_COUNT_LIMIT.
> +     * For our purposes, we can only request even multiples, so do not
> +     * attempt to read remainders. */
> +    limit = BYTE_COUNT_LIMIT & ~1;

Does nothing, BYTE_COUNT_LIMIT is 5120.  Build-time assertion
!(BYTE_COUNT_LIMIT & 1) would do.

> +    for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
> +        size_t r = (rxsize - (limit * i)) / 2;
> +        for (j = 0; j < MIN((limit / 2), r); j++) {
> +            ((uint16_t *)rx)[(i * (limit/2)) + j] = inw(IDE_BASE + reg_data);

Would be more readable with uint16_t *rx.  Only if it doesn't require
casts elsewhere.

I guess I would've tried a single loop instead of nesting two.  But
since this works, keep it.

> +        }
> +    }
> +    data = ide_wait_clear(DRQ);
> +    assert_bit_set(data, DRDY);
> +    assert_bit_clear(data, DRQ | ERR | DF | BSY);
> +
> +    g_assert_cmpint(memcmp(pattern, rx, rxsize), ==, 0);
> +    g_free(pattern);
> +    g_free(rx);
> +    test_bmdma_teardown();
> +}
> +
> +static void test_cdrom_pio(void)
> +{
> +    cdrom_pio_impl(1);
> +}
> +
> +static void test_cdrom_pio_large(void)
> +{
> +    /* Test a few loops of the PIO DRQ mechanism. */
> +    cdrom_pio_impl(BYTE_COUNT_LIMIT * 4 / ATAPI_BLOCK_SIZE);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      const char *arch = qtest_get_arch();
> @@ -628,6 +769,9 @@ int main(int argc, char **argv)
>      qtest_add_func("/ide/flush/retry_pci", test_pci_retry_flush);
>      qtest_add_func("/ide/flush/retry_isa", test_isa_retry_flush);
>  
> +    qtest_add_func("/ide/cdrom/pio", test_cdrom_pio);
> +    qtest_add_func("/ide/cdrom/pio_large", test_cdrom_pio_large);
> +
>      ret = g_test_run();
>  
>      /* Cleanup */