Re: [Qemu-devel] [RFC v3 04/10] vfio: add pcie extanded capability support

[Chen Fan]

On 02/11/2015 12:39 AM, Alex Williamson wrote:
> On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote:
> > when we detect extanded capability in vfio device, then
> > we should initialize the vfio device corresponding feature
> > register bits.
> > so guest OS can find it and set those bits as needed.
> > and initialize aer capability.
> >
> > Signed-off-by: Chen Fan <address@hidden>
> > ---
> >   hw/vfio/pci.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 84 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 014a92c..75c932b 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2435,6 +2435,20 @@ static uint8_t vfio_std_cap_max_size(PCIDevice *pdev, 
> > uint8_t pos)
> >       return next - pos;
> >   }
> >   
> > +static uint16_t vfio_ext_cap_max_size(PCIDevice *pdev, uint16_t pos)
> > +{
> > +    uint16_t tmp, next = PCIE_CONFIG_SPACE_SIZE - 1;
> > +
> > +    for (tmp = PCI_CONFIG_SPACE_SIZE; tmp;
> > +        tmp = PCI_EXT_CAP_NEXT(pci_get_long(pdev->config + tmp))) {
> > +        if (tmp > pos && tmp < next) {
> > +            next = tmp;
> > +       }
> > +    }
> > +
> > +    return next - pos;
> > +}
> > +
> >   static void vfio_set_word_bits(uint8_t *buf, uint16_t val, uint16_t mask)
> >   {
> >       pci_set_word(buf, (pci_get_word(buf) & ~mask) | val);
> > @@ -2658,16 +2672,85 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, 
> > uint8_t pos)
> >       return 0;
> >   }
> >   
> > +static void vfio_setup_pcie_aer(VFIOPCIDevice *vdev, uint16_t pos)
> > +{
> > +    PCIDevice *pdev = &vdev->pdev;
> > +    uint32_t err_cap;
> > +
> > +    err_cap = pci_get_long(pdev->config + pos + PCI_ERR_CAP);
> > +
> > +    if (err_cap & PCI_ERR_CAP_MHRC) {
> > +        pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
> > +    } else {
> > +        pdev->exp.aer_log.log_max = 1;
> > +    }
> > +
> > +    pdev->exp.aer_log.log = g_malloc0(sizeof pdev->exp.aer_log.log[0] *
> > +                                      pdev->exp.aer_log.log_max);
> > +
> > +    pcie_aer_setup(pdev, pos, pdev->exp.aer_log.log_max);
> > +}
> > +
> > +static int vfio_add_ext_cap(VFIOPCIDevice *vdev, uint16_t pos)
> > +{
> > +    PCIDevice *pdev = &vdev->pdev;
> > +    uint32_t header;
> > +    uint16_t cap_id, next, size;
> > +    uint8_t cap_ver;
> > +    int ret;
> > +
> > +    header = pci_get_long(pdev->config + pos);
> > +    cap_id = PCI_EXT_CAP_ID(header);
> > +    cap_ver = PCI_EXT_CAP_VER(header);
> > +    next = PCI_EXT_CAP_NEXT(header);
> > +
> > +    size = vfio_ext_cap_max_size(pdev, pos);
> > +
> There's a big comment in the standard capability version of this that
> indicates that pci_add_capability() always adds capabilities to the head
> of the chain and therefore we use this recursion to keep the same
> ordering as hardware.  pcie_add_capability() seems to add at the tail of
> the list, so I think this actually reverses the list versus hardware,
> which is undesirable.  Code comments would be nice too.

IIUC, Compare pcie_add_capability() with pci_add_capability(), 
pcie_add_capability()
not only add caps at the tail of linked list, but also always add a new cap
at the tail of linked list. but for vfio device, we don't need to add a 
new cap.
we only need to parse the existed capability in config space.
so I think we should modify  pcie_add_capability() as pci_add_capability2().

Thanks,
Chen


> > +    if (next) {
> > +        ret = vfio_add_ext_cap(vdev, next);
> > +        if (ret) {
> > +            return ret;
> > +        }
> > +    }
> > +
> > +    pci_set_long(vdev->emulated_config_bits + pos, 0xffff);
> > +
> > +    switch (cap_id) {
> > +    case PCI_EXT_CAP_ID_ERR:
> > +        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
> > +        vfio_setup_pcie_aer(vdev, pos);
> > +        break;
> > +    default:
> > +        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
> > +        break;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >   static int vfio_add_capabilities(VFIOPCIDevice *vdev)
> >   {
> >       PCIDevice *pdev = &vdev->pdev;
> > +    int ret;
> >   
> >       if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST) ||
> >           !pdev->config[PCI_CAPABILITY_LIST]) {
> >           return 0; /* Nothing to add */
> >       }
> >   
> > -    return vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]);
> > +    ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]);
> > +    if (ret) {
> > +        goto out;
> > +    }
> > +
> > +    if (!pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
> > +        goto out;
> > +    }
> > +
> > +    ret = vfio_add_ext_cap(vdev, PCI_CONFIG_SPACE_SIZE);
> > +
> > +out:
> > +    return ret;
> >   }
> >   
> >   static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
>
>
> .