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 <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().
>
> 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
>
--
yamahata
