Re: [Qemu-devel] [PATCH v5 0/4] pcie: Add support for Single Root I/O Virtualization

[Knut Omang <knut.omang@oracle.com>]

On Sun, 2015-10-18 at 14:02 +0300, Marcel Apfelbaum wrote:
> On 10/14/2015 06:51 PM, Knut Omang wrote:
> > This patch set implements generic support for SR/IOV as an
> > extension to the
> > core PCIe functionality, similar to the way other capabilities such
> > as AER
> > is implemented.
> > 
> > There is no implementation of any device that provides
> > SR/IOV support included, but I have implemented a test
> > example which can be found together with this patch set here:
> > 
> >    git://github.com/knuto/qemu.git sriov_patches_v5
> > 
> > Testing with the example device was documented here:
> > 
> >    http://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg05110
> > .html
> > 
> > Changes since v4:
> >    - Mostly based on feeback in Marcel Apfelbaum's review:
> >    - The patch with changes to pci_regs.h got eliminated by rebase
> >    - Added some documentation as an additional patch
> >    - Some trivial fixes moved to separate patch
> >    - Modified code to use error and trace functions instead of
> > printfs
> 
> 
> Hi, thanks for posting another version!
> 
> Regarding the submission:
> Please run ./scripts/checkpatch.pl on your patches, there are some
> minor issues there (ms-dos line end...).

Strange, I only see lines > 80 chars errors except that it does not
like/understand the deliberate lack of parentheses in my SRIOV_ID
macro. I can assure you it has not even been close to any tool that
uses ms-dos line ends ;-)

It has only been subject to emacs and git on Linux..

> This would not prevent the maintainer to take the code, I think
> he has some scripts to take care of it, however it may discover
> some issues.
> 
> Otherwise, I only have some questions on patch 2/4.
> I don't think it justifies a re-submission, I just wanted to point to
> some places
> it may be worth to look again.

I think you found a bug (introduced in v5) there, unfortunately,
looking at it..

Knut

> Thanks,
> Marcel
> 
> > 
> > Changes since v3:
> >    - Reworked 'pci: Update pci_regs header' to merge kernel version
> > improvements
> >      with the current qemu version instead of copying from the
> > kernel version.
> > 
> > Changes since v2:
> >    - Rebased onto 090d0bfd
> >    - Un-qdev'ified - avoids issues when resetting NUM_VFS
> >    - Fixed handling of vf_offset/vf_stride
> > 
> > Changes since v1:
> >    - Rebased on top of latest master, eliminating prereqs.
> >    - Implement proper support for VF_STRIDE, VF_OFFSET and
> > SUP_PGSIZE
> >      Time better spent fixing it than explaining what the previous
> >      limitations were.
> >      - Added new first patch to fix pci bug related to this
> >    - Split out patch to pci_default_config_write to a separate
> > patch 2
> >      to highlight bug fix.
> >    - Refactored out logic into new source files
> >      hw/pci/pcie_sriov.c include/hw/pci/pcie_sriov.h
> >      similar to pcie_aer.c/h.
> >    - Rename functions and introduce structs to better separate
> >      pf and vf functionality.
> >    - Replaced is_vf member with pci_is_vf() function abstraction
> >    - Fix numerous syntax, whitespace and comment issues
> >      according to Michael's review.
> >    - Fix memory leaks.
> >    - Removed igb example device - a rebased version available
> >      on github instead.
> > 
> > Knut Omang (4):
> >    pci: Make use of the devfn property when registering new devices
> >    pcie: Add support for Single Root I/O Virtualization (SR/IOV)
> >    pcie: Add some SR/IOV API documentation in docs/pcie_sriov.txt
> >    pcie: A few minor fixes (type+code simplify)
> > 
> >   docs/pcie_sriov.txt         | 115 +++++++++++++++++++
> >   hw/pci/Makefile.objs        |   2 +-
> >   hw/pci/pci.c                | 104 +++++++++++++-----
> >   hw/pci/pcie.c               |   9 +-
> >   hw/pci/pcie_sriov.c         | 263
> > ++++++++++++++++++++++++++++++++++++++++++++
> >   include/hw/pci/pci.h        |  11 +-
> >   include/hw/pci/pcie.h       |   6 +
> >   include/hw/pci/pcie_sriov.h |  58 ++++++++++
> >   include/qemu/typedefs.h     |   2 +
> >   trace-events                |   5 +
> >   10 files changed, 539 insertions(+), 36 deletions(-)
> >   create mode 100644 docs/pcie_sriov.txt
> >   create mode 100644 hw/pci/pcie_sriov.c
> >   create mode 100644 include/hw/pci/pcie_sriov.h
> > 
> > --
> > 2.4.3
> > 
>