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

[Michael S. Tsirkin]

On Mon, Oct 25, 2010 at 03:17:37PM +0900, Isaku Yamahata wrote:
> 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.

I think no, because the logic behind pcie_cap_slot_event guarantees that
interrupt is only sent when something changes.  What am I missing?

> >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.

That would definitely be right if we did anything with the command
besides just sending an interrupt.  In that case, command processing
could would go to within range overlap, the rest would stay without.

> -- 
> yamahata