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

[Chen Fan <chen.fan.fnst@cn.fujitsu.com>]

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

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