From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43011) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TCWst-0002A7-6O for qemu-devel@nongnu.org; Fri, 14 Sep 2012 10:28:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TCWsb-0003sk-Be for qemu-devel@nongnu.org; Fri, 14 Sep 2012 10:28:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:28312) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TCWsb-0003sP-3b for qemu-devel@nongnu.org; Fri, 14 Sep 2012 10:28:25 -0400 Date: Fri, 14 Sep 2012 10:28:22 -0400 From: Jason Baron Message-ID: <20120914142821.GG1821@redhat.com> References: <057c93b4be1cc933dab09e3ef9bdddcaeff57fc4.1347561356.git.jbaron@redhat.com> <5052D726.8010006@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5052D726.8010006@redhat.com> Subject: Re: [Qemu-devel] [PATCH 18/25] q35: Fix irr initialization for slots 25..31 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: aliguori@us.ibm.com, juzhang@redhat.com, mst@redhat.com, jan.kiszka@siemens.com, qemu-devel@nongnu.org, agraf@suse.de, yamahata@valinux.co.jp, alex.williamson@redhat.com, kevin@koconnor.net, avi@redhat.com, mkletzan@redhat.com, lcapitulino@redhat.com, afaerber@suse.de, armbru@redhat.com On Fri, Sep 14, 2012 at 09:05:10AM +0200, Paolo Bonzini wrote: > Il 13/09/2012 22:12, Jason Baron ha scritto: > > From: Isaku Yamahata > > > > This was totally off: The CC registers are 16 bit (stored as little > > endian), their offsets run in reverse order, and D26IR as well as D25IR > > have 4 bytes offset to their successors. > > > > Reported-by: Jan Kiszka > > Signed-off-by: Isaku Yamahata > > Signed-off-by: Jason Baron > > --- > > hw/q35.c | 29 ++++++++++++++++++++--------- > > 1 files changed, 20 insertions(+), 9 deletions(-) > > > > diff --git a/hw/q35.c b/hw/q35.c > > index 8b6a2e5..295344e 100644 > > --- a/hw/q35.c > > +++ b/hw/q35.c > > @@ -474,7 +474,7 @@ static void ich9_lpc_reset(DeviceState *qdev); > > * Although it's not pci configuration space, it's little endian as Intel. > > */ > > > > -static void ich9_cc_update_ir(uint8_t irr[PCI_NUM_PINS], uint32_t ir) > > +static void ich9_cc_update_ir(uint8_t irr[PCI_NUM_PINS], uint16_t ir) > > { > > int intx; > > for (intx = 0; intx < PCI_NUM_PINS; intx++) { > > @@ -485,15 +485,26 @@ static void ich9_cc_update_ir(uint8_t irr[PCI_NUM_PINS], uint32_t ir) > > static void ich9_cc_update(ICH9_LPCState *lpc) > > { > > int slot; > > - int reg_offset; > > - int intx; > > + int pci_intx; > > + > > + const int reg_offsets[] = { > > + ICH9_CC_D25IR, > > + ICH9_CC_D26IR, > > + ICH9_CC_D27IR, > > + ICH9_CC_D28IR, > > + ICH9_CC_D29IR, > > + ICH9_CC_D30IR, > > + ICH9_CC_D31IR, > > + }; > > + const int *offset; > > > > /* D{25 - 31}IR, but D30IR is read only to 0. */ > > - for (slot = 25, reg_offset = 0; slot < 32; slot++, reg_offset++) { > > - if (slot != 30) { > > - ich9_cc_update_ir(lpc->irr[slot], > > - lpc->chip_config[ICH9_CC_D31IR + reg_offset]); > > + for (slot = 25, offset = reg_offsets; slot < 32; slot++, offset++) { > > + if (slot == 30) { > > + continue; > > } > > + ich9_cc_update_ir(lpc->irr[slot], > > + pci_get_word(lpc->chip_config + *offset)); > > } > > > > /* > > @@ -502,8 +513,8 @@ static void ich9_cc_update(ICH9_LPCState *lpc) > > * are connected to pirq lines. Our choice is PIRQ[E-H]. > > * INT[A-D] are connected to PIRQ[E-H] > > */ > > - for (intx = 0; intx < PCI_NUM_PINS; intx++) { > > - lpc->irr[30][intx] = intx + 4; > > + for (pci_intx = 0; pci_intx < PCI_NUM_PINS; pci_intx++) { > > + lpc->irr[30][pci_intx] = pci_intx + 4; > > } > > } > > > > > > I guess this patch and patch 12 could/should be squashed in patch 11 > (the one that introduces q35.c)? > > Paolo Michael Tsirkin also suggested combining them. I kept them separate to make it clear what Yamahata had written, and the re-base I had done. I agree it would be cleaner to combine. That said, Michael also suggested not adding the initial one to the build so its still bi-sectable, I think that could be a reasonable option too. Thanks, -Jason