Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Kevin Wolf <kwolf@redhat.com>]

Am 20.11.2015 um 15:00 hat Peter Lieven geschrieben:
> Am 20.11.2015 um 14:53 schrieb Kevin Wolf:
> > 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.
> 
> I would actually move the check (which is already there) into the loop:
> 
> diff --git a/tests/ide-test.c b/tests/ide-test.c
> index d1014bb..d7ee376 100644
> --- a/tests/ide-test.c
> +++ b/tests/ide-test.c
> @@ -712,11 +712,6 @@ static void cdrom_pio_impl(int nblocks)
>      /* HPD3: INTRQ_Wait */
>      ide_wait_intr(IDE_PRIMARY_IRQ);
>  
> -    /* 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
> @@ -728,6 +723,10 @@ 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;
> +        /* HPD2: Check_Status_B */
> +        data = ide_wait_clear(BSY);
> +        assert_bit_set(data, DRQ | DRDY);
> +        assert_bit_clear(data, ERR | DF | BSY);
>          for (j = 0; j < MIN((limit / 2), rem); j++) {
>              rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
>          }
> 
> Are you okay with that? @John, you also?

Oh, yes, I missed that the check was already there, just in the wrong
place. I agree that this is better.

I also see that we have the state names from the ATA spec in a comment,
so getting that part right might be nice, too. For a start, HPD* are the
wrong states (they are for DMA transfers), it should be HP* everywhere.
And for the part that your patch touches, the loop that actually
transfers data is part of "HP4: Transfer_Data", so we might add a
comment right before the (nested) for.

Kevin