Re: [Qemu-devel] [PATCH v16 05/14] vfio: add pcie extanded capability support

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

On 01/17/2016 09:22 PM, Marcel Apfelbaum wrote:
> On 01/12/2016 04:43 AM, Cao jin wrote:
>> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>
>
> Hi,
>
> I noticed a type in the subject, extanded -> extended
>
>> For vfio pcie device, we could expose the extended capability on
>> 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.
>
> Maybe we can re-word this. Will someone with better English skills
> advice :) ?
>
that will be helpful. ;)

>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   hw/vfio/pci.c | 70 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 69 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 288f2c7..64b0867 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -1482,6 +1482,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;
>> +
>> +    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;
>> +}
>
> Can't we reuse vfio_std_cap_max_size here? if only the config size 
> differs,
> we can pass it as parameter.
not only the config size differ, but also the PCI express Extended 
Capability header,
the pci express head use 16bit to store the cap id and 12bit to store 
the next offset.

>
>> +
>>   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);
>> @@ -1817,16 +1832,69 @@ static int vfio_add_std_cap(VFIOPCIDevice 
>> *vdev, uint8_t pos)
>>       return 0;
>>   }
>>
>> +static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
>> +{
>> +    PCIDevice *pdev = &vdev->pdev;
>> +    uint32_t header;
>> +    uint16_t cap_id, next, size;
>> +    uint8_t cap_ver;
>> +    uint8_t *config;
>> +
>> +    /*
>> +     * In order to avoid breaking config space, create a copy to
>> +     * use for parsing extended capabilities.
>
> It will be nice to know *how* do we break/*what* will break the config
> space, I confess that I didn't see it :(.
I will improve it.

>
>> +     */
>> +    config = g_memdup(pdev->config, vdev->config_size);
>> +
>> +    for (next = PCI_CONFIG_SPACE_SIZE; next;
>> +         next = PCI_EXT_CAP_NEXT(pci_get_long(config + next))) {
>> +        header = pci_get_long(config + next);
>> +        cap_id = PCI_EXT_CAP_ID(header);
>> +        cap_ver = PCI_EXT_CAP_VER(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, next);
>> +
>> +        pcie_add_capability(pdev, cap_id, cap_ver, next, size);
>> +        pci_set_long(dev->config + next, PCI_EXT_CAP(cap_id, 
>> cap_ver, 0));
>> +
>> +        /* Use emulated next pointer to allow dropping extended caps */
>> + pci_long_test_and_set_mask(vdev->emulated_config_bits + next,
>> +                                   PCI_EXT_CAP_NEXT_MASK);
>> +    }
>> +
>> +    g_free(config);
>> +    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) {
>> +        return ret;
>> +    }
>> +
>> +    /* on PCI bus, it doesn't make sense to expose extended 
>> capabilities. */
>> +    if (!pci_is_express(pdev) ||
>> +        !pci_bus_is_express(pdev->bus) ||
>> +        !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
>
> I am curious about the last check, "!pci_get_long(pdev->config + 
> PCI_CONFIG_SPACE_SIZE)",
> can you please explain?
the pcie spec 3.0 defines that:

7.9.1. Extended Capabilities in Configuration Space
Extended Capabilities in Configuration Space always begin at offset 100h 
with a PCI Express
Extended Capability header (Section 7.9.3). Absence of any Extended 
Capabilities is required to be
indicated by an Extended Capability header with a Capability ID of 
0000h, a Capability Version of
0h, and a Next Capability Offset of 000h.

so here we test whether the offset 100h is zero.

Thanks,
Chen


>
> Thank you,
> Marcel
>
>> +        return 0;
>> +    }
>> +
>> +    return vfio_add_ext_cap(vdev);
>>   }
>>
>>   static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
>>
>
>
>
> .
>