From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44695) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TjZ5f-0000BQ-Qc for qemu-devel@nongnu.org; Fri, 14 Dec 2012 12:30:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TjZ5a-0006mo-EA for qemu-devel@nongnu.org; Fri, 14 Dec 2012 12:30:27 -0500 Received: from cantor2.suse.de ([195.135.220.15]:51263 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TjZ5a-0006mH-4O for qemu-devel@nongnu.org; Fri, 14 Dec 2012 12:30:22 -0500 Message-ID: <50CB6228.4080005@suse.de> Date: Fri, 14 Dec 2012 18:30:16 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <8491483f6c0154f26be31bf7fad11566eb4810d3.1355478583.git.julien.grall@citrix.com> In-Reply-To: <8491483f6c0154f26be31bf7fad11566eb4810d3.1355478583.git.julien.grall@citrix.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [[Bug 108996]] hw/dma.c: Fix conversion ioport_register* to MemoryRegion List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Julien Grall Cc: Kevin Wolf , gson@gson.org, 1089996@bugs.launchpad.net, Markus Armbruster , qemu-devel@nongnu.org, =?ISO-8859-15?Q?Herv=E9_Poussineau?= , avi@redhat.com, Stefan Hajnoczi Am 14.12.2012 10:52, schrieb Julien Grall: > The commit 582299336879504353e60c7937fbc70fea93f3da introduced a bug in > dma emulation due to a bad conversion between ioport_register* and Memo= ryRegion. >=20 > Cc: 1089996@bugs.launchpad.net > Reported-by: Andreas Gustafsson > Signed-off-by: Julien Grall I had trouble following here, having handled the offending patch myself: "Fix", "a bug" and "a bad conversion" is not really telling me what went wrong and how the numbers are calculated correctly. Please suggest an additional explanatory paragraph for the commit message (as a reply). Formally the patch looks fine (modulo missing "of" or s/conversion/converting/g in $subject). >>From what I gather, the cont region starts at base + 8 << dshift. Why is the size in memory_region_init_io() 8 << d->dshift and not just 8 when it previously looped over 0..7? Same question for the channel region. Could be fixed as follow-up. More comments inline: > --- > hw/dma.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) >=20 > diff --git a/hw/dma.c b/hw/dma.c > index c2d7b21..1b1d406 100644 > --- a/hw/dma.c > +++ b/hw/dma.c > @@ -200,7 +200,7 @@ static void write_cont(void *opaque, hwaddr nport, = uint64_t data, > =20 > iport =3D (nport >> d->dshift) & 0x0f; > switch (iport) { > - case 0x01: /* command */ > + case 0x00: /* command */ Since the shift is "reverted" above, we effectively have an 0x8 -> 0x8+0x1 -> 0x8+0x0 change, which looks correct. This delta seems consistent for the other case changes ... > if ((data !=3D 0) && (data & CMD_NOT_SUPPORTED)) { > dolog("command %"PRIx64" not supported\n", data); > return; > @@ -208,7 +208,7 @@ static void write_cont(void *opaque, hwaddr nport, = uint64_t data, > d->command =3D data; > break; > =20 > - case 0x02: > + case 0x01: > ichan =3D data & 3; > if (data & 4) { > d->status |=3D 1 << (ichan + 4); > @@ -220,7 +220,7 @@ static void write_cont(void *opaque, hwaddr nport, = uint64_t data, > DMA_run(); > break; > =20 > - case 0x03: /* single mask */ > + case 0x02: /* single mask */ > if (data & 4) > d->mask |=3D 1 << (data & 3); > else > @@ -228,7 +228,7 @@ static void write_cont(void *opaque, hwaddr nport, = uint64_t data, > DMA_run(); > break; > =20 > - case 0x04: /* mode */ > + case 0x03: /* mode */ > { > ichan =3D data & 3; > #ifdef DEBUG_DMA > @@ -247,23 +247,23 @@ static void write_cont(void *opaque, hwaddr nport= , uint64_t data, > break; > } > =20 > - case 0x05: /* clear flip flop */ > + case 0x04: /* clear flip flop */ > d->flip_flop =3D 0; > break; > =20 > - case 0x06: /* reset */ > + case 0x05: /* reset */ > d->flip_flop =3D 0; > d->mask =3D ~0; > d->status =3D 0; > d->command =3D 0; > break; > =20 > - case 0x07: /* clear mask for all channels */ > + case 0x06: /* clear mask for all channels */ > d->mask =3D 0; > DMA_run(); > break; > =20 > - case 0x08: /* write mask for all channels */ > + case 0x07: /* write mask for all channels */ > d->mask =3D data; > DMA_run(); > break; > @@ -288,11 +288,11 @@ static uint64_t read_cont(void *opaque, hwaddr np= ort, unsigned size) > =20 > iport =3D (nport >> d->dshift) & 0x0f; > switch (iport) { > - case 0x08: /* status */ > + case 0x00: /* status */ > val =3D d->status; > d->status &=3D 0xf0; > break; > - case 0x0f: /* mask */ > + case 0x01: /* mask */ > val =3D d->mask; > break; > default: > @@ -467,7 +467,7 @@ void DMA_schedule(int nchan) > static void dma_reset(void *opaque) > { > struct dma_cont *d =3D opaque; > - write_cont(d, (0x06 << d->dshift), 0, 1); > + write_cont(d, (0x05 << d->dshift), 0, 1); ... and for the (weird :)) reuse of the write_cont() callback function from within the reset function. > } > =20 > static int dma_phony_handler (void *opaque, int nchan, int dma_pos, in= t dma_len) Reviewed-by: Andreas F=E4rber make check runs an fdc-test that passed okay. Can one of you add a test case to avoid another regression here? Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg