On 2/3/2015 6:30 PM, Tetsuya Mukawa wrote:
> On 2015/02/03 18:14, Qiu, Michael wrote:
>> On 2/3/2015 2:16 PM, Qiu, Michael wrote:
>>> On 2/1/2015 12:02 PM, Tetsuya Mukawa wrote:
>>>> The patch introduces following commands.
>>>> - port attach [ident]
>>>> - port detach [port_id]
>>>> - attach: attaching a port
>>>> - detach: detaching a port
>>>> - ident: pci address of physical device.
>>>> Or device name and paramerters of virtual device.
>>>> (ex. 0000:02:00.0, eth_pcap0,iface=eth0)
>>>> - port_id: port identifier
>>>>
>>>> v5:
>>>> - Add testpmd documentation.
>>>> (Thanks to Iremonger, Bernard)
>>>> v4:
>>>> - Fix strings of command help.
>>>>
>>>> Signed-off-by: Tetsuya Mukawa <mukawa at igel.co.jp>
>> [...]
>>
>>>> +static int
>>>> +port_is_closed(portid_t port_id)
>>>> +{
>>>> + if (port_id_is_invalid(port_id, ENABLED_WARN))
>>>> + return 0;
>>>> +
>>>> + if (ports[port_id].port_status != RTE_PORT_CLOSED)
>>>> + return 0;
>>>> +
>>>> + return 1;
>>>> +}
>>>> +
>>>> +int
>>>> start_port(portid_t pid)
>>>> {
>>>> int diag, need_check_link_status = 0;
>>>> @@ -1296,8 +1347,8 @@ start_port(portid_t pid)
>>>>
>>>> if(dcb_config)
>>>> dcb_test = 1;
>>>> - for (pi = 0; pi < nb_ports; pi++) {
>>>> - if (pid < nb_ports && pid != pi)
>>>> + FOREACH_PORT(pi, ports) {
>>>> + if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi)
>>> Here may it be:
>>>
>>> if (!port_id_is_invalid(pid, DISABLED_WARN) && (pid != pi || pid ==
>>> RET_PORT_ALL))
>> Sorry, should be:
>>
>> if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi && pid !=
>> (portid_t)RET_PORT_ALL)
>>
>> Otherwise, should check for "RET_PORT_ALL" in function port_id_is_invalid()
> Thanks for comment. I've found 2 issues.
> (I guess the original code has same issue.)
Original code may not have this issue, cause RET_PORT_ALL is always
greater than nb_ports, so "continue" will not exec. The logic may be
right, but it is a little hard to understand.
> One is that "port_id_is_invalid" should receives "pi" instead of "pid".
> The other is if statement is wrong as you said.
>
> I guess following statement will be good.
>
> if (port_id_is_invalid(pi, DISABLED_WARN) || (pid != pi && pid !=
> (portid_t)RTE_PORT_ALL))
Actually, "port_id_is_invalid(pi, DISABLED_WARN)" could be removed as
"FOREACH_PORT" will find a valid port.
So it could be:
if (pid != pi && pid != (portid_t)RTE_PORT_ALL)
What do you think?
Thanks,
Michael
> How about it?
>
> Thanks,
> Tetsuya
>
>
>> Thanks,
>> Michael
>>
>>> Otherwise no port will be start by default.
>>>
>>>
>>> Thanks,
>>> Michael
>>>
>>>> continue;
>>>>
>>>> port = &ports[pi];
>>>> @@ -1421,7 +1472,7 @@ start_port(portid_t pid)
>>>> }
>>>>
>>>> if (need_check_link_status && !no_link_check)
>>>> - check_all_ports_link_status(nb_ports, RTE_PORT_ALL);
>>>> + check_all_ports_link_status(RTE_PORT_ALL);
>>>> else
>>>> printf("Please stop the ports first\n");
>>>>
>>>> @@ -1446,8 +1497,8 @@ stop_port(portid_t pid)
>>>> }
>>>> printf("Stopping ports...\n");
>>>>
>>>> - for (pi = 0; pi < nb_ports; pi++) {
>>>> - if (pid < nb_ports && pid != pi)
>>>> + FOREACH_PORT(pi, ports) {
>>>> + if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi)
>>>> continue;
>>>>
>>>> port = &ports[pi];
>>>> @@ -1463,7 +1514,7 @@ stop_port(portid_t pid)
>>>> need_check_link_status = 1;
>>>> }
>>>> if (need_check_link_status && !no_link_check)
>>>> - check_all_ports_link_status(nb_ports, RTE_PORT_ALL);
>>>> + check_all_ports_link_status(RTE_PORT_ALL);
>>>>
>>>> printf("Done\n");
>>>> }
>>>> @@ -1481,8 +1532,8 @@ close_port(portid_t pid)
>>>>
>>>> printf("Closing ports...\n");
>>>>
>>>> - for (pi = 0; pi < nb_ports; pi++) {
>>>> - if (pid < nb_ports && pid != pi)
>>>> + FOREACH_PORT(pi, ports) {
>>>> + if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi)
>>>> continue;
>>>>
>>>> port = &ports[pi];
>>>> @@ -1502,31 +1553,83 @@ close_port(portid_t pid)
>>>> printf("Done\n");
>>>> }
>>>>
>>>> -int
>>>> -all_ports_stopped(void)
>>>> +void
>>>> +attach_port(char *identifier)
>>>> {
>>>> - portid_t pi;
>>>> - struct rte_port *port;
>>>> + portid_t i, j, pi = 0;
>>>>
>>>> - for (pi = 0; pi < nb_ports; pi++) {
>>>> - port = &ports[pi];
>>>> - if (port->port_status != RTE_PORT_STOPPED)
>>>> - return 0;
>>>> + printf("Attaching a new port...\n");
>>>> +
>>>> + if (identifier == NULL) {
>>>> + printf("Invalid parameters are speficied\n");
>>>> + return;
>>>> }
>>>>
>>>> - return 1;
>>>> + if (test_done == 0) {
>>>> + printf("Please stop forwarding first\n");
>>>> + return;
>>>> + }
>>>> +
>>>> + if (rte_eal_dev_attach(identifier, &pi))
>>>> + return;
>>>> +
>>>> + ports[pi].enabled = 1;
>>>> + reconfig(pi, rte_eth_dev_socket_id(pi));
>>>> + rte_eth_promiscuous_enable(pi);
>>>> +
>>>> + nb_ports = rte_eth_dev_count();
>>>> +
>>>> + /* set_default_fwd_ports_config(); */
>>>> + bzero(fwd_ports_ids, sizeof(fwd_ports_ids));
>>>> + i = 0;
>>>> + FOREACH_PORT(j, ports) {
>>>> + fwd_ports_ids[i] = j;
>>>> + i++;
>>>> + }
>>>> + nb_cfg_ports = nb_ports;
>>>> + nb_fwd_ports++;
>>>> +
>>>> + ports[pi].port_status = RTE_PORT_STOPPED;
>>>> +
>>>> + printf("Port %d is attached. Now total ports is %d\n", pi, nb_ports);
>>>> + printf("Done\n");
>>>> }
>>>>
>>>> -int
>>>> -port_is_started(portid_t port_id)
>>>> +void
>>>> +detach_port(uint8_t port_id)
>>>> {
>>>> - if (port_id_is_invalid(port_id))
>>>> - return -1;
>>>> + portid_t i, pi = 0;
>>>> + char name[RTE_ETH_NAME_MAX_LEN];
>>>>
>>>> - if (ports[port_id].port_status != RTE_PORT_STARTED)
>>>> - return 0;
>>>> + printf("Detaching a port...\n");
>>>>
>>>> - return 1;
>>>> + if (!port_is_closed(port_id)) {
>>>> + printf("Please close port first\n");
>>>> + return;
>>>> + }
>>>> +
>>>> + rte_eth_promiscuous_disable(port_id);
>>>> +
>>>> + if (rte_eal_dev_detach(port_id, name))
>>>> + return;
>>>> +
>>>> + ports[port_id].enabled = 0;
>>>> + nb_ports = rte_eth_dev_count();
>>>> +
>>>> + /* set_default_fwd_ports_config(); */
>>>> + bzero(fwd_ports_ids, sizeof(fwd_ports_ids));
>>>> + i = 0;
>>>> + FOREACH_PORT(pi, ports) {
>>>> + fwd_ports_ids[i] = pi;
>>>> + i++;
>>>> + }
>>>> + nb_cfg_ports = nb_ports;
>>>> + nb_fwd_ports--;
>>>> +
>>>> + printf("Port '%s' is detached. Now total ports is %d\n",
>>>> + name, nb_ports);
>>>> + printf("Done\n");
>>>> + return;
>>>> }
>>>>
>>>> void
>>>> @@ -1534,7 +1637,7 @@ pmd_test_exit(void)
>>>> {
>>>> portid_t pt_id;
>>>>
>>>> - for (pt_id = 0; pt_id < nb_ports; pt_id++) {
>>>> + FOREACH_PORT(pt_id, ports) {
>>>> printf("Stopping port %d...", pt_id);
>>>> fflush(stdout);
>>>> rte_eth_dev_close(pt_id);
>>>> @@ -1553,7 +1656,7 @@ struct pmd_test_command {
>>>>
>>>> /* Check the link status of all ports in up to 9s, and print them finally
>>>> */
>>>> static void
>>>> -check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
>>>> +check_all_ports_link_status(uint32_t port_mask)
>>>> {
>>>> #define CHECK_INTERVAL 100 /* 100ms */
>>>> #define MAX_CHECK_TIME 90 /* 9s (90 * 100ms) in total */
>>>> @@ -1564,7 +1667,7 @@ check_all_ports_link_status(uint8_t port_num,
>>>> uint32_t port_mask)
>>>> fflush(stdout);
>>>> for (count = 0; count <= MAX_CHECK_TIME; count++) {
>>>> all_ports_up = 1;
>>>> - for (portid = 0; portid < port_num; portid++) {
>>>> + FOREACH_PORT(portid, ports) {
>>>> if ((port_mask & (1 << portid)) == 0)
>>>> continue;
>>>> memset(&link, 0, sizeof(link));
>>>> @@ -1688,7 +1791,7 @@ init_port_config(void)
>>>> portid_t pid;
>>>> struct rte_port *port;
>>>>
>>>> - for (pid = 0; pid < nb_ports; pid++) {
>>>> + FOREACH_PORT(pid, ports) {
>>>> port = &ports[pid];
>>>> port->dev_conf.rxmode = rx_mode;
>>>> port->dev_conf.fdir_conf = fdir_conf;
>>>> @@ -1877,7 +1980,7 @@ main(int argc, char** argv)
>>>>
>>>> nb_ports = (portid_t) rte_eth_dev_count();
>>>> if (nb_ports == 0)
>>>> - rte_exit(EXIT_FAILURE, "No probed ethernet device\n");
>>>> + RTE_LOG(WARNING, EAL, "No probed ethernet devices\n");
>>>>
>>>> set_def_fwd_config();
>>>> if (nb_lcores == 0)
>>>> @@ -1899,7 +2002,7 @@ main(int argc, char** argv)
>>>> rte_exit(EXIT_FAILURE, "Start ports failed\n");
>>>>
>>>> /* set all ports to promiscuous mode by default */
>>>> - for (port_id = 0; port_id < nb_ports; port_id++)
>>>> + FOREACH_PORT(port_id, ports)
>>>> rte_eth_promiscuous_enable(port_id);
>>>>
>>>> #ifdef RTE_LIBRTE_CMDLINE
>>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>>>> index 8f5e6c7..109c670 100644
>>>> --- a/app/test-pmd/testpmd.h
>>>> +++ b/app/test-pmd/testpmd.h
>>>> @@ -134,6 +134,7 @@ struct fwd_stream {
>>>> * The data structure associated with each port.
>>>> */
>>>> struct rte_port {
>>>> + uint8_t enabled; /**< Port enabled or not */
>>>> struct rte_eth_dev_info dev_info; /**< PCI info + driver name */
>>>> struct rte_eth_conf dev_conf; /**< Port configuration. */
>>>> struct ether_addr eth_addr; /**< Port ethernet address */
>>>> @@ -159,6 +160,14 @@ struct rte_port {
>>>> struct rte_eth_txconf tx_conf; /**< tx configuration */
>>>> };
>>>>
>>>> +extern portid_t __rte_unused
>>>> +find_next_port(portid_t p, struct rte_port *ports, int size);
>>>> +
>>>> +#define FOREACH_PORT(p, ports) \
>>>> + for (p = find_next_port(0, ports, RTE_MAX_ETHPORTS); \
>>>> + p < RTE_MAX_ETHPORTS; \
>>>> + p = find_next_port(p + 1, ports, RTE_MAX_ETHPORTS))
>>>> +
>>>> /**
>>>> * The data structure associated with each forwarding logical core.
>>>> * The logical cores are internally numbered by a core index from 0 to
>>>> @@ -515,6 +524,8 @@ int init_port_dcb_config(portid_t pid,struct
>>>> dcb_config *dcb_conf);
>>>> int start_port(portid_t pid);
>>>> void stop_port(portid_t pid);
>>>> void close_port(portid_t pid);
>>>> +void attach_port(char *identifier);
>>>> +void detach_port(uint8_t port_id);
>>>> int all_ports_stopped(void);
>>>> int port_is_started(portid_t port_id);
>>>> void pmd_test_exit(void);
>>>> @@ -558,10 +569,15 @@ void get_ethertype_filter(uint8_t port_id, uint16_t
>>>> index);
>>>> void get_2tuple_filter(uint8_t port_id, uint16_t index);
>>>> void get_5tuple_filter(uint8_t port_id, uint16_t index);
>>>> void get_flex_filter(uint8_t port_id, uint16_t index);
>>>> -int port_id_is_invalid(portid_t port_id);
>>>> int rx_queue_id_is_invalid(queueid_t rxq_id);
>>>> int tx_queue_id_is_invalid(queueid_t txq_id);
>>>>
>>>> +enum print_warning {
>>>> + ENABLED_WARN = 0,
>>>> + DISABLED_WARN
>>>> +};
>>>> +int port_id_is_invalid(portid_t port_id, enum print_warning warning);
>>>> +
>>>> /*
>>>> * Work-around of a compilation error with ICC on invocations of the
>>>> * rte_be_to_cpu_16() function.
>>>> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>> index 218835a..1cacbcf 100644
>>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>> @@ -808,6 +808,63 @@ The following sections show functions for configuring
>>>> ports.
>>>>
>>>> Port configuration changes only become active when forwarding is
>>>> started/restarted.
>>>>
>>>> +port attach
>>>> +~~~~~~~~~~~
>>>> +
>>>> +Attach a port specified by pci address or virtual device args.
>>>> +
>>>> +To attach a new pci device, the device should be recognized by kernel
>>>> first.
>>>> +Then it should be moved under DPDK management.
>>>> +Finally the port can be attached to testpmd.
>>>> +On the other hand, to attach a port created by virtual device, above
>>>> steps are not needed.
>>>> +
>>>> +port attach (identifier)
>>>> +
>>>> +For example, to attach a port that pci address is 0000:02:00.0.
>>>> +
>>>> +.. code-block:: console
>>>> +
>>>> + testpmd> port attach 0000:02:00.0
>>>> + Attaching a new port...
>>>> + ... snip ...
>>>> + Port 0 is attached. Now total ports is 1
>>>> + Done
>>>> +
>>>> +For example, to attach a port created by pcap PMD.
>>>> +
>>>> +.. code-block:: console
>>>> +
>>>> + testpmd> port attach eth_pcap0,iface=eth0
>>>> + Attaching a new port...
>>>> + ... snip ...
>>>> + Port 0 is attached. Now total ports is 1
>>>> + Done
>>>> +
>>>> +In this case, identifier is "eth_pcap0,iface=eth0".
>>>> +This identifier format is the same as "--vdev" format of DPDK
>>>> applications.
>>>> +
>>>> +port detach
>>>> +~~~~~~~~~~~
>>>> +
>>>> +Detach a specific port.
>>>> +
>>>> +Before detaching a port, the port should be closed.
>>>> +Also to remove a pci device completely from the system, first detach the
>>>> port from testpmd.
>>>> +Then the device should be moved under kernel management.
>>>> +Finally the device can be remove using kernel pci hotplug functionality.
>>>> +On the other hand, to remove a port created by virtual device, above
>>>> steps are not needed.
>>>> +
>>>> +port detach (port_id)
>>>> +
>>>> +For example, to detach a port 0.
>>>> +
>>>> +.. code-block:: console
>>>> +
>>>> + testpmd> port detach 0
>>>> + Detaching a port...
>>>> + ... snip ...
>>>> + Done
>>>> +
>>>> port start
>>>> ~~~~~~~~~~
>>>>
>
>
