Some comments below. On Mon, Oct 25, 2010 at 07:05:51AM +0200, Michael S. Tsirkin wrote: > config cycle operations should be idempotent, so > there's no need to complicate code with range checks. > > Signed-off-by: Michael S. Tsirkin <m...@redhat.com> > --- > > Untested. Pls comment. > > hw/pcie.c | 96 > ++++++++++++++++++++++++++++++------------------------------- > 1 files changed, 47 insertions(+), 49 deletions(-) > > diff --git a/hw/pcie.c b/hw/pcie.c > index 53d1fce..c972337 100644 > --- a/hw/pcie.c > +++ b/hw/pcie.c > @@ -302,59 +302,57 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > addr, val, len, sltctl_prev, sltctl, sltsta); > > /* SLTCTL */ > - if (ranges_overlap(addr, len, pos + PCI_EXP_SLTCTL, 2)) { > - PCIE_DEV_PRINTF(dev, "sltctl: 0x%02"PRIx16" -> 0x%02"PRIx16"\n", > - sltctl_prev, sltctl); > - if (pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTCTL, > - PCI_EXP_SLTCTL_EIC)) { > - sltsta ^= PCI_EXP_SLTSTA_EIS; /* toggle PCI_EXP_SLTSTA_EIS bit */ > - pci_set_word(exp_cap + PCI_EXP_SLTSTA, sltsta); > - PCIE_DEV_PRINTF(dev, "PCI_EXP_SLTCTL_EIC: " > - "sltsta -> 0x%02"PRIx16"\n", > - sltsta); > - } > - > - /* > - * The events control bits might be enabled or disabled, > - * Check if the software notificastion condition is satisfied > - * or disatisfied. > - * > - * 6.7.3.4 Software Notification of Hot-plug events > - */ > - if (pci_msi_enabled(dev)) { > - bool msi_trigger = > - (sltctl & PCI_EXP_SLTCTL_HPIE) && > - ((sltctl_prev ^ sltctl) & sltctl & /* stlctl: 0 -> 1 */ > - sltsta & PCI_EXP_HP_EV_SUPPORTED); > - if (msi_trigger) { > - pci_msi_notify(dev, pcie_cap_flags_get_vector(dev)); > - } > - } else { > - int int_level = > - (sltctl & PCI_EXP_SLTCTL_HPIE) && > - (sltctl & sltsta & PCI_EXP_HP_EV_SUPPORTED); > - qemu_set_irq(dev->irq[dev->exp.hpev_intx], int_level); > - } > + PCIE_DEV_PRINTF(dev, "sltctl: 0x%02"PRIx16" -> 0x%02"PRIx16"\n", > + sltctl_prev, sltctl); > + if (pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTCTL, > + PCI_EXP_SLTCTL_EIC)) { > + sltsta ^= PCI_EXP_SLTSTA_EIS; /* toggle PCI_EXP_SLTSTA_EIS bit */ > + pci_set_word(exp_cap + PCI_EXP_SLTSTA, sltsta); > + PCIE_DEV_PRINTF(dev, "PCI_EXP_SLTCTL_EIC: " > + "sltsta -> 0x%02"PRIx16"\n", > + sltsta); > + } > > - if (!((sltctl_prev ^ sltctl) & PCI_EXP_SLTCTL_SUPPORTED)) { > - PCIE_DEV_PRINTF(dev, > - "sprious command completion slctl " > - "0x%"PRIx16" -> 0x%"PRIx16"\n", > - sltctl_prev, sltctl); > + /* > + * The events control bits might be enabled or disabled, > + * Check if the software notificastion condition is satisfied > + * or disatisfied. > + * > + * 6.7.3.4 Software Notification of Hot-plug events > + */ > + if (pci_msi_enabled(dev)) { > + bool msi_trigger = > + (sltctl & PCI_EXP_SLTCTL_HPIE) && > + ((sltctl_prev ^ sltctl) & sltctl & /* stlctl: 0 -> 1 */ > + sltsta & PCI_EXP_HP_EV_SUPPORTED); > + if (msi_trigger) { > + pci_msi_notify(dev, pcie_cap_flags_get_vector(dev)); > } > + } else { > + int int_level = > + (sltctl & PCI_EXP_SLTCTL_HPIE) && > + (sltctl & sltsta & PCI_EXP_HP_EV_SUPPORTED); > + qemu_set_irq(dev->irq[dev->exp.hpev_intx], int_level); > + } >
The above two changes look okay. > - /* command completion. > - * Real hardware might take a while to complete > - * requested command because physical movement would be involved > - * like locking the electromechanical lock. > - * However in our case, command is completed instantaneously above, > - * so send a command completion event right now. > - * > - * 6.7.3.2 Command Completed Events > - */ > - /* set command completed bit */ > - pcie_cap_slot_event(dev, PCI_EXP_HP_EV_CCI); > + if (!((sltctl_prev ^ sltctl) & PCI_EXP_SLTCTL_SUPPORTED)) { > + PCIE_DEV_PRINTF(dev, > + "sprious command completion slctl " > + "0x%"PRIx16" -> 0x%"PRIx16"\n", > + sltctl_prev, sltctl); > } > + > + /* command completion. > + * Real hardware might take a while to complete > + * requested command because physical movement would be involved > + * like locking the electromechanical lock. > + * However in our case, command is completed instantaneously above, > + * so send a command completion event right now. > + * > + * 6.7.3.2 Command Completed Events > + */ > + /* set command completed bit */ > + pcie_cap_slot_event(dev, PCI_EXP_HP_EV_CCI); But this isn't correct. A single command to the slot means a single write to the slot control register. With this patch, the interrupt is raised at every time the configuration space is written no matter which offset is used. >From 6.7.3.2 > Software issues a command to a hot-plug capable Downstream Port by > issuing a write transaction that targets any portion of the > Port's Slot Control register. A single write to the Slot Control > register is considered to be a single command ... > The Port must process the command normally even if the status field is > already set when the command is issued. With this clause, I don't see any other better way than range check unfortunately. -- yamahata