On Thu, 9 Mar 2017 14:05:51 +0000
Ferruh Yigit <ferruh.yi...@intel.com> wrote:
> On 3/7/2017 4:31 PM, Pascal Mazon wrote:
> > Set a random MAC address when probing the device, as to not leave an
> > empty MAC in pmd->eth_addr.
> >
> > This MAC will be set on the tap netdevice as soon as it's been
> > created using tun_alloc(). As the tap_mac_add() function depend on
> > the fd in the first rxq, move code from tun_alloc() to
> > tap_setup_queue(), after it's been set.
> >
> > Signed-off-by: Pascal Mazon <pascal.ma...@6wind.com>
> > ---
> > doc/guides/nics/features/tap.ini | 1 +
> > drivers/net/tap/rte_eth_tap.c | 97
> > ++++++++++++++++++++++++++++++++++------ 2 files changed, 85
> > insertions(+), 13 deletions(-)
> >
> > diff --git a/doc/guides/nics/features/tap.ini
> > b/doc/guides/nics/features/tap.ini index f4aca6921ddc..d9b47a003654
> > 100644 --- a/doc/guides/nics/features/tap.ini
> > +++ b/doc/guides/nics/features/tap.ini
> > @@ -9,6 +9,7 @@ Jumbo frame = Y
> > Promiscuous mode = Y
> > Allmulticast mode = Y
> > Basic stats = Y
> > +Unicast MAC filter = Y
> > Other kdrv = Y
> > ARMv7 = Y
> > ARMv8 = Y
> > diff --git a/drivers/net/tap/rte_eth_tap.c
> > b/drivers/net/tap/rte_eth_tap.c index ece3a5fcc897..1e46ee36efa2
> > 100644 --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -63,6 +63,8 @@
> > #define RTE_PMD_TAP_MAX_QUEUES 1
> > #endif
> >
> > +#define RTE_PMD_TAP_MAX_MAC_ADDRS 1
>
> mac_addr_add and mac_addr_remove not really supported, because only
> one MAC is supported. For mac_addr_add() all indexes other than 0
> will give an error. So only mac_addr_set is supported.
>
> For this case what is the benefit of implementing these functions and
> claim support, instead of just leaving mac_addr_add and
> mac_addr_remove NULL?
>
Well, I wanted to implement those along with the mac_addr_set, as they
all dealt with mac addresses. But you're right, I might as well leave
the ops NULL.
I'll send a new version reflecting this.
>
> <...>
>
> > + if (qid == 0) {
> > + /*
> > + * tap_setup_queue() is called for both tx and rx.
> > + * Let's use dev->data->r/tx_queues[qid] to
> > determine if init
> > + * has already been done.
> > + */
> > + if (dev->data->rx_queues[qid] &&
> > dev->data->tx_queues[qid])
> > + return fd;
> > +
> > + tap_mac_set(dev, &pmd->eth_addr);
>
> What is the reason of changing behavior here?
>
> Tap devices assigned random MAC by kernel, and previous implementation
> was reading that value and using it for DPDK.
>
> Now kernel assigns a random MAC, and DPDK overwrites it another random
> MAC, previous implementation was simpler I think.
>
> It is OK to move this code tap_setup_queue(), I just missed the
> benefit of overwriting with DPDK random MAC?
>
> <...>
As far as I remember, I did it because somewhere the mac_addr_set was
checked as part of reconfiguration, which happenened before queue setup
was done. The default mac address (dev->data->mac_addrs[0]) got set to 0
and later call for the default mac address tried using this mac address.
Or something along those lines.
I'll definitely re-take a closer look at all this for my next version.
Regards,
Pascal
