Re: [PATCH v3 2/2] tests/ide-test: Create a single unit-test covering more PRDT cases
Am 07.01.2020 um 23:39 hat Alexander Popov geschrieben: > > 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. Ok, don't bother then. I seem to remember that I ran into something similar some time ago and found out that it was related to some integer overflow, I think during the PCI BAR mapping. This might be the same. Maybe I'll have another look later. Kevin
Re: [PATCH v3 2/2] tests/ide-test: Create a single unit-test covering more PRDT cases
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
>
> 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
Re: [PATCH v3 2/2] tests/ide-test: Create a single unit-test covering more PRDT cases
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
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.
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?
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.
> +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.
Kevin
