Re: [Qemu-devel] [PATCH] pcie: simplify pcie_add_capability()

[Cao jin]

Hi peter

On 02/14/2017 03:51 PM, Peter Xu wrote:
> When we add PCIe extended capabilities, we should be following the rule
> that we add the head extended cap (at offset 0x100) first, then the rest
> of them. Meanwhile, we are always adding new capability bits at the end
> of the list. Here the "next" looks meaningless in all cases since it
> should always be zero (along with the "header").
> 
> Simplify the function a bit, and it looks more readable now.
> 

See if this suggestion could be incorporated into your patch:)
http://lists.nongnu.org/archive/html/qemu-devel/2017-01/msg01418.html

-- 
Sincerely,
Cao jin

> Signed-off-by: Peter Xu <address@hidden>
> ---
>  hw/pci/pcie.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index cbd4bb4..e0e6f6a 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -664,30 +664,23 @@ void pcie_add_capability(PCIDevice *dev,
>                           uint16_t cap_id, uint8_t cap_ver,
>                           uint16_t offset, uint16_t size)
>  {
> -    uint32_t header;
> -    uint16_t next;
> -
>      assert(offset >= PCI_CONFIG_SPACE_SIZE);
>      assert(offset < offset + size);
>      assert(offset + size <= PCIE_CONFIG_SPACE_SIZE);
>      assert(size >= 8);
>      assert(pci_is_express(dev));
>  
> -    if (offset == PCI_CONFIG_SPACE_SIZE) {
> -        header = pci_get_long(dev->config + offset);
> -        next = PCI_EXT_CAP_NEXT(header);
> -    } else {
> +    if (offset != PCI_CONFIG_SPACE_SIZE) {
>          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);
> -
> +        assert(pcie_find_capability_list(dev, 0, &prev) == 0);
>          assert(prev >= PCI_CONFIG_SPACE_SIZE);
> -        assert(next == 0);
>          pcie_ext_cap_set_next(dev, prev, offset);
>      }
> -    pci_set_long(dev->config + offset, PCI_EXT_CAP(cap_id, cap_ver, next));
> +
> +    pci_set_long(dev->config + offset, PCI_EXT_CAP(cap_id, cap_ver, 0));
>  
>      /* Make capability read-only by default */
>      memset(dev->wmask + offset, 0, size);
>