[PULL,00/14] Migration pull request

Am 19.11.2015 um 16:16 schrieb Dr. David Alan Gilbert:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> On 19 November 2015 at 14:44, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 19 November 2015 at 13:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> On 19 November 2015 at 13:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>> Hi. Unfortunately this failed in 'make check' (x86-64 Linux, debug build):
>>>>>
>>>>> GTESTER check-qtest-i386
>>>>> blkdebug: Suspended request 'A'
>>>>> blkdebug: Resuming request 'A'
>>>>> **
>>>>> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/ide-test.c:656:ide_wait_clear:
>>>>> code should not be reached
>>>>> GTester: last random seed: R02Sf1d98a814dee34e3985479976b17883c
>>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>>> [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio
>>>>> extension. Task offloads will be emulated.
>>>>> make: *** [check-qtest-i386] Error 1
>>>>>
>>>>> It might be an intermittent test fail from an earlier IDE pull?
>>>> Yep, intermittent. Second run of the tests passed...
>>> and repro'd on current master, so definitely not the fault of anything
>>> in this pull.
>> while QTEST_QEMU_BINARY=i386-softmmu/qemu-system-i386
>> QTEST_QEMU_IMG=qemu-img gtester -k --verbose -m=quick tests/ide-test ;
>> do true; done
>>
>> will eventually hang on /i386/ide/cdrom/pio_large (may take a fair
>> number of loops; when it works fine the test does not take an
>> appreciable amount of time). After a long time the timeout in the
>> test kicks in and the assert happens.
> cc'ing in Peter Lieven given his recent IDE buffering changes.

I wonder if there is a glitch in the PIO implementation of test-ide.c. As far as I understand the specs
it is not allowed to read data while the BSY flag is set. With the following change to the test-ide script
the test does not race:


Note: in the old sync version of the ATAPI PIO implementation this could not happen.

Peter

On 20 November 2015 at 09:38, Peter Lieven <pl@kamp.de> wrote:
> I wonder if there is a glitch in the PIO implementation of test-ide.c. As far as I understand the specs
> it is not allowed to read data while the BSY flag is set. With the following change to the test-ide script
> the test does not race:
>
> diff --git a/tests/ide-test.c b/tests/ide-test.c
> index d1014bb..ab0489e 100644
> --- a/tests/ide-test.c
> +++ b/tests/ide-test.c
> @@ -728,6 +728,7 @@ static void cdrom_pio_impl(int nblocks)
>      for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
>          size_t offset = i * (limit / 2);
>          size_t rem = (rxsize / 2) - offset;
> +        ide_wait_clear(BSY);
>          for (j = 0; j < MIN((limit / 2), rem); j++) {
>              rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
>          }
>
> Note: in the old sync version of the ATAPI PIO implementation this could not happen.

This certainly fixes the stalls for me, though I don't know enough
IDE to say whether it is the correct fix.

thanks
-- PMM

Am 20.11.2015 um 12:33 schrieb Peter Maydell:
> On 20 November 2015 at 09:38, Peter Lieven <pl@kamp.de> wrote:
>> I wonder if there is a glitch in the PIO implementation of test-ide.c. As far as I understand the specs
>> it is not allowed to read data while the BSY flag is set. With the following change to the test-ide script
>> the test does not race:
>>
>> diff --git a/tests/ide-test.c b/tests/ide-test.c
>> index d1014bb..ab0489e 100644
>> --- a/tests/ide-test.c
>> +++ b/tests/ide-test.c
>> @@ -728,6 +728,7 @@ static void cdrom_pio_impl(int nblocks)
>>      for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
>>          size_t offset = i * (limit / 2);
>>          size_t rem = (rxsize / 2) - offset;
>> +        ide_wait_clear(BSY);
>>          for (j = 0; j < MIN((limit / 2), rem); j++) {
>>              rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
>>          }
>>
>> Note: in the old sync version of the ATAPI PIO implementation this could not happen.
> This certainly fixes the stalls for me, though I don't know enough
> IDE to say whether it is the correct fix.
Thanks for testing.

I hope that John or Kevin can verify this fix?

Peter

Am 20.11.2015 um 14:44 hat Peter Lieven geschrieben:
> Am 20.11.2015 um 12:33 schrieb Peter Maydell:
> > On 20 November 2015 at 09:38, Peter Lieven <pl@kamp.de> wrote:
> >> I wonder if there is a glitch in the PIO implementation of test-ide.c. As far as I understand the specs
> >> it is not allowed to read data while the BSY flag is set. With the following change to the test-ide script
> >> the test does not race:
> >>
> >> diff --git a/tests/ide-test.c b/tests/ide-test.c
> >> index d1014bb..ab0489e 100644
> >> --- a/tests/ide-test.c
> >> +++ b/tests/ide-test.c
> >> @@ -728,6 +728,7 @@ static void cdrom_pio_impl(int nblocks)
> >>      for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
> >>          size_t offset = i * (limit / 2);
> >>          size_t rem = (rxsize / 2) - offset;
> >> +        ide_wait_clear(BSY);
> >>          for (j = 0; j < MIN((limit / 2), rem); j++) {
> >>              rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
> >>          }
> >>
> >> Note: in the old sync version of the ATAPI PIO implementation this could not happen.
> > This certainly fixes the stalls for me, though I don't know enough
> > IDE to say whether it is the correct fix.
> Thanks for testing.
> 
> I hope that John or Kevin can verify this fix?

The fix looks correct to me.

While you're improving the test, you could also add an assertion that
DRQ is set after BSY has been cleared.

Kevin