DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Ning Li" <muziding001@163.com>
To: "'Tiwei Bie'" <tiwei.bie@intel.com>
Cc: "'Yuanhan Liu'" <yliu@fridaylinux.org>,
	"'Maxime Coquelin'" <maxime.coquelin@redhat.com>, <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] net/virtio-user: specify MAC address for tap port
Date: Fri, 29 Dec 2017 11:04:33 +0800	[thread overview]
Message-ID: <000701d38051$c1368b80$43a3a280$@163.com> (raw)
In-Reply-To: <20171228084345.li5xfnfkbuahp4ur@debian-xvivbkq.sh.intel.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
> > > >
> > > >
> >
> >

  reply	other threads:[~2017-12-29  3:04 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-14 11:38 Ning Li
2017-12-18  6:19 ` Tiwei Bie
2017-12-19  9:27   ` Ning Li
2017-12-19 10:08   ` Ning Li
2017-12-28  8:43     ` Tiwei Bie
2017-12-29  3:04       ` Ning Li [this message]
2017-12-29  3:38 ` [dpdk-dev] [PATCH] net/virtio-user: specify the MAC of the tap Ning Li
2017-12-29  9:44   ` Tiwei Bie
2018-03-28 16:54     ` Seán Harte
2018-03-29  0:50       ` Tan, Jianfeng
2018-03-29  1:15         ` Tiwei Bie
2018-03-29 11:45   ` Maxime Coquelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='000701d38051$c1368b80$43a3a280$@163.com' \
    --to=muziding001@163.com \
    --cc=dev@dpdk.org \
    --cc=maxime.coquelin@redhat.com \
    --cc=tiwei.bie@intel.com \
    --cc=yliu@fridaylinux.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).