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 <yamah...@valinux.co.jp> > > > > 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 <jan.kis...@siemens.com> > > Signed-off-by: Isaku Yamahata <yamah...@valinux.co.jp> > > Signed-off-by: Jason Baron <jba...@redhat.com> > > --- > > 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