[Qemu-devel] Re: [PATCH rfc] pcie: get rid of range checks

[Isaku Yamahata]

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