From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49487) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzmTr-0003Ff-8V for qemu-devel@nongnu.org; Fri, 20 Nov 2015 09:16:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZzmTo-0005DO-25 for qemu-devel@nongnu.org; Fri, 20 Nov 2015 09:16:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53355) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzmTn-0005DI-PO for qemu-devel@nongnu.org; Fri, 20 Nov 2015 09:15:59 -0500 Date: Fri, 20 Nov 2015 15:15:55 +0100 From: Kevin Wolf Message-ID: <20151120141554.GC4130@noname.redhat.com> References: <20151119151627.GH2653@work-vm> <564EEA17.9080006@kamp.de> <564F23AC.5030903@kamp.de> <20151120135355.GB4130@noname.redhat.com> <564F2761.8000200@kamp.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <564F2761.8000200@kamp.de> Subject: Re: [Qemu-devel] [PULL 00/14] Migration pull request List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven Cc: Peter Maydell , Juan Quintela , QEMU Developers , "Dr. David Alan Gilbert" , Amit Shah , John Snow 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 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