Re: [dpdk-dev] [PATCH] net/virtio-user: specify MAC address for tap port

["Ning Li" <muziding001@163.com>]

HI Tiwei,

Thanks for the reply!

> Hi Ning,
> 
> On Tue, Dec 19, 2017 at 06:08:10PM +0800, Ning Li wrote:
> > Hi Tiwei,
> >
> > > Hi Ning,
> > >
> > > On Thu, Dec 14, 2017 at 07:38:14PM +0800, Ning Li wrote:
> > > > When use virtio_user as exception path, we need to specify a MAC
> > > > address for the tap port.
> > >
> > > Is this a fix? Did you meet any issue? If so, please describe
> > > the issue and add a fixline.
> >
> > Specify the MAC address for tap0 may be a requirement for some applications.
> >
> > As described in doc/guides/howto/virtio_user_as_exceptional_path.rst, when
> > virtio-user with vhost-kernel is used to exchange packet with kernel
> networking
> > stack, instead of KNI, application need to set the physical NIC in promiscuous
> > mode, otherwise, the packet send to tap0 will be discarded by physical NIC. So,
> > this will be a probleam, when some applications not set NIC in promiscuous
> mode.
> > This problem will be easy to solve, If application can specify the MAC address
> > of the NIC for tap0.
> >
> 
> Thanks for the detailed info!
> 
> From my understanding, it's not necessary to set the MAC
> of the tap via virtio-user. Because the tap is a virtual
> interface which belongs to kernel, and it's connected to
> the virtual interface which is emulated by virtio-user.
> 
> So actually they are different virtual interfaces. If an
> application wants the virtio-user, tap and any other NIC
> share the same MAC address, it just needs to set the MAC
> for each interface individually in the standard way. Any
> thoughts?
> 
> Even if we really want to support specifying the MAC via
> virtio-user for the corresponding tap, the MAC should be
> passed via another device argument, e.g. tap_mac.
> 

You are right. It is not necessary to set the MAC of tap0 via virtio-user.
Applications can use ioctl() to set the MAC of tap0 by themselves.

> Besides, I think you missed my other comments in my last
> mail. E.g. The patch isn't buildable.

Sorry for this. I will submit a buildable patch later.

Thanks,
Ning Li

> 
> Thanks,
> Tiwei
> 
> > Thanks,
> > Ning Li
> >
> > >
> > > >
> > > > Signed-off-by: Ning Li <muziding001@163.com>
> > > > ---
> > > >  drivers/net/virtio/virtio_user/vhost_kernel.c     |  3 ++-
> > > >  drivers/net/virtio/virtio_user/vhost_kernel_tap.c |  8 ++++++++
> > > >  drivers/net/virtio/virtio_user/vhost_kernel_tap.h | 12 +++++++++++-
> > > >  3 files changed, 21 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c
> > > b/drivers/net/virtio/virtio_user/vhost_kernel.c
> > > > index 68d28b1..dd24b6b 100644
> > > > --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
> > > > +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
> > > > @@ -380,7 +380,8 @@ struct vhost_memory_kernel {
> > > >  	else
> > > >  		hdr_size = sizeof(struct virtio_net_hdr);
> > > >
> > > > -	tapfd = vhost_kernel_open_tap(&dev->ifname, hdr_size, req_mq);
> > > > +	tapfd = vhost_kernel_open_tap(&dev->ifname, hdr_size, req_mq,
> > > > +			 (char *)dev->mac_addr);
> > > >  	if (tapfd < 0) {
> > > >  		PMD_DRV_LOG(ERR, "fail to open tap for vhost kernel");
> > > >  		return -1;
> > > > diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> > > b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> > > > index 689a5cf..756fde2 100644
> > > > --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> > > > +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> > > > @@ -123,6 +123,14 @@
> > >
> > > You forgot to add the mac param for vhost_kernel_open_tap().
> > > The patch isn't buildable on my machine.
> > >
> > > >  		PMD_DRV_LOG(ERR, "TUNSETOFFLOAD ioctl() failed: %s",
> > > >  			   strerror(errno));
> > > >
> > > > +	memset(&ifr, 0, sizeof(ifr));
> > > > +	ifr.ifr_hwaddr.sa_family = ARPHRD_ETHER;
> > >
> > > ARPHRD_ETHER? Please explain.
> > >
> > > > +	memcpy(ifr.ifr_hwaddr.sa_data, mac, ETH_ALEN);
> > >
> > > You can use ETHER_ADDR_LEN.
> > > There is no need to define ETH_ALEN.
> > >
> > > > +	if (ioctl(tapfd, SIOCSIFHWADDR, (void *)&ifr) == -1) {
> > > > +		PMD_DRV_LOG(ERR, "SIOCSIFHWADDR failed: %s",
> strerror(errno));
> > > > +		goto error;
> > > > +	}
> > > > +
> > > >  	if (!(*p_ifname))
> > > >  		*p_ifname = strdup(ifr.ifr_name);
> > > >
> > > > diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> > > b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> > > > index eae340c..f59b1ac 100644
> > > > --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> > > > +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> > > > @@ -64,4 +64,14 @@
> > > >  /* Constants */
> > > >  #define PATH_NET_TUN	"/dev/net/tun"
> > > >
> > > > -int vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq);
> > > > +/* Socket configuration controls. */
> > > > +#define SIOCSIFHWADDR   0x8924          /* set hardware address
> */
> > >
> > > There is no need to define this.
> > >
> > > > +
> > > > +/* ARP protocol HARDWARE identifiers. */
> > > > +#define ARPHRD_ETHER    1               /* Ethernet
> > > */
> > > > +
> > > > +/* Length of Ethernet address. */
> > > > +#define ETH_ALEN  6
> > > > +
> > >
> > > There is no need to define this.
> > >
> > > Thanks,
> > > Tiwei
> > >
> > > > +int vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq,
> > > > +			 const char *mac);
> > > > --
> > > > 1.8.3.1
> > > >
> > > >
> >
> >