Re: [Qemu-devel] [PATCH] pci/pcie: don't assume cap id 0 is reserved

[Alex Williamson]

On Wed, 15 Feb 2017 22:49:47 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> VFIO actually wants to create a capability with ID == 0.
> This is done to make guest drivers skip the given capability.
> pcie_add_capability then trips up on this capability
> when looking for end of capability list.
> 
> To support this use-case, it's easy enough to switch to
> e.g. 0xffffffff for these comparisons - we can be sure
> it will never match a 16-bit capability ID.
> 
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---


Reviewed-by: Alex Williamson <address@hidden>


>  hw/pci/pcie.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index cbd4bb4..f4dd177 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -610,7 +610,8 @@ bool pcie_cap_is_arifwd_enabled(const PCIDevice *dev)
>   * uint16_t ext_cap_size
>   */
>  
> -static uint16_t pcie_find_capability_list(PCIDevice *dev, uint16_t cap_id,
> +/* Passing a cap_id value > 0xffff will return 0 and put end of list in prev 
> */
> +static uint16_t pcie_find_capability_list(PCIDevice *dev, uint32_t cap_id,
>                                            uint16_t *prev_p)
>  {
>      uint16_t prev = 0;
> @@ -679,9 +680,11 @@ void pcie_add_capability(PCIDevice *dev,
>      } else {
>          uint16_t prev;
>  
> -        /* 0 is reserved cap id. use internally to find the last capability
> -           in the linked list */
> -        next = pcie_find_capability_list(dev, 0, &prev);
> +        /*
> +         * 0xffffffff is not a valid cap id (it's a 16 bit field). use
> +         * internally to find the last capability in the linked list.
> +         */
> +        next = pcie_find_capability_list(dev, 0xffffffff, &prev);
>  
>          assert(prev >= PCI_CONFIG_SPACE_SIZE);
>          assert(next == 0);