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

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

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.

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