Message ID | 1989bebbf4a990b0c401b18815d8f6dbbdbcd4b6.1349749915.git.jbaron@redhat.com |
---|---|
State | New |
Headers | show |
Il 09/10/2012 05:30, 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> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/q35.c | 29 ++++++++++++++++++++--------- > 1 files changed, 20 insertions(+), 9 deletions(-) > > diff --git a/hw/q35.c b/hw/q35.c > index 5d256cb..e4f313e 100644 > --- a/hw/q35.c > +++ b/hw/q35.c > @@ -480,7 +480,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++) { > @@ -491,15 +491,26 @@ static void ich9_cc_update_ir(uint8_t irr[PCI_NUM_PINS], uint32_t ir) > static void ich9_cc_update(ICH9LPCState *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)); > } > > /* > @@ -508,8 +519,8 @@ static void ich9_cc_update(ICH9LPCState *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; > } > } > >
On Mon, Oct 08, 2012 at 11:30:37PM -0400, Jason Baron wrote: > 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> Probably should be smashed into the original patch too? More contained so less critical. > --- > hw/q35.c | 29 ++++++++++++++++++++--------- > 1 files changed, 20 insertions(+), 9 deletions(-) > > diff --git a/hw/q35.c b/hw/q35.c > index 5d256cb..e4f313e 100644 > --- a/hw/q35.c > +++ b/hw/q35.c > @@ -480,7 +480,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++) { > @@ -491,15 +491,26 @@ static void ich9_cc_update_ir(uint8_t irr[PCI_NUM_PINS], uint32_t ir) > static void ich9_cc_update(ICH9LPCState *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)); > } > > /* > @@ -508,8 +519,8 @@ static void ich9_cc_update(ICH9LPCState *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; > } > } > > -- > 1.7.1
diff --git a/hw/q35.c b/hw/q35.c index 5d256cb..e4f313e 100644 --- a/hw/q35.c +++ b/hw/q35.c @@ -480,7 +480,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++) { @@ -491,15 +491,26 @@ static void ich9_cc_update_ir(uint8_t irr[PCI_NUM_PINS], uint32_t ir) static void ich9_cc_update(ICH9LPCState *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)); } /* @@ -508,8 +519,8 @@ static void ich9_cc_update(ICH9LPCState *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; } }