mbox series

[v8,0/2] support for VFIO-PCI VF token interface

Message ID 20200418173035.8000-1-haiyue.wang@intel.com (mailing list archive)
Headers
Series support for VFIO-PCI VF token interface |

Message

Wang, Haiyue April 18, 2020, 5:30 p.m. UTC
  v8: Update the document.

v7: Add the Fixes tag in uuid, the release note and help
    document.
    https://patchwork.dpdk.org/cover/68845/

v6: Drop the Fixes tag in uuid, since the file has been
    moved to another place, not suitable to apply on stable.
    And this is not a bug, just some kind of enhancement.
    https://patchwork.dpdk.org/cover/68367/

v5: 1. Add the VF token parse error handling.
    2. Split into two patches for different logic module.
    3. Add more comments into the code for explaining the design.
    4. Drop the ABI change workaround, this patch set focuses on code review.
    https://patchwork.dpdk.org/cover/68364/

v4: 1. Ignore rte_vfio_setup_device ABI check since it is
       for Linux driver use.
    https://patchwork.dpdk.org/patch/68255/

v3: Fix the Travis build failed:
           (1). rte_uuid.h:97:55: error: unknown type name ‘size_t’
           (2). rte_uuid.h:58:2: error: implicit declaration of function ‘memcpy’
    https://patchwork.dpdk.org/patch/68254/

v2: Fix the FreeBSD build error.
         https://patchwork.dpdk.org/patch/68240/

v1: Update the commit message.
        https://patchwork.dpdk.org/patch/68237/

RFC v2: https://patchwork.dpdk.org/patch/68114/ 
         Based on Vamsi's RFC v1, and Alex's patch for Qemu
        [https://lore.kernel.org/lkml/20200204161737.34696b91@w520.home/]: 
       Use the devarg to pass-down the VF token.

RFC v1: https://patchwork.dpdk.org/patch/66281/ by Vamsi.

Haiyue Wang (2):
  eal: add uuid dependent header files explicitly
  eal: support for VFIO-PCI VF token

 doc/guides/linux_gsg/linux_drivers.rst | 38 ++++++++++++-
 doc/guides/rel_notes/release_20_05.rst |  5 ++
 drivers/bus/pci/linux/pci_vfio.c       | 74 +++++++++++++++++++++++++-
 lib/librte_eal/freebsd/eal.c           |  3 +-
 lib/librte_eal/include/rte_uuid.h      |  2 +
 lib/librte_eal/include/rte_vfio.h      | 21 +++++++-
 lib/librte_eal/linux/eal_vfio.c        | 20 +++++--
 7 files changed, 155 insertions(+), 8 deletions(-)
  

Comments

David Marchand April 20, 2020, 4:53 p.m. UTC | #1
Hello,

On Sat, Apr 18, 2020 at 7:36 PM Haiyue Wang <haiyue.wang@intel.com> wrote:
>
> v8: Update the document.

Thanks for the last version.
I had a look at the CI, I can see we are still missing bits to handle
the ABI failure on rte_vfio_setup_device.
  
Wang, Haiyue April 20, 2020, 5:02 p.m. UTC | #2
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, April 21, 2020 00:53
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev <dev@dpdk.org>; Burakov, Anatoly <anatoly.burakov@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Vamsi Attunuru <vattunuru@marvell.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Alex Williamson <alex.williamson@redhat.com>
> Subject: Re: [PATCH v8 0/2] support for VFIO-PCI VF token interface
> 
> Hello,
> 
> On Sat, Apr 18, 2020 at 7:36 PM Haiyue Wang <haiyue.wang@intel.com> wrote:
> >
> > v8: Update the document.
> 
> Thanks for the last version.
> I had a look at the CI, I can see we are still missing bits to handle
> the ABI failure on rte_vfio_setup_device.

Yes, not handle it now.

If 'rte_vfio_setup_device' can be internal, not official DPDK API, then __rte_internal
is the best way to handle ABI issue.

> 
> 
> --
> David Marchand
  
Thomas Monjalon April 20, 2020, 5:13 p.m. UTC | #3
20/04/2020 19:02, Wang, Haiyue:
> From: David Marchand <david.marchand@redhat.com>
> > 
> > Hello,
> > 
> > On Sat, Apr 18, 2020 at 7:36 PM Haiyue Wang <haiyue.wang@intel.com> wrote:
> > >
> > > v8: Update the document.
> > 
> > Thanks for the last version.
> > I had a look at the CI, I can see we are still missing bits to handle
> > the ABI failure on rte_vfio_setup_device.
> 
> Yes, not handle it now.
> 
> If 'rte_vfio_setup_device' can be internal, not official DPDK API, then __rte_internal
> is the best way to handle ABI issue.

Please could you help finishing integration of __rte_internal?

This patch is blocked for now.
  
Wang, Haiyue April 20, 2020, 5:37 p.m. UTC | #4
+Neil,

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, April 21, 2020 01:13
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: David Marchand <david.marchand@redhat.com>; dev <dev@dpdk.org>; Burakov, Anatoly
> <anatoly.burakov@intel.com>; Vamsi Attunuru <vattunuru@marvell.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Alex Williamson <alex.williamson@redhat.com>
> Subject: Re: [PATCH v8 0/2] support for VFIO-PCI VF token interface
> 
> 20/04/2020 19:02, Wang, Haiyue:
> > From: David Marchand <david.marchand@redhat.com>
> > >
> > > Hello,
> > >
> > > On Sat, Apr 18, 2020 at 7:36 PM Haiyue Wang <haiyue.wang@intel.com> wrote:
> > > >
> > > > v8: Update the document.
> > >
> > > Thanks for the last version.
> > > I had a look at the CI, I can see we are still missing bits to handle
> > > the ABI failure on rte_vfio_setup_device.
> >
> > Yes, not handle it now.
> >
> > If 'rte_vfio_setup_device' can be internal, not official DPDK API, then __rte_internal
> > is the best way to handle ABI issue.
> 
> Please could you help finishing integration of __rte_internal?
> 

I thought it should be Neil ? "Yes, I'll get back to this today" ;-)
http://inbox.dpdk.org/dev/CAJFAV8ydLkV0afEHqbh6KeA3Box0Yxb3N0MNGtMD4S9bmSgT0A@mail.gmail.com/

> This patch is blocked for now.
>
  
Thomas Monjalon April 20, 2020, 5:42 p.m. UTC | #5
20/04/2020 19:37, Wang, Haiyue:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 20/04/2020 19:02, Wang, Haiyue:
> > > From: David Marchand <david.marchand@redhat.com>
> > > > I had a look at the CI, I can see we are still missing bits to handle
> > > > the ABI failure on rte_vfio_setup_device.
> > >
> > > Yes, not handle it now.
> > >
> > > If 'rte_vfio_setup_device' can be internal, not official DPDK API, then __rte_internal
> > > is the best way to handle ABI issue.
> > 
> > Please could you help finishing integration of __rte_internal?
> 
> I thought it should be Neil ? "Yes, I'll get back to this today" ;-)
> http://inbox.dpdk.org/dev/CAJFAV8ydLkV0afEHqbh6KeA3Box0Yxb3N0MNGtMD4S9bmSgT0A@mail.gmail.com/

It did not happen after several months.
If you want to unblock your patches, I think it is safer to finish yourself.
  
Wang, Haiyue April 21, 2020, 1:38 a.m. UTC | #6
Hi Thomas & David,

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, April 21, 2020 01:43
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>; David Marchand <david.marchand@redhat.com>; dev
> <dev@dpdk.org>; Burakov, Anatoly <anatoly.burakov@intel.com>; Vamsi Attunuru <vattunuru@marvell.com>;
> Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Alex Williamson <alex.williamson@redhat.com>
> Subject: Re: [PATCH v8 0/2] support for VFIO-PCI VF token interface
> 
> 20/04/2020 19:37, Wang, Haiyue:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 20/04/2020 19:02, Wang, Haiyue:
> > > > From: David Marchand <david.marchand@redhat.com>
> > > > > I had a look at the CI, I can see we are still missing bits to handle
> > > > > the ABI failure on rte_vfio_setup_device.
> > > >
> > > > Yes, not handle it now.
> > > >
> > > > If 'rte_vfio_setup_device' can be internal, not official DPDK API, then __rte_internal
> > > > is the best way to handle ABI issue.
> > >
> > > Please could you help finishing integration of __rte_internal?
> >
> > I thought it should be Neil ? "Yes, I'll get back to this today" ;-)
> > http://inbox.dpdk.org/dev/CAJFAV8ydLkV0afEHqbh6KeA3Box0Yxb3N0MNGtMD4S9bmSgT0A@mail.gmail.com/
> 
> It did not happen after several months.
> If you want to unblock your patches, I think it is safer to finish yourself.
> 

Unfortunately, this __rte_internal will still make the ci fail to run when move the function
to INTERNAL session:

--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -3,6 +3,11 @@
 [suppress_variable]
         symbol_version = EXPERIMENTAL

+[suppress_function]
+        symbol_version = INTERNAL
+[suppress_variable]
+        symbol_version = INTERNAL
+


--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -213,7 +213,6 @@ DPDK_20.0 {
        rte_vfio_is_enabled;
        rte_vfio_noiommu_is_enabled;
        rte_vfio_release_device;
-       rte_vfio_setup_device;
        rte_vlog;
        rte_zmalloc;
        rte_zmalloc_socket;
@@ -339,3 +338,10 @@ EXPERIMENTAL {
        # added in 20.05
        rte_log_can_log;
 };
+
+INTERNAL {
+       global:
+
+       # added in 20.05
+       rte_vfio_setup_device;
+};


Functions changes summary: 1 Removed, 0 Changed, 0 Added function
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

1 Removed function:

  [D] 'function int rte_vfio_setup_device(const char*, const char*, int*, vfio_device_info*)'    {rte_vfio_setup_device@@DPDK_20.0}

Error: ABI issue reported for 'abidiff --suppr ./devtools/libabigail.abignore --no-added-syms --headers-dir1 old_abi/include --headers-dir2 new_abi/include old_abi/dump/librte_eal.dump new_abi/dump/librte_eal.dump'
  
Thomas Monjalon April 21, 2020, 2:12 a.m. UTC | #7
21/04/2020 03:38, Wang, Haiyue:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 20/04/2020 19:37, Wang, Haiyue:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 20/04/2020 19:02, Wang, Haiyue:
> > > > > From: David Marchand <david.marchand@redhat.com>
> > > > > > I had a look at the CI, I can see we are still missing bits to handle
> > > > > > the ABI failure on rte_vfio_setup_device.
> > > > >
> > > > > Yes, not handle it now.
> > > > >
> > > > > If 'rte_vfio_setup_device' can be internal, not official DPDK API, then __rte_internal
> > > > > is the best way to handle ABI issue.
> > > >
> > > > Please could you help finishing integration of __rte_internal?
> > >
> > > I thought it should be Neil ? "Yes, I'll get back to this today" ;-)
> > > http://inbox.dpdk.org/dev/CAJFAV8ydLkV0afEHqbh6KeA3Box0Yxb3N0MNGtMD4S9bmSgT0A@mail.gmail.com/
> > 
> > It did not happen after several months.
> > If you want to unblock your patches, I think it is safer to finish yourself.
> > 
> 
> Unfortunately, this __rte_internal will still make the ci fail to run when move the function
> to INTERNAL session:
[...]
> +INTERNAL {
> +       global:
> +
> +       # added in 20.05
> +       rte_vfio_setup_device;
> +};

Why do you need to move the symbol explicitly in .map?

The tool should ignore symbols moving to internal, as an exception until 20.11.
  
Wang, Haiyue April 21, 2020, 2:52 a.m. UTC | #8
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, April 21, 2020 10:13
> To: David Marchand <david.marchand@redhat.com>; Wang, Haiyue <haiyue.wang@intel.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>; dev <dev@dpdk.org>; Burakov, Anatoly
> <anatoly.burakov@intel.com>; Vamsi Attunuru <vattunuru@marvell.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Kinsella, Ray <ray.kinsella@intel.com>
> Subject: Re: [PATCH v8 0/2] support for VFIO-PCI VF token interface
> 
> 21/04/2020 03:38, Wang, Haiyue:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 20/04/2020 19:37, Wang, Haiyue:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > 20/04/2020 19:02, Wang, Haiyue:
> > > > > > From: David Marchand <david.marchand@redhat.com>
> > > > > > > I had a look at the CI, I can see we are still missing bits to handle
> > > > > > > the ABI failure on rte_vfio_setup_device.
> > > > > >
> > > > > > Yes, not handle it now.
> > > > > >
> > > > > > If 'rte_vfio_setup_device' can be internal, not official DPDK API, then __rte_internal
> > > > > > is the best way to handle ABI issue.
> > > > >
> > > > > Please could you help finishing integration of __rte_internal?
> > > >
> > > > I thought it should be Neil ? "Yes, I'll get back to this today" ;-)
> > > > http://inbox.dpdk.org/dev/CAJFAV8ydLkV0afEHqbh6KeA3Box0Yxb3N0MNGtMD4S9bmSgT0A@mail.gmail.com/
> > >
> > > It did not happen after several months.
> > > If you want to unblock your patches, I think it is safer to finish yourself.
> > >
> >
> > Unfortunately, this __rte_internal will still make the ci fail to run when move the function
> > to INTERNAL session:
> [...]
> > +INTERNAL {
> > +       global:
> > +
> > +       # added in 20.05
> > +       rte_vfio_setup_device;
> > +};
> 
> Why do you need to move the symbol explicitly in .map?
> The tool should ignore symbols moving to internal, as an exception until 20.11.
>

If not move the symbol explicitly in .map, another kind of error happened.

./devtools/check-abi.sh old_abi new_abi
Functions changes summary: 0 Removed, 1 Changed, 0 Added function
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

1 function with some indirect sub-type change:

  [C] 'function int rte_vfio_setup_device(const char*, const char*, int*, vfio_device_info*)' at eal_vfio.c:704:1 has some indirect sub-type changes:
    parameter 5 of type 'unsigned char*' was added

Error: ABI issue reported for 'abidiff --suppr ./devtools/libabigail.abignore --no-added-syms --headers-dir1 old_abi/include --headers-dir2 new_abi/include old_abi/dump/librte_eal.dump new_abi/dump/librte_eal.dump'
  
Thomas Monjalon April 21, 2020, 8:47 a.m. UTC | #9
21/04/2020 04:52, Wang, Haiyue:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 21/04/2020 03:38, Wang, Haiyue:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 20/04/2020 19:37, Wang, Haiyue:
> > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > 20/04/2020 19:02, Wang, Haiyue:
> > > > > > > From: David Marchand <david.marchand@redhat.com>
> > > > > > > > I had a look at the CI, I can see we are still missing bits to handle
> > > > > > > > the ABI failure on rte_vfio_setup_device.
> > > > > > >
> > > > > > > Yes, not handle it now.
> > > > > > >
> > > > > > > If 'rte_vfio_setup_device' can be internal, not official DPDK API, then __rte_internal
> > > > > > > is the best way to handle ABI issue.
> > > > > >
> > > > > > Please could you help finishing integration of __rte_internal?
> > > > >
> > > > > I thought it should be Neil ? "Yes, I'll get back to this today" ;-)
> > > > > http://inbox.dpdk.org/dev/CAJFAV8ydLkV0afEHqbh6KeA3Box0Yxb3N0MNGtMD4S9bmSgT0A@mail.gmail.com/
> > > >
> > > > It did not happen after several months.
> > > > If you want to unblock your patches, I think it is safer to finish yourself.
> > > >
> > >
> > > Unfortunately, this __rte_internal will still make the ci fail to run when move the function
> > > to INTERNAL session:
> > [...]
> > > +INTERNAL {
> > > +       global:
> > > +
> > > +       # added in 20.05
> > > +       rte_vfio_setup_device;
> > > +};
> > 
> > Why do you need to move the symbol explicitly in .map?
> > The tool should ignore symbols moving to internal, as an exception until 20.11.
> 
> If not move the symbol explicitly in .map, another kind of error happened.
> 
> ./devtools/check-abi.sh old_abi new_abi
> Functions changes summary: 0 Removed, 1 Changed, 0 Added function
> Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> 
> 1 function with some indirect sub-type change:
> 
>   [C] 'function int rte_vfio_setup_device(const char*, const char*, int*, vfio_device_info*)' at eal_vfio.c:704:1 has some indirect sub-type changes:
>     parameter 5 of type 'unsigned char*' was added
> 
> Error: ABI issue reported for 'abidiff --suppr ./devtools/libabigail.abignore --no-added-syms --headers-dir1 old_abi/include --headers-dir2 new_abi/include old_abi/dump/librte_eal.dump new_abi/dump/librte_eal.dump'

This is what I said: you need to add a rule to ignore internal symbols.
  
Wang, Haiyue April 21, 2020, 5:35 p.m. UTC | #10
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, April 21, 2020 16:48
> To: David Marchand <david.marchand@redhat.com>; Wang, Haiyue <haiyue.wang@intel.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>; dev <dev@dpdk.org>; Burakov, Anatoly
> <anatoly.burakov@intel.com>; Vamsi Attunuru <vattunuru@marvell.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Kinsella, Ray <ray.kinsella@intel.com>
> Subject: Re: [PATCH v8 0/2] support for VFIO-PCI VF token interface
> 
> 21/04/2020 04:52, Wang, Haiyue:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 21/04/2020 03:38, Wang, Haiyue:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > 20/04/2020 19:37, Wang, Haiyue:
> > > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > 20/04/2020 19:02, Wang, Haiyue:
> > > > > > > > From: David Marchand <david.marchand@redhat.com>
> > > > > > > > > I had a look at the CI, I can see we are still missing bits to handle
> > > > > > > > > the ABI failure on rte_vfio_setup_device.
> > > > > > > >
> > > > > > > > Yes, not handle it now.
> > > > > > > >
> > > > > > > > If 'rte_vfio_setup_device' can be internal, not official DPDK API, then __rte_internal
> > > > > > > > is the best way to handle ABI issue.
> > > > > > >
> > > > > > > Please could you help finishing integration of __rte_internal?
> > > > > >
> > > > > > I thought it should be Neil ? "Yes, I'll get back to this today" ;-)
> > > > > >
> http://inbox.dpdk.org/dev/CAJFAV8ydLkV0afEHqbh6KeA3Box0Yxb3N0MNGtMD4S9bmSgT0A@mail.gmail.com/
> > > > >
> > > > > It did not happen after several months.
> > > > > If you want to unblock your patches, I think it is safer to finish yourself.
> > > > >
> > > >
> > > > Unfortunately, this __rte_internal will still make the ci fail to run when move the function
> > > > to INTERNAL session:
> > > [...]
> > > > +INTERNAL {
> > > > +       global:
> > > > +
> > > > +       # added in 20.05
> > > > +       rte_vfio_setup_device;
> > > > +};
> > >
> > > Why do you need to move the symbol explicitly in .map?
> > > The tool should ignore symbols moving to internal, as an exception until 20.11.
> >
> > If not move the symbol explicitly in .map, another kind of error happened.
> >
> > ./devtools/check-abi.sh old_abi new_abi
> > Functions changes summary: 0 Removed, 1 Changed, 0 Added function
> > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> >
> > 1 function with some indirect sub-type change:
> >
> >   [C] 'function int rte_vfio_setup_device(const char*, const char*, int*, vfio_device_info*)' at
> eal_vfio.c:704:1 has some indirect sub-type changes:
> >     parameter 5 of type 'unsigned char*' was added
> >
> > Error: ABI issue reported for 'abidiff --suppr ./devtools/libabigail.abignore --no-added-syms --
> headers-dir1 old_abi/include --headers-dir2 new_abi/include old_abi/dump/librte_eal.dump
> new_abi/dump/librte_eal.dump'
> 
> This is what I said: you need to add a rule to ignore internal symbols.
> 

Got the kind reply from Dodji, the maintainer of libabigail, his suggestion
meets what we have done for 'EXPERIMENTAL'. So it seems that have to mark
all needed functions as INTERNAL firstly, which obviously will break the CI
as the public functions are removed....


"
__PROJECT_INTERNAL_USE_ONLY_VERSION__ {
  global: funA
};
...
Then, once you have all your internal functions marked in the ELF binary
with the proper ELF version string, you can tell libabigail to suppress
all functions that have that version by writing a suppression
specification file that has this content:


[suppress_function]
  symbol_version = __PROJECT_INTERNAL_USE_ONLY_VERSION__
"