[dpdk-dev] vfio: use right index when tracking devices in a vfio group
Checks
Commit Message
Previous fix for properly handling devices from the same VFIO group
introduced another bug where the file descriptor of a kernel vfio
group is used as the index for tracking number of devices of a vfio
group struct handled by dpdk vfio code. Instead of the file
descriptor itself, the vfio group object that file descriptor is
registered with has to be used.
This patch introduces specific functions for incrementing or
decrementing the device counter for a specific vfio group using the
vfio file descriptor as a parameter. Note the code is not optimized
as the vfio group is found sequentially going through the vfio group
array but this should not be a problem as this is not related to
packet handling at all.
Fixes: a9c349e3a100 ("vfio: fix device unplug when several devices per group")
Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
lib/librte_eal/linuxapp/eal/eal_vfio.c | 60 +++++++++++++++++++++++++++-------
1 file changed, 49 insertions(+), 11 deletions(-)
Comments
-----Original Message-----
> Date: Mon, 8 May 2017 18:44:18 +0100
> From: Alejandro Lucero <alejandro.lucero@netronome.com>
> To: dev@dpdk.org
> CC: anatoly.burakov@intel.com, jerin.jacob@caviumnetworks.com,
> thomas@monjalon.net
> Subject: [PATCH] vfio: use right index when tracking devices in a vfio group
> X-Mailer: git-send-email 1.9.1
>
> Previous fix for properly handling devices from the same VFIO group
> introduced another bug where the file descriptor of a kernel vfio
> group is used as the index for tracking number of devices of a vfio
> group struct handled by dpdk vfio code. Instead of the file
> descriptor itself, the vfio group object that file descriptor is
> registered with has to be used.
>
> This patch introduces specific functions for incrementing or
> decrementing the device counter for a specific vfio group using the
> vfio file descriptor as a parameter. Note the code is not optimized
> as the vfio group is found sequentially going through the vfio group
> array but this should not be a problem as this is not related to
> packet handling at all.
>
> Fixes: a9c349e3a100 ("vfio: fix device unplug when several devices per group")
>
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
Tested-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> ---
> lib/librte_eal/linuxapp/eal/eal_vfio.c | 60 +++++++++++++++++++++++++++-------
> 1 file changed, 49 insertions(+), 11 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> index d3eae20..21d126f 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> @@ -172,6 +172,44 @@
> return -1;
> }
>
> +
> +static int
> +get_vfio_group_idx(int vfio_group_fd)
> +{
> + int i;
> + for (i = 0; i < VFIO_MAX_GROUPS; i++)
> + if (vfio_cfg.vfio_groups[i].fd == vfio_group_fd)
> + return i;
> + return -1;
> +}
> +
> +static void
> +vfio_group_device_get(int vfio_group_fd)
> +{
> + int i;
> +
> + i = get_vfio_group_idx(vfio_group_fd);
> + vfio_cfg.vfio_groups[i].devices++;
> +}
> +
> +static void
> +vfio_group_device_put(int vfio_group_fd)
> +{
> + int i;
> +
> + i = get_vfio_group_idx(vfio_group_fd);
> + vfio_cfg.vfio_groups[i].devices--;
> +}
> +
> +static int
> +vfio_group_device_count(int vfio_group_fd)
> +{
> + int i;
> +
> + i = get_vfio_group_idx(vfio_group_fd);
> + return vfio_cfg.vfio_groups[i].devices;
> +}
> +
> int
> clear_group(int vfio_group_fd)
> {
> @@ -180,15 +218,14 @@
>
> if (internal_config.process_type == RTE_PROC_PRIMARY) {
>
> - for (i = 0; i < VFIO_MAX_GROUPS; i++)
> - if (vfio_cfg.vfio_groups[i].fd == vfio_group_fd) {
> - vfio_cfg.vfio_groups[i].group_no = -1;
> - vfio_cfg.vfio_groups[i].fd = -1;
> - vfio_cfg.vfio_groups[i].devices = 0;
> - vfio_cfg.vfio_active_groups--;
> - return 0;
> - }
> - return -1;
> + i = get_vfio_group_idx(vfio_group_fd);
> + if ( i < 0)
> + return -1;
> + vfio_cfg.vfio_groups[i].group_no = -1;
> + vfio_cfg.vfio_groups[i].fd = -1;
> + vfio_cfg.vfio_groups[i].devices = 0;
> + vfio_cfg.vfio_active_groups--;
> + return 0;
> }
>
> /* This is just for SECONDARY processes */
> @@ -358,7 +395,7 @@
> clear_group(vfio_group_fd);
> return -1;
> }
> - vfio_cfg.vfio_groups[vfio_group_fd].devices++;
> + vfio_group_device_get(vfio_group_fd);
>
> return 0;
> }
> @@ -406,7 +443,8 @@
> /* An VFIO group can have several devices attached. Just when there is
> * no devices remaining should the group be closed.
> */
> - if (--vfio_cfg.vfio_groups[vfio_group_fd].devices == 0) {
> + vfio_group_device_put(vfio_group_fd);
> + if (!vfio_group_device_count(vfio_group_fd)) {
>
> if (close(vfio_group_fd) < 0) {
> RTE_LOG(INFO, EAL, "Error when closing vfio_group_fd for %s\n",
> --
> 1.9.1
>
Hi Alejandro,
> From: Alejandro Lucero [mailto:alejandro.lucero@netronome.com]
> Sent: Monday, May 8, 2017 6:44 PM
> To: dev@dpdk.org
> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>;
> jerin.jacob@caviumnetworks.com; thomas@monjalon.net
> Subject: [PATCH] vfio: use right index when tracking devices in a vfio group
>
> Previous fix for properly handling devices from the same VFIO group
> introduced another bug where the file descriptor of a kernel vfio group is
> used as the index for tracking number of devices of a vfio group struct
> handled by dpdk vfio code. Instead of the file descriptor itself, the vfio group
> object that file descriptor is registered with has to be used.
>
> This patch introduces specific functions for incrementing or decrementing
> the device counter for a specific vfio group using the vfio file descriptor as a
> parameter. Note the code is not optimized as the vfio group is found
> sequentially going through the vfio group array but this should not be a
> problem as this is not related to packet handling at all.
>
> Fixes: a9c349e3a100 ("vfio: fix device unplug when several devices per
> group")
>
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> ---
> lib/librte_eal/linuxapp/eal/eal_vfio.c | 60
> +++++++++++++++++++++++++++-------
> 1 file changed, 49 insertions(+), 11 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> index d3eae20..21d126f 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> @@ -172,6 +172,44 @@
> return -1;
> }
>
> +
> +static int
> +get_vfio_group_idx(int vfio_group_fd)
> +{
> + int i;
> + for (i = 0; i < VFIO_MAX_GROUPS; i++)
> + if (vfio_cfg.vfio_groups[i].fd == vfio_group_fd)
> + return i;
> + return -1;
> +}
> +
> +static void
> +vfio_group_device_get(int vfio_group_fd) {
> + int i;
> +
> + i = get_vfio_group_idx(vfio_group_fd);
> + vfio_cfg.vfio_groups[i].devices++;
Maybe add a check for I < 0?
> +}
> +
> +static void
> +vfio_group_device_put(int vfio_group_fd) {
> + int i;
> +
> + i = get_vfio_group_idx(vfio_group_fd);
> + vfio_cfg.vfio_groups[i].devices--;
Same here.
> +}
> +
> +static int
> +vfio_group_device_count(int vfio_group_fd) {
> + int i;
> +
> + i = get_vfio_group_idx(vfio_group_fd);
> + return vfio_cfg.vfio_groups[i].devices; }
And here.
> +
> int
> clear_group(int vfio_group_fd)
> {
> @@ -180,15 +218,14 @@
>
> if (internal_config.process_type == RTE_PROC_PRIMARY) {
>
> - for (i = 0; i < VFIO_MAX_GROUPS; i++)
> - if (vfio_cfg.vfio_groups[i].fd == vfio_group_fd) {
> - vfio_cfg.vfio_groups[i].group_no = -1;
> - vfio_cfg.vfio_groups[i].fd = -1;
> - vfio_cfg.vfio_groups[i].devices = 0;
> - vfio_cfg.vfio_active_groups--;
> - return 0;
> - }
> - return -1;
> + i = get_vfio_group_idx(vfio_group_fd);
> + if ( i < 0)
> + return -1;
> + vfio_cfg.vfio_groups[i].group_no = -1;
> + vfio_cfg.vfio_groups[i].fd = -1;
> + vfio_cfg.vfio_groups[i].devices = 0;
> + vfio_cfg.vfio_active_groups--;
> + return 0;
> }
>
> /* This is just for SECONDARY processes */ @@ -358,7 +395,7 @@
> clear_group(vfio_group_fd);
> return -1;
> }
> - vfio_cfg.vfio_groups[vfio_group_fd].devices++;
> + vfio_group_device_get(vfio_group_fd);
>
> return 0;
> }
> @@ -406,7 +443,8 @@
> /* An VFIO group can have several devices attached. Just when there
> is
> * no devices remaining should the group be closed.
> */
> - if (--vfio_cfg.vfio_groups[vfio_group_fd].devices == 0) {
> + vfio_group_device_put(vfio_group_fd);
> + if (!vfio_group_device_count(vfio_group_fd)) {
>
> if (close(vfio_group_fd) < 0) {
> RTE_LOG(INFO, EAL, "Error when closing
> vfio_group_fd for %s\n",
> --
> 1.9.1
Hi Anatoly,
On Tue, May 9, 2017 at 4:18 PM, Burakov, Anatoly <anatoly.burakov@intel.com>
wrote:
> Hi Alejandro,
>
> > From: Alejandro Lucero [mailto:alejandro.lucero@netronome.com]
> > Sent: Monday, May 8, 2017 6:44 PM
> > To: dev@dpdk.org
> > Cc: Burakov, Anatoly <anatoly.burakov@intel.com>;
> > jerin.jacob@caviumnetworks.com; thomas@monjalon.net
> > Subject: [PATCH] vfio: use right index when tracking devices in a vfio
> group
> >
> > Previous fix for properly handling devices from the same VFIO group
> > introduced another bug where the file descriptor of a kernel vfio group
> is
> > used as the index for tracking number of devices of a vfio group struct
> > handled by dpdk vfio code. Instead of the file descriptor itself, the
> vfio group
> > object that file descriptor is registered with has to be used.
> >
> > This patch introduces specific functions for incrementing or decrementing
> > the device counter for a specific vfio group using the vfio file
> descriptor as a
> > parameter. Note the code is not optimized as the vfio group is found
> > sequentially going through the vfio group array but this should not be a
> > problem as this is not related to packet handling at all.
> >
> > Fixes: a9c349e3a100 ("vfio: fix device unplug when several devices per
> > group")
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > ---
> > lib/librte_eal/linuxapp/eal/eal_vfio.c | 60
> > +++++++++++++++++++++++++++-------
> > 1 file changed, 49 insertions(+), 11 deletions(-)
> >
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > index d3eae20..21d126f 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > @@ -172,6 +172,44 @@
> > return -1;
> > }
> >
> > +
> > +static int
> > +get_vfio_group_idx(int vfio_group_fd)
> > +{
> > + int i;
> > + for (i = 0; i < VFIO_MAX_GROUPS; i++)
> > + if (vfio_cfg.vfio_groups[i].fd == vfio_group_fd)
> > + return i;
> > + return -1;
> > +}
> > +
> > +static void
> > +vfio_group_device_get(int vfio_group_fd) {
> > + int i;
> > +
> > + i = get_vfio_group_idx(vfio_group_fd);
> > + vfio_cfg.vfio_groups[i].devices++;
>
> Maybe add a check for I < 0?
>
>
Right. I doubted about adding such a check, since being < 0 or >
VFIO_MAX_GROUPS are bad news. But better to avoid using a wrong index which
could led to a random write elsewhere. I will send another patch version
with that check and a RTE_LOG(ERR ... call.
And same for the other functions.
Thanks
> > +}
> > +
> > +static void
> > +vfio_group_device_put(int vfio_group_fd) {
> > + int i;
> > +
> > + i = get_vfio_group_idx(vfio_group_fd);
> > + vfio_cfg.vfio_groups[i].devices--;
>
> Same here.
>
> > +}
> > +
> > +static int
> > +vfio_group_device_count(int vfio_group_fd) {
> > + int i;
> > +
> > + i = get_vfio_group_idx(vfio_group_fd);
> > + return vfio_cfg.vfio_groups[i].devices; }
>
> And here.
>
> > +
> > int
> > clear_group(int vfio_group_fd)
> > {
> > @@ -180,15 +218,14 @@
> >
> > if (internal_config.process_type == RTE_PROC_PRIMARY) {
> >
> > - for (i = 0; i < VFIO_MAX_GROUPS; i++)
> > - if (vfio_cfg.vfio_groups[i].fd == vfio_group_fd) {
> > - vfio_cfg.vfio_groups[i].group_no = -1;
> > - vfio_cfg.vfio_groups[i].fd = -1;
> > - vfio_cfg.vfio_groups[i].devices = 0;
> > - vfio_cfg.vfio_active_groups--;
> > - return 0;
> > - }
> > - return -1;
> > + i = get_vfio_group_idx(vfio_group_fd);
> > + if ( i < 0)
> > + return -1;
> > + vfio_cfg.vfio_groups[i].group_no = -1;
> > + vfio_cfg.vfio_groups[i].fd = -1;
> > + vfio_cfg.vfio_groups[i].devices = 0;
> > + vfio_cfg.vfio_active_groups--;
> > + return 0;
> > }
> >
> > /* This is just for SECONDARY processes */ @@ -358,7 +395,7 @@
> > clear_group(vfio_group_fd);
> > return -1;
> > }
> > - vfio_cfg.vfio_groups[vfio_group_fd].devices++;
> > + vfio_group_device_get(vfio_group_fd);
> >
> > return 0;
> > }
> > @@ -406,7 +443,8 @@
> > /* An VFIO group can have several devices attached. Just when there
> > is
> > * no devices remaining should the group be closed.
> > */
> > - if (--vfio_cfg.vfio_groups[vfio_group_fd].devices == 0) {
> > + vfio_group_device_put(vfio_group_fd);
> > + if (!vfio_group_device_count(vfio_group_fd)) {
> >
> > if (close(vfio_group_fd) < 0) {
> > RTE_LOG(INFO, EAL, "Error when closing
> > vfio_group_fd for %s\n",
> > --
> > 1.9.1
>
>
@@ -172,6 +172,44 @@
return -1;
}
+
+static int
+get_vfio_group_idx(int vfio_group_fd)
+{
+ int i;
+ for (i = 0; i < VFIO_MAX_GROUPS; i++)
+ if (vfio_cfg.vfio_groups[i].fd == vfio_group_fd)
+ return i;
+ return -1;
+}
+
+static void
+vfio_group_device_get(int vfio_group_fd)
+{
+ int i;
+
+ i = get_vfio_group_idx(vfio_group_fd);
+ vfio_cfg.vfio_groups[i].devices++;
+}
+
+static void
+vfio_group_device_put(int vfio_group_fd)
+{
+ int i;
+
+ i = get_vfio_group_idx(vfio_group_fd);
+ vfio_cfg.vfio_groups[i].devices--;
+}
+
+static int
+vfio_group_device_count(int vfio_group_fd)
+{
+ int i;
+
+ i = get_vfio_group_idx(vfio_group_fd);
+ return vfio_cfg.vfio_groups[i].devices;
+}
+
int
clear_group(int vfio_group_fd)
{
@@ -180,15 +218,14 @@
if (internal_config.process_type == RTE_PROC_PRIMARY) {
- for (i = 0; i < VFIO_MAX_GROUPS; i++)
- if (vfio_cfg.vfio_groups[i].fd == vfio_group_fd) {
- vfio_cfg.vfio_groups[i].group_no = -1;
- vfio_cfg.vfio_groups[i].fd = -1;
- vfio_cfg.vfio_groups[i].devices = 0;
- vfio_cfg.vfio_active_groups--;
- return 0;
- }
- return -1;
+ i = get_vfio_group_idx(vfio_group_fd);
+ if ( i < 0)
+ return -1;
+ vfio_cfg.vfio_groups[i].group_no = -1;
+ vfio_cfg.vfio_groups[i].fd = -1;
+ vfio_cfg.vfio_groups[i].devices = 0;
+ vfio_cfg.vfio_active_groups--;
+ return 0;
}
/* This is just for SECONDARY processes */
@@ -358,7 +395,7 @@
clear_group(vfio_group_fd);
return -1;
}
- vfio_cfg.vfio_groups[vfio_group_fd].devices++;
+ vfio_group_device_get(vfio_group_fd);
return 0;
}
@@ -406,7 +443,8 @@
/* An VFIO group can have several devices attached. Just when there is
* no devices remaining should the group be closed.
*/
- if (--vfio_cfg.vfio_groups[vfio_group_fd].devices == 0) {
+ vfio_group_device_put(vfio_group_fd);
+ if (!vfio_group_device_count(vfio_group_fd)) {
if (close(vfio_group_fd) < 0) {
RTE_LOG(INFO, EAL, "Error when closing vfio_group_fd for %s\n",