| Message ID | 20180816184750.30843-2-bluca@debian.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Thomas Monjalon |
| Headers | show |
| Series | [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 Thu, 2018-08-16 at 19:47 +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 > > drivers/net/virtio/virtio_pci.c | 35 +++++++++++++++++++++-------- > ---- > 1 file changed, 22 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/virtio/virtio_pci.c > b/drivers/net/virtio/virtio_pci.c > index 6bd22e54a6..ff6f96f361 100644 > --- a/drivers/net/virtio/virtio_pci.c > +++ b/drivers/net/virtio/virtio_pci.c ... > @@ -610,9 +613,13 @@ virtio_read_caps(struct rte_pci_device *dev, > struct virtio_hw *hw) > hw->common_cfg = get_cfg_addr(dev, &cap); > break; > case VIRTIO_PCI_CAP_NOTIFY_CFG: > - rte_pci_read_config(dev, &hw- > >notify_off_multiplier, > - 4, pos + sizeof(cap)); > - hw->notify_base = get_cfg_addr(dev, &cap); > + /* Avoid half-reads into hw */ > + ret = rte_pci_read_config(dev, &multiplier, > 4, > + pos + sizeof(cap)); > + if (ret == 4) { > + hw->notify_off_multiplier = > multiplier; > + hw->notify_base = get_cfg_addr(dev, > &cap); > + } > break; > case VIRTIO_PCI_CAP_DEVICE_CFG: > hw->dev_cfg = get_cfg_addr(dev, &cap); Tiwei: not 100% sure what's the best way to handle a failure here, this will avoid writing to *hw in case of errors. Let me know if this is OK.
On Thu, Aug 16, 2018 at 07:49:43PM +0100, Luca Boccassi wrote: > On Thu, 2018-08-16 at 19:47 +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 > > > > drivers/net/virtio/virtio_pci.c | 35 +++++++++++++++++++++-------- > > ---- > > 1 file changed, 22 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/net/virtio/virtio_pci.c > > b/drivers/net/virtio/virtio_pci.c > > index 6bd22e54a6..ff6f96f361 100644 > > --- a/drivers/net/virtio/virtio_pci.c > > +++ b/drivers/net/virtio/virtio_pci.c > ... > > @@ -610,9 +613,13 @@ virtio_read_caps(struct rte_pci_device *dev, > > struct virtio_hw *hw) > > hw->common_cfg = get_cfg_addr(dev, &cap); > > break; > > case VIRTIO_PCI_CAP_NOTIFY_CFG: > > - rte_pci_read_config(dev, &hw- > > >notify_off_multiplier, > > - 4, pos + sizeof(cap)); > > - hw->notify_base = get_cfg_addr(dev, &cap); > > + /* Avoid half-reads into hw */ > > + ret = rte_pci_read_config(dev, &multiplier, > > 4, > > + pos + sizeof(cap)); > > + if (ret == 4) { > > + hw->notify_off_multiplier = > > multiplier; > > + hw->notify_base = get_cfg_addr(dev, > > &cap); > > + } > > break; > > case VIRTIO_PCI_CAP_DEVICE_CFG: > > hw->dev_cfg = get_cfg_addr(dev, &cap); > > Tiwei: not 100% sure what's the best way to handle a failure here, this > will avoid writing to *hw in case of errors. Let me know if this is OK. My concern is about reading the virtio capability directly. With this patch, it will give up reading other capabilities when failed to read a full virtio PCI capability structure (the returned bytes are less than the expected bytes) even though when the capability it's reading isn't a virtio vendor specific capability. I'm not quite sure whether it will bring any bad consequence. How about just reading the first two bytes first? Something like this: https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/pci/pci.c#L1491-L1497 Thanks
On Mon, 2018-08-20 at 16:18 +0800, Tiwei Bie wrote: > On Thu, Aug 16, 2018 at 07:49:43PM +0100, Luca Boccassi wrote: > > On Thu, 2018-08-16 at 19:47 +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 > > > > > > drivers/net/virtio/virtio_pci.c | 35 +++++++++++++++++++++---- > > > ---- > > > ---- > > > 1 file changed, 22 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/net/virtio/virtio_pci.c > > > b/drivers/net/virtio/virtio_pci.c > > > index 6bd22e54a6..ff6f96f361 100644 > > > --- a/drivers/net/virtio/virtio_pci.c > > > +++ b/drivers/net/virtio/virtio_pci.c > > > > ... > > > @@ -610,9 +613,13 @@ virtio_read_caps(struct rte_pci_device *dev, > > > struct virtio_hw *hw) > > > hw->common_cfg = get_cfg_addr(dev, > > > &cap); > > > break; > > > case VIRTIO_PCI_CAP_NOTIFY_CFG: > > > - rte_pci_read_config(dev, &hw- > > > > notify_off_multiplier, > > > > > > - 4, pos + sizeof(cap)); > > > - hw->notify_base = get_cfg_addr(dev, > > > &cap); > > > + /* Avoid half-reads into hw */ > > > + ret = rte_pci_read_config(dev, > > > &multiplier, > > > 4, > > > + pos + sizeof(cap)); > > > + if (ret == 4) { > > > + hw->notify_off_multiplier = > > > multiplier; > > > + hw->notify_base = > > > get_cfg_addr(dev, > > > &cap); > > > + } > > > break; > > > case VIRTIO_PCI_CAP_DEVICE_CFG: > > > hw->dev_cfg = get_cfg_addr(dev, &cap); > > > > Tiwei: not 100% sure what's the best way to handle a failure here, > > this > > will avoid writing to *hw in case of errors. Let me know if this is > > OK. > > My concern is about reading the virtio capability directly. > With this patch, it will give up reading other capabilities > when failed to read a full virtio PCI capability structure > (the returned bytes are less than the expected bytes) even > though when the capability it's reading isn't a virtio vendor > specific capability. I'm not quite sure whether it will bring > any bad consequence. How about just reading the first two > bytes first? Something like this: > > https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/pci/pci. > c#L1491-L1497 I'm not sure, to be honest. That bit didn't give me trouble at all, so at this point I'd much rather leave it alone so that the maintainers can take care of it how they see fit, if necessary :-) I've sent a v3 that removes that individual change.
On Mon, Aug 20, 2018 at 05:45:35PM +0100, Luca Boccassi wrote: > On Mon, 2018-08-20 at 16:18 +0800, Tiwei Bie wrote: > > On Thu, Aug 16, 2018 at 07:49:43PM +0100, Luca Boccassi wrote: > > > On Thu, 2018-08-16 at 19:47 +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 > > > > > > > > drivers/net/virtio/virtio_pci.c | 35 +++++++++++++++++++++---- > > > > ---- > > > > ---- > > > > 1 file changed, 22 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/drivers/net/virtio/virtio_pci.c > > > > b/drivers/net/virtio/virtio_pci.c > > > > index 6bd22e54a6..ff6f96f361 100644 > > > > --- a/drivers/net/virtio/virtio_pci.c > > > > +++ b/drivers/net/virtio/virtio_pci.c > > > > > > ... > > > > @@ -610,9 +613,13 @@ virtio_read_caps(struct rte_pci_device *dev, > > > > struct virtio_hw *hw) > > > > hw->common_cfg = get_cfg_addr(dev, > > > > &cap); > > > > break; > > > > case VIRTIO_PCI_CAP_NOTIFY_CFG: > > > > - rte_pci_read_config(dev, &hw- > > > > > notify_off_multiplier, > > > > > > > > - 4, pos + sizeof(cap)); > > > > - hw->notify_base = get_cfg_addr(dev, > > > > &cap); > > > > + /* Avoid half-reads into hw */ > > > > + ret = rte_pci_read_config(dev, > > > > &multiplier, > > > > 4, > > > > + pos + sizeof(cap)); > > > > + if (ret == 4) { > > > > + hw->notify_off_multiplier = > > > > multiplier; > > > > + hw->notify_base = > > > > get_cfg_addr(dev, > > > > &cap); > > > > + } > > > > break; > > > > case VIRTIO_PCI_CAP_DEVICE_CFG: > > > > hw->dev_cfg = get_cfg_addr(dev, &cap); > > > > > > Tiwei: not 100% sure what's the best way to handle a failure here, > > > this > > > will avoid writing to *hw in case of errors. Let me know if this is > > > OK. > > > > My concern is about reading the virtio capability directly. > > With this patch, it will give up reading other capabilities > > when failed to read a full virtio PCI capability structure > > (the returned bytes are less than the expected bytes) even > > though when the capability it's reading isn't a virtio vendor > > specific capability. I'm not quite sure whether it will bring > > any bad consequence. How about just reading the first two > > bytes first? Something like this: > > > > https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/pci/pci. > > c#L1491-L1497 > > I'm not sure, to be honest. That bit didn't give me trouble at all, so > at this point I'd much rather leave it alone so that the maintainers > can take care of it how they see fit, if necessary :-) > > I've sent a v3 that removes that individual change. My concern isn't about the above change (which handled the errors when reading multiplier). In fact, above change looks good to me! :-) I mean the below changes in this patch: 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); + if (ret != sizeof(cap)) { + PMD_INIT_LOG(DEBUG, + "failed to read pci cap at pos: %x ret %d", + pos, ret); break; } With this change, it will give up reading other capabilities when failed to read a full virtio PCI capability structure (the returned bytes are less than the expected bytes) even though when the capability it's reading isn't a virtio vendor specific capability. Maybe it would be better to read the first two bytes first and check whether it's the capability we want to parse (i.e. vendor capability and MSIX capability). Something like this: https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/pci/pci.c#L1491-L1497 How do you think? Thanks
On Tue, 2018-08-21 at 10:40 +0800, Tiwei Bie wrote: > On Mon, Aug 20, 2018 at 05:45:35PM +0100, Luca Boccassi wrote: > > On Mon, 2018-08-20 at 16:18 +0800, Tiwei Bie wrote: > > > On Thu, Aug 16, 2018 at 07:49:43PM +0100, Luca Boccassi wrote: > > > > On Thu, 2018-08-16 at 19:47 +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 > > > > > > > > > > drivers/net/virtio/virtio_pci.c | 35 +++++++++++++++++++++ > > > > > ---- > > > > > ---- > > > > > ---- > > > > > 1 file changed, 22 insertions(+), 13 deletions(-) > > > > > > > > > > diff --git a/drivers/net/virtio/virtio_pci.c > > > > > b/drivers/net/virtio/virtio_pci.c > > > > > index 6bd22e54a6..ff6f96f361 100644 > > > > > --- a/drivers/net/virtio/virtio_pci.c > > > > > +++ b/drivers/net/virtio/virtio_pci.c > > > > > > > > ... > > > > > @@ -610,9 +613,13 @@ virtio_read_caps(struct rte_pci_device > > > > > *dev, > > > > > struct virtio_hw *hw) > > > > > hw->common_cfg = get_cfg_addr(dev, > > > > > &cap); > > > > > break; > > > > > case VIRTIO_PCI_CAP_NOTIFY_CFG: > > > > > - rte_pci_read_config(dev, &hw- > > > > > > notify_off_multiplier, > > > > > > > > > > - 4, pos + > > > > > sizeof(cap)); > > > > > - hw->notify_base = get_cfg_addr(dev, > > > > > &cap); > > > > > + /* Avoid half-reads into hw */ > > > > > + ret = rte_pci_read_config(dev, > > > > > &multiplier, > > > > > 4, > > > > > + pos + sizeof(cap)); > > > > > + if (ret == 4) { > > > > > + hw->notify_off_multiplier = > > > > > multiplier; > > > > > + hw->notify_base = > > > > > get_cfg_addr(dev, > > > > > &cap); > > > > > + } > > > > > break; > > > > > case VIRTIO_PCI_CAP_DEVICE_CFG: > > > > > hw->dev_cfg = get_cfg_addr(dev, > > > > > &cap); > > > > > > > > Tiwei: not 100% sure what's the best way to handle a failure > > > > here, > > > > this > > > > will avoid writing to *hw in case of errors. Let me know if > > > > this is > > > > OK. > > > > > > My concern is about reading the virtio capability directly. > > > With this patch, it will give up reading other capabilities > > > when failed to read a full virtio PCI capability structure > > > (the returned bytes are less than the expected bytes) even > > > though when the capability it's reading isn't a virtio vendor > > > specific capability. I'm not quite sure whether it will bring > > > any bad consequence. How about just reading the first two > > > bytes first? Something like this: > > > > > > https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/pci/ > > > pci. > > > c#L1491-L1497 > > > > I'm not sure, to be honest. That bit didn't give me trouble at all, > > so > > at this point I'd much rather leave it alone so that the > > maintainers > > can take care of it how they see fit, if necessary :-) > > > > I've sent a v3 that removes that individual change. > > My concern isn't about the above change (which handled the > errors when reading multiplier). In fact, above change looks > good to me! :-) I mean the below changes in this patch: > > 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); > + if (ret != sizeof(cap)) { > + PMD_INIT_LOG(DEBUG, > + "failed to read pci cap at pos: > %x ret %d", > + pos, ret); > break; > } > > With this change, it will give up reading other capabilities > when failed to read a full virtio PCI capability structure > (the returned bytes are less than the expected bytes) even > though when the capability it's reading isn't a virtio vendor > specific capability. Maybe it would be better to read the > first two bytes first and check whether it's the capability > we want to parse (i.e. vendor capability and MSIX capability). > Something like this: > > https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/pci/pci. > c#L1491-L1497 > > How do you think? > > Thanks Ah I see what you mean now, sorry. Would something like the following be what you are looking for: 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 != sizeof(cap)) { PMD_INIT_LOG(DEBUG, "failed to read pci cap at pos: %x ret %d", pos, ret); break; }
On Thu, Aug 23, 2018 at 01:52:25PM +0100, Luca Boccassi wrote: > On Tue, 2018-08-21 at 10:40 +0800, Tiwei Bie wrote: > > On Mon, Aug 20, 2018 at 05:45:35PM +0100, Luca Boccassi wrote: > > > On Mon, 2018-08-20 at 16:18 +0800, Tiwei Bie wrote: > > > > On Thu, Aug 16, 2018 at 07:49:43PM +0100, Luca Boccassi wrote: > > > > > On Thu, 2018-08-16 at 19:47 +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 > > > > > > > > > > > > drivers/net/virtio/virtio_pci.c | 35 +++++++++++++++++++++ > > > > > > ---- > > > > > > ---- > > > > > > ---- > > > > > > 1 file changed, 22 insertions(+), 13 deletions(-) > > > > > > > > > > > > diff --git a/drivers/net/virtio/virtio_pci.c > > > > > > b/drivers/net/virtio/virtio_pci.c > > > > > > index 6bd22e54a6..ff6f96f361 100644 > > > > > > --- a/drivers/net/virtio/virtio_pci.c > > > > > > +++ b/drivers/net/virtio/virtio_pci.c > > > > > > > > > > ... > > > > > > @@ -610,9 +613,13 @@ virtio_read_caps(struct rte_pci_device > > > > > > *dev, > > > > > > struct virtio_hw *hw) > > > > > > hw->common_cfg = get_cfg_addr(dev, > > > > > > &cap); > > > > > > break; > > > > > > case VIRTIO_PCI_CAP_NOTIFY_CFG: > > > > > > - rte_pci_read_config(dev, &hw- > > > > > > > notify_off_multiplier, > > > > > > > > > > > > - 4, pos + > > > > > > sizeof(cap)); > > > > > > - hw->notify_base = get_cfg_addr(dev, > > > > > > &cap); > > > > > > + /* Avoid half-reads into hw */ > > > > > > + ret = rte_pci_read_config(dev, > > > > > > &multiplier, > > > > > > 4, > > > > > > + pos + sizeof(cap)); > > > > > > + if (ret == 4) { > > > > > > + hw->notify_off_multiplier = > > > > > > multiplier; > > > > > > + hw->notify_base = > > > > > > get_cfg_addr(dev, > > > > > > &cap); > > > > > > + } > > > > > > break; > > > > > > case VIRTIO_PCI_CAP_DEVICE_CFG: > > > > > > hw->dev_cfg = get_cfg_addr(dev, > > > > > > &cap); > > > > > > > > > > Tiwei: not 100% sure what's the best way to handle a failure > > > > > here, > > > > > this > > > > > will avoid writing to *hw in case of errors. Let me know if > > > > > this is > > > > > OK. > > > > > > > > My concern is about reading the virtio capability directly. > > > > With this patch, it will give up reading other capabilities > > > > when failed to read a full virtio PCI capability structure > > > > (the returned bytes are less than the expected bytes) even > > > > though when the capability it's reading isn't a virtio vendor > > > > specific capability. I'm not quite sure whether it will bring > > > > any bad consequence. How about just reading the first two > > > > bytes first? Something like this: > > > > > > > > https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/pci/ > > > > pci. > > > > c#L1491-L1497 > > > > > > I'm not sure, to be honest. That bit didn't give me trouble at all, > > > so > > > at this point I'd much rather leave it alone so that the > > > maintainers > > > can take care of it how they see fit, if necessary :-) > > > > > > I've sent a v3 that removes that individual change. > > > > My concern isn't about the above change (which handled the > > errors when reading multiplier). In fact, above change looks > > good to me! :-) I mean the below changes in this patch: > > > > 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); > > + if (ret != sizeof(cap)) { > > + PMD_INIT_LOG(DEBUG, > > + "failed to read pci cap at pos: > > %x ret %d", > > + pos, ret); > > break; > > } > > > > With this change, it will give up reading other capabilities > > when failed to read a full virtio PCI capability structure > > (the returned bytes are less than the expected bytes) even > > though when the capability it's reading isn't a virtio vendor > > specific capability. Maybe it would be better to read the > > first two bytes first and check whether it's the capability > > we want to parse (i.e. vendor capability and MSIX capability). > > Something like this: > > > > https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/pci/pci. > > c#L1491-L1497 > > > > How do you think? > > > > Thanks > > Ah I see what you mean now, sorry. Would something like the following > be what you are looking for: > > 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 != sizeof(cap)) { > PMD_INIT_LOG(DEBUG, > "failed to read pci cap at pos: %x ret %d", > pos, ret); > break; > } > Yeah, that's what I want. :-) Just one nit, we don't need to read it as a virtio cap when dealing with the MSIX cap. When dealing with the MSIX cap, we just need to read the next two bytes: https://github.com/DPDK/dpdk/blob/7281cf520f89/drivers/net/virtio/virtio_pci.c#L584-L594 https://github.com/freebsd/freebsd/blob/59e4185a9358/sys/dev/pci/pci.c#L892 Thanks! > -- > Kind regards, > Luca Boccassi
On Fri, 2018-08-24 at 12:23 +0800, Tiwei Bie wrote: > On Thu, Aug 23, 2018 at 01:52:25PM +0100, Luca Boccassi wrote: > > On Tue, 2018-08-21 at 10:40 +0800, Tiwei Bie wrote: > > > On Mon, Aug 20, 2018 at 05:45:35PM +0100, Luca Boccassi wrote: > > > > On Mon, 2018-08-20 at 16:18 +0800, Tiwei Bie wrote: > > > > > On Thu, Aug 16, 2018 at 07:49:43PM +0100, Luca Boccassi > > > > > wrote: > > > > > > On Thu, 2018-08-16 at 19:47 +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 > > > > > > > > > > > > > > drivers/net/virtio/virtio_pci.c | 35 > > > > > > > +++++++++++++++++++++ > > > > > > > ---- > > > > > > > ---- > > > > > > > ---- > > > > > > > 1 file changed, 22 insertions(+), 13 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/net/virtio/virtio_pci.c > > > > > > > b/drivers/net/virtio/virtio_pci.c > > > > > > > index 6bd22e54a6..ff6f96f361 100644 > > > > > > > --- a/drivers/net/virtio/virtio_pci.c > > > > > > > +++ b/drivers/net/virtio/virtio_pci.c > > > > > > > > > > > > ... > > > > > > > @@ -610,9 +613,13 @@ virtio_read_caps(struct > > > > > > > rte_pci_device > > > > > > > *dev, > > > > > > > struct virtio_hw *hw) > > > > > > > hw->common_cfg = > > > > > > > get_cfg_addr(dev, > > > > > > > &cap); > > > > > > > break; > > > > > > > case VIRTIO_PCI_CAP_NOTIFY_CFG: > > > > > > > - rte_pci_read_config(dev, &hw- > > > > > > > > notify_off_multiplier, > > > > > > > > > > > > > > - 4, pos + > > > > > > > sizeof(cap)); > > > > > > > - hw->notify_base = > > > > > > > get_cfg_addr(dev, > > > > > > > &cap); > > > > > > > + /* Avoid half-reads into hw */ > > > > > > > + ret = rte_pci_read_config(dev, > > > > > > > &multiplier, > > > > > > > 4, > > > > > > > + pos + > > > > > > > sizeof(cap)); > > > > > > > + if (ret == 4) { > > > > > > > + hw- > > > > > > > >notify_off_multiplier = > > > > > > > multiplier; > > > > > > > + hw->notify_base = > > > > > > > get_cfg_addr(dev, > > > > > > > &cap); > > > > > > > + } > > > > > > > break; > > > > > > > case VIRTIO_PCI_CAP_DEVICE_CFG: > > > > > > > hw->dev_cfg = get_cfg_addr(dev, > > > > > > > &cap); > > > > > > > > > > > > Tiwei: not 100% sure what's the best way to handle a > > > > > > failure > > > > > > here, > > > > > > this > > > > > > will avoid writing to *hw in case of errors. Let me know if > > > > > > this is > > > > > > OK. > > > > > > > > > > My concern is about reading the virtio capability directly. > > > > > With this patch, it will give up reading other capabilities > > > > > when failed to read a full virtio PCI capability structure > > > > > (the returned bytes are less than the expected bytes) even > > > > > though when the capability it's reading isn't a virtio vendor > > > > > specific capability. I'm not quite sure whether it will bring > > > > > any bad consequence. How about just reading the first two > > > > > bytes first? Something like this: > > > > > > > > > > https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/ > > > > > pci/ > > > > > pci. > > > > > c#L1491-L1497 > > > > > > > > I'm not sure, to be honest. That bit didn't give me trouble at > > > > all, > > > > so > > > > at this point I'd much rather leave it alone so that the > > > > maintainers > > > > can take care of it how they see fit, if necessary :-) > > > > > > > > I've sent a v3 that removes that individual change. > > > > > > My concern isn't about the above change (which handled the > > > errors when reading multiplier). In fact, above change looks > > > good to me! :-) I mean the below changes in this patch: > > > > > > 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); > > > + if (ret != sizeof(cap)) { > > > + PMD_INIT_LOG(DEBUG, > > > + "failed to read pci cap at > > > pos: > > > %x ret %d", > > > + pos, ret); > > > break; > > > } > > > > > > With this change, it will give up reading other capabilities > > > when failed to read a full virtio PCI capability structure > > > (the returned bytes are less than the expected bytes) even > > > though when the capability it's reading isn't a virtio vendor > > > specific capability. Maybe it would be better to read the > > > first two bytes first and check whether it's the capability > > > we want to parse (i.e. vendor capability and MSIX capability). > > > Something like this: > > > > > > https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/pci/ > > > pci. > > > c#L1491-L1497 > > > > > > How do you think? > > > > > > Thanks > > > > Ah I see what you mean now, sorry. Would something like the > > following > > be what you are looking for: > > > > 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 != sizeof(cap)) { > > PMD_INIT_LOG(DEBUG, > > "failed to read pci cap at pos: %x > > ret %d", > > pos, ret); > > break; > > } > > > > Yeah, that's what I want. :-) > Just one nit, we don't need to read it as a > virtio cap when dealing with the MSIX cap. > When dealing with the MSIX cap, we just need > to read the next two bytes: > > https://github.com/DPDK/dpdk/blob/7281cf520f89/drivers/net/virtio/vir > tio_pci.c#L584-L594 > https://github.com/freebsd/freebsd/blob/59e4185a9358/sys/dev/pci/pci. > c#L892 > > Thanks! Ok, done, see v4.
diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c index 6bd22e54a6..ff6f96f361 100644 --- a/drivers/net/virtio/virtio_pci.c +++ b/drivers/net/virtio/virtio_pci.c @@ -560,6 +560,7 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw) uint8_t pos; struct virtio_pci_cap cap; int ret; + uint32_t multiplier; if (rte_pci_map_device(dev)) { PMD_INIT_LOG(DEBUG, "failed to map pci device!"); @@ -567,16 +568,18 @@ 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, 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; } @@ -610,9 +613,13 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw) hw->common_cfg = get_cfg_addr(dev, &cap); break; case VIRTIO_PCI_CAP_NOTIFY_CFG: - rte_pci_read_config(dev, &hw->notify_off_multiplier, - 4, pos + sizeof(cap)); - hw->notify_base = get_cfg_addr(dev, &cap); + /* Avoid half-reads into hw */ + ret = rte_pci_read_config(dev, &multiplier, 4, + pos + sizeof(cap)); + if (ret == 4) { + hw->notify_off_multiplier = multiplier; + hw->notify_base = get_cfg_addr(dev, &cap); + } break; case VIRTIO_PCI_CAP_DEVICE_CFG: hw->dev_cfg = get_cfg_addr(dev, &cap); @@ -693,16 +700,18 @@ vtpci_msix_detect(struct rte_pci_device *dev) 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); + if (ret != sizeof(cap)) { + PMD_INIT_LOG(DEBUG, + "failed to read pci cap at pos: %x ret %d", + pos, ret); break; }