| Message ID | 20180824171420.31246-2-bluca@debian.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Thomas Monjalon |
| Headers | show |
| Series | [v4,1/2] bus/pci: harmonize and document rte_pci_read_config return value | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/Intel-compilation | success | Compilation OK |
On Fri, Aug 24, 2018 at 06:14:20PM +0100, Luca Boccassi wrote: > From: Brian Russell <brussell@brocade.com> > > In virtio_read_caps and vtpci_msix_detect, rte_pci_read_config returns > the number of bytes read from PCI config or < 0 on error. > If less than the expected number of bytes are read then log the > failure and return rather than carrying on with garbage. > > Fixes: 6ba1f63b5ab0 ("virtio: support specification 1.0") > > Signed-off-by: Brian Russell <brussell@brocade.com> > Signed-off-by: Luca Boccassi <bluca@debian.org> > --- > v2: handle additional rte_pci_read_config incomplete reads > v3: do not handle rte_pci_read_config of virtio cap, added in v2, > as it's less clear what the right thing to do there is > v4: do a more robust check - first check what the vendor is, and > skip the cap entirely if it's not what we are looking for. > > drivers/net/virtio/virtio_pci.c | 57 ++++++++++++++++++++++++--------- > 1 file changed, 42 insertions(+), 15 deletions(-) > > diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c > index 6bd22e54a6..cfefa9789b 100644 > --- a/drivers/net/virtio/virtio_pci.c > +++ b/drivers/net/virtio/virtio_pci.c > @@ -567,16 +567,30 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw) > } > > ret = rte_pci_read_config(dev, &pos, 1, PCI_CAPABILITY_LIST); > - if (ret < 0) { > - PMD_INIT_LOG(DEBUG, "failed to read pci capability list"); > + if (ret != 1) { > + PMD_INIT_LOG(DEBUG, > + "failed to read pci capability list, ret %d", ret); > return -1; > } > > while (pos) { > + ret = rte_pci_read_config(dev, &cap, 2, pos); > + if (ret != 2) { > + PMD_INIT_LOG(DEBUG, > + "failed to read pci cap at pos: %x ret %d", > + pos, ret); > + break; > + } > + if (cap.cap_vndr != PCI_CAP_ID_MSIX && > + cap.cap_vndr != PCI_CAP_ID_VNDR) { > + goto next; > + } > + > ret = rte_pci_read_config(dev, &cap, sizeof(cap), pos); > - if (ret < 0) { > - PMD_INIT_LOG(ERR, > - "failed to read pci cap at pos: %x", pos); > + if (ret != sizeof(cap)) { > + PMD_INIT_LOG(DEBUG, > + "failed to read pci cap at pos: %x ret %d", > + pos, ret); > break; > } > It seems that I didn't make myself clear in my previous comments. I mean it's better to handle MSIX cap and virtio cap respectively in this function. Currently we're always reading them as virtio caps. As we are strictly requiring that _read_config() should return the required number of bytes, it's not perfect to require it to return "virtio cap size" of bytes while we're trying to read a MSIX cap. So please change the code to something similar to this: while (pos) { ret = rte_pci_read_config(dev, &cap, 2, pos); if (ret != 2) { PMD_INIT_LOG(DEBUG, "failed to read pci cap at pos: %x ret %d", pos, ret); break; } if (cap.cap_vndr == PCI_CAP_ID_MSIX) { /* Transitional devices would also have this capability, * that's why we also check if msix is enabled. * 1st byte is cap ID; 2nd byte is the position of next * cap; next two bytes are the flags. */ uint16_t flags; ret = rte_pci_read_config(dev, &flags, sizeof(flags), pos + 2); if (ret != sizeof(flags)) { PMD_INIT_LOG(DEBUG, "failed to read pci cap at pos: %x ret %d", pos + 2, ret); break; } if (flags & PCI_MSIX_ENABLE) hw->use_msix = VIRTIO_MSIX_ENABLED; else hw->use_msix = VIRTIO_MSIX_DISABLED; } if (cap.cap_vndr != PCI_CAP_ID_VNDR) { PMD_INIT_LOG(DEBUG, "[%2x] skipping non VNDR cap id: %02x", pos, cap.cap_vndr); goto next; } ret = rte_pci_read_config(dev, &cap, sizeof(cap), pos); if (ret != sizeof(cap)) { PMD_INIT_LOG(DEBUG, "failed to read pci cap at pos: %x ret %d", pos, ret); break; } PMD_INIT_LOG(DEBUG, "[%2x] cfg type: %u, bar: %u, offset: %04x, len: %u", pos, cap.cfg_type, cap.bar, cap.offset, cap.length); switch (cap.cfg_type) { ...... > @@ -689,25 +703,38 @@ enum virtio_msix_status > vtpci_msix_detect(struct rte_pci_device *dev) > { > uint8_t pos; > - struct virtio_pci_cap cap; > int ret; > > ret = rte_pci_read_config(dev, &pos, 1, PCI_CAPABILITY_LIST); > - if (ret < 0) { > - PMD_INIT_LOG(DEBUG, "failed to read pci capability list"); > + if (ret != 1) { > + PMD_INIT_LOG(DEBUG, > + "failed to read pci capability list, ret %d", ret); > return VIRTIO_MSIX_NONE; > } > > while (pos) { > - ret = rte_pci_read_config(dev, &cap, sizeof(cap), pos); > - if (ret < 0) { > - PMD_INIT_LOG(ERR, > - "failed to read pci cap at pos: %x", pos); > + uint8_t cap[2]; > + > + ret = rte_pci_read_config(dev, cap, sizeof(cap), pos); > + if (ret != sizeof(cap)) { > + PMD_INIT_LOG(DEBUG, > + "failed to read pci cap at pos: %x ret %d", > + pos, ret); > break; > } > > - if (cap.cap_vndr == PCI_CAP_ID_MSIX) { > - uint16_t flags = ((uint16_t *)&cap)[1]; > + if (cap[0] == PCI_CAP_ID_MSIX) { > + uint16_t flags; > + > + ret = rte_pci_read_config(dev, &flags, sizeof(flags), > + pos + sizeof(cap)); > + if (ret != sizeof(flags)) { > + PMD_INIT_LOG(DEBUG, > + "failed to read pci cap at pos:" > + " %x ret %d", pos + sizeof(cap), There is a build error: In file included from drivers/net/virtio/virtio_pci.c:15: drivers/net/virtio/virtio_pci.c: In function ‘vtpci_msix_detect’: drivers/net/virtio/virtio_logs.h:13:3: error: format ‘%x’ expects argument of type ‘unsigned int’, but argument 5 has type ‘long unsigned int’ [-Werror=format=] "%s(): " fmt "\n", __func__, ##args) ^~~~~~~~ drivers/net/virtio/virtio_pci.c:732:5: note: in expansion of macro ‘PMD_INIT_LOG’ PMD_INIT_LOG(DEBUG, ^~~~~~~~~~~~ drivers/net/virtio/virtio_pci.c:734:14: note: format string is defined here " %x ret %d", pos + sizeof(cap), ~^ %lx > + ret); > + break; > + } > > if (flags & PCI_MSIX_ENABLE) > return VIRTIO_MSIX_ENABLED; > @@ -715,7 +742,7 @@ vtpci_msix_detect(struct rte_pci_device *dev) > return VIRTIO_MSIX_DISABLED; > } > > - pos = cap.cap_next; > + pos = cap[1]; > } > > return VIRTIO_MSIX_NONE; > -- > 2.18.0 >
On Mon, 2018-08-27 at 13:29 +0800, Tiwei Bie wrote: > On Fri, Aug 24, 2018 at 06:14:20PM +0100, Luca Boccassi wrote: > > From: Brian Russell <brussell@brocade.com> > > > > In virtio_read_caps and vtpci_msix_detect, rte_pci_read_config > > returns > > the number of bytes read from PCI config or < 0 on error. > > If less than the expected number of bytes are read then log the > > failure and return rather than carrying on with garbage. > > > > Fixes: 6ba1f63b5ab0 ("virtio: support specification 1.0") > > > > Signed-off-by: Brian Russell <brussell@brocade.com> > > Signed-off-by: Luca Boccassi <bluca@debian.org> > > --- > > v2: handle additional rte_pci_read_config incomplete reads > > v3: do not handle rte_pci_read_config of virtio cap, added in v2, > > as it's less clear what the right thing to do there is > > v4: do a more robust check - first check what the vendor is, and > > skip the cap entirely if it's not what we are looking for. > > > > drivers/net/virtio/virtio_pci.c | 57 ++++++++++++++++++++++++----- > > ---- > > 1 file changed, 42 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/net/virtio/virtio_pci.c > > b/drivers/net/virtio/virtio_pci.c > > index 6bd22e54a6..cfefa9789b 100644 > > --- a/drivers/net/virtio/virtio_pci.c > > +++ b/drivers/net/virtio/virtio_pci.c > > @@ -567,16 +567,30 @@ virtio_read_caps(struct rte_pci_device *dev, > > struct virtio_hw *hw) > > } > > > > ret = rte_pci_read_config(dev, &pos, 1, > > PCI_CAPABILITY_LIST); > > - if (ret < 0) { > > - PMD_INIT_LOG(DEBUG, "failed to read pci capability > > list"); > > + if (ret != 1) { > > + PMD_INIT_LOG(DEBUG, > > + "failed to read pci capability list, > > ret %d", ret); > > return -1; > > } > > > > while (pos) { > > + ret = rte_pci_read_config(dev, &cap, 2, pos); > > + if (ret != 2) { > > + PMD_INIT_LOG(DEBUG, > > + "failed to read pci cap at > > pos: %x ret %d", > > + pos, ret); > > + break; > > + } > > + if (cap.cap_vndr != PCI_CAP_ID_MSIX && > > + cap.cap_vndr != PCI_CAP_ID_VNDR) { > > + goto next; > > + } > > + > > ret = rte_pci_read_config(dev, &cap, sizeof(cap), > > pos); > > - if (ret < 0) { > > - PMD_INIT_LOG(ERR, > > - "failed to read pci cap at pos: > > %x", pos); > > + if (ret != sizeof(cap)) { > > + PMD_INIT_LOG(DEBUG, > > + "failed to read pci cap at > > pos: %x ret %d", > > + pos, ret); > > break; > > } > > > > It seems that I didn't make myself clear in my previous > comments. I mean it's better to handle MSIX cap and virtio > cap respectively in this function. Currently we're always > reading them as virtio caps. As we are strictly requiring > that _read_config() should return the required number of > bytes, it's not perfect to require it to return "virtio > cap size" of bytes while we're trying to read a MSIX cap. > So please change the code to something similar to this: Sorry, though you meant in the vtpci_msix_detect function, which I changed. Fixed in v5. > > @@ -689,25 +703,38 @@ enum virtio_msix_status > > vtpci_msix_detect(struct rte_pci_device *dev) > > { > > uint8_t pos; > > - struct virtio_pci_cap cap; > > int ret; > > > > ret = rte_pci_read_config(dev, &pos, 1, > > PCI_CAPABILITY_LIST); > > - if (ret < 0) { > > - PMD_INIT_LOG(DEBUG, "failed to read pci capability > > list"); > > + if (ret != 1) { > > + PMD_INIT_LOG(DEBUG, > > + "failed to read pci capability list, > > ret %d", ret); > > return VIRTIO_MSIX_NONE; > > } > > > > while (pos) { > > - ret = rte_pci_read_config(dev, &cap, sizeof(cap), > > pos); > > - if (ret < 0) { > > - PMD_INIT_LOG(ERR, > > - "failed to read pci cap at pos: > > %x", pos); > > + uint8_t cap[2]; > > + > > + ret = rte_pci_read_config(dev, cap, sizeof(cap), > > pos); > > + if (ret != sizeof(cap)) { > > + PMD_INIT_LOG(DEBUG, > > + "failed to read pci cap at > > pos: %x ret %d", > > + pos, ret); > > break; > > } > > > > - if (cap.cap_vndr == PCI_CAP_ID_MSIX) { > > - uint16_t flags = ((uint16_t *)&cap)[1]; > > + if (cap[0] == PCI_CAP_ID_MSIX) { > > + uint16_t flags; > > + > > + ret = rte_pci_read_config(dev, &flags, > > sizeof(flags), > > + pos + sizeof(cap)); > > + if (ret != sizeof(flags)) { > > + PMD_INIT_LOG(DEBUG, > > + "failed to read pci > > cap at pos:" > > + " %x ret %d", pos + > > sizeof(cap), > > There is a build error: > > In file included from drivers/net/virtio/virtio_pci.c:15: > drivers/net/virtio/virtio_pci.c: In function ‘vtpci_msix_detect’: > drivers/net/virtio/virtio_logs.h:13:3: error: format ‘%x’ expects > argument of type ‘unsigned int’, but argument 5 has type ‘long > unsigned int’ [-Werror=format=] > "%s(): " fmt "\n", __func__, ##args) > ^~~~~~~~ > drivers/net/virtio/virtio_pci.c:732:5: note: in expansion of macro > ‘PMD_INIT_LOG’ > PMD_INIT_LOG(DEBUG, > ^~~~~~~~~~~~ > drivers/net/virtio/virtio_pci.c:734:14: note: format string is > defined here > " %x ret %d", pos + sizeof(cap), > ~^ > %lx Ok, changed to %lx - which GCC version was that with? Didn't get any warning with 6.3 on Debian 9.
On Mon, Aug 27, 2018 at 05:52:56PM +0100, Luca Boccassi wrote: > On Mon, 2018-08-27 at 13:29 +0800, Tiwei Bie wrote: > > On Fri, Aug 24, 2018 at 06:14:20PM +0100, Luca Boccassi wrote: > > > From: Brian Russell <brussell@brocade.com> > > > > > > In virtio_read_caps and vtpci_msix_detect, rte_pci_read_config > > > returns > > > the number of bytes read from PCI config or < 0 on error. > > > If less than the expected number of bytes are read then log the > > > failure and return rather than carrying on with garbage. > > > > > > Fixes: 6ba1f63b5ab0 ("virtio: support specification 1.0") > > > > > > Signed-off-by: Brian Russell <brussell@brocade.com> > > > Signed-off-by: Luca Boccassi <bluca@debian.org> > > > --- > > > v2: handle additional rte_pci_read_config incomplete reads > > > v3: do not handle rte_pci_read_config of virtio cap, added in v2, > > > as it's less clear what the right thing to do there is > > > v4: do a more robust check - first check what the vendor is, and > > > skip the cap entirely if it's not what we are looking for. > > > > > > drivers/net/virtio/virtio_pci.c | 57 ++++++++++++++++++++++++----- > > > ---- > > > 1 file changed, 42 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/net/virtio/virtio_pci.c > > > b/drivers/net/virtio/virtio_pci.c > > > index 6bd22e54a6..cfefa9789b 100644 > > > --- a/drivers/net/virtio/virtio_pci.c > > > +++ b/drivers/net/virtio/virtio_pci.c > > > @@ -567,16 +567,30 @@ virtio_read_caps(struct rte_pci_device *dev, > > > struct virtio_hw *hw) > > > } > > > > > > ret = rte_pci_read_config(dev, &pos, 1, > > > PCI_CAPABILITY_LIST); > > > - if (ret < 0) { > > > - PMD_INIT_LOG(DEBUG, "failed to read pci capability > > > list"); > > > + if (ret != 1) { > > > + PMD_INIT_LOG(DEBUG, > > > + "failed to read pci capability list, > > > ret %d", ret); > > > return -1; > > > } > > > > > > while (pos) { > > > + ret = rte_pci_read_config(dev, &cap, 2, pos); > > > + if (ret != 2) { > > > + PMD_INIT_LOG(DEBUG, > > > + "failed to read pci cap at > > > pos: %x ret %d", > > > + pos, ret); > > > + break; > > > + } > > > + if (cap.cap_vndr != PCI_CAP_ID_MSIX && > > > + cap.cap_vndr != PCI_CAP_ID_VNDR) { > > > + goto next; > > > + } > > > + > > > ret = rte_pci_read_config(dev, &cap, sizeof(cap), > > > pos); > > > - if (ret < 0) { > > > - PMD_INIT_LOG(ERR, > > > - "failed to read pci cap at pos: > > > %x", pos); > > > + if (ret != sizeof(cap)) { > > > + PMD_INIT_LOG(DEBUG, > > > + "failed to read pci cap at > > > pos: %x ret %d", > > > + pos, ret); > > > break; > > > } > > > > > > > It seems that I didn't make myself clear in my previous > > comments. I mean it's better to handle MSIX cap and virtio > > cap respectively in this function. Currently we're always > > reading them as virtio caps. As we are strictly requiring > > that _read_config() should return the required number of > > bytes, it's not perfect to require it to return "virtio > > cap size" of bytes while we're trying to read a MSIX cap. > > So please change the code to something similar to this: > > Sorry, though you meant in the vtpci_msix_detect function, which I > changed. Fixed in v5. Thanks! > > > > @@ -689,25 +703,38 @@ enum virtio_msix_status > > > vtpci_msix_detect(struct rte_pci_device *dev) > > > { > > > uint8_t pos; > > > - struct virtio_pci_cap cap; > > > int ret; > > > > > > ret = rte_pci_read_config(dev, &pos, 1, > > > PCI_CAPABILITY_LIST); > > > - if (ret < 0) { > > > - PMD_INIT_LOG(DEBUG, "failed to read pci capability > > > list"); > > > + if (ret != 1) { > > > + PMD_INIT_LOG(DEBUG, > > > + "failed to read pci capability list, > > > ret %d", ret); > > > return VIRTIO_MSIX_NONE; > > > } > > > > > > while (pos) { > > > - ret = rte_pci_read_config(dev, &cap, sizeof(cap), > > > pos); > > > - if (ret < 0) { > > > - PMD_INIT_LOG(ERR, > > > - "failed to read pci cap at pos: > > > %x", pos); > > > + uint8_t cap[2]; > > > + > > > + ret = rte_pci_read_config(dev, cap, sizeof(cap), > > > pos); > > > + if (ret != sizeof(cap)) { > > > + PMD_INIT_LOG(DEBUG, > > > + "failed to read pci cap at > > > pos: %x ret %d", > > > + pos, ret); > > > break; > > > } > > > > > > - if (cap.cap_vndr == PCI_CAP_ID_MSIX) { > > > - uint16_t flags = ((uint16_t *)&cap)[1]; > > > + if (cap[0] == PCI_CAP_ID_MSIX) { > > > + uint16_t flags; > > > + > > > + ret = rte_pci_read_config(dev, &flags, > > > sizeof(flags), > > > + pos + sizeof(cap)); > > > + if (ret != sizeof(flags)) { > > > + PMD_INIT_LOG(DEBUG, > > > + "failed to read pci > > > cap at pos:" > > > + " %x ret %d", pos + > > > sizeof(cap), > > > > There is a build error: > > > > In file included from drivers/net/virtio/virtio_pci.c:15: > > drivers/net/virtio/virtio_pci.c: In function ‘vtpci_msix_detect’: > > drivers/net/virtio/virtio_logs.h:13:3: error: format ‘%x’ expects > > argument of type ‘unsigned int’, but argument 5 has type ‘long > > unsigned int’ [-Werror=format=] > > "%s(): " fmt "\n", __func__, ##args) > > ^~~~~~~~ > > drivers/net/virtio/virtio_pci.c:732:5: note: in expansion of macro > > ‘PMD_INIT_LOG’ > > PMD_INIT_LOG(DEBUG, > > ^~~~~~~~~~~~ > > drivers/net/virtio/virtio_pci.c:734:14: note: format string is > > defined here > > " %x ret %d", pos + sizeof(cap), > > ~^ > > %lx > > Ok, changed to %lx - which GCC version was that with? Didn't get any > warning with 6.3 on Debian 9. I saw this with gcc (Debian 8.2.0-4) 8.2.0 on Debian sid. I also saw similar warnings with clang version 6.0.0 (tags/RELEASE_600/final 326565) on FreeBSD 11.2 > > -- > Kind regards, > Luca Boccassi
diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c index 6bd22e54a6..cfefa9789b 100644 --- a/drivers/net/virtio/virtio_pci.c +++ b/drivers/net/virtio/virtio_pci.c @@ -567,16 +567,30 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw) } ret = rte_pci_read_config(dev, &pos, 1, PCI_CAPABILITY_LIST); - if (ret < 0) { - PMD_INIT_LOG(DEBUG, "failed to read pci capability list"); + if (ret != 1) { + PMD_INIT_LOG(DEBUG, + "failed to read pci capability list, ret %d", ret); return -1; } while (pos) { + ret = rte_pci_read_config(dev, &cap, 2, pos); + if (ret != 2) { + PMD_INIT_LOG(DEBUG, + "failed to read pci cap at pos: %x ret %d", + pos, ret); + break; + } + if (cap.cap_vndr != PCI_CAP_ID_MSIX && + cap.cap_vndr != PCI_CAP_ID_VNDR) { + goto next; + } + ret = rte_pci_read_config(dev, &cap, sizeof(cap), pos); - if (ret < 0) { - PMD_INIT_LOG(ERR, - "failed to read pci cap at pos: %x", pos); + if (ret != sizeof(cap)) { + PMD_INIT_LOG(DEBUG, + "failed to read pci cap at pos: %x ret %d", + pos, ret); break; } @@ -689,25 +703,38 @@ enum virtio_msix_status vtpci_msix_detect(struct rte_pci_device *dev) { uint8_t pos; - struct virtio_pci_cap cap; int ret; ret = rte_pci_read_config(dev, &pos, 1, PCI_CAPABILITY_LIST); - if (ret < 0) { - PMD_INIT_LOG(DEBUG, "failed to read pci capability list"); + if (ret != 1) { + PMD_INIT_LOG(DEBUG, + "failed to read pci capability list, ret %d", ret); return VIRTIO_MSIX_NONE; } while (pos) { - ret = rte_pci_read_config(dev, &cap, sizeof(cap), pos); - if (ret < 0) { - PMD_INIT_LOG(ERR, - "failed to read pci cap at pos: %x", pos); + uint8_t cap[2]; + + ret = rte_pci_read_config(dev, cap, sizeof(cap), pos); + if (ret != sizeof(cap)) { + PMD_INIT_LOG(DEBUG, + "failed to read pci cap at pos: %x ret %d", + pos, ret); break; } - if (cap.cap_vndr == PCI_CAP_ID_MSIX) { - uint16_t flags = ((uint16_t *)&cap)[1]; + if (cap[0] == PCI_CAP_ID_MSIX) { + uint16_t flags; + + ret = rte_pci_read_config(dev, &flags, sizeof(flags), + pos + sizeof(cap)); + if (ret != sizeof(flags)) { + PMD_INIT_LOG(DEBUG, + "failed to read pci cap at pos:" + " %x ret %d", pos + sizeof(cap), + ret); + break; + } if (flags & PCI_MSIX_ENABLE) return VIRTIO_MSIX_ENABLED; @@ -715,7 +742,7 @@ vtpci_msix_detect(struct rte_pci_device *dev) return VIRTIO_MSIX_DISABLED; } - pos = cap.cap_next; + pos = cap[1]; } return VIRTIO_MSIX_NONE;