Message ID | 057c93b4be1cc933dab09e3ef9bdddcaeff57fc4.1347561356.git.jbaron@redhat.com |
---|---|
State | New |
Headers | show |
Il 13/09/2012 22:12, Jason Baron ha scritto: > From: Isaku Yamahata <yamahata@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.kiszka@siemens.com> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > Signed-off-by: Jason Baron <jbaron@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
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 <yamahata@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.kiszka@siemens.com> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > Signed-off-by: Jason Baron <jbaron@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
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; } }