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.po...@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