[Qemu-devel] Re: [PATCH v3 05/13] pcie: helper functions for pcie capability and extended capability.

[Isaku Yamahata]

On Wed, Sep 15, 2010 at 02:43:10PM +0200, Michael S. Tsirkin wrote:
> > +/***************************************************************************
> > + * pci express capability helper functions
> > + */
> > +void pcie_notify(PCIDevice *dev, uint16_t vector, bool trigger, int level)
> 
> Why is this not static? It makes sense for internal stuff possibly,
> but I think functions will need to know what to do: they can't
> treat msi/msix/irq identically anyway.

The aer code which I split out uses it.


> The API seems confusing, I think this is what is creating
> code for you. Specifically level = 0 does not notify at all.
> So I think we need two:
> 1. pcie_assert_interrupt which sends msi or sets level to 1
> 2. pcie_deassert_interrupt which sets level to 0, or nothing
>    for non msi.
> 
> Then below you can e.g.
> if (!sltctrl) {
>       pcie_deassert(...);
>       return;
> }

As I already mentioned in the other mail, when to assert MSI
can be different from INTx. The express spec utilizes it.
For example hot plug, aer, and so on.


> > +                    vector, trigger, level);
> > +    if (msix_enabled(dev)) {
> > +        if (trigger) {
> > +            msix_notify(dev, vector);
> > +        }
> > +    } else if (msi_enabled(dev)) {
> > +        if (trigger){
> > +            msi_notify(dev, vector);
> > +        }
> > +    } else {
> > +        qemu_set_irq(dev->irq[0], level);
> 
> always 0? really? This is INTA# - is this what the spec says?

It depends on each device implementation. So I just picked INTA#.
Okay, I'll make it customizable by using property.


> > +    }
> > +}
> > +
> > +int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t 
> > port)
> > +{
> > +    int exp_cap;
> > +    uint8_t *pcie_cap;
> > +
> > +    assert(pci_is_express(dev));
> > +
> > +    exp_cap = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
> > +                                 PCI_EXP_VER2_SIZEOF);
> > +    if (exp_cap < 0) {
> > +        return exp_cap;
> > +    }
> > +    dev->exp.exp_cap = exp_cap;
> > +
> > +    /* already done in pci_qdev_init() */
> > +    assert(dev->cap_present & QEMU_PCI_CAP_EXPRESS);
> 
> Hmm. Why do we set this flag in qdev init but do the
> rest of it here?

pci_qdev_init() needs to know it for allocating config array.


> > +        return -EBUSY;
> > +    }
> > +
> > +    if (state) {
> > +        if (PCI_FUNC(pci_dev->devfn) == 0) {
> > +            /* event is per slot. Not per function
> > +             * only generates event for function = 0.
> > +             * When hot plug, populate functions > 0
> > +             * and then add function = 0 last.
> > +             */
> > +            pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC, PCI_EXP_SLTSTA_PDS);
> > +        }
> > +    } else {
> > +        PCIBridge *br;
> > +        PCIBus *bus;
> > +        DeviceState *next;
> > +        if (PCI_FUNC(pci_dev->devfn) != 0) {
> > +            /* event is per slot. Not per function.
> > +               accepts function = 0 only. */
> > +            return -EINVAL;
> 
> Can user or guest trigger this?
> If yes print an error.
> IF no, assert.

Not yet. When multi function device hot plug is supported in some sense,
it will be triggered in some way. The code is just place holder until then.
Some TODO comment should have been there.


> > +                level = 1;
> > +            } else {
> > +                level = 0;
> > +            }
> 
> What is this trying to implement?

hot plut event notification. Please refer to
6.7. PCI Express Hot-Plug Support
6.7.3. PCI Express Hot-Plug Events
6.7.3.4. Software Notification of Hot-Plug Events

> > +        assert(offset == PCI_CONFIG_SPACE_SIZE);
> > +        pci_set_long(dev->config + offset,
> > +                     PCI_EXT_CAP(0, 0, PCI_EXT_CAP_NEXT(header)));
> > +    }
> > +
> > +    /* Make those registers read-only reserved zero */
> 
> So you make them readonly in both add and delete?
> delete should revert add: let's put the
> masks back the way they were: writeable.

In fact zeroing in add is redundant, but I added it following msix code.

The usage model is
- At first the registers are unused, so should be read only zero.
- add capability
  (zeroing is redundant)
- the device specific code sets config/mask/cmask/w1cmask as it likes
- del capability
  This makes the register unused, i.e. read only zero.

Maybe it's possible to make zeroing them caller's responsible.


> > +    /* the bits match the bits in Slot Control/Status registers.
> > +     * PCI_EXP_HP_EV_xxx = PCI_EXP_SLTCTL_xxxE = PCI_EXP_SLTSTA_xxx
> 
> Is it important that they match?
> We don't assume this in code, do we?

Yes, we're using it when setting stlsta.


> > +    /* events not listed aren't supported */
> > +};
> > +
> > +typedef void (*pcie_flr_fn)(PCIDevice *dev);
> 
> Is flr special?  Can't we use the generic reset handlers?
> If not why?

Reset(cold reset/warm reset) in generic sense corresponds to
conventional reset in express sense which corresponds to PCI RST#.
On the other hand FLR is different from the conventional one.

Cited from the spec

6.6. PCI Express Reset - Rules
6.6.1. Conventional Reset
 Conventional Reset includes all reset mechanisms other than Function
 Level Reset.
6.6.2. Function-Level Reset (FLR)

Most devices would implement FLR as just calling something like
qdev_reset. But the spec differentiates FLR from conventional reset,
so the generic pcie layer should do.
-- 
yamahata