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

[Thomas Monjalon <thomas@monjalon.net>]

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.