[PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess

[PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess

[David Marchand]

At the moment, if VFIO is not available at DPDK init, it won't be
available unless a subsequent rte_vfio_enable() is done.

Yet, even if rte_vfio_enable() is called again in primary and secondary
processes, a secondary process will never get to know that VFIO has been
enabled in the primary process as the MP requests handler is only
registered in EAL init.

On the other hand, moving the MP requests handler registration earlier
in EAL init is ok, as secondary process are supposed to be waiting on
eal_mcfg_wait_complete() until the primary process calls
eal_mcfg_complete().

Move vfio_mp_sync_setup() in rte_vfio_enable().

Besides, rte_eal_vfio_setup() is useless and its name with a rte_ prefix
is ambiguous as it gives the impression it is an exported/public symbol.
Remove it and directly call rte_vfio_enable() where needed.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/linux/eal.c      | 18 +-----------------
 lib/eal/linux/eal_vfio.c |  9 ++++++---
 2 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index d742cc98e2..54577b7718 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -867,16 +867,6 @@ rte_eal_iopl_init(void)
 	return 0;
 }
 
-#ifdef VFIO_PRESENT
-static int rte_eal_vfio_setup(void)
-{
-	if (rte_vfio_enable("vfio"))
-		return -1;
-
-	return 0;
-}
-#endif
-
 static void rte_eal_init_alert(const char *msg)
 {
 	fprintf(stderr, "EAL: FATAL: %s\n", msg);
@@ -1162,7 +1152,7 @@ rte_eal_init(int argc, char **argv)
 	}
 
 #ifdef VFIO_PRESENT
-	if (rte_eal_vfio_setup() < 0) {
+	if (rte_vfio_enable("vfio")) {
 		rte_eal_init_alert("Cannot init VFIO");
 		rte_errno = EAGAIN;
 		rte_atomic_store_explicit(&run_once, 0, rte_memory_order_relaxed);
@@ -1291,12 +1281,6 @@ rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
-#ifdef VFIO_PRESENT
-	/* Register mp action after probe() so that we got enough info */
-	if (rte_vfio_is_enabled("vfio") && vfio_mp_sync_setup() < 0)
-		return -1;
-#endif
-
 	/* initialize default service/lcore mappings and start running. Ignore
 	 * -ENOTSUP, as it indicates no service coremask passed to EAL.
 	 */
diff --git a/lib/eal/linux/eal_vfio.c b/lib/eal/linux/eal_vfio.c
index 4e69e72e3b..7132e24cba 100644
--- a/lib/eal/linux/eal_vfio.c
+++ b/lib/eal/linux/eal_vfio.c
@@ -1120,9 +1120,12 @@ rte_vfio_enable(const char *modname)
 	}
 
 	if (internal_conf->process_type == RTE_PROC_PRIMARY) {
-		/* open a new container */
-		default_vfio_cfg->vfio_container_fd =
-				rte_vfio_get_container_fd();
+		if (vfio_mp_sync_setup() == -1) {
+			default_vfio_cfg->vfio_container_fd = -1;
+		} else {
+			/* open a new container */
+			default_vfio_cfg->vfio_container_fd = rte_vfio_get_container_fd();
+		}
 	} else {
 		/* get the default container from the primary process */
 		default_vfio_cfg->vfio_container_fd =
-- 
2.46.0

[PATCH 2/2] bus/pci: enhance hotplug for VFIO bound devices

[David Marchand]

VFIO modules may get loaded/initialized after DPDK initialized (like when
starting an application without knowing which devices will be used, and
whether their drivers require VFIO).

Retry enabling VFIO if not available.
This way, it is not required to restart the DPDK application anymore.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/bus/pci/linux/pci_vfio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index adc34a5c51..5317170231 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -1319,6 +1319,12 @@ pci_vfio_mmio_write(const struct rte_pci_device *dev, int bar,
 int
 pci_vfio_is_enabled(void)
 {
-	return rte_vfio_is_enabled("vfio_pci");
+	int status = rte_vfio_is_enabled("vfio_pci");
+
+	if (!status) {
+		rte_vfio_enable("vfio");
+		status = rte_vfio_is_enabled("vfio_pci");
+	}
+	return status;
 }
 #endif
-- 
2.46.0

Re: [PATCH 2/2] bus/pci: enhance hotplug for VFIO bound devices

[Maxime Coquelin]

On 9/16/24 14:30, David Marchand wrote:
> VFIO modules may get loaded/initialized after DPDK initialized (like when
> starting an application without knowing which devices will be used, and
> whether their drivers require VFIO).
> 
> Retry enabling VFIO if not available.
> This way, it is not required to restart the DPDK application anymore.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   drivers/bus/pci/linux/pci_vfio.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index adc34a5c51..5317170231 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -1319,6 +1319,12 @@ pci_vfio_mmio_write(const struct rte_pci_device *dev, int bar,
>   int
>   pci_vfio_is_enabled(void)
>   {
> -	return rte_vfio_is_enabled("vfio_pci");
> +	int status = rte_vfio_is_enabled("vfio_pci");
> +
> +	if (!status) {
> +		rte_vfio_enable("vfio");
> +		status = rte_vfio_is_enabled("vfio_pci");
> +	}
> +	return status;
>   }
>   #endif

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Also wondering if it could be considered a fix.

Thanks,
Maxime

Re: [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess

[Maxime Coquelin]

On 9/16/24 14:30, David Marchand wrote:
> At the moment, if VFIO is not available at DPDK init, it won't be
> available unless a subsequent rte_vfio_enable() is done.
> 
> Yet, even if rte_vfio_enable() is called again in primary and secondary
> processes, a secondary process will never get to know that VFIO has been
> enabled in the primary process as the MP requests handler is only
> registered in EAL init.
> 
> On the other hand, moving the MP requests handler registration earlier
> in EAL init is ok, as secondary process are supposed to be waiting on
> eal_mcfg_wait_complete() until the primary process calls
> eal_mcfg_complete().
> 
> Move vfio_mp_sync_setup() in rte_vfio_enable().
> 
> Besides, rte_eal_vfio_setup() is useless and its name with a rte_ prefix
> is ambiguous as it gives the impression it is an exported/public symbol.
> Remove it and directly call rte_vfio_enable() where needed.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   lib/eal/linux/eal.c      | 18 +-----------------
>   lib/eal/linux/eal_vfio.c |  9 ++++++---
>   2 files changed, 7 insertions(+), 20 deletions(-)
> 

Should it be considered as a fix, and so a candidate for stable?
Or do you think it is too risky to change the behaviour?

Other than that, it looks good to me:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess

[David Marchand]

On Tue, Sep 17, 2024 at 10:20 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> On 9/16/24 14:30, David Marchand wrote:
> > At the moment, if VFIO is not available at DPDK init, it won't be
> > available unless a subsequent rte_vfio_enable() is done.
> >
> > Yet, even if rte_vfio_enable() is called again in primary and secondary
> > processes, a secondary process will never get to know that VFIO has been
> > enabled in the primary process as the MP requests handler is only
> > registered in EAL init.
> >
> > On the other hand, moving the MP requests handler registration earlier
> > in EAL init is ok, as secondary process are supposed to be waiting on
> > eal_mcfg_wait_complete() until the primary process calls
> > eal_mcfg_complete().
> >
> > Move vfio_mp_sync_setup() in rte_vfio_enable().
> >
> > Besides, rte_eal_vfio_setup() is useless and its name with a rte_ prefix
> > is ambiguous as it gives the impression it is an exported/public symbol.
> > Remove it and directly call rte_vfio_enable() where needed.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >   lib/eal/linux/eal.c      | 18 +-----------------
> >   lib/eal/linux/eal_vfio.c |  9 ++++++---
> >   2 files changed, 7 insertions(+), 20 deletions(-)
> >
>
> Should it be considered as a fix, and so a candidate for stable?
> Or do you think it is too risky to change the behaviour?

Cc: stable maintainers (fyi)

I don't think it is risky: existing applications which relied on multi
process had no choice but to make sure VFIO was properly setup before
DPDK init.
Yet, a change in behavior impact is always hard to estimate.

(same comment for the second patch of the series)


-- 
David Marchand

Re: [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess

[Kevin Traynor]

On 02/10/2024 14:31, David Marchand wrote:
> On Tue, Sep 17, 2024 at 10:20 AM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>> On 9/16/24 14:30, David Marchand wrote:
>>> At the moment, if VFIO is not available at DPDK init, it won't be
>>> available unless a subsequent rte_vfio_enable() is done.
>>>
>>> Yet, even if rte_vfio_enable() is called again in primary and secondary
>>> processes, a secondary process will never get to know that VFIO has been
>>> enabled in the primary process as the MP requests handler is only
>>> registered in EAL init.
>>>
>>> On the other hand, moving the MP requests handler registration earlier
>>> in EAL init is ok, as secondary process are supposed to be waiting on
>>> eal_mcfg_wait_complete() until the primary process calls
>>> eal_mcfg_complete().
>>>
>>> Move vfio_mp_sync_setup() in rte_vfio_enable().
>>>
>>> Besides, rte_eal_vfio_setup() is useless and its name with a rte_ prefix
>>> is ambiguous as it gives the impression it is an exported/public symbol.
>>> Remove it and directly call rte_vfio_enable() where needed.
>>>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> ---
>>>   lib/eal/linux/eal.c      | 18 +-----------------
>>>   lib/eal/linux/eal_vfio.c |  9 ++++++---
>>>   2 files changed, 7 insertions(+), 20 deletions(-)
>>>
>>
>> Should it be considered as a fix, and so a candidate for stable?
>> Or do you think it is too risky to change the behaviour?
> 
> Cc: stable maintainers (fyi)
> 
> I don't think it is risky: existing applications which relied on multi
> process had no choice but to make sure VFIO was properly setup before
> DPDK init.
> Yet, a change in behavior impact is always hard to estimate.
> 
> (same comment for the second patch of the series)
> 
> 

They seem borderline fixes for usability. I think it's a good candidate
for 22.11/23.11.

For 21.11, it is the last release and the behaviour has been like that
for 3 years, so would probably skip that one.

Re: [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess

[David Marchand]

On Tue, Sep 17, 2024 at 10:20 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> > At the moment, if VFIO is not available at DPDK init, it won't be
> > available unless a subsequent rte_vfio_enable() is done.
> >
> > Yet, even if rte_vfio_enable() is called again in primary and secondary
> > processes, a secondary process will never get to know that VFIO has been
> > enabled in the primary process as the MP requests handler is only
> > registered in EAL init.
> >
> > On the other hand, moving the MP requests handler registration earlier
> > in EAL init is ok, as secondary process are supposed to be waiting on
> > eal_mcfg_wait_complete() until the primary process calls
> > eal_mcfg_complete().
> >
> > Move vfio_mp_sync_setup() in rte_vfio_enable().
> >
> > Besides, rte_eal_vfio_setup() is useless and its name with a rte_ prefix
> > is ambiguous as it gives the impression it is an exported/public symbol.
> > Remove it and directly call rte_vfio_enable() where needed.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Series applied for rc1.
Thanks for the review Maxime.


-- 
David Marchand

Re: [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess

[David Marchand]

Hello Anatoly,

On Mon, Sep 16, 2024 at 2:31 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> At the moment, if VFIO is not available at DPDK init, it won't be
> available unless a subsequent rte_vfio_enable() is done.
>
> Yet, even if rte_vfio_enable() is called again in primary and secondary
> processes, a secondary process will never get to know that VFIO has been
> enabled in the primary process as the MP requests handler is only
> registered in EAL init.
>
> On the other hand, moving the MP requests handler registration earlier
> in EAL init is ok, as secondary process are supposed to be waiting on
> eal_mcfg_wait_complete() until the primary process calls
> eal_mcfg_complete().
>
> Move vfio_mp_sync_setup() in rte_vfio_enable().
>
> Besides, rte_eal_vfio_setup() is useless and its name with a rte_ prefix
> is ambiguous as it gives the impression it is an exported/public symbol.
> Remove it and directly call rte_vfio_enable() where needed.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Can I get a review on this 2 patches series?
Thanks.


-- 
David Marchand