[Qemu-devel] Re: [PATCH RFC] pcie: clean up hot plug notification

[Isaku Yamahata]

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 <address@hidden>
> ---
>  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