[v1] pcie: simplify pcie_add_capability()

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

Posted by Peter Xu 7 years, 2 months ago

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.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 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);
-- 
2.7.4

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

Posted by Marcel Apfelbaum 7 years, 2 months ago

On 02/14/2017 09:51 AM, 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.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  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);

Hi Peter,

It is not recommended to use assert with an expression with side-effects.

Thanks,
Marcel

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

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

Posted by Peter Xu 7 years, 2 months ago

On Wed, Feb 15, 2017 at 04:25:05PM +0200, Marcel Apfelbaum wrote:
> On 02/14/2017 09:51 AM, 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.
> >
> >Signed-off-by: Peter Xu <peterx@redhat.com>
> >---
> > 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);
> 
> Hi Peter,
> 
> It is not recommended to use assert with an expression with side-effects.

Exactly. Thanks Marcel, I'll repost.

-- peterx

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

Posted by Cao jin 7 years, 2 months ago

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 <peterx@redhat.com>
> ---
>  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);
>

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

Posted by Peter Xu 7 years, 2 months ago

On Thu, Feb 16, 2017 at 10:18:00AM +0800, Cao jin wrote:
> 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

Sure. But imho that's really trivial and as long as the assertions are
working correctly (no matter in which order) I can live with both. :)

Anyway, thanks for the pointer!

-- peterx