Re: [dpdk-dev] [RFC] ethdev: configure SR-IOV VF from host

Re: [dpdk-dev] [RFC] ethdev: configure SR-IOV VF from host

[Jerin Jacob Kollanukkaran]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, August 15, 2019 11:29 PM
> To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; dev@dpdk.org; Bernard Iremonger
> <bernard.iremonger@intel.com>; Shahaf Shuler <shahafs@mellanox.com>;
> E. Scott Daniels <daniels@research.att.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; Alex Zelezniak <alexz@att.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Declan Doherty
> <declan.doherty@intel.com>
> Subject: [EXT] Re: [dpdk-dev] [RFC] ethdev: configure SR-IOV VF from host
> 
> External Email
> 
> ----------------------------------------------------------------------
> 15/08/2019 17:34, Jerin Jacob Kollanukkaran:
> > From: Thomas Monjalon
> > >
> > > In a virtual environment, the network controller may have to
> > > configure some SR-IOV VF parameters for security reasons.
> > >
> > > When the PF (host port) is drived by DPDK (OVS-DPDK case), we face
> > > two different cases:
> > > 	- driver is bifurcated (Mellanox case),
> > > 	so the VF can be configured via the kernel.
> > > 	- driver is on top of UIO or VFIO, so DPDK API is required.
> > >
> > > This RFC proposes to use generic DPDK API for VF configuration.
> > > The impacted functions are (can be extended):
> > >
> > > 	- rte_eth_dev_is_valid_port
> > > 	- rte_eth_promiscuous_enable
> > > 	- rte_eth_promiscuous_disable
> > > 	- rte_eth_promiscuous_get
> > > 	- rte_eth_allmulticast_enable
> > > 	- rte_eth_allmulticast_disable
> > > 	- rte_eth_allmulticast_get
> > > 	- rte_eth_dev_set_mc_addr_list
> > > 	- rte_eth_dev_default_mac_addr_set
> > > 	- rte_eth_macaddr_get
> > > 	- rte_eth_dev_mac_addr_add
> > > 	- rte_eth_dev_mac_addr_remove
> > > 	- rte_eth_dev_vlan_filter
> > > 	- rte_eth_dev_get_mtu
> > > 	- rte_eth_dev_set_mtu
> > >
> > > In order to target these functions to a VF (which has no port id in
> > > the host), the higher bit of port id is reserved:
> > >
> > > #define RTE_ETH_VF_PORT_FLAG (1 << 15)
> >
> > Instead of changing the port number behavior, How about adding a bit
> > field/ I think, There is no ABI breakage as the parent type of bit
> > field  is uint8_t and there is still more room.
> >
> > --- a/lib/librte_ethdev/rte_ethdev_core.h
> > +++ b/lib/librte_ethdev/rte_ethdev_core.h
> > @@ -621,6 +621,7 @@ struct rte_eth_dev_data {
> >                 all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */
> >                 dev_started : 1,   /**< Device state: STARTED(1) / STOPPED(0). */
> >                 lro         : 1;   /**< RX LRO is ON(1) / OFF(0) */
> > +               vf         : 1;   /**< SR-IOV VF device */
> >         uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
> >                         /**< Queues state: STARTED(1) / STOPPED(0). */
> >         uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT];
> 
> Sorry I don't understand how it can help.
> We need to specify which VF we want to configure.
> 
> My proposal is to use the representor port, which is connected to a VF.

I got confused with non-representor case. Yes, My comment is not valid for
port representor case.


> We distinguish the representor and the VF with a flag in the port id
> parameter passed to the functions.
> 

[dpdk-dev] [RFC] ethdev: configure SR-IOV VF from host

[Thomas Monjalon]

In a virtual environment, the network controller may have to configure
some SR-IOV VF parameters for security reasons.

When the PF (host port) is drived by DPDK (OVS-DPDK case),
we face two different cases:
	- driver is bifurcated (Mellanox case),
	so the VF can be configured via the kernel.
	- driver is on top of UIO or VFIO, so DPDK API is required.

This RFC proposes to use generic DPDK API for VF configuration.
The impacted functions are (can be extended):

	- rte_eth_dev_is_valid_port
	- rte_eth_promiscuous_enable
	- rte_eth_promiscuous_disable
	- rte_eth_promiscuous_get
	- rte_eth_allmulticast_enable
	- rte_eth_allmulticast_disable
	- rte_eth_allmulticast_get
	- rte_eth_dev_set_mc_addr_list
	- rte_eth_dev_default_mac_addr_set
	- rte_eth_macaddr_get
	- rte_eth_dev_mac_addr_add
	- rte_eth_dev_mac_addr_remove
	- rte_eth_dev_vlan_filter
	- rte_eth_dev_get_mtu
	- rte_eth_dev_set_mtu

In order to target these functions to a VF (which has no port id
in the host), the higher bit of port id is reserved:

#define RTE_ETH_VF_PORT_FLAG (1 << 15)

This bit can be combined only with the port id of a representor.
The meaning is to target the VF connected with the representor port,
instead of the representor port itself.

If a function is not expected to support VF configuration,
it will return -EINVAL, i.e. there is no code change.
If an API function (listed above) can support VF configuration,
but the PMD does not support it, then -ENOTSUP must be returned.

As an example, this is the change required in rte_eth_dev_is_valid_port:

 int
 rte_eth_dev_is_valid_port(uint16_t port_id)
 {
+       uint32_t dev_flags;
+       uint16_t vf_flag;
+
+       vf_flag = port_id & RTE_ETH_VF_PORT_FLAG;
+       port_id &= RTE_ETH_VF_PORT_FLAG - 1; /* remove VF flag */
+
        if (port_id >= RTE_MAX_ETHPORTS ||
            (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED))
                return 0;
-       else
-               return 1;
+
+       dev_flags = rte_eth_dev_shared_data->data[port_id].dev_flags;
+       if (vf_flag != 0 && (dev_flags & RTE_ETH_DEV_REPRESENTOR) == 0)
+               return 0; /* VF flag has no meaning if not a representor */
+
+       return 1;
 }

Re: [dpdk-dev] [RFC] ethdev: configure SR-IOV VF from host

[Jerin Jacob Kollanukkaran]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Thomas Monjalon
> Sent: Thursday, August 15, 2019 8:36 PM
> To: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>
> Cc: dev@dpdk.org; Bernard Iremonger <bernard.iremonger@intel.com>;
> Shahaf Shuler <shahafs@mellanox.com>; E. Scott Daniels
> <daniels@research.att.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>; Alex
> Zelezniak <alexz@att.com>; Ajit Khaparde <ajit.khaparde@broadcom.com>;
> Declan Doherty <declan.doherty@intel.com>
> Subject: [dpdk-dev] [RFC] ethdev: configure SR-IOV VF from host
> 
> In a virtual environment, the network controller may have to configure some
> SR-IOV VF parameters for security reasons.
> 
> When the PF (host port) is drived by DPDK (OVS-DPDK case), we face two
> different cases:
> 	- driver is bifurcated (Mellanox case),
> 	so the VF can be configured via the kernel.
> 	- driver is on top of UIO or VFIO, so DPDK API is required.
> 
> This RFC proposes to use generic DPDK API for VF configuration.
> The impacted functions are (can be extended):
> 
> 	- rte_eth_dev_is_valid_port
> 	- rte_eth_promiscuous_enable
> 	- rte_eth_promiscuous_disable
> 	- rte_eth_promiscuous_get
> 	- rte_eth_allmulticast_enable
> 	- rte_eth_allmulticast_disable
> 	- rte_eth_allmulticast_get
> 	- rte_eth_dev_set_mc_addr_list
> 	- rte_eth_dev_default_mac_addr_set
> 	- rte_eth_macaddr_get
> 	- rte_eth_dev_mac_addr_add
> 	- rte_eth_dev_mac_addr_remove
> 	- rte_eth_dev_vlan_filter
> 	- rte_eth_dev_get_mtu
> 	- rte_eth_dev_set_mtu
> 
> In order to target these functions to a VF (which has no port id in the host),
> the higher bit of port id is reserved:
> 
> #define RTE_ETH_VF_PORT_FLAG (1 << 15)

Instead of changing the port number behavior, How about adding a bit field/
I think, There is no ABI breakage as the parent type of bit field  is uint8_t and there
is still more room.

diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
index 2922d5b7c..19a60cab2 100644
--- a/lib/librte_ethdev/rte_ethdev_core.h
+++ b/lib/librte_ethdev/rte_ethdev_core.h
@@ -621,6 +621,7 @@ struct rte_eth_dev_data {
                all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */
                dev_started : 1,   /**< Device state: STARTED(1) / STOPPED(0). */
                lro         : 1;   /**< RX LRO is ON(1) / OFF(0) */
+               vf         : 1;   /**< SR-IOV VF device */
        uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
                        /**< Queues state: STARTED(1) / STOPPED(0). */
        uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT];


> 
> This bit can be combined only with the port id of a representor.
> The meaning is to target the VF connected with the representor port, instead
> of the representor port itself.
> 
> If a function is not expected to support VF configuration, it will return -
> EINVAL, i.e. there is no code change.
> If an API function (listed above) can support VF configuration, but the PMD
> does not support it, then -ENOTSUP must be returned.
> 
> As an example, this is the change required in rte_eth_dev_is_valid_port:
> 
>  int
>  rte_eth_dev_is_valid_port(uint16_t port_id)  {
> +       uint32_t dev_flags;
> +       uint16_t vf_flag;
> +
> +       vf_flag = port_id & RTE_ETH_VF_PORT_FLAG;
> +       port_id &= RTE_ETH_VF_PORT_FLAG - 1; /* remove VF flag */
> +
>         if (port_id >= RTE_MAX_ETHPORTS ||
>             (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED))
>                 return 0;
> -       else
> -               return 1;
> +
> +       dev_flags = rte_eth_dev_shared_data->data[port_id].dev_flags;
> +       if (vf_flag != 0 && (dev_flags & RTE_ETH_DEV_REPRESENTOR) == 0)
> +               return 0; /* VF flag has no meaning if not a representor
> + */
> +
> +       return 1;
>  }
> 
> 

Re: [dpdk-dev] [RFC] ethdev: configure SR-IOV VF from host

[Thomas Monjalon]

15/08/2019 17:34, Jerin Jacob Kollanukkaran:
> From: Thomas Monjalon
> > 
> > In a virtual environment, the network controller may have to configure some
> > SR-IOV VF parameters for security reasons.
> > 
> > When the PF (host port) is drived by DPDK (OVS-DPDK case), we face two
> > different cases:
> > 	- driver is bifurcated (Mellanox case),
> > 	so the VF can be configured via the kernel.
> > 	- driver is on top of UIO or VFIO, so DPDK API is required.
> > 
> > This RFC proposes to use generic DPDK API for VF configuration.
> > The impacted functions are (can be extended):
> > 
> > 	- rte_eth_dev_is_valid_port
> > 	- rte_eth_promiscuous_enable
> > 	- rte_eth_promiscuous_disable
> > 	- rte_eth_promiscuous_get
> > 	- rte_eth_allmulticast_enable
> > 	- rte_eth_allmulticast_disable
> > 	- rte_eth_allmulticast_get
> > 	- rte_eth_dev_set_mc_addr_list
> > 	- rte_eth_dev_default_mac_addr_set
> > 	- rte_eth_macaddr_get
> > 	- rte_eth_dev_mac_addr_add
> > 	- rte_eth_dev_mac_addr_remove
> > 	- rte_eth_dev_vlan_filter
> > 	- rte_eth_dev_get_mtu
> > 	- rte_eth_dev_set_mtu
> > 
> > In order to target these functions to a VF (which has no port id in the host),
> > the higher bit of port id is reserved:
> > 
> > #define RTE_ETH_VF_PORT_FLAG (1 << 15)
> 
> Instead of changing the port number behavior, How about adding a bit field/
> I think, There is no ABI breakage as the parent type of bit field  is uint8_t and there
> is still more room.
> 
> --- a/lib/librte_ethdev/rte_ethdev_core.h
> +++ b/lib/librte_ethdev/rte_ethdev_core.h
> @@ -621,6 +621,7 @@ struct rte_eth_dev_data {
>                 all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */
>                 dev_started : 1,   /**< Device state: STARTED(1) / STOPPED(0). */
>                 lro         : 1;   /**< RX LRO is ON(1) / OFF(0) */
> +               vf         : 1;   /**< SR-IOV VF device */
>         uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>                         /**< Queues state: STARTED(1) / STOPPED(0). */
>         uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT];

Sorry I don't understand how it can help.
We need to specify which VF we want to configure.

My proposal is to use the representor port,
which is connected to a VF.
We distinguish the representor and the VF with a flag in the port id
parameter passed to the functions.

Re: [dpdk-dev] [RFC] ethdev: configure SR-IOV VF from host

[Iremonger, Bernard]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, August 15, 2019 4:06 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>
> Cc: dev@dpdk.org; Iremonger, Bernard <bernard.iremonger@intel.com>;
> Shahaf Shuler <shahafs@mellanox.com>; E. Scott Daniels
> <daniels@research.att.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Alex
> Zelezniak <alexz@att.com>; Ajit Khaparde <ajit.khaparde@broadcom.com>;
> Doherty, Declan <declan.doherty@intel.com>
> Subject: [RFC] ethdev: configure SR-IOV VF from host
> 
> In a virtual environment, the network controller may have to configure some
> SR-IOV VF parameters for security reasons.
> 
> When the PF (host port) is drived by DPDK (OVS-DPDK case), we face two
> different cases:
> 	- driver is bifurcated (Mellanox case),
> 	so the VF can be configured via the kernel.
> 	- driver is on top of UIO or VFIO, so DPDK API is required.
> 
> This RFC proposes to use generic DPDK API for VF configuration.
> The impacted functions are (can be extended):
> 
> 	- rte_eth_dev_is_valid_port
> 	- rte_eth_promiscuous_enable
> 	- rte_eth_promiscuous_disable
> 	- rte_eth_promiscuous_get
> 	- rte_eth_allmulticast_enable
> 	- rte_eth_allmulticast_disable
> 	- rte_eth_allmulticast_get
> 	- rte_eth_dev_set_mc_addr_list
> 	- rte_eth_dev_default_mac_addr_set
> 	- rte_eth_macaddr_get
> 	- rte_eth_dev_mac_addr_add
> 	- rte_eth_dev_mac_addr_remove
> 	- rte_eth_dev_vlan_filter
> 	- rte_eth_dev_get_mtu
> 	- rte_eth_dev_set_mtu
> 
> In order to target these functions to a VF (which has no port id in the host),
> the higher bit of port id is reserved:
> 
> #define RTE_ETH_VF_PORT_FLAG (1 << 15)
> 
> This bit can be combined only with the port id of a representor.
> The meaning is to target the VF connected with the representor port, instead
> of the representor port itself.
> 
> If a function is not expected to support VF configuration, it will return -
> EINVAL, i.e. there is no code change.
> If an API function (listed above) can support VF configuration, but the PMD
> does not support it, then -ENOTSUP must be returned.
> 
> As an example, this is the change required in rte_eth_dev_is_valid_port:
> 
>  int
>  rte_eth_dev_is_valid_port(uint16_t port_id)  {
> +       uint32_t dev_flags;
> +       uint16_t vf_flag;
> +
> +       vf_flag = port_id & RTE_ETH_VF_PORT_FLAG;
> +       port_id &= RTE_ETH_VF_PORT_FLAG - 1; /* remove VF flag */
> +
>         if (port_id >= RTE_MAX_ETHPORTS ||
>             (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED))
>                 return 0;
> -       else
> -               return 1;
> +
> +       dev_flags = rte_eth_dev_shared_data->data[port_id].dev_flags;
> +       if (vf_flag != 0 && (dev_flags & RTE_ETH_DEV_REPRESENTOR) == 0)
> +               return 0; /* VF flag has no meaning if not a representor
> + */
> +
> +       return 1;
>  }
> 
> 


Some of the functions in the list above for example, rte_eth_dev_promiscuous_enable() use the dev_ops structure, is it intended to add more rte_eth_dev_* functions to the dev_ops structure?

At present the ixgbe and i40e PMD's have sets of private functions for configuring SRIOV VF's from the DPDK PF,  rte_pmd_ixgbe_*  and rte_pmd_i40e_* functions (see rte_pmd_ixgbe.h and rte_pmd_i40e.h).

At the time these functions were not allowed to be added to the dev_ops structure as there were so many of them.  There was a proposal to add a dev_ctrl function to the dev_ops structure which would access the private functions. Maybe adding the dev_ctrl  function should be considered again.

Having two ways (through dev_ops and private PMD functions) to configure DPDK VF's from the DPDK PF will be confusing for developers.

Regards,

Bernard.

Re: [dpdk-dev] [RFC] ethdev: configure SR-IOV VF from host

[Thomas Monjalon]

29/08/2019 17:02, Iremonger, Bernard:
> Hi Thomas,
> 
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 
> > In a virtual environment, the network controller may have to configure some
> > SR-IOV VF parameters for security reasons.
> > 
> > When the PF (host port) is drived by DPDK (OVS-DPDK case), we face two
> > different cases:
> > 	- driver is bifurcated (Mellanox case),
> > 	so the VF can be configured via the kernel.
> > 	- driver is on top of UIO or VFIO, so DPDK API is required.
> > 
> > This RFC proposes to use generic DPDK API for VF configuration.
> > The impacted functions are (can be extended):
> > 
> > 	- rte_eth_dev_is_valid_port
> > 	- rte_eth_promiscuous_enable
> > 	- rte_eth_promiscuous_disable
> > 	- rte_eth_promiscuous_get
> > 	- rte_eth_allmulticast_enable
> > 	- rte_eth_allmulticast_disable
> > 	- rte_eth_allmulticast_get
> > 	- rte_eth_dev_set_mc_addr_list
> > 	- rte_eth_dev_default_mac_addr_set
> > 	- rte_eth_macaddr_get
> > 	- rte_eth_dev_mac_addr_add
> > 	- rte_eth_dev_mac_addr_remove
> > 	- rte_eth_dev_vlan_filter
> > 	- rte_eth_dev_get_mtu
> > 	- rte_eth_dev_set_mtu
> > 
> > In order to target these functions to a VF (which has no port id in the host),
> > the higher bit of port id is reserved:
> > 
> > #define RTE_ETH_VF_PORT_FLAG (1 << 15)
> > 
> > This bit can be combined only with the port id of a representor.
> > The meaning is to target the VF connected with the representor port, instead
> > of the representor port itself.
> > 
> > If a function is not expected to support VF configuration, it will return -
> > EINVAL, i.e. there is no code change.
> > If an API function (listed above) can support VF configuration, but the PMD
> > does not support it, then -ENOTSUP must be returned.
> > 
> > As an example, this is the change required in rte_eth_dev_is_valid_port:
> > 
> >  int
> >  rte_eth_dev_is_valid_port(uint16_t port_id)  {
> > +       uint32_t dev_flags;
> > +       uint16_t vf_flag;
> > +
> > +       vf_flag = port_id & RTE_ETH_VF_PORT_FLAG;
> > +       port_id &= RTE_ETH_VF_PORT_FLAG - 1; /* remove VF flag */
> > +
> >         if (port_id >= RTE_MAX_ETHPORTS ||
> >             (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED))
> >                 return 0;
> > -       else
> > -               return 1;
> > +
> > +       dev_flags = rte_eth_dev_shared_data->data[port_id].dev_flags;
> > +       if (vf_flag != 0 && (dev_flags & RTE_ETH_DEV_REPRESENTOR) == 0)
> > +               return 0; /* VF flag has no meaning if not a representor
> > + */
> > +
> > +       return 1;
> >  }
> 
> 
> Some of the functions in the list above for example, rte_eth_dev_promiscuous_enable() use the dev_ops structure, is it intended to add more rte_eth_dev_* functions to the dev_ops structure?

I propose to use the same functions for PF and VF.

> At present the ixgbe and i40e PMD's have sets of private functions for configuring SRIOV VF's from the DPDK PF,  rte_pmd_ixgbe_*  and rte_pmd_i40e_* functions (see rte_pmd_ixgbe.h and rte_pmd_i40e.h).
> 
> At the time these functions were not allowed to be added to the dev_ops structure as there were so many of them.  There was a proposal to add a dev_ctrl function to the dev_ops structure which would access the private functions. Maybe adding the dev_ctrl  function should be considered again.
> 
> Having two ways (through dev_ops and private PMD functions) to configure DPDK VF's from the DPDK PF will be confusing for developers.

No, I propose to replace the private API with the representor magic.