RE: [PATCH v2 00/13] Introduce igb

[Sriram Yagnaraman]

> -----Original Message-----
> From: Sriram Yagnaraman
> Sent: Tuesday, 24 January 2023 09:54
> To: Akihiko Odaki <akihiko.odaki@daynix.com>; Jason Wang
> <jasowang@redhat.com>
> Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>; Michael S. Tsirkin
> <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>;
> Alex Bennée <alex.bennee@linaro.org>; Philippe Mathieu-Daudé
> <philmd@linaro.org>; Thomas Huth <thuth@redhat.com>; Wainer dos Santos
> Moschetta <wainersm@redhat.com>; Beraldo Leal <bleal@redhat.com>;
> Cleber Rosa <crosa@redhat.com>; Laurent Vivier <lvivier@redhat.com>;
> Paolo Bonzini <pbonzini@redhat.com>; Alexander Bulekov <alxndr@bu.edu>;
> Bandan Das <bsd@redhat.com>; Stefan Hajnoczi <stefanha@redhat.com>;
> Darren Kenny <darren.kenny@oracle.com>; Qiuhao Li
> <Qiuhao.Li@outlook.com>; qemu-devel@nongnu.org; qemu-
> ppc@nongnu.org; devel@daynix.com; Yan Vugenfirer
> <yvugenfi@redhat.com>; Yuri Benditovich <yuri.benditovich@daynix.com>
> Subject: RE: [PATCH v2 00/13] Introduce igb
> 
> 
> > -----Original Message-----
> > From: Akihiko Odaki <akihiko.odaki@daynix.com>
> > Sent: Tuesday, 24 January 2023 05:54
> > To: Jason Wang <jasowang@redhat.com>; Sriram Yagnaraman
> > <sriram.yagnaraman@est.tech>
> > Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>; Michael S. Tsirkin
> > <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>;
> Alex
> > Bennée <alex.bennee@linaro.org>; Philippe Mathieu-Daudé
> > <philmd@linaro.org>; Thomas Huth <thuth@redhat.com>; Wainer dos
> Santos
> > Moschetta <wainersm@redhat.com>; Beraldo Leal <bleal@redhat.com>;
> > Cleber Rosa <crosa@redhat.com>; Laurent Vivier <lvivier@redhat.com>;
> > Paolo Bonzini <pbonzini@redhat.com>; Alexander Bulekov
> > <alxndr@bu.edu>; Bandan Das <bsd@redhat.com>; Stefan Hajnoczi
> > <stefanha@redhat.com>; Darren Kenny <darren.kenny@oracle.com>;
> Qiuhao
> > Li <Qiuhao.Li@outlook.com>; qemu-devel@nongnu.org; qemu-
> > ppc@nongnu.org; devel@daynix.com; Yan Vugenfirer
> > <yvugenfi@redhat.com>; Yuri Benditovich <yuri.benditovich@daynix.com>
> > Subject: Re: [PATCH v2 00/13] Introduce igb
> >
> > On 2023/01/16 17:01, Jason Wang wrote:
> > > On Sat, Jan 14, 2023 at 12:10 PM Akihiko Odaki
> > <akihiko.odaki@daynix.com> wrote:
> > >>
> > >> Based-on: <20230114035919.35251-1-akihiko.odaki@daynix.com>
> > >> ([PATCH 00/19] e1000x cleanups (preliminary for IGB))
> > >>
> > >> igb is a family of Intel's gigabit ethernet controllers. This
> > >> series implements
> > >> 82576 emulation in particular. You can see the last patch for the
> > documentation.
> > >>
> > >> Note that there is another effort to bring 82576 emulation. This
> > >> series was developed independently by Sriram Yagnaraman.
> > >> https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg04670.html
> > >>
> > >> It is possible to merge the work from Sriram Yagnaraman and to
> > >> cherry-pick useful changes from this series later.
> > >>
> > >> I think there are several different ways to get the changes into the
> mainline.
> > >> I'm open to any options.
> > >
> > > I can only do reviews for the general networking part but not the
> > > 82576 specific part. It would be better if either of the series can
> > > get some ACKs from some ones that they are familiar with 82576, then
> > > I can try to merge.
> > >
> > > Thanks
> >
> > I have just sent v3 to the list.
> >
> > Sriram Yagnaraman, who wrote another series for 82576, is the only
> > person I know who is familiar with the device.
> >
> > Sriram, can you take a look at v3 I have just sent?
> 
> I am at best a good interpreter of the 82576 datasheet. I will review your
> changes get back here.

I have reviewed and tested your changes and it looks great to me in general.
I would like to note some features that I would like to add on top of your 
patch, if you have not worked on these already :)
- PFRSTD (PF reset done)
- SRRCTL (Rx desc buf size)
- RLPML (oversized packet handling)
- MAC/VLAN anti-spoof checks
- VMOLR_STRVLAN and RPLOLR_STRVLAN (VLAN stripping for VFs)
- VMVIR (VLAN insertion for VFs)
- VF reset
- VFTE, VFRE, VFLRE
- VF stats
- Set EITR initial value

Since this is a new device and there are no existing users, is it possible to 
get the change into baseline first and fix missing features and bugs soon after?

> 
> >
> > Regards,
> > Akihiko Odaki
> >
> > >
> > >>
> > >> V1 -> V2:
> > >> - Spun off e1000e general improvements to a distinct series.
> > >> - Restored vnet_hdr offload as there seems nothing preventing from that.
> > >>
> > >> Akihiko Odaki (13):
> > >>    hw/net/net_tx_pkt: Introduce net_tx_pkt_get_eth_hdr
> > >>    pcie: Introduce pcie_sriov_num_vfs
> > >>    e1000: Split header files
> > >>    igb: Copy e1000e code
> > >>    igb: Rename identifiers
> > >>    igb: Build igb
> > >>    igb: Transform to 82576 implementation
> > >>    tests/qtest/e1000e-test: Fabricate ethernet header
> > >>    tests/qtest/libqos/e1000e: Export macreg functions
> > >>    tests/qtest/libqos/igb: Copy e1000e code
> > >>    tests/qtest/libqos/igb: Transform to igb tests
> > >>    tests/avocado: Add igb test
> > >>    docs/system/devices/igb: Add igb documentation
> > >>
> > >>   MAINTAINERS                                   |    9 +
> > >>   docs/system/device-emulation.rst              |    1 +
> > >>   docs/system/devices/igb.rst                   |   70 +
> > >>   hw/net/Kconfig                                |    5 +
> > >>   hw/net/e1000.c                                |    1 +
> > >>   hw/net/e1000_common.h                         |  102 +
> > >>   hw/net/e1000_regs.h                           |  927 +---
> > >>   hw/net/e1000e.c                               |    3 +-
> > >>   hw/net/e1000e_core.c                          |    1 +
> > >>   hw/net/e1000x_common.c                        |    1 +
> > >>   hw/net/e1000x_common.h                        |   74 -
> > >>   hw/net/e1000x_regs.h                          |  940 ++++
> > >>   hw/net/igb.c                                  |  615 +++
> > >>   hw/net/igb_common.h                           |  144 +
> > >>   hw/net/igb_core.c                             | 3946 +++++++++++++++++
> > >>   hw/net/igb_core.h                             |  147 +
> > >>   hw/net/igb_regs.h                             |  624 +++
> > >>   hw/net/igbvf.c                                |  327 ++
> > >>   hw/net/meson.build                            |    2 +
> > >>   hw/net/net_tx_pkt.c                           |    6 +
> > >>   hw/net/net_tx_pkt.h                           |    8 +
> > >>   hw/net/trace-events                           |   32 +
> > >>   hw/pci/pcie_sriov.c                           |    5 +
> > >>   include/hw/pci/pcie_sriov.h                   |    3 +
> > >>   .../org.centos/stream/8/x86_64/test-avocado   |    1 +
> > >>   tests/avocado/igb.py                          |   38 +
> > >>   tests/qtest/e1000e-test.c                     |   17 +-
> > >>   tests/qtest/fuzz/generic_fuzz_configs.h       |    5 +
> > >>   tests/qtest/igb-test.c                        |  243 +
> > >>   tests/qtest/libqos/e1000e.c                   |   12 -
> > >>   tests/qtest/libqos/e1000e.h                   |   14 +
> > >>   tests/qtest/libqos/igb.c                      |  185 +
> > >>   tests/qtest/libqos/meson.build                |    1 +
> > >>   tests/qtest/meson.build                       |    1 +
> > >>   34 files changed, 7492 insertions(+), 1018 deletions(-)
> > >>   create mode 100644 docs/system/devices/igb.rst
> > >>   create mode 100644 hw/net/e1000_common.h
> > >>   create mode 100644 hw/net/e1000x_regs.h
> > >>   create mode 100644 hw/net/igb.c
> > >>   create mode 100644 hw/net/igb_common.h
> > >>   create mode 100644 hw/net/igb_core.c
> > >>   create mode 100644 hw/net/igb_core.h
> > >>   create mode 100644 hw/net/igb_regs.h
> > >>   create mode 100644 hw/net/igbvf.c
> > >>   create mode 100644 tests/avocado/igb.py
> > >>   create mode 100644 tests/qtest/igb-test.c
> > >>   create mode 100644 tests/qtest/libqos/igb.c
> > >>
> > >> --
> > >> 2.39.0
> > >>
> > >