On Mon, Oct 25, 2010 at 03:44:01PM +0900, Isaku Yamahata wrote:
> On Mon, Oct 25, 2010 at 07:49:41AM +0200, Michael S. Tsirkin wrote:
> > Simplify logic for hotplug notification, by tracking state of the
> > logical interrupt condition. We then simply use this variable to make
> > the interrupt decision, according to spec.
> >
> > API is made cleaner as we no longer force users to pass in
> > old slot control value.
>
> Thank you for looking into it.
> Some comments below.
Thanks! Care fixing up the remaining issues?
I will fold the fixup in.
> >
> > Untested.
> >
> > Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> > ---
> > hw/ioh3420.c | 5 +--
> > hw/pcie.c | 79
> > ++++++++++++++++++++++------------------------
> > hw/pcie.h | 8 +++-
> > hw/xio3130_downstream.c | 5 +--
> > 4 files changed, 46 insertions(+), 51 deletions(-)
> >
> > diff --git a/hw/ioh3420.c b/hw/ioh3420.c
> > index 1f340d3..23aecbf 100644
> > --- a/hw/ioh3420.c
> > +++ b/hw/ioh3420.c
> > @@ -39,12 +39,9 @@
> > static void ioh3420_write_config(PCIDevice *d,
> > uint32_t address, uint32_t val, int len)
> > {
> > - uint16_t sltctl =
> > - pci_get_word(d->config + d->exp.exp_cap + PCI_EXP_SLTCTL);
> > -
> > pci_bridge_write_config(d, address, val, len);
> > msi_write_config(d, address, val, len);
> > - pcie_cap_slot_write_config(d, address, val, len, sltctl);
> > + pcie_cap_slot_write_config(d, address, val, len);
> > /* TODO: AER */
> > }
> >
> > diff --git a/hw/pcie.c b/hw/pcie.c
> > index c972337..74d2c18 100644
> > --- a/hw/pcie.c
> > +++ b/hw/pcie.c
> > @@ -155,20 +155,11 @@ static void pcie_cap_slot_event(PCIDevice *dev,
> > PCIExpressHotPlugEvent event)
> > "sltctl: 0x%02"PRIx16" sltsta: 0x%02"PRIx16" event:
> > %x\n",
> > sltctl, sltsta, event);
> >
> > + /* Minor optimization: if nothing changed - no event is needed. */
> > if (pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, event)) {
> > return;
> > }
> > - sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
> > - PCIE_DEV_PRINTF(dev, "sltsta -> %02"PRIx16"\n", sltsta);
> > -
> > - if ((sltctl & PCI_EXP_SLTCTL_HPIE) &&
> > - (sltctl & event & PCI_EXP_HP_EV_SUPPORTED)) {
> > - if (pci_msi_enabled(dev)) {
> > - pci_msi_notify(dev, pcie_cap_flags_get_vector(dev));
> > - } else {
> > - qemu_set_irq(dev->irq[dev->exp.hpev_intx], 1);
> > - }
> > - }
> > + hotplug_event_notify(dev);
> > }
>
> The forward declaration of hotplug_event_notify() is necessary.
> Or move up hotplug_event_notify().
fixed up already
> >
> > static int pcie_cap_slot_hotplug(DeviceState *qdev,
> > @@ -212,6 +203,36 @@ static int pcie_cap_slot_hotplug(DeviceState *qdev,
> > return 0;
> > }
> >
> > +static void hotplug_event_notify(PCIDevice *dev)
> > +{
> > + bool prev = dev->exp.hpev_notified;
> > + uint32_t pos = dev->exp.exp_cap;
> > + uint8_t *exp_cap = dev->config + pos;
> > + uint16_t sltctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL);
> > + uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
> > +
> > + dev->exp.hpev_notified = (sltctl & PCI_EXP_SLTCTL_HPIE) &&
> > + (sltsta & PCI_EXP_HP_EV_SUPPORTED)
>
> should be (sltctl & sltsta & PCI_EXP_HP_EV_SUPPORTED) instead of
> (sltsta & PCI_EXP_HP_EV_SUPPORTED).
Right. Care sending a patch on top of current pci branch?
>
> > +
> > + if (prev == dev->exp.hpev_notified) {
> > + return;
> > + }
> > +
> > + /* Note: the logic above does not take into account whether interrupts
> > + * are masked. The result is that interrupt will be sent when it is
> > + * subsequently unmasked. This appears to be legal: Section 6.7.3.4:
> > + * The Port may optionally send an MSI when there are hot-plug events
> > that
> > + * occur while interrupt generation is disabled, and interrupt
> > generation is
> > + * subsequently enabled. */
> > + if (!pci_msi_enabled(dev)) {
> > + qemu_set_irq(dev->irq[dev->exp.hpev_intx], dev->exp.hpev_notified);
> > + } else if (dev->exp.hpev_notified) {
> > + pci_msi_notify(dev, pcie_cap_flags_get_vector(dev));
> > + return;
>
> This return seems redundant.
right. remove it
> > + }
> > +
> > +}
> > +
> > /* pci express slot for pci express root/downstream port
> > PCI express capability slot registers */
> > void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
> > @@ -256,6 +277,8 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
> > pci_word_test_and_set_mask(dev->w1cmask + pos + PCI_EXP_SLTSTA,
> > PCI_EXP_HP_EV_SUPPORTED);
> >
> > + dev->cap.hpev_notified = false;
> > +
> > pci_bus_hotplug(pci_bridge_get_sec_bus(DO_UPCAST(PCIBridge, dev, dev)),
> > pcie_cap_slot_hotplug, &dev->qdev);
> > }
> > @@ -284,11 +307,12 @@ void pcie_cap_slot_reset(PCIDevice *dev)
> > PCI_EXP_SLTSTA_CC |
> > PCI_EXP_SLTSTA_PDC |
> > PCI_EXP_SLTSTA_ABP);
> > +
> > + hotplug_event_notify(dev);
> > }
> >
> > void pcie_cap_slot_write_config(PCIDevice *dev,
> > - uint32_t addr, uint32_t val, int len,
> > - uint16_t sltctl_prev)
> > + uint32_t addr, uint32_t val, int len)
> > {
> > uint32_t pos = dev->exp.exp_cap;
> > uint8_t *exp_cap = dev->config + pos;
> > @@ -313,34 +337,7 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
> > 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);
> > - }
> > -
> > - if (!((sltctl_prev ^ sltctl) & PCI_EXP_SLTCTL_SUPPORTED)) {
> > - PCIE_DEV_PRINTF(dev,
> > - "sprious command completion slctl "
> > - "0x%"PRIx16" -> 0x%"PRIx16"\n",
> > - sltctl_prev, sltctl);
> > - }
> > + hotplug_event_notify(dev);
> >
> > /* command completion.
> > * Real hardware might take a while to complete
> > diff --git a/hw/pcie.h b/hw/pcie.h
> > index 2871e27..39c6e47 100644
> > --- a/hw/pcie.h
> > +++ b/hw/pcie.h
> > @@ -74,6 +74,11 @@ struct PCIExpressDevice {
> > * also initialize it when loaded as
> > * appropreately.
> > */
> > + bool hpev_notified; /* Logical AND of conditions for hot plug event.
> > + Following 6.7.3.4:
> > + Software Notification of Hot-Plug Events, an
> > interrupt
> > + is sent whenever the logical and of these
> > conditions
> > + transitions from false to true. */
> > };
>
> This breaks save/load.
Right. We'll need to restore this on load.
Take a subfunction of hotplug_event_notify
and put it in the appropriate _load function.
>
> >
> > /* PCI express capability helper functions */
> > @@ -89,8 +94,7 @@ void pcie_cap_deverr_reset(PCIDevice *dev);
> > void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot);
> > void pcie_cap_slot_reset(PCIDevice *dev);
> > void pcie_cap_slot_write_config(PCIDevice *dev,
> > - uint32_t addr, uint32_t val, int len,
> > - uint16_t sltctl_prev);
> > + uint32_t addr, uint32_t val, int len);
> > void pcie_cap_slot_push_attention_button(PCIDevice *dev);
> >
> > void pcie_cap_root_init(PCIDevice *dev);
> > diff --git a/hw/xio3130_downstream.c b/hw/xio3130_downstream.c
> > index a44e188..d46f911 100644
> > --- a/hw/xio3130_downstream.c
> > +++ b/hw/xio3130_downstream.c
> > @@ -38,12 +38,9 @@
> > static void xio3130_downstream_write_config(PCIDevice *d, uint32_t address,
> > uint32_t val, int len)
> > {
> > - uint16_t sltctl =
> > - pci_get_word(d->config + d->exp.exp_cap + PCI_EXP_SLTCTL);
> > -
> > pci_bridge_write_config(d, address, val, len);
> > pcie_cap_flr_write_config(d, address, val, len);
> > - pcie_cap_slot_write_config(d, address, val, len, sltctl);
> > + pcie_cap_slot_write_config(d, address, val, len);
> > msi_write_config(d, address, val, len);
> > /* TODO: AER */
> > }
> > --
> > 1.7.3-rc1
> >
>
> --
> yamahata
