| Message ID | 20101025054941.GA3223@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > > Untested. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> I made some changes to this and other patches, and pushed them out on my branch so everyone can take a look there.
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. > > Untested. > > Signed-off-by: Michael S. Tsirkin <mst@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(). > > 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). > + > + 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. > + } > + > +} > + > /* 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. > > /* 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 >
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 <mst@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
On Mon, Oct 25, 2010 at 08:46:38AM +0200, Michael S. Tsirkin wrote: > 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. Will do.
On Mon, Oct 25, 2010 at 04:04:26PM +0900, Isaku Yamahata wrote: > On Mon, Oct 25, 2010 at 08:46:38AM +0200, Michael S. Tsirkin wrote: > > 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. > > Will do. One other thing: I think when we validate data on load need to take w1c mask into account, not just wmask. > yamahata
On Mon, Oct 25, 2010 at 08:46:38AM +0200, Michael S. Tsirkin wrote: > > > --- 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. Or just call the notify - worst case we get an extra msi interrupt ... Not sure whether this is required but this seems to be what we do in virtio-pci. Up to you really ...
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); } 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); + + 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; + } + +} + /* 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. */ }; /* 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 */ }
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. Untested. Signed-off-by: Michael S. Tsirkin <mst@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(-)