[dpdk-dev,v5,09/11] virtio_pci: do not parse if interface is vfio-noiommu

Message ID 1453203972-24855-10-git-send-email-sshukla@mvista.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Commit Message

Santosh Shukla Jan. 19, 2016, 11:46 a.m. UTC
  If virtio interface attached to vfio-noiommu driver then
do not parse for virtio resource. Instead exit with return 0;

Note: Applicable for virtio spec 0.95.

Signed-off-by: Santosh Shukla <sshukla@mvista.com>
---
v4-->v5:
- added _NOIOMMU drv check for lagecy virtio. No need for resource_init in vfio
  case. And resource_pasrsing/interface validation done by pci_eal module for
  pmd driver.
 
 drivers/net/virtio/virtio_pci.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
  

Comments

Yuanhan Liu Jan. 29, 2016, 7:17 a.m. UTC | #1
Two minor nits.

Firstly about your title, you should be consistent: sometimes you
use virtio_pci, and sometimes you use virtio_pic.h. And for virtio
pmd driver, "virtio: " prefix is pretty enough, no need another
extra "vfio: " or "pci: " prefix.

And the same to your EAL changes. EAL is a bigger, having more
components, thus sometimes 2 prefixs are used. And if you are
not sure how to add prefix, dig the git history to get the answer.

On Tue, Jan 19, 2016 at 05:16:10PM +0530, Santosh Shukla wrote:
> If virtio interface attached to vfio-noiommu driver then
> do not parse for virtio resource. Instead exit with return 0;
> 
> Note: Applicable for virtio spec 0.95.

And this is not necessary: io port stuff is for virtio 0.95 only.
virtio 1.0 won't use that, at all.

	--yliu
  
Santosh Shukla Jan. 29, 2016, 7:22 a.m. UTC | #2
On Fri, Jan 29, 2016 at 12:47 PM, Yuanhan Liu
<yuanhan.liu@linux.intel.com> wrote:
> Two minor nits.
>
> Firstly about your title, you should be consistent: sometimes you
> use virtio_pci, and sometimes you use virtio_pic.h. And for virtio
> pmd driver, "virtio: " prefix is pretty enough, no need another
> extra "vfio: " or "pci: " prefix.
>
> And the same to your EAL changes. EAL is a bigger, having more
> components, thus sometimes 2 prefixs are used. And if you are
> not sure how to add prefix, dig the git history to get the answer.
>
> On Tue, Jan 19, 2016 at 05:16:10PM +0530, Santosh Shukla wrote:
>> If virtio interface attached to vfio-noiommu driver then
>> do not parse for virtio resource. Instead exit with return 0;
>>
>> Note: Applicable for virtio spec 0.95.
>
> And this is not necessary: io port stuff is for virtio 0.95 only.
> virtio 1.0 won't use that, at all.
>

Okay,

I removed [08/11] patch from v5 series and modified this patch
accordingly [1]. So, ignore this patch and pl. review provided link.

[1] http://dpdk.org/dev/patchwork/patch/10143/

>         --yliu
  
Yuanhan Liu Jan. 29, 2016, 7:34 a.m. UTC | #3
On Fri, Jan 29, 2016 at 12:52:53PM +0530, Santosh Shukla wrote:
> On Fri, Jan 29, 2016 at 12:47 PM, Yuanhan Liu
> <yuanhan.liu@linux.intel.com> wrote:
> > Two minor nits.
> >
> > Firstly about your title, you should be consistent: sometimes you
> > use virtio_pci, and sometimes you use virtio_pic.h. And for virtio
> > pmd driver, "virtio: " prefix is pretty enough, no need another
> > extra "vfio: " or "pci: " prefix.
> >
> > And the same to your EAL changes. EAL is a bigger, having more
> > components, thus sometimes 2 prefixs are used. And if you are
> > not sure how to add prefix, dig the git history to get the answer.
> >
> > On Tue, Jan 19, 2016 at 05:16:10PM +0530, Santosh Shukla wrote:
> >> If virtio interface attached to vfio-noiommu driver then
> >> do not parse for virtio resource. Instead exit with return 0;
> >>
> >> Note: Applicable for virtio spec 0.95.
> >
> > And this is not necessary: io port stuff is for virtio 0.95 only.
> > virtio 1.0 won't use that, at all.
> >
> 
> Okay,
> 
> I removed [08/11] patch from v5 series and modified this patch
> accordingly [1]. So, ignore this patch and pl. review provided link.
> 
> [1] http://dpdk.org/dev/patchwork/patch/10143/

This patch actually looks good to me, despites the two minor nits.

Another note is that while sending just one single update patch,
it'd be better if you could link it to the old patch, to make it
in the same email thread, otherwise, it's difficult for me to
notice that single-alone patch: it's easily get lost.

There is another option for that: the git scissors option; you could
check the git format-patch man page for more detailed info (by searching
"scissors" keyword). I'm just not quite sure Thomas like it or not.

	--yliu
  
Thomas Monjalon Jan. 29, 2016, 9:02 a.m. UTC | #4
2016-01-29 15:34, Yuanhan Liu:
> There is another option for that: the git scissors option; you could
> check the git format-patch man page for more detailed info (by searching
> "scissors" keyword). I'm just not quite sure Thomas like it or not.

For simple discussions, patch after scissors may be a good option.

In this series, there are too many patches sent alone and it would
be good to send the whole series  to make things flatter (while keeping
the --in-reply-to of course).
  
Yuanhan Liu Jan. 29, 2016, 9:14 a.m. UTC | #5
On Fri, Jan 29, 2016 at 10:02:26AM +0100, Thomas Monjalon wrote:
> 2016-01-29 15:34, Yuanhan Liu:
> > There is another option for that: the git scissors option; you could
> > check the git format-patch man page for more detailed info (by searching
> > "scissors" keyword). I'm just not quite sure Thomas like it or not.
> 
> For simple discussions, patch after scissors may be a good option.
> 
> In this series, there are too many patches sent alone and it would
> be good to send the whole series  to make things flatter (while keeping
> the --in-reply-to of course).

Agreed.

	--yliu
  
Santosh Shukla Jan. 29, 2016, 9:16 a.m. UTC | #6
On Fri, Jan 29, 2016 at 2:32 PM, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
> 2016-01-29 15:34, Yuanhan Liu:
>> There is another option for that: the git scissors option; you could
>> check the git format-patch man page for more detailed info (by searching
>> "scissors" keyword). I'm just not quite sure Thomas like it or not.
>

I agree and my mistake.. sorry for confusion,

> For simple discussions, patch after scissors may be a good option.
>
> In this series, there are too many patches sent alone and it would
> be good to send the whole series  to make things flatter (while keeping
> the --in-reply-to of course).

Sending whole series taging patch version v6 version shortly, Thanks.
  

Patch

diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index 537c552..520e540 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -514,7 +514,9 @@  virtio_resource_init_by_ioports(struct rte_pci_device *pci_dev)
 static int
 legacy_virtio_resource_init(struct rte_pci_device *pci_dev)
 {
-	if (virtio_resource_init_by_uio(pci_dev) == 0)
+	if (pci_dev->kdrv == RTE_KDRV_VFIO_NOIOMMU)
+		return 0;
+	else if (virtio_resource_init_by_uio(pci_dev) == 0)
 		return 0;
 	else
 		return virtio_resource_init_by_ioports(pci_dev);