Re: [Qemu-devel] [RFC v4 3/9] vfio: add pcie extanded capability support

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

On 03/10/2015 04:28 AM, Alex Williamson wrote:
> On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
>> For vfio pcie device, we could expose the extanded capability on
> s/extanded/extended/
>
>> PCIE bus. in order to avoid config space broken, we introduce
>> a copy config for parsing extended caps. and rebuild the pcie
>> extended config space.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   hw/vfio/pci.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 82 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 84e9d99..96cb52b 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2482,6 +2482,21 @@ static uint8_t vfio_std_cap_max_size(PCIDevice *pdev, uint8_t pos)
>>       return next - pos;
>>   }
>>   
>> +
>> +static uint16_t vfio_ext_cap_max_size(const uint8_t *config, 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(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);
>> @@ -2705,16 +2720,82 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>>       return 0;
>>   }
>>   
>> +static int vfio_add_ext_cap(VFIOPCIDevice *vdev, const uint8_t *config,
>> +                            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(config + pos);
>> +    cap_id = PCI_EXT_CAP_ID(header);
>> +    cap_ver = PCI_EXT_CAP_VER(header);
>> +    next = PCI_EXT_CAP_NEXT(header);
>> +
>> +    /*
>> +     * If it becomes important to configure extended capabilities to their
>> +     * actual size, use this as the default when it's something we don't
>> +     * recognize. Since QEMU doesn't actually handle many of the config
>> +     * accesses, exact size doesn't seem worthwhile.
>> +     */
>> +    size = vfio_ext_cap_max_size(config, pos);
>> +
>> +    pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
>> +    if (pos == PCI_CONFIG_SPACE_SIZE) {
>> +        /* Begin the rebuild, we should set the next offset zero. */
>> +        pci_set_long(pdev->config + pos, PCI_EXT_CAP(cap_id, cap_ver, 0));
>> +    }
>> +
>> +    /* Use emulated header pointer to allow dropping extended caps */
>> +    pci_set_long(vdev->emulated_config_bits + pos, 0xffffffff);
>> +
>> +    if (next) {
>> +        ret = vfio_add_ext_cap(vdev, config, next);
>> +        if (ret) {
>> +            return ret;
>> +        }
>> +    }
> This recursion seems pointless.  We use it for the standard capabilities
> to make the capability list ordering correct and to avoid the config
> space copy that is necessary for this version, but I don't see the
> advantage to using it here vs a for-loop in the below function.
> Recursion is more complicated, so let's only use it if it provides some
> benefit.
indeed.

Thanks,
Chen


>
>> +
>> +    return 0;
>> +}
>> +
>>   static int vfio_add_capabilities(VFIOPCIDevice *vdev)
>>   {
>>       PCIDevice *pdev = &vdev->pdev;
>> +    int ret;
>> +    uint8_t *config;
>>   
>>       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) {
>> +        return ret;
>> +    }
>> +
>> +    /* on PCI bus, it doesn't make sense to expose extended capabilities. */
>> +    if (!pci_bus_is_express(vdev->pdev.bus) ||
>> +        !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
>
> Don't we need a pci_is_express(pdev) here too?  Also, we already have
> pdev->bus to use as an argument, no need to start with the vdev.
>
>> +        return 0;
>> +    }
>> +
>> +    /*
>> +     * In order to avoid config space broken, here using a copy config to
>> +     * parse extended capabilitiess.
> spelling
>
>> +     */
>> +    config = g_malloc0(vdev->config_size);
>> +    if (!config) {
>> +        return -ENOMEM;
>> +    }
> g_malloc can't fail, no need for this error out condition.  It's
> strange, but it's the QEMU way.
>
>> +    memcpy(config, pdev->config, vdev->config_size);
> There's a g_memdup() function
>
>> +    ret = vfio_add_ext_cap(vdev, config, PCI_CONFIG_SPACE_SIZE);
>> +
>> +    g_free(config);
>> +    return ret;
>>   }
>>   
>>   static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
>
>
> .
>