Re: [dts] [PATCH V4] tests/etag: add vf driver vfio-pci

["Tu, Lijuan" <lijuan.tu@intel.com>]

Sorry, I can't list all questions at once. I am not a full-time maintainer, I can't stare at mail all the time. But I am managing to do bi-weekly review,  and trying to do weekly review.
DTS is an open source project, anyone could review your patch and raise his/her concern. Also welcome you to review other's patches. As an submitter, you should convince the reviewers, not all comments from reviewers  are reasonable, you could challenge them.
It's good idea to have a standard, do you have any suggestion for the standard, what it should contain?  Could you provide an draft, or maybe an abstract, I think if we have come items, we could discuss them and get more comments from community.


Here are what we do to review a patch:
1, read your comments, if the comment is not align with your changes, the patch will be rejected.
2, read the difference, and try to understand why you do these changes.
3, more consideration about the third party, your code maybe break other's tests, or can't run at the party platform, such as arm.
4, code styles, pep8 style, we prefer to flexibility with readable, it's more important for a code readable than pep8 style.
5, static analysis, based on my knowledge.
6, typos, and anything else I found.



> -----Original Message-----
> From: Mei, JianweiX
> Sent: Sunday, April 14, 2019 6:47 PM
> To: Tu, Lijuan <lijuan.tu@intel.com>; dts@dpdk.org
> Subject: RE: [dts] [PATCH V4] tests/etag: add vf driver vfio-pci
> 
> Hi, Lijuan
> 
> Could you list all questions at once? I suggest there should provide a
> standard for a modifying  patch or a new patch. Thanks for your patience.
> 
> -----Original Message-----
> From: Tu, Lijuan
> Sent: Saturday, April 13, 2019 12:44 AM
> To: Mei, JianweiX <jianweix.mei@intel.com>; dts@dpdk.org
> Cc: Mei, JianweiX <jianweix.mei@intel.com>
> Subject: RE: [dts] [PATCH V4] tests/etag: add vf driver vfio-pci
> 
> 
> 
> > -----Original Message-----
> > From: dts [mailto:dts-bounces@dpdk.org] On Behalf Of Jianwei Mei
> > Sent: Thursday, April 11, 2019 7:19 PM
> > To: dts@dpdk.org
> > Cc: Mei, JianweiX <jianweix.mei@intel.com>
> > Subject: [dts] [PATCH V4] tests/etag: add vf driver vfio-pci
> >
> > add vf driver vfio-pci
> >
> > Signed-off-by: Jianwei Mei <jianweix.mei@intel.com>
> > ---
> >  tests/TestSuite_etag.py | 45
> > ++++++++++++++++++++++++++++-------------
> >  1 file changed, 31 insertions(+), 14 deletions(-)
> >
> > diff --git a/tests/TestSuite_etag.py b/tests/TestSuite_etag.py index
> > 7cf93eb..fd855de 100644
> > --- a/tests/TestSuite_etag.py
> > +++ b/tests/TestSuite_etag.py
> > @@ -33,27 +33,24 @@
> >  DPDK Test suite.
> >
> >  '''
> > -
> >  import re
> >  import time
> >  import sys
> > -
> >  import utils
> >  from qemu_kvm import QEMUKvm
> >  from test_case import TestCase
> >  from pmd_output import PmdOutput
> >  from exception import VerifyFailure
> > -
> >  from scapy.utils import rdpcap
> > -
> >  from packet import Packet
> >
> >  VM_CORES_MASK = 'all'
> >
> >  class TestEtag(TestCase):
> > +    supported_vf_driver = ['pci-stub', 'vfio-pci']
> >      def set_up_all(self):
> >          self.dut_ports = self.dut.get_ports(self.nic)
> > -        self.verify(self.nic in ['sagepond'], '802.1BR only support by sagepond')
> > +        self.verify(self.nic in ['sagepond','sageville'], '802.1BR
> > + only support by sagepond and sageville')
> >          self.verify(len(self.dut_ports) >= 1, 'Insufficient ports')
> >          self.src_intf = self.tester.get_interface(self.tester.get_local_port(0))
> >          self.src_mac =
> > self.tester.get_mac(self.tester.get_local_port(0))
> > @@ -78,25 +75,45 @@ class TestEtag(TestCase):
> >          self.dut.generate_sriov_vfs_by_port(self.used_dut_port_0, 2,
> > driver=driver)
> >          self.sriov_vfs_port_0 =
> > self.dut.ports_info[self.used_dut_port_0]['vfs_port']
> >
> > +        # set vf assign method and vf driver
> > +        self.vf_driver = self.get_suite_cfg()['vf_driver']
> > +        if self.vf_driver is None:
> > +            self.vf_driver = 'pci-stub'
> > +        self.verify(self.vf_driver in self.supported_vf_driver,
> > + "Unspported vf
> > driver")
> > +        if self.vf_driver == 'pci-stub':
> > +            self.vf_assign_method = 'pci-assign'
> > +        else:
> > +            self.vf_assign_method = 'vfio-pci'
> > +            self.dut.send_expect('modprobe vfio-pci', '#')
> > +
> >          try:
> >              for port in self.sriov_vfs_port_0:
> > -                port.bind_driver('pci-stub')
> > +                port.bind_driver(self.vf_driver)
> >
> >              time.sleep(1)
> >              vf0_prop = {'opt_host': self.sriov_vfs_port_0[0].pci}
> >              vf1_prop = {'opt_host': self.sriov_vfs_port_0[1].pci}
> >
> > -            # start testpmd without the two VFs on the host
> [Lijuan] this interpretation should be kept here.
> > -            self.host_testpmd = PmdOutput(self.dut)
> > -            eal_param = '-b %(vf0)s -b %(vf1)s' % {'vf0':
> > self.sriov_vfs_port_0[0].pci,
> [Lijuan] Same code in if and else, should be put out of conditions
> > +            if driver == 'igb_uio':
> > +                # start testpmd without the two VFs on the host
> > +                self.host_testpmd = PmdOutput(self.dut)
> > +                eal_param = '-b %(vf0)s -b %(vf1)s' % {'vf0':
> > self.sriov_vfs_port_0[0].pci,
> > +                                                       'vf1': self.sriov_vfs_port_0[1].pci}
> > +                if (self.nic in ["niantic", "sageville", "sagepond"]):
> > +                    self.host_testpmd.start_testpmd("1S/2C/2T",
> > + "--txq=4 --rxq=4 ",
> > eal_param=eal_param)
> [Lijuan] Not all platform has 2 thread, so please examine it could get enough
> cores and could meet your requirement.
> > +                else:
> > +                    self.host_testpmd.start_testpmd("1S/2C/2T", "",
> > eal_param=eal_param)
> > +            else:
> > +                # start testpmd without the two VFs on the host
> > +                self.host_testpmd = PmdOutput(self.dut)
> > +                eal_param = '-b %(vf0)s -b %(vf1)s' % {'vf0':
> > + self.sriov_vfs_port_0[0].pci,
> >                                                     'vf1':
> > self.sriov_vfs_port_0[1].pci}
> > -
> > -            self.preset_host_testpmd('1S/2C/2T', eal_param)
> > +                self.preset_host_testpmd('1S/2C/2T', eal_param)
> >
> >              # set up VM0 ENV
> >              self.vm0 = QEMUKvm(self.dut, 'vm0', 'vf_etag')
> > -            self.vm0.set_vm_device(driver='pci-assign', **vf0_prop)
> > -            self.vm0.set_vm_device(driver='pci-assign', **vf1_prop)
> > +            self.vm0.set_vm_device(driver=self.vf_assign_method, **vf0_prop)
> > +            self.vm0.set_vm_device(driver=self.vf_assign_method,
> > + **vf1_prop)
> >              self.vm_dut_0 = self.vm0.start()
> >              if self.vm_dut_0 is None:
> >                  raise Exception('Set up VM0 ENV failed!') @@ -290,7
> > +307,7 @@ class TestEtag(TestCase):
> >              else:
> >                  # Same E-tag forwarding to VF0, Send 802.1BR packet
> > with broadcast mac and
> >                  # check packet only received on VF0 or VF1
> > -                host_cmds = [['E-tag set filter add e-tag-id 1000 dst-pool %d port
> > 0'%test_type[-1:]],
> > +                host_cmds = [['E-tag set filter add e-tag-id 1000
> > + dst-pool %d port 0'% int(test_type[-1:])],
> >                               ['set fwd rxonly'],
> >                               ['set verbose 1'],
> >                               ['start']]
> > --
> > 2.17.2