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

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 2015-02-26 at 17:37 +0800, Chen Fan wrote:
> 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 <chen.fan.fnst@cn.fujitsu.com>
> >> ---
> >>   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().

We want vfio to use the same interfaces to the QEMU core PCI code as an
emulated device.  We add capabilities so that we can inform the core
code that they're there and potentially get benefits later as emulated
devices add capabilities.  I don't see why we'd add a new interface to
the PCI code for this, we simply need to analyze how they operate and
use them accordingly.  In this case we should not simply do a blind copy
of the standard capability code because the ordering is different.
Thanks,

Alex

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