[Qemu-devel] Re: [PATCH 02/14] pci: consolidate pci_add_capability_at_offset() into pci_add_capability().

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Sep 06, 2010 at 04:46:16PM +0900, Isaku Yamahata wrote:
> By making pci_add_capability() the special case of
> pci_add_capability_at_offset() of offset = 0,
> consolidate pci_add_capability_at_offset() into pci_add_capability().
> 
> Cc: Stefan Weil <weil@mail.berlios.de>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Good stuff.

Here's an idea: Long term I think we should just give up on the ability
to allocate capabilities dynamically, and always pass a valid offset.
In particular, this would help us get rid of the used mask, and we won't
have to pass in capability size.

virtio would then have
#define VIRTIO_PCI_MSIX_CAP_OFF 0x40
and msix.h would be changed to get the offset.

The reason is that we need to preserve the offsets for migration
to work, anyway.

Have said all this, this is just an idea, it is not a must
and can be a separate patch, or I might do this change myself.

> ---
>  hw/eepro100.c |    4 ++--
>  hw/msix.c     |    3 ++-
>  hw/pci.c      |   33 ++++++++++++++++++---------------
>  hw/pci.h      |    5 ++---
>  4 files changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index 2b75c8f..8cbc3aa 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -539,8 +539,8 @@ static void e100_pci_reset(EEPRO100State * s, E100PCIDeviceInfo *e100_device)
>      if (e100_device->power_management) {
>          /* Power Management Capabilities */
>          int cfg_offset = 0xdc;
> -        int r = pci_add_capability_at_offset(&s->dev, PCI_CAP_ID_PM,
> -                                             cfg_offset, PCI_PM_SIZEOF);
> +        int r = pci_add_capability(&s->dev, PCI_CAP_ID_PM,
> +                                   cfg_offset, PCI_PM_SIZEOF);
>          assert(r >= 0);
>          pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
>  #if 0 /* TODO: replace dummy code for power management emulation. */
> diff --git a/hw/msix.c b/hw/msix.c
> index d99403a..7ce63eb 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -73,7 +73,8 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
>      }
>  
>      pdev->msix_bar_size = new_size;
> -    config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> +    config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
> +                                       0, MSIX_CAP_LENGTH);
>      if (config_offset < 0)
>          return config_offset;
>      config = pdev->config + config_offset;
> diff --git a/hw/pci.c b/hw/pci.c
> index 2dc1577..754ffb3 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1682,11 +1682,25 @@ static void pci_del_option_rom(PCIDevice *pdev)
>      pdev->rom_offset = 0;
>  }
>  
> -/* Reserve space and add capability to the linked list in pci config space */
> -int pci_add_capability_at_offset(PCIDevice *pdev, uint8_t cap_id,
> -                                 uint8_t offset, uint8_t size)
> +/*
> + * if !offset
> + * Reserve space and add capability to the linked list in pci config space
> + *
> + * if offset = 0,
> + * Find and reserve space and add capability to the linked list
> + * in pci config space */
> +int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> +                       uint8_t offset, uint8_t size)
>  {
> -    uint8_t *config = pdev->config + offset;
> +    uint8_t *config;
> +    if (!offset) {
> +        offset = pci_find_space(pdev, size);
> +        if (!offset) {
> +            return -ENOSPC;
> +        }
> +    }
> +
> +    config = pdev->config + offset;
>      config[PCI_CAP_LIST_ID] = cap_id;
>      config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
>      pdev->config[PCI_CAPABILITY_LIST] = offset;
> @@ -1699,17 +1713,6 @@ int pci_add_capability_at_offset(PCIDevice *pdev, uint8_t cap_id,
>      return offset;
>  }
>  
> -/* Find and reserve space and add capability to the linked list
> - * in pci config space */
> -int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
> -{
> -    uint8_t offset = pci_find_space(pdev, size);
> -    if (!offset) {
> -        return -ENOSPC;
> -    }
> -    return pci_add_capability_at_offset(pdev, cap_id, offset, size);
> -}
> -
>  /* Unlink capability from the pci config space. */
>  void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
>  {
> diff --git a/hw/pci.h b/hw/pci.h
> index c551f96..2ddba59 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -183,9 +183,8 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>                              pcibus_t size, int type,
>                              PCIMapIORegionFunc *map_func);
>  
> -int pci_add_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
> -int pci_add_capability_at_offset(PCIDevice *pci_dev, uint8_t cap_id,
> -                                 uint8_t cap_offset, uint8_t cap_size);
> +int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> +                       uint8_t offset, uint8_t size);
>  
>  void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
>  
> -- 
> 1.7.1.1