[PATCH] net/hns3: support disable IOVA as PA mode

[PATCH] net/hns3: support disable IOVA as PA mode

[Chengwen Feng]

Claim PMD supports pmd_supports_disable_iova_as_pa.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/hns3/hns3_rxtx_vec_neon.h |  2 +-
 drivers/net/hns3/hns3_rxtx_vec_sve.c  | 13 +++++++++----
 drivers/net/hns3/meson.build          |  7 +------
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/hns3/hns3_rxtx_vec_neon.h b/drivers/net/hns3/hns3_rxtx_vec_neon.h
index 55d9bf817d..6c49c70fc7 100644
--- a/drivers/net/hns3/hns3_rxtx_vec_neon.h
+++ b/drivers/net/hns3/hns3_rxtx_vec_neon.h
@@ -13,7 +13,7 @@ static inline void
 hns3_vec_tx(volatile struct hns3_desc *desc, struct rte_mbuf *pkt)
 {
 	uint64x2_t val1 = {
-		pkt->buf_iova + pkt->data_off,
+		rte_pktmbuf_iova(pkt),
 		((uint64_t)pkt->data_len) << HNS3_TXD_SEND_SIZE_SHIFT
 	};
 	uint64x2_t val2 = {
diff --git a/drivers/net/hns3/hns3_rxtx_vec_sve.c b/drivers/net/hns3/hns3_rxtx_vec_sve.c
index 6f23ba674d..4bf933882a 100644
--- a/drivers/net/hns3/hns3_rxtx_vec_sve.c
+++ b/drivers/net/hns3/hns3_rxtx_vec_sve.c
@@ -261,10 +261,10 @@ hns3_rxq_rearm_mbuf_sve(struct hns3_rx_queue *rxq)
 	for (i = 0; i < HNS3_DEFAULT_RXQ_REARM_THRESH; i += REARM_LOOP_STEP_NUM,
 		rxep += REARM_LOOP_STEP_NUM, rxdp += REARM_LOOP_STEP_NUM) {
 		uint64_t iova[REARM_LOOP_STEP_NUM];
-		iova[0] = rxep[0].mbuf->buf_iova;
-		iova[1] = rxep[1].mbuf->buf_iova;
-		iova[2] = rxep[2].mbuf->buf_iova;
-		iova[3] = rxep[3].mbuf->buf_iova;
+		iova[0] = rte_mbuf_iova_get(rxep[0].mbuf);
+		iova[1] = rte_mbuf_iova_get(rxep[1].mbuf);
+		iova[2] = rte_mbuf_iova_get(rxep[2].mbuf);
+		iova[3] = rte_mbuf_iova_get(rxep[3].mbuf);
 		svuint64_t siova = svld1_u64(PG64_256BIT, iova);
 		siova = svadd_n_u64_z(PG64_256BIT, siova, RTE_PKTMBUF_HEADROOM);
 		svuint64_t ol_base = svdup_n_u64(0);
@@ -397,8 +397,13 @@ hns3_tx_fill_hw_ring_sve(struct hns3_tx_queue *txq,
 		pg = svwhilelt_b64_u32(i, nb_pkts);
 		base_addr = svld1_u64(pg, (uint64_t *)pkts);
 		/* calc mbuf's field buf_iova address */
+#if RTE_IOVA_AS_PA
 		buf_iova = svadd_n_u64_z(pg, base_addr,
 					 offsetof(struct rte_mbuf, buf_iova));
+#else
+		buf_iova = svadd_n_u64_z(pg, base_addr,
+					 offsetof(struct rte_mbuf, buf_addr));
+#endif
 		/* calc mbuf's field data_off address */
 		data_off = svadd_n_u64_z(pg, base_addr,
 					 offsetof(struct rte_mbuf, data_off));
diff --git a/drivers/net/hns3/meson.build b/drivers/net/hns3/meson.build
index e1a5afa2ec..b8c0f80a20 100644
--- a/drivers/net/hns3/meson.build
+++ b/drivers/net/hns3/meson.build
@@ -13,12 +13,6 @@ if arch_subdir != 'x86' and arch_subdir != 'arm' or not dpdk_conf.get('RTE_ARCH_
     subdir_done()
 endif
 
-if dpdk_conf.get('RTE_IOVA_AS_PA') == 0
-    build = false
-    reason = 'driver does not support disabling IOVA as PA mode'
-    subdir_done()
-endif
-
 sources = files(
         'hns3_cmd.c',
         'hns3_dcb.c',
@@ -38,6 +32,7 @@ sources = files(
         'hns3_common.c',
         'hns3_dump.c',
 )
+pmd_supports_disable_iova_as_pa = true
 
 deps += ['hash']
 
-- 
2.17.1

Re: [PATCH] net/hns3: support disable IOVA as PA mode

[Dongdong Liu]

Hi Chengwen

On 2023/2/14 15:11, Chengwen Feng wrote:
> Claim PMD supports pmd_supports_disable_iova_as_pa.
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

This patch looks good to me. Many thanks for your work.

Acked-by: Dongdong Liu <liudongdong3@huawei.com>

Thanks,
Dongdong

RE: [PATCH] net/hns3: support disable IOVA as PA mode

[Ruifeng Wang]

> -----Original Message-----
> From: Chengwen Feng <fengchengwen@huawei.com>
> Sent: Tuesday, February 14, 2023 3:12 PM
> To: thomas@monjalon.net; ferruh.yigit@amd.com; Ruifeng Wang <Ruifeng.Wang@arm.com>;
> Dongdong Liu <liudongdong3@huawei.com>; Yisen Zhuang <yisen.zhuang@huawei.com>
> Cc: dev@dpdk.org
> Subject: [PATCH] net/hns3: support disable IOVA as PA mode
> 
> Claim PMD supports pmd_supports_disable_iova_as_pa.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  drivers/net/hns3/hns3_rxtx_vec_neon.h |  2 +-  drivers/net/hns3/hns3_rxtx_vec_sve.c  | 13
> +++++++++----
>  drivers/net/hns3/meson.build          |  7 +------
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

Re: [PATCH] net/hns3: support disable IOVA as PA mode

[Thomas Monjalon]

16/02/2023 09:36, Ruifeng Wang:
> From: Chengwen Feng <fengchengwen@huawei.com>
> > Subject: [PATCH] net/hns3: support disable IOVA as PA mode

Could we change the title to "support IOVA as VA" ?

> > 
> > Claim PMD supports pmd_supports_disable_iova_as_pa.
> > 
> > Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > ---
> >  drivers/net/hns3/hns3_rxtx_vec_neon.h |  2 +-  drivers/net/hns3/hns3_rxtx_vec_sve.c  | 13
> > +++++++++----
> >  drivers/net/hns3/meson.build          |  7 +------
> >  3 files changed, 11 insertions(+), 11 deletions(-)
> > 
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

[PATCH v2] net/hns3: support IOVA as VA

[Chengwen Feng]

Claim PMD supports pmd_supports_disable_iova_as_pa.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Acked-by: Dongdong Liu <liudongdong3@huawei.com>

---
v2: modify commit head which address Thomas's comment
    add reviewed and acked by of v1.

---
 drivers/net/hns3/hns3_rxtx_vec_neon.h |  2 +-
 drivers/net/hns3/hns3_rxtx_vec_sve.c  | 13 +++++++++----
 drivers/net/hns3/meson.build          |  7 +------
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/hns3/hns3_rxtx_vec_neon.h b/drivers/net/hns3/hns3_rxtx_vec_neon.h
index 55d9bf817d..6c49c70fc7 100644
--- a/drivers/net/hns3/hns3_rxtx_vec_neon.h
+++ b/drivers/net/hns3/hns3_rxtx_vec_neon.h
@@ -13,7 +13,7 @@ static inline void
 hns3_vec_tx(volatile struct hns3_desc *desc, struct rte_mbuf *pkt)
 {
 	uint64x2_t val1 = {
-		pkt->buf_iova + pkt->data_off,
+		rte_pktmbuf_iova(pkt),
 		((uint64_t)pkt->data_len) << HNS3_TXD_SEND_SIZE_SHIFT
 	};
 	uint64x2_t val2 = {
diff --git a/drivers/net/hns3/hns3_rxtx_vec_sve.c b/drivers/net/hns3/hns3_rxtx_vec_sve.c
index 6f23ba674d..4bf933882a 100644
--- a/drivers/net/hns3/hns3_rxtx_vec_sve.c
+++ b/drivers/net/hns3/hns3_rxtx_vec_sve.c
@@ -261,10 +261,10 @@ hns3_rxq_rearm_mbuf_sve(struct hns3_rx_queue *rxq)
 	for (i = 0; i < HNS3_DEFAULT_RXQ_REARM_THRESH; i += REARM_LOOP_STEP_NUM,
 		rxep += REARM_LOOP_STEP_NUM, rxdp += REARM_LOOP_STEP_NUM) {
 		uint64_t iova[REARM_LOOP_STEP_NUM];
-		iova[0] = rxep[0].mbuf->buf_iova;
-		iova[1] = rxep[1].mbuf->buf_iova;
-		iova[2] = rxep[2].mbuf->buf_iova;
-		iova[3] = rxep[3].mbuf->buf_iova;
+		iova[0] = rte_mbuf_iova_get(rxep[0].mbuf);
+		iova[1] = rte_mbuf_iova_get(rxep[1].mbuf);
+		iova[2] = rte_mbuf_iova_get(rxep[2].mbuf);
+		iova[3] = rte_mbuf_iova_get(rxep[3].mbuf);
 		svuint64_t siova = svld1_u64(PG64_256BIT, iova);
 		siova = svadd_n_u64_z(PG64_256BIT, siova, RTE_PKTMBUF_HEADROOM);
 		svuint64_t ol_base = svdup_n_u64(0);
@@ -397,8 +397,13 @@ hns3_tx_fill_hw_ring_sve(struct hns3_tx_queue *txq,
 		pg = svwhilelt_b64_u32(i, nb_pkts);
 		base_addr = svld1_u64(pg, (uint64_t *)pkts);
 		/* calc mbuf's field buf_iova address */
+#if RTE_IOVA_AS_PA
 		buf_iova = svadd_n_u64_z(pg, base_addr,
 					 offsetof(struct rte_mbuf, buf_iova));
+#else
+		buf_iova = svadd_n_u64_z(pg, base_addr,
+					 offsetof(struct rte_mbuf, buf_addr));
+#endif
 		/* calc mbuf's field data_off address */
 		data_off = svadd_n_u64_z(pg, base_addr,
 					 offsetof(struct rte_mbuf, data_off));
diff --git a/drivers/net/hns3/meson.build b/drivers/net/hns3/meson.build
index e1a5afa2ec..b8c0f80a20 100644
--- a/drivers/net/hns3/meson.build
+++ b/drivers/net/hns3/meson.build
@@ -13,12 +13,6 @@ if arch_subdir != 'x86' and arch_subdir != 'arm' or not dpdk_conf.get('RTE_ARCH_
     subdir_done()
 endif
 
-if dpdk_conf.get('RTE_IOVA_AS_PA') == 0
-    build = false
-    reason = 'driver does not support disabling IOVA as PA mode'
-    subdir_done()
-endif
-
 sources = files(
         'hns3_cmd.c',
         'hns3_dcb.c',
@@ -38,6 +32,7 @@ sources = files(
         'hns3_common.c',
         'hns3_dump.c',
 )
+pmd_supports_disable_iova_as_pa = true
 
 deps += ['hash']
 
-- 
2.17.1

Re: [PATCH] net/hns3: support disable IOVA as PA mode

[fengchengwen]

On 2023/2/20 15:44, Thomas Monjalon wrote:
> 16/02/2023 09:36, Ruifeng Wang:
>> From: Chengwen Feng <fengchengwen@huawei.com>
>>> Subject: [PATCH] net/hns3: support disable IOVA as PA mode
> 
> Could we change the title to "support IOVA as VA" ?

v2 fix it, please review, thanks.

> 
>>>
>>> Claim PMD supports pmd_supports_disable_iova_as_pa.
>>>
>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>> ---
>>>  drivers/net/hns3/hns3_rxtx_vec_neon.h |  2 +-  drivers/net/hns3/hns3_rxtx_vec_sve.c  | 13
>>> +++++++++----
>>>  drivers/net/hns3/meson.build          |  7 +------
>>>  3 files changed, 11 insertions(+), 11 deletions(-)
>>>
>> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> 
> 
> 
> .
> 

RE: [PATCH] net/hns3: support disable IOVA as PA mode

[Morten Brørup]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, 20 February 2023 08.45
> 
> 16/02/2023 09:36, Ruifeng Wang:
> > From: Chengwen Feng <fengchengwen@huawei.com>
> > > Subject: [PATCH] net/hns3: support disable IOVA as PA mode
> 
> Could we change the title to "support IOVA as VA" ?

The underlying problem is the meson configuration option name for this feature [1]:

option('enable_iova_as_pa', type: 'boolean', value: true, description:
       'Support for IOVA as physical address. Disabling removes the buf_iova field of mbuf.')

[1]: https://elixir.bootlin.com/dpdk/v22.11.1/source/meson_options.txt#L43

Formally, the patch provides the ability to set a boolean configuration value ("enable_iova_as_pa") to false, and thus the patch title is correct.

Nonetheless, I agree that the title suggested by Thomas is an improvement.


Going back to the root cause, I think the configuration option should be an enum instead of a boolean, e.g. "iova_mode" with values "iova_pa" and "iova_va".

It's somewhat similar to CPU endian macros. We have macros defining both Big Endian and Little Endian, not just one macro defining Big Endian or not.

@Bruce, would it be hard for you to change the IOVA configuration option from a boolean to a two-value enum?

Or - also considering the resulting #define's - would it be too difficult to keep a sufficient level of backwards/API compatibility?

Re: [PATCH] net/hns3: support disable IOVA as PA mode

[Thomas Monjalon]

20/02/2023 10:43, Morten Brørup:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Monday, 20 February 2023 08.45
> > 
> > 16/02/2023 09:36, Ruifeng Wang:
> > > From: Chengwen Feng <fengchengwen@huawei.com>
> > > > Subject: [PATCH] net/hns3: support disable IOVA as PA mode
> > 
> > Could we change the title to "support IOVA as VA" ?
> 
> The underlying problem is the meson configuration option name for this feature [1]:
> 
> option('enable_iova_as_pa', type: 'boolean', value: true, description:
>        'Support for IOVA as physical address. Disabling removes the buf_iova field of mbuf.')
> 
> [1]: https://elixir.bootlin.com/dpdk/v22.11.1/source/meson_options.txt#L43
> 
> Formally, the patch provides the ability to set a boolean configuration value ("enable_iova_as_pa") to false, and thus the patch title is correct.
> 
> Nonetheless, I agree that the title suggested by Thomas is an improvement.
> 
> 
> Going back to the root cause, I think the configuration option should be an enum instead of a boolean, e.g. "iova_mode" with values "iova_pa" and "iova_va".

We can enable both and have it decided at runtime. So I think the boolean is OK.

> It's somewhat similar to CPU endian macros. We have macros defining both Big Endian and Little Endian, not just one macro defining Big Endian or not.
> 
> @Bruce, would it be hard for you to change the IOVA configuration option from a boolean to a two-value enum?
> 
> Or - also considering the resulting #define's - would it be too difficult to keep a sufficient level of backwards/API compatibility?

RE: [PATCH] net/hns3: support disable IOVA as PA mode

[Morten Brørup]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, 20 February 2023 11.17
> 
> 20/02/2023 10:43, Morten Brørup:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Monday, 20 February 2023 08.45
> > >
> > > 16/02/2023 09:36, Ruifeng Wang:
> > > > From: Chengwen Feng <fengchengwen@huawei.com>
> > > > > Subject: [PATCH] net/hns3: support disable IOVA as PA mode
> > >
> > > Could we change the title to "support IOVA as VA" ?
> >
> > The underlying problem is the meson configuration option name for
> this feature [1]:
> >
> > option('enable_iova_as_pa', type: 'boolean', value: true,
> description:
> >        'Support for IOVA as physical address. Disabling removes the
> buf_iova field of mbuf.')
> >
> > [1]:
> https://elixir.bootlin.com/dpdk/v22.11.1/source/meson_options.txt#L43
> >
> > Formally, the patch provides the ability to set a boolean
> configuration value ("enable_iova_as_pa") to false, and thus the patch
> title is correct.
> >
> > Nonetheless, I agree that the title suggested by Thomas is an
> improvement.
> >
> >
> > Going back to the root cause, I think the configuration option should
> be an enum instead of a boolean, e.g. "iova_mode" with values "iova_pa"
> and "iova_va".
> 
> We can enable both and have it decided at runtime. So I think the
> boolean is OK.

I forgot that it could be changed at runtime.

I'll share a few thoughts for consideration, but expect no further replies. Sorry about the noise. ;-)

The documentation [2] says that IOVA as PA is always supported, and is the default mode. Support for IOVA as VA is optional.

[2]: https://www.intel.com/content/www/us/en/developer/articles/technical/memory-in-dpdk-part-2-deep-dive-into-iova.html

IOVA as VA can be selected at runtime, as you mention, or at build time. But selecting IOVA as VA (at runtime or build time) requires support by the underlying environment/hardware.

If IOVA as PA is always supported (and is the default), the name of this meson configuration option could be improved. Its current name says "enable feature X", but if feature X is already supported by default, the name seems meaningless. If we want to keep it boolean, it could be inverted, e.g.: "iova_as_va_only" with default value "false".

However, if modifying the meson configuration option (name and/or type) doesn't reduce the risk of confusion with the various IOVA modes, it's not worth the effort.

> 
> > It's somewhat similar to CPU endian macros. We have macros defining
> both Big Endian and Little Endian, not just one macro defining Big
> Endian or not.
> >
> > @Bruce, would it be hard for you to change the IOVA configuration
> option from a boolean to a two-value enum?
> >
> > Or - also considering the resulting #define's - would it be too
> difficult to keep a sufficient level of backwards/API compatibility?
> 
> 
> 

Re: [PATCH] net/hns3: support disable IOVA as PA mode

[Bruce Richardson]

On Mon, Feb 20, 2023 at 12:12:50PM +0100, Morten Brørup wrote:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Monday, 20 February 2023 11.17
> > 
> > 20/02/2023 10:43, Morten Brørup:
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > Sent: Monday, 20 February 2023 08.45
> > > >
> > > > 16/02/2023 09:36, Ruifeng Wang:
> > > > > From: Chengwen Feng <fengchengwen@huawei.com>
> > > > > > Subject: [PATCH] net/hns3: support disable IOVA as PA mode
> > > >
> > > > Could we change the title to "support IOVA as VA" ?
> > >
> > > The underlying problem is the meson configuration option name for
> > this feature [1]:
> > >
> > > option('enable_iova_as_pa', type: 'boolean', value: true,
> > description:
> > >        'Support for IOVA as physical address. Disabling removes the
> > buf_iova field of mbuf.')
> > >
> > > [1]:
> > https://elixir.bootlin.com/dpdk/v22.11.1/source/meson_options.txt#L43
> > >
> > > Formally, the patch provides the ability to set a boolean
> > configuration value ("enable_iova_as_pa") to false, and thus the patch
> > title is correct.
> > >
> > > Nonetheless, I agree that the title suggested by Thomas is an
> > improvement.
> > >
> > >
> > > Going back to the root cause, I think the configuration option should
> > be an enum instead of a boolean, e.g. "iova_mode" with values "iova_pa"
> > and "iova_va".
> > 
> > We can enable both and have it decided at runtime. So I think the
> > boolean is OK.
> 
> I forgot that it could be changed at runtime.
> 
> I'll share a few thoughts for consideration, but expect no further replies. Sorry about the noise. ;-)
> 
> The documentation [2] says that IOVA as PA is always supported, and is the default mode. Support for IOVA as VA is optional.
> 
> [2]: https://www.intel.com/content/www/us/en/developer/articles/technical/memory-in-dpdk-part-2-deep-dive-into-iova.html
> 
> IOVA as VA can be selected at runtime, as you mention, or at build time. But selecting IOVA as VA (at runtime or build time) requires support by the underlying environment/hardware.
> 
> If IOVA as PA is always supported (and is the default), the name of this meson configuration option could be improved. Its current name says "enable feature X", but if feature X is already supported by default, the name seems meaningless. If we want to keep it boolean, it could be inverted, e.g.: "iova_as_va_only" with default value "false".
> 
> However, if modifying the meson configuration option (name and/or type) doesn't reduce the risk of confusion with the various IOVA modes, it's not worth the effort.
> 
I agree that this option is confusing, and thinking about it, I agree that
a pair of named option is probably better than just a true/false booleans.
My current thinking is that a combo option is best - maybe named:
"supported_iova_modes", with possible values ["va_and_pa", "va_only"] may
be clearest. However, that would be a change in how things are currently
configured.

A alternative if we want to keep compatibility, is to expand or clarify the
help text for the existing "enable_iova_as_pa" option. The current help
text reads:

"Support for IOVA as physical address. Disabling removes the buf_iova field
of mbuf."

We could expand that to e.g.:

"Support the use of physical addresses for IO addresses, such as used by
VFIO in no-iommu mode, or UIO-based drivers. When disabled, DPDK can only
run with IOMMU support for address mappings, but will have more space
available in the mbuf structure".

Such an explanation is quite a bit longer, but I see meson does a decent
job of wrapping the output of "meson configure" in latest versions.

/Bruce

RE: [PATCH] net/hns3: support disable IOVA as PA mode

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 20 February 2023 12.53
> 
> On Mon, Feb 20, 2023 at 12:12:50PM +0100, Morten Brørup wrote:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Monday, 20 February 2023 11.17
> > >
> > > 20/02/2023 10:43, Morten Brørup:
> > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > Sent: Monday, 20 February 2023 08.45
> > > > >
> > > > > 16/02/2023 09:36, Ruifeng Wang:
> > > > > > From: Chengwen Feng <fengchengwen@huawei.com>
> > > > > > > Subject: [PATCH] net/hns3: support disable IOVA as PA mode
> > > > >
> > > > > Could we change the title to "support IOVA as VA" ?
> > > >
> > > > The underlying problem is the meson configuration option name for
> > > this feature [1]:
> > > >
> > > > option('enable_iova_as_pa', type: 'boolean', value: true,
> > > description:
> > > >        'Support for IOVA as physical address. Disabling removes
> the
> > > buf_iova field of mbuf.')
> > > >
> > > > [1]:
> > >
> https://elixir.bootlin.com/dpdk/v22.11.1/source/meson_options.txt#L43
> > > >
> > > > Formally, the patch provides the ability to set a boolean
> > > configuration value ("enable_iova_as_pa") to false, and thus the
> patch
> > > title is correct.
> > > >
> > > > Nonetheless, I agree that the title suggested by Thomas is an
> > > improvement.
> > > >
> > > >
> > > > Going back to the root cause, I think the configuration option
> should
> > > be an enum instead of a boolean, e.g. "iova_mode" with values
> "iova_pa"
> > > and "iova_va".
> > >
> > > We can enable both and have it decided at runtime. So I think the
> > > boolean is OK.
> >
> > I forgot that it could be changed at runtime.
> >
> > I'll share a few thoughts for consideration, but expect no further
> replies. Sorry about the noise. ;-)
> >
> > The documentation [2] says that IOVA as PA is always supported, and
> is the default mode. Support for IOVA as VA is optional.
> >
> > [2]:
> https://www.intel.com/content/www/us/en/developer/articles/technical/me
> mory-in-dpdk-part-2-deep-dive-into-iova.html
> >
> > IOVA as VA can be selected at runtime, as you mention, or at build
> time. But selecting IOVA as VA (at runtime or build time) requires
> support by the underlying environment/hardware.
> >
> > If IOVA as PA is always supported (and is the default), the name of
> this meson configuration option could be improved. Its current name
> says "enable feature X", but if feature X is already supported by
> default, the name seems meaningless. If we want to keep it boolean, it
> could be inverted, e.g.: "iova_as_va_only" with default value "false".
> >
> > However, if modifying the meson configuration option (name and/or
> type) doesn't reduce the risk of confusion with the various IOVA modes,
> it's not worth the effort.
> >
> I agree that this option is confusing, and thinking about it, I agree
> that
> a pair of named option is probably better than just a true/false
> booleans.
> My current thinking is that a combo option is best - maybe named:
> "supported_iova_modes", with possible values ["va_and_pa", "va_only"]
> may
> be clearest. However, that would be a change in how things are
> currently
> configured.
> 
> A alternative if we want to keep compatibility, is to expand or clarify
> the
> help text for the existing "enable_iova_as_pa" option. The current help
> text reads:
> 
> "Support for IOVA as physical address. Disabling removes the buf_iova
> field
> of mbuf."
> 
> We could expand that to e.g.:
> 
> "Support the use of physical addresses for IO addresses, such as used
> by
> VFIO in no-iommu mode, or UIO-based drivers. When disabled, DPDK can
> only
> run with IOMMU support for address mappings, but will have more space
> available in the mbuf structure".
> 
> Such an explanation is quite a bit longer, but I see meson does a
> decent
> job of wrapping the output of "meson configure" in latest versions.
> 
> /Bruce

Updating the description of the meson configuration option would be an improvement.

But I'm thinking more about the ripple effect into the resulting #define's, and the code using those. It would be nice getting this cleaned up. Which is why I compare the IOVA mode to CPU Endianness, as an example of a Boolean value represented by multiple #define's for code readability purposes.

But I suppose such a change has too widely reaching side effects, regarding backwards compatibility.

Re: [PATCH] net/hns3: support disable IOVA as PA mode

[Bruce Richardson]

On Mon, Feb 20, 2023 at 01:04:02PM +0100, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Monday, 20 February 2023 12.53
> > 
> > On Mon, Feb 20, 2023 at 12:12:50PM +0100, Morten Brørup wrote:
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > Sent: Monday, 20 February 2023 11.17
> > > >
> > > > 20/02/2023 10:43, Morten Brørup:
> > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > Sent: Monday, 20 February 2023 08.45
> > > > > >
> > > > > > 16/02/2023 09:36, Ruifeng Wang:
> > > > > > > From: Chengwen Feng <fengchengwen@huawei.com>
> > > > > > > > Subject: [PATCH] net/hns3: support disable IOVA as PA mode
> > > > > >
> > > > > > Could we change the title to "support IOVA as VA" ?
> > > > >
> > > > > The underlying problem is the meson configuration option name for
> > > > this feature [1]:
> > > > >
> > > > > option('enable_iova_as_pa', type: 'boolean', value: true,
> > > > description:
> > > > >        'Support for IOVA as physical address. Disabling removes
> > the
> > > > buf_iova field of mbuf.')
> > > > >
> > > > > [1]:
> > > >
> > https://elixir.bootlin.com/dpdk/v22.11.1/source/meson_options.txt#L43
> > > > >
> > > > > Formally, the patch provides the ability to set a boolean
> > > > configuration value ("enable_iova_as_pa") to false, and thus the
> > patch
> > > > title is correct.
> > > > >
> > > > > Nonetheless, I agree that the title suggested by Thomas is an
> > > > improvement.
> > > > >
> > > > >
> > > > > Going back to the root cause, I think the configuration option
> > should
> > > > be an enum instead of a boolean, e.g. "iova_mode" with values
> > "iova_pa"
> > > > and "iova_va".
> > > >
> > > > We can enable both and have it decided at runtime. So I think the
> > > > boolean is OK.
> > >
> > > I forgot that it could be changed at runtime.
> > >
> > > I'll share a few thoughts for consideration, but expect no further
> > replies. Sorry about the noise. ;-)
> > >
> > > The documentation [2] says that IOVA as PA is always supported, and
> > is the default mode. Support for IOVA as VA is optional.
> > >
> > > [2]:
> > https://www.intel.com/content/www/us/en/developer/articles/technical/me
> > mory-in-dpdk-part-2-deep-dive-into-iova.html
> > >
> > > IOVA as VA can be selected at runtime, as you mention, or at build
> > time. But selecting IOVA as VA (at runtime or build time) requires
> > support by the underlying environment/hardware.
> > >
> > > If IOVA as PA is always supported (and is the default), the name of
> > this meson configuration option could be improved. Its current name
> > says "enable feature X", but if feature X is already supported by
> > default, the name seems meaningless. If we want to keep it boolean, it
> > could be inverted, e.g.: "iova_as_va_only" with default value "false".
> > >
> > > However, if modifying the meson configuration option (name and/or
> > type) doesn't reduce the risk of confusion with the various IOVA modes,
> > it's not worth the effort.
> > >
> > I agree that this option is confusing, and thinking about it, I agree
> > that
> > a pair of named option is probably better than just a true/false
> > booleans.
> > My current thinking is that a combo option is best - maybe named:
> > "supported_iova_modes", with possible values ["va_and_pa", "va_only"]
> > may
> > be clearest. However, that would be a change in how things are
> > currently
> > configured.
> > 
> > A alternative if we want to keep compatibility, is to expand or clarify
> > the
> > help text for the existing "enable_iova_as_pa" option. The current help
> > text reads:
> > 
> > "Support for IOVA as physical address. Disabling removes the buf_iova
> > field
> > of mbuf."
> > 
> > We could expand that to e.g.:
> > 
> > "Support the use of physical addresses for IO addresses, such as used
> > by
> > VFIO in no-iommu mode, or UIO-based drivers. When disabled, DPDK can
> > only
> > run with IOMMU support for address mappings, but will have more space
> > available in the mbuf structure".
> > 
> > Such an explanation is quite a bit longer, but I see meson does a
> > decent
> > job of wrapping the output of "meson configure" in latest versions.
> > 
> > /Bruce
> 
> Updating the description of the meson configuration option would be an improvement.
> 
> But I'm thinking more about the ripple effect into the resulting #define's, and the code using those. It would be nice getting this cleaned up. Which is why I compare the IOVA mode to CPU Endianness, as an example of a Boolean value represented by multiple #define's for code readability purposes.
> 
> But I suppose such a change has too widely reaching side effects, regarding backwards compatibility.

Actually, I think the current internal define is pretty ok right now.
RTE_IOVA_AS_PA would probably be better as "RTE_SUPPORT_IOVA_AS_PA", but I
don't think the lack of the alternative value "RTE_SUPPORT_IOVA_AS_VA" is
an issue since a DPDK build always supports that - it's only at runtime it
may not be supported e.g. if no IOMMU is present.

/Bruce

RE: [PATCH] net/hns3: support disable IOVA as PA mode

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> 
> On Mon, Feb 20, 2023 at 01:04:02PM +0100, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Monday, 20 February 2023 12.53
> > >
> > > On Mon, Feb 20, 2023 at 12:12:50PM +0100, Morten Brørup wrote:
> > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > Sent: Monday, 20 February 2023 11.17
> > > > >
> > > > > 20/02/2023 10:43, Morten Brørup:
> > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > > Sent: Monday, 20 February 2023 08.45
> > > > > > >
> > > > > > > 16/02/2023 09:36, Ruifeng Wang:
> > > > > > > > From: Chengwen Feng <fengchengwen@huawei.com>
> > > > > > > > > Subject: [PATCH] net/hns3: support disable IOVA as PA
> mode
> > > > > > >
> > > > > > > Could we change the title to "support IOVA as VA" ?
> > > > > >
> > > > > > The underlying problem is the meson configuration option name
> for
> > > > > this feature [1]:
> > > > > >
> > > > > > option('enable_iova_as_pa', type: 'boolean', value: true,
> > > > > description:
> > > > > >        'Support for IOVA as physical address. Disabling
> removes
> > > the
> > > > > buf_iova field of mbuf.')
> > > > > >
> > > > > > [1]:
> > > > >
> > >
> https://elixir.bootlin.com/dpdk/v22.11.1/source/meson_options.txt#L43
> > > > > >
> > > > > > Formally, the patch provides the ability to set a boolean
> > > > > configuration value ("enable_iova_as_pa") to false, and thus
> the
> > > patch
> > > > > title is correct.
> > > > > >
> > > > > > Nonetheless, I agree that the title suggested by Thomas is an
> > > > > improvement.
> > > > > >
> > > > > >
> > > > > > Going back to the root cause, I think the configuration
> option
> > > should
> > > > > be an enum instead of a boolean, e.g. "iova_mode" with values
> > > "iova_pa"
> > > > > and "iova_va".
> > > > >
> > > > > We can enable both and have it decided at runtime. So I think
> the
> > > > > boolean is OK.
> > > >
> > > > I forgot that it could be changed at runtime.
> > > >
> > > > I'll share a few thoughts for consideration, but expect no
> further
> > > replies. Sorry about the noise. ;-)
> > > >
> > > > The documentation [2] says that IOVA as PA is always supported,
> and
> > > is the default mode. Support for IOVA as VA is optional.
> > > >
> > > > [2]:
> > >
> https://www.intel.com/content/www/us/en/developer/articles/technical/me
> > > mory-in-dpdk-part-2-deep-dive-into-iova.html
> > > >
> > > > IOVA as VA can be selected at runtime, as you mention, or at
> build
> > > time. But selecting IOVA as VA (at runtime or build time) requires
> > > support by the underlying environment/hardware.
> > > >
> > > > If IOVA as PA is always supported (and is the default), the name
> of
> > > this meson configuration option could be improved. Its current name
> > > says "enable feature X", but if feature X is already supported by
> > > default, the name seems meaningless. If we want to keep it boolean,
> it
> > > could be inverted, e.g.: "iova_as_va_only" with default value
> "false".
> > > >
> > > > However, if modifying the meson configuration option (name and/or
> > > type) doesn't reduce the risk of confusion with the various IOVA
> modes,
> > > it's not worth the effort.
> > > >
> > > I agree that this option is confusing, and thinking about it, I
> agree
> > > that
> > > a pair of named option is probably better than just a true/false
> > > booleans.
> > > My current thinking is that a combo option is best - maybe named:
> > > "supported_iova_modes", with possible values ["va_and_pa",
> "va_only"]
> > > may
> > > be clearest. However, that would be a change in how things are
> > > currently
> > > configured.
> > >
> > > A alternative if we want to keep compatibility, is to expand or
> clarify
> > > the
> > > help text for the existing "enable_iova_as_pa" option. The current
> help
> > > text reads:
> > >
> > > "Support for IOVA as physical address. Disabling removes the
> buf_iova
> > > field
> > > of mbuf."
> > >
> > > We could expand that to e.g.:
> > >
> > > "Support the use of physical addresses for IO addresses, such as
> used
> > > by
> > > VFIO in no-iommu mode, or UIO-based drivers. When disabled, DPDK
> can
> > > only
> > > run with IOMMU support for address mappings, but will have more
> space
> > > available in the mbuf structure".
> > >
> > > Such an explanation is quite a bit longer, but I see meson does a
> > > decent
> > > job of wrapping the output of "meson configure" in latest versions.
> > >
> > > /Bruce
> >
> > Updating the description of the meson configuration option would be
> an improvement.
> >
> > But I'm thinking more about the ripple effect into the resulting
> #define's, and the code using those. It would be nice getting this
> cleaned up. Which is why I compare the IOVA mode to CPU Endianness, as
> an example of a Boolean value represented by multiple #define's for
> code readability purposes.
> >
> > But I suppose such a change has too widely reaching side effects,
> regarding backwards compatibility.
> 
> Actually, I think the current internal define is pretty ok right now.
> RTE_IOVA_AS_PA would probably be better as "RTE_SUPPORT_IOVA_AS_PA",
> but I
> don't think the lack of the alternative value "RTE_SUPPORT_IOVA_AS_VA"
> is
> an issue since a DPDK build always supports that - it's only at runtime
> it
> may not be supported e.g. if no IOMMU is present.

It's not that simple. There's a difference between runtime IOVA as VA mode and build time IOVA as VA mode.

E.g. this hns3 patch adds support for build time IOVA_AS_VA mode.

Re: [PATCH] net/hns3: support disable IOVA as PA mode

[Bruce Richardson]

On Mon, Feb 20, 2023 at 01:47:13PM +0100, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > 
> > On Mon, Feb 20, 2023 at 01:04:02PM +0100, Morten Brørup wrote:
> > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > Sent: Monday, 20 February 2023 12.53
> > > >
> > > > On Mon, Feb 20, 2023 at 12:12:50PM +0100, Morten Brørup wrote:
> > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > Sent: Monday, 20 February 2023 11.17
> > > > > >
> > > > > > 20/02/2023 10:43, Morten Brørup:
> > > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > > > Sent: Monday, 20 February 2023 08.45
> > > > > > > >
> > > > > > > > 16/02/2023 09:36, Ruifeng Wang:
> > > > > > > > > From: Chengwen Feng <fengchengwen@huawei.com>
> > > > > > > > > > Subject: [PATCH] net/hns3: support disable IOVA as PA
> > mode
> > > > > > > >
> > > > > > > > Could we change the title to "support IOVA as VA" ?
> > > > > > >
> > > > > > > The underlying problem is the meson configuration option name
> > for
> > > > > > this feature [1]:
> > > > > > >
> > > > > > > option('enable_iova_as_pa', type: 'boolean', value: true,
> > > > > > description:
> > > > > > >        'Support for IOVA as physical address. Disabling
> > removes
> > > > the
> > > > > > buf_iova field of mbuf.')
> > > > > > >
> > > > > > > [1]:
> > > > > >
> > > >
> > https://elixir.bootlin.com/dpdk/v22.11.1/source/meson_options.txt#L43
> > > > > > >
> > > > > > > Formally, the patch provides the ability to set a boolean
> > > > > > configuration value ("enable_iova_as_pa") to false, and thus
> > the
> > > > patch
> > > > > > title is correct.
> > > > > > >
> > > > > > > Nonetheless, I agree that the title suggested by Thomas is an
> > > > > > improvement.
> > > > > > >
> > > > > > >
> > > > > > > Going back to the root cause, I think the configuration
> > option
> > > > should
> > > > > > be an enum instead of a boolean, e.g. "iova_mode" with values
> > > > "iova_pa"
> > > > > > and "iova_va".
> > > > > >
> > > > > > We can enable both and have it decided at runtime. So I think
> > the
> > > > > > boolean is OK.
> > > > >
> > > > > I forgot that it could be changed at runtime.
> > > > >
> > > > > I'll share a few thoughts for consideration, but expect no
> > further
> > > > replies. Sorry about the noise. ;-)
> > > > >
> > > > > The documentation [2] says that IOVA as PA is always supported,
> > and
> > > > is the default mode. Support for IOVA as VA is optional.
> > > > >
> > > > > [2]:
> > > >
> > https://www.intel.com/content/www/us/en/developer/articles/technical/me
> > > > mory-in-dpdk-part-2-deep-dive-into-iova.html
> > > > >
> > > > > IOVA as VA can be selected at runtime, as you mention, or at
> > build
> > > > time. But selecting IOVA as VA (at runtime or build time) requires
> > > > support by the underlying environment/hardware.
> > > > >
> > > > > If IOVA as PA is always supported (and is the default), the name
> > of
> > > > this meson configuration option could be improved. Its current name
> > > > says "enable feature X", but if feature X is already supported by
> > > > default, the name seems meaningless. If we want to keep it boolean,
> > it
> > > > could be inverted, e.g.: "iova_as_va_only" with default value
> > "false".
> > > > >
> > > > > However, if modifying the meson configuration option (name and/or
> > > > type) doesn't reduce the risk of confusion with the various IOVA
> > modes,
> > > > it's not worth the effort.
> > > > >
> > > > I agree that this option is confusing, and thinking about it, I
> > agree
> > > > that
> > > > a pair of named option is probably better than just a true/false
> > > > booleans.
> > > > My current thinking is that a combo option is best - maybe named:
> > > > "supported_iova_modes", with possible values ["va_and_pa",
> > "va_only"]
> > > > may
> > > > be clearest. However, that would be a change in how things are
> > > > currently
> > > > configured.
> > > >
> > > > A alternative if we want to keep compatibility, is to expand or
> > clarify
> > > > the
> > > > help text for the existing "enable_iova_as_pa" option. The current
> > help
> > > > text reads:
> > > >
> > > > "Support for IOVA as physical address. Disabling removes the
> > buf_iova
> > > > field
> > > > of mbuf."
> > > >
> > > > We could expand that to e.g.:
> > > >
> > > > "Support the use of physical addresses for IO addresses, such as
> > used
> > > > by
> > > > VFIO in no-iommu mode, or UIO-based drivers. When disabled, DPDK
> > can
> > > > only
> > > > run with IOMMU support for address mappings, but will have more
> > space
> > > > available in the mbuf structure".
> > > >
> > > > Such an explanation is quite a bit longer, but I see meson does a
> > > > decent
> > > > job of wrapping the output of "meson configure" in latest versions.
> > > >
> > > > /Bruce
> > >
> > > Updating the description of the meson configuration option would be
> > an improvement.
> > >
> > > But I'm thinking more about the ripple effect into the resulting
> > #define's, and the code using those. It would be nice getting this
> > cleaned up. Which is why I compare the IOVA mode to CPU Endianness, as
> > an example of a Boolean value represented by multiple #define's for
> > code readability purposes.
> > >
> > > But I suppose such a change has too widely reaching side effects,
> > regarding backwards compatibility.
> > 
> > Actually, I think the current internal define is pretty ok right now.
> > RTE_IOVA_AS_PA would probably be better as "RTE_SUPPORT_IOVA_AS_PA",
> > but I
> > don't think the lack of the alternative value "RTE_SUPPORT_IOVA_AS_VA"
> > is
> > an issue since a DPDK build always supports that - it's only at runtime
> > it
> > may not be supported e.g. if no IOMMU is present.
> 
> It's not that simple. There's a difference between runtime IOVA as VA mode and build time IOVA as VA mode.
> 
> E.g. this hns3 patch adds support for build time IOVA_AS_VA mode.
>
Ok, that is another way of looking at it, though to me I still view that
mode as dropping support for PA rather than doing anything with VA
mode. However, no problem putting in an extra build-time define for
RTE_IOVA_AS_VA_ONLY or similar.

/Bruce

RE: [PATCH] net/hns3: support disable IOVA as PA mode

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 20 February 2023 15.13
> 
> On Mon, Feb 20, 2023 at 01:47:13PM +0100, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > >
> > > On Mon, Feb 20, 2023 at 01:04:02PM +0100, Morten Brørup wrote:
> > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > > Sent: Monday, 20 February 2023 12.53
> > > > >
> > > > > On Mon, Feb 20, 2023 at 12:12:50PM +0100, Morten Brørup wrote:
> > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > > Sent: Monday, 20 February 2023 11.17
> > > > > > >
> > > > > > > 20/02/2023 10:43, Morten Brørup:
> > > > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > > > > Sent: Monday, 20 February 2023 08.45
> > > > > > > > >
> > > > > > > > > 16/02/2023 09:36, Ruifeng Wang:
> > > > > > > > > > From: Chengwen Feng <fengchengwen@huawei.com>
> > > > > > > > > > > Subject: [PATCH] net/hns3: support disable IOVA as
> PA
> > > mode
> > > > > > > > >
> > > > > > > > > Could we change the title to "support IOVA as VA" ?
> > > > > > > >
> > > > > > > > The underlying problem is the meson configuration option
> name
> > > for
> > > > > > > this feature [1]:
> > > > > > > >
> > > > > > > > option('enable_iova_as_pa', type: 'boolean', value: true,
> > > > > > > description:
> > > > > > > >        'Support for IOVA as physical address. Disabling
> > > removes
> > > > > the
> > > > > > > buf_iova field of mbuf.')
> > > > > > > >
> > > > > > > > [1]:
> > > > > > >
> > > > >
> > >
> https://elixir.bootlin.com/dpdk/v22.11.1/source/meson_options.txt#L43
> > > > > > > >
> > > > > > > > Formally, the patch provides the ability to set a boolean
> > > > > > > configuration value ("enable_iova_as_pa") to false, and
> thus
> > > the
> > > > > patch
> > > > > > > title is correct.
> > > > > > > >
> > > > > > > > Nonetheless, I agree that the title suggested by Thomas
> is an
> > > > > > > improvement.
> > > > > > > >
> > > > > > > >
> > > > > > > > Going back to the root cause, I think the configuration
> > > option
> > > > > should
> > > > > > > be an enum instead of a boolean, e.g. "iova_mode" with
> values
> > > > > "iova_pa"
> > > > > > > and "iova_va".
> > > > > > >
> > > > > > > We can enable both and have it decided at runtime. So I
> think
> > > the
> > > > > > > boolean is OK.
> > > > > >
> > > > > > I forgot that it could be changed at runtime.
> > > > > >
> > > > > > I'll share a few thoughts for consideration, but expect no
> > > further
> > > > > replies. Sorry about the noise. ;-)
> > > > > >
> > > > > > The documentation [2] says that IOVA as PA is always
> supported,
> > > and
> > > > > is the default mode. Support for IOVA as VA is optional.
> > > > > >
> > > > > > [2]:
> > > > >
> > >
> https://www.intel.com/content/www/us/en/developer/articles/technical/me
> > > > > mory-in-dpdk-part-2-deep-dive-into-iova.html
> > > > > >
> > > > > > IOVA as VA can be selected at runtime, as you mention, or at
> > > build
> > > > > time. But selecting IOVA as VA (at runtime or build time)
> requires
> > > > > support by the underlying environment/hardware.
> > > > > >
> > > > > > If IOVA as PA is always supported (and is the default), the
> name
> > > of
> > > > > this meson configuration option could be improved. Its current
> name
> > > > > says "enable feature X", but if feature X is already supported
> by
> > > > > default, the name seems meaningless. If we want to keep it
> boolean,
> > > it
> > > > > could be inverted, e.g.: "iova_as_va_only" with default value
> > > "false".
> > > > > >
> > > > > > However, if modifying the meson configuration option (name
> and/or
> > > > > type) doesn't reduce the risk of confusion with the various
> IOVA
> > > modes,
> > > > > it's not worth the effort.
> > > > > >
> > > > > I agree that this option is confusing, and thinking about it, I
> > > agree
> > > > > that
> > > > > a pair of named option is probably better than just a
> true/false
> > > > > booleans.
> > > > > My current thinking is that a combo option is best - maybe
> named:
> > > > > "supported_iova_modes", with possible values ["va_and_pa",
> > > "va_only"]
> > > > > may
> > > > > be clearest. However, that would be a change in how things are
> > > > > currently
> > > > > configured.
> > > > >
> > > > > A alternative if we want to keep compatibility, is to expand or
> > > clarify
> > > > > the
> > > > > help text for the existing "enable_iova_as_pa" option. The
> current
> > > help
> > > > > text reads:
> > > > >
> > > > > "Support for IOVA as physical address. Disabling removes the
> > > buf_iova
> > > > > field
> > > > > of mbuf."
> > > > >
> > > > > We could expand that to e.g.:
> > > > >
> > > > > "Support the use of physical addresses for IO addresses, such
> as
> > > used
> > > > > by
> > > > > VFIO in no-iommu mode, or UIO-based drivers. When disabled,
> DPDK
> > > can
> > > > > only
> > > > > run with IOMMU support for address mappings, but will have more
> > > space
> > > > > available in the mbuf structure".
> > > > >
> > > > > Such an explanation is quite a bit longer, but I see meson does
> a
> > > > > decent
> > > > > job of wrapping the output of "meson configure" in latest
> versions.
> > > > >
> > > > > /Bruce
> > > >
> > > > Updating the description of the meson configuration option would
> be
> > > an improvement.
> > > >
> > > > But I'm thinking more about the ripple effect into the resulting
> > > #define's, and the code using those. It would be nice getting this
> > > cleaned up. Which is why I compare the IOVA mode to CPU Endianness,
> as
> > > an example of a Boolean value represented by multiple #define's for
> > > code readability purposes.
> > > >
> > > > But I suppose such a change has too widely reaching side effects,
> > > regarding backwards compatibility.
> > >
> > > Actually, I think the current internal define is pretty ok right
> now.
> > > RTE_IOVA_AS_PA would probably be better as
> "RTE_SUPPORT_IOVA_AS_PA",
> > > but I
> > > don't think the lack of the alternative value
> "RTE_SUPPORT_IOVA_AS_VA"
> > > is
> > > an issue since a DPDK build always supports that - it's only at
> runtime
> > > it
> > > may not be supported e.g. if no IOMMU is present.
> >
> > It's not that simple. There's a difference between runtime IOVA as VA
> mode and build time IOVA as VA mode.
> >
> > E.g. this hns3 patch adds support for build time IOVA_AS_VA mode.
> >
> Ok, that is another way of looking at it, though to me I still view
> that
> mode as dropping support for PA rather than doing anything with VA
> mode.

It seems I have just been looking at it the wrong way, and needed to shake my head.

> However, no problem putting in an extra build-time define for
> RTE_IOVA_AS_VA_ONLY or similar.

With the new viewing angle, the current define RTE_IOVA_AS_PA makes more sense to me now than before. So we should probably stick with it, rather than introduce something that might confuse developers who already have the same viewing angle.

But it still seems counterintuitive to me that disabling some feature ("enable_iova_as_pa") is not supported throughout DPDK; the logic seems inverted. Apparently, it also makes it difficult to assign good titles to patches that support disabling such a feature. :-)

<irony>
On the positive side, since everything supports this "enable_iova_as_pa" feature, we don't need to add it to the PMD feature list. If the logic wasn't inverted like this, the PMD feature list should probably reflect which PMDs supported the "iova_as_va_only" compile time option. ;-)
</irony>

Re: [PATCH] net/hns3: support disable IOVA as PA mode

[Thomas Monjalon]

20/02/2023 16:07, Morten Brørup:
> With the new viewing angle, the current define RTE_IOVA_AS_PA makes more sense to me now than before. So we should probably stick with it, rather than introduce something that might confuse developers who already have the same viewing angle.
> 
> But it still seems counterintuitive to me that disabling some feature ("enable_iova_as_pa") is not supported throughout DPDK; the logic seems inverted. Apparently, it also makes it difficult to assign good titles to patches that support disabling such a feature. :-)
> 
> <irony>
> On the positive side, since everything supports this "enable_iova_as_pa" feature, we don't need to add it to the PMD feature list. If the logic wasn't inverted like this, the PMD feature list should probably reflect which PMDs supported the "iova_as_va_only" compile time option. ;-)
> </irony>

That's a change I would like to do:
The Meson variable in the drivers should be "support_iova_as_va"
and would mean we can compile the driver when "enable_iova_as_pa" is false.

Re: [PATCH] net/hns3: support disable IOVA as PA mode

[Bruce Richardson]

On Mon, Feb 20, 2023 at 04:30:20PM +0100, Thomas Monjalon wrote:
> 20/02/2023 16:07, Morten Brørup:
> > With the new viewing angle, the current define RTE_IOVA_AS_PA makes more sense to me now than before. So we should probably stick with it, rather than introduce something that might confuse developers who already have the same viewing angle.
> > 
> > But it still seems counterintuitive to me that disabling some feature ("enable_iova_as_pa") is not supported throughout DPDK; the logic seems inverted. Apparently, it also makes it difficult to assign good titles to patches that support disabling such a feature. :-)
> > 
> > <irony>
> > On the positive side, since everything supports this "enable_iova_as_pa" feature, we don't need to add it to the PMD feature list. If the logic wasn't inverted like this, the PMD feature list should probably reflect which PMDs supported the "iova_as_va_only" compile time option. ;-)
> > </irony>
> 
> That's a change I would like to do:
> The Meson variable in the drivers should be "support_iova_as_va"
> and would mean we can compile the driver when "enable_iova_as_pa" is false.
> 
All drivers (that I am aware of) support iova_as_va. What is missing is
drivers supporting "iova_as_va_only". Any reference to va without the word
"only" on it will be misleading.

A third way of looking at it, is to work with the fact that the reason
drivers require changes to support this "va_only" mode, ro no-pa mode, is
due to the fact that the mbuf no longer tracks iovas and only VAs.
Therefore, we can have a variable called "require_iova_in_mbuf", which
would hopefully cut through this whole va vs pa addition/subtraction mess.
What do you think?

/Bruce

Re: [PATCH] net/hns3: support disable IOVA as PA mode

[Thomas Monjalon]

20/02/2023 16:35, Bruce Richardson:
> On Mon, Feb 20, 2023 at 04:30:20PM +0100, Thomas Monjalon wrote:
> > 20/02/2023 16:07, Morten Brørup:
> > > With the new viewing angle, the current define RTE_IOVA_AS_PA makes more sense to me now than before. So we should probably stick with it, rather than introduce something that might confuse developers who already have the same viewing angle.
> > > 
> > > But it still seems counterintuitive to me that disabling some feature ("enable_iova_as_pa") is not supported throughout DPDK; the logic seems inverted. Apparently, it also makes it difficult to assign good titles to patches that support disabling such a feature. :-)
> > > 
> > > <irony>
> > > On the positive side, since everything supports this "enable_iova_as_pa" feature, we don't need to add it to the PMD feature list. If the logic wasn't inverted like this, the PMD feature list should probably reflect which PMDs supported the "iova_as_va_only" compile time option. ;-)
> > > </irony>
> > 
> > That's a change I would like to do:
> > The Meson variable in the drivers should be "support_iova_as_va"
> > and would mean we can compile the driver when "enable_iova_as_pa" is false.
> > 
> All drivers (that I am aware of) support iova_as_va. What is missing is
> drivers supporting "iova_as_va_only". Any reference to va without the word
> "only" on it will be misleading.
> 
> A third way of looking at it, is to work with the fact that the reason
> drivers require changes to support this "va_only" mode, ro no-pa mode, is
> due to the fact that the mbuf no longer tracks iovas and only VAs.
> Therefore, we can have a variable called "require_iova_in_mbuf", which
> would hopefully cut through this whole va vs pa addition/subtraction mess.
> What do you think?

Yes "require_iova_in_mbuf" describes better the reality,
so it is simpler to understand.

RE: [PATCH] net/hns3: support disable IOVA as PA mode

[Morten Brørup]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, 20 February 2023 16.41
> 
> 20/02/2023 16:35, Bruce Richardson:
> > On Mon, Feb 20, 2023 at 04:30:20PM +0100, Thomas Monjalon wrote:
> > > 20/02/2023 16:07, Morten Brørup:
> > > > With the new viewing angle, the current define RTE_IOVA_AS_PA
> makes more sense to me now than before. So we should probably stick
> with it, rather than introduce something that might confuse developers
> who already have the same viewing angle.
> > > >
> > > > But it still seems counterintuitive to me that disabling some
> feature ("enable_iova_as_pa") is not supported throughout DPDK; the
> logic seems inverted. Apparently, it also makes it difficult to assign
> good titles to patches that support disabling such a feature. :-)
> > > >
> > > > <irony>
> > > > On the positive side, since everything supports this
> "enable_iova_as_pa" feature, we don't need to add it to the PMD feature
> list. If the logic wasn't inverted like this, the PMD feature list
> should probably reflect which PMDs supported the "iova_as_va_only"
> compile time option. ;-)
> > > > </irony>
> > >
> > > That's a change I would like to do:
> > > The Meson variable in the drivers should be "support_iova_as_va"
> > > and would mean we can compile the driver when "enable_iova_as_pa"
> is false.
> > >
> > All drivers (that I am aware of) support iova_as_va. What is missing
> is
> > drivers supporting "iova_as_va_only". Any reference to va without the
> word
> > "only" on it will be misleading.
> >
> > A third way of looking at it, is to work with the fact that the
> reason
> > drivers require changes to support this "va_only" mode, ro no-pa
> mode, is
> > due to the fact that the mbuf no longer tracks iovas and only VAs.
> > Therefore, we can have a variable called "require_iova_in_mbuf",
> which
> > would hopefully cut through this whole va vs pa addition/subtraction
> mess.
> > What do you think?
> 
> Yes "require_iova_in_mbuf" describes better the reality,
> so it is simpler to understand.

Agreed. It is a good idea narrowing the scope to the concrete issue.

Proceeding to also consider the source code using the corresponding #define, e.g. the rte_mbuf:
https://elixir.bootlin.com/dpdk/v23.03-rc1/source/lib/mbuf/rte_mbuf_core.h#L469

Would you also change the corresponding #define from RTE_IOVA_AS_PA to RTE_REQUIRE_IOVA_IN_MBUF?

Perhaps we could invert the value, e.g. "mbuf_without_iova" and #define RTE_MBUF_WITHOUT_IOVA? Intuitively, a default value of "false" better reflects that the feature is optional.

PS: I don't mind if you limit this to meson, and ignore my feature creep expanding into #define land.