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

[John Snow]

On 09/10/2015 05:42 AM, Markus Armbruster wrote:
> 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.
> 

Good point, bad habit.

>> +}
>> +
>> +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.
> 

"I expected better from you, John."

>> +    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?
> 

It's not used by the PACKET command. Most fields aren't, but
lba_middle/lba_high (lcyl and hcyl) are re-purposed to represent a 16
bit "byte count limit" field.

>> +    /* HPD0: Check_Status_A State */
>> +    nsleep(400);
>> +    data = ide_wait_clear(BSY);
> 
> Ignorant question: why do you need to wait 400ns before you wait?
> 

Blindly following spec to a fault -- the purpose on real hardware is to
allow the drive a chance to set the BSY flag before we witness it being
reset back to zero.

QEMU of course will set and clear BSY synchronously before we ever make
it back, but I try to write these tests in a manner where they are
ignorant of QEMU's internals as much as I can, so you see some weird
timing stuff here and there.

Secretly I want to leave these tests generic enough to try and test them
with pass-through devices and real hardware someday to see how my
assertions match up against decidedly real hardware.

>> +    /* 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?
> 

Ran out of thinking fluid. Ugly loop.

>> +
>> +    /* 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.
> 

Allowing for the possibility of tests to exercise this bizarre property
of the BCL in the future, though I think there'd be more work to do in
the loop below.

I intend to expand these tests bit-by-bit over time, so this is just
some evidence of where I'm thinking on that.

>> +    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.
> 

It's to accommodate the DRQ blocks, and to allow myself the chance to
query the device between the distinct DRQ segments.

It could just be a flat loop and it'd work okay...

>> +        }
>> +    }
>> +    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 */

I'll polish just a pinch more. Thanks for the sanity check.

--js