Re: [PATCH v3 2/2] tests/ide-test: Create a single unit-test covering more PRDT cases

[Alexander Popov <alex.popov@linux.com>]

On 07.01.2020 10:44, Kevin Wolf wrote:
> Am 23.12.2019 um 18:51 hat Alexander Popov geschrieben:
>> Fuzzing the Linux kernel with syzkaller allowed to find how to crash qemu
>> using a special SCSI_IOCTL_SEND_COMMAND. It hits the assertion in
>> ide_dma_cb() introduced in the commit a718978ed58a in July 2015.
>> Currently this bug is not reproduced by the unit tests.
>>
>> Let's improve the ide-test to cover more PRDT cases including one
>> that causes this particular qemu crash.
>>
>> The test is developed according to the Programming Interface for
>> Bus Master IDE Controller (Revision 1.0 5/16/94).
>>
>> Signed-off-by: Alexander Popov <alex.popov@linux.com>
> 
> The time this test takes is much better now (~5s for me).
> 
>> +/*
>> + * This test is developed according to the Programming Interface for
>> + * Bus Master IDE Controller (Revision 1.0 5/16/94)
>> + */
>> +static void test_bmdma_various_prdts(void)
>>  {
>> -    QTestState *qts;
>> -    QPCIDevice *dev;
>> -    QPCIBar bmdma_bar, ide_bar;
>> -    uint8_t status;
>> -
>> -    PrdtEntry prdt[] = {
>> -        {
>> -            .addr = 0,
>> -            .size = cpu_to_le32(0x1000 | PRDT_EOT),
>> -        },
>> -    };
>> -
>> -    qts = test_bmdma_setup();
>> -
>> -    dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
>> -
>> -    /* Normal request */
>> -    status = send_dma_request(qts, CMD_READ_DMA, 0, 1,
>> -                              prdt, ARRAY_SIZE(prdt), NULL);
>> -    g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
>> -    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
>> +    int sectors = 0;
>> +    uint32_t size = 0;
>> +
>> +    for (sectors = 1; sectors <= 256; sectors *= 2) {
>> +        QTestState *qts = NULL;
>> +        QPCIDevice *dev = NULL;
>> +        QPCIBar bmdma_bar, ide_bar;
>> +
>> +        qts = test_bmdma_setup();
>> +        dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
> 
> I'm wondering why the initialisation has to be inside the outer for
> loop. I expected that moving it outside would further improve the speed.
> But sure enough, doing that makes the test fail.

Yes, that's why I came to the current solution.

> Did you have a look why this happens? I suppose we might be running out
> of some resources in the qtest framework becasue each send_dma_request()
> calls get_pci_device() again?

I've spent some time on investigating, but didn't succeed.

1. After several hundreds of send_dma_request() calls the following assertion in
that function fails:
    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), BSY | DRQ);

2. If I comment out this assertion, the test system proceeds but eventually stalls.

3. I tried to send the CMD_FLUSH_CACHE command to the device, it didn't help.

4. That behavior is not influenced by ide_dma_cb() code that I changed.

I guess it would be better if that effect is examined by somebody with more
knowledge about DMA and qtest.

> 5 seconds isn't that bad, so this shouldn't block this series, but it's
> still by far the slowest test in ide-test, so any improvement certainly
> wouldn't hurt.

Thanks for not making that mandatory. It would take me much more time.

>> +        for (size = 0; size < 65536; size += 256) {
>> +            uint32_t req_size = sectors * 512;
>> +            uint32_t prd_size = size & 0xfffe; /* bit 0 is always set to 0 */
>> +            uint8_t ret = 0;
>> +            uint8_t req_status = 0;
> 
> If you end up sending another version for some reason, I would also
> consider renaming req_status, because reg_status already exists, which
> looks almost the same. This confused me for a moment when reading the
> code below.

Heh! Ok, let's wait for more reviews.

Best regards,
Alexander