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 <muziding...@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
> > > >
> > > >
> >
> >
