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

[Marcel Apfelbaum]

On 01/12/2016 04:43 AM, Cao jin wrote:
> From: Chen Fan <address@hidden>

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 :) ?

> Signed-off-by: Chen Fan <address@hidden>
> ---
>   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.

> +
>   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 :(.

> +     */
> +    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?

Thank you,
Marcel

> +        return 0;
> +    }
> +
> +    return vfio_add_ext_cap(vdev);
>   }
>
>   static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)