diff mbox

[QEMU,v2] virtio-net: introduce a new macaddr control

Message ID 1358317007-29972-1-git-send-email-akong@redhat.com
State New
Headers show

Commit Message

Amos Kong Jan. 16, 2013, 6:16 a.m. UTC 
From: Amos Kong <akong@redhat.com>

In virtio-net guest driver, currently we write MAC address to
pci config space byte by byte, this means that we have an
intermediate step where mac is wrong. This patch introduced
a new control command to set MAC address in one time.

VIRTIO_NET_F_CTRL_MAC_ADDR is a new feature bit for compatibility.

Signed-off-by: Amos Kong <akong@redhat.com>
---
V2: check guest's iov_len before memcpy
---
 hw/virtio-net.c | 10 ++++++++++
 hw/virtio-net.h |  9 ++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

Comments

Jason Wang Jan. 16, 2013, 6:37 a.m. UTC | #1 
On Wednesday, January 16, 2013 02:16:47 PM akong@redhat.com wrote:
> From: Amos Kong <akong@redhat.com>
> 
> In virtio-net guest driver, currently we write MAC address to
> pci config space byte by byte, this means that we have an
> intermediate step where mac is wrong. This patch introduced
> a new control command to set MAC address in one time.
> 
> VIRTIO_NET_F_CTRL_MAC_ADDR is a new feature bit for compatibility.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
> V2: check guest's iov_len before memcpy
> ---
>  hw/virtio-net.c | 10 ++++++++++
>  hw/virtio-net.h |  9 ++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index dc7c6d6..d05f98f 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -247,6 +247,7 @@ static uint32_t virtio_net_get_features(VirtIODevice
> *vdev, uint32_t features) VirtIONet *n = to_virtio_net(vdev);
> 
>      features |= (1 << VIRTIO_NET_F_MAC);
> +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
> 
>      if (!peer_has_vnet_hdr(n)) {
>          features &= ~(0x1 << VIRTIO_NET_F_CSUM);
> @@ -282,6 +283,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice
> *vdev) /* Linux kernel 2.6.25.  It understood MAC (as everyone must), * but
> also these: */
>      features |= (1 << VIRTIO_NET_F_MAC);
> +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
>      features |= (1 << VIRTIO_NET_F_CSUM);
>      features |= (1 << VIRTIO_NET_F_HOST_TSO4);
>      features |= (1 << VIRTIO_NET_F_HOST_TSO6);
> @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t
> cmd, {
>      struct virtio_net_ctrl_mac mac_data;
> 
> +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> +        elem->out_sg[1].iov_len == ETH_ALEN) {
> +        /* Set MAC address */
> +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> +        return VIRTIO_NET_OK;
> +    }
> +
>      if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET || elem->out_num != 3 ||
>          elem->out_sg[1].iov_len < sizeof(mac_data) ||
>          elem->out_sg[2].iov_len < sizeof(mac_data))
> diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> index d46fb98..9394cc0 100644
> --- a/hw/virtio-net.h
> +++ b/hw/virtio-net.h
> @@ -44,6 +44,8 @@
>  #define VIRTIO_NET_F_CTRL_VLAN  19      /* Control channel VLAN filtering
> */ #define VIRTIO_NET_F_CTRL_RX_EXTRA 20   /* Extra RX mode control support
> */
> 
> +#define VIRTIO_NET_F_CTRL_MAC_ADDR   23 /* Set MAC address */
> +

I wonder whether we need a DEFINE_PROP_BIT to disable and compat this feature. 
Consider we may migrate from a new version to an old version.
>  #define VIRTIO_NET_S_LINK_UP    1       /* Link is up */
> 
>  #define TX_TIMER_INTERVAL 150000 /* 150 us */
> @@ -106,7 +108,7 @@ typedef uint8_t virtio_net_ctrl_ack;
>   #define VIRTIO_NET_CTRL_RX_MODE_NOBCAST      5
> 
>  /*
> - * Control the MAC filter table.
> + * Control the MAC
>   *
>   * The MAC filter table is managed by the hypervisor, the guest should
>   * assume the size is infinite.  Filtering should be considered
> @@ -119,6 +121,10 @@ typedef uint8_t virtio_net_ctrl_ack;
>   * first sg list contains unicast addresses, the second is for multicast.
>   * This functionality is present if the VIRTIO_NET_F_CTRL_RX feature
>   * is available.
> + *
> + * The ADDR_SET command requests one out scatterlist, it contains a
> + * 6 bytes MAC address. This functionality is present if the
> + * VIRTIO_NET_F_CTRL_MAC_ADDR feature is available.
>   */
>  struct virtio_net_ctrl_mac {
>      uint32_t entries;
> @@ -126,6 +132,7 @@ struct virtio_net_ctrl_mac {
>  };
>  #define VIRTIO_NET_CTRL_MAC    1
>   #define VIRTIO_NET_CTRL_MAC_TABLE_SET        0
> + #define VIRTIO_NET_CTRL_MAC_ADDR_SET         1
> 
>  /*
>   * Control VLAN filtering
Michael S. Tsirkin Jan. 16, 2013, 8:19 a.m. UTC | #2 
On Wed, Jan 16, 2013 at 02:37:34PM +0800, Jason Wang wrote:
> On Wednesday, January 16, 2013 02:16:47 PM akong@redhat.com wrote:
> > From: Amos Kong <akong@redhat.com>
> > 
> > In virtio-net guest driver, currently we write MAC address to
> > pci config space byte by byte, this means that we have an
> > intermediate step where mac is wrong. This patch introduced
> > a new control command to set MAC address in one time.
> > 
> > VIRTIO_NET_F_CTRL_MAC_ADDR is a new feature bit for compatibility.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> > V2: check guest's iov_len before memcpy
> > ---
> >  hw/virtio-net.c | 10 ++++++++++
> >  hw/virtio-net.h |  9 ++++++++-
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index dc7c6d6..d05f98f 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -247,6 +247,7 @@ static uint32_t virtio_net_get_features(VirtIODevice
> > *vdev, uint32_t features) VirtIONet *n = to_virtio_net(vdev);
> > 
> >      features |= (1 << VIRTIO_NET_F_MAC);
> > +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
> > 
> >      if (!peer_has_vnet_hdr(n)) {
> >          features &= ~(0x1 << VIRTIO_NET_F_CSUM);
> > @@ -282,6 +283,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice
> > *vdev) /* Linux kernel 2.6.25.  It understood MAC (as everyone must), * but
> > also these: */
> >      features |= (1 << VIRTIO_NET_F_MAC);
> > +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
> >      features |= (1 << VIRTIO_NET_F_CSUM);
> >      features |= (1 << VIRTIO_NET_F_HOST_TSO4);
> >      features |= (1 << VIRTIO_NET_F_HOST_TSO6);
> > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t
> > cmd, {
> >      struct virtio_net_ctrl_mac mac_data;
> > 
> > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > +        /* Set MAC address */
> > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > +        return VIRTIO_NET_OK;
> > +    }
> > +
> >      if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET || elem->out_num != 3 ||
> >          elem->out_sg[1].iov_len < sizeof(mac_data) ||
> >          elem->out_sg[2].iov_len < sizeof(mac_data))
> > diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> > index d46fb98..9394cc0 100644
> > --- a/hw/virtio-net.h
> > +++ b/hw/virtio-net.h
> > @@ -44,6 +44,8 @@
> >  #define VIRTIO_NET_F_CTRL_VLAN  19      /* Control channel VLAN filtering
> > */ #define VIRTIO_NET_F_CTRL_RX_EXTRA 20   /* Extra RX mode control support
> > */
> > 
> > +#define VIRTIO_NET_F_CTRL_MAC_ADDR   23 /* Set MAC address */
> > +
> 
> I wonder whether we need a DEFINE_PROP_BIT to disable and compat this feature. 
> Consider we may migrate from a new version to an old version.

Yes I think we do.

> >  #define VIRTIO_NET_S_LINK_UP    1       /* Link is up */
> > 
> >  #define TX_TIMER_INTERVAL 150000 /* 150 us */
> > @@ -106,7 +108,7 @@ typedef uint8_t virtio_net_ctrl_ack;
> >   #define VIRTIO_NET_CTRL_RX_MODE_NOBCAST      5
> > 
> >  /*
> > - * Control the MAC filter table.
> > + * Control the MAC
> >   *
> >   * The MAC filter table is managed by the hypervisor, the guest should
> >   * assume the size is infinite.  Filtering should be considered
> > @@ -119,6 +121,10 @@ typedef uint8_t virtio_net_ctrl_ack;
> >   * first sg list contains unicast addresses, the second is for multicast.
> >   * This functionality is present if the VIRTIO_NET_F_CTRL_RX feature
> >   * is available.
> > + *
> > + * The ADDR_SET command requests one out scatterlist, it contains a
> > + * 6 bytes MAC address. This functionality is present if the
> > + * VIRTIO_NET_F_CTRL_MAC_ADDR feature is available.
> >   */
> >  struct virtio_net_ctrl_mac {
> >      uint32_t entries;
> > @@ -126,6 +132,7 @@ struct virtio_net_ctrl_mac {
> >  };
> >  #define VIRTIO_NET_CTRL_MAC    1
> >   #define VIRTIO_NET_CTRL_MAC_TABLE_SET        0
> > + #define VIRTIO_NET_CTRL_MAC_ADDR_SET         1
> > 
> >  /*
> >   * Control VLAN filtering
Stefan Hajnoczi Jan. 16, 2013, 8:59 a.m. UTC | #3 
On Wed, Jan 16, 2013 at 02:37:34PM +0800, Jason Wang wrote:
> On Wednesday, January 16, 2013 02:16:47 PM akong@redhat.com wrote:
> > From: Amos Kong <akong@redhat.com>
> > 
> > In virtio-net guest driver, currently we write MAC address to
> > pci config space byte by byte, this means that we have an
> > intermediate step where mac is wrong. This patch introduced
> > a new control command to set MAC address in one time.
> > 
> > VIRTIO_NET_F_CTRL_MAC_ADDR is a new feature bit for compatibility.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> > V2: check guest's iov_len before memcpy
> > ---
> >  hw/virtio-net.c | 10 ++++++++++
> >  hw/virtio-net.h |  9 ++++++++-
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index dc7c6d6..d05f98f 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -247,6 +247,7 @@ static uint32_t virtio_net_get_features(VirtIODevice
> > *vdev, uint32_t features) VirtIONet *n = to_virtio_net(vdev);
> > 
> >      features |= (1 << VIRTIO_NET_F_MAC);
> > +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
> > 
> >      if (!peer_has_vnet_hdr(n)) {
> >          features &= ~(0x1 << VIRTIO_NET_F_CSUM);
> > @@ -282,6 +283,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice
> > *vdev) /* Linux kernel 2.6.25.  It understood MAC (as everyone must), * but
> > also these: */
> >      features |= (1 << VIRTIO_NET_F_MAC);
> > +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
> >      features |= (1 << VIRTIO_NET_F_CSUM);
> >      features |= (1 << VIRTIO_NET_F_HOST_TSO4);
> >      features |= (1 << VIRTIO_NET_F_HOST_TSO6);
> > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t
> > cmd, {
> >      struct virtio_net_ctrl_mac mac_data;
> > 
> > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > +        /* Set MAC address */
> > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > +        return VIRTIO_NET_OK;
> > +    }
> > +
> >      if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET || elem->out_num != 3 ||
> >          elem->out_sg[1].iov_len < sizeof(mac_data) ||
> >          elem->out_sg[2].iov_len < sizeof(mac_data))
> > diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> > index d46fb98..9394cc0 100644
> > --- a/hw/virtio-net.h
> > +++ b/hw/virtio-net.h
> > @@ -44,6 +44,8 @@
> >  #define VIRTIO_NET_F_CTRL_VLAN  19      /* Control channel VLAN filtering
> > */ #define VIRTIO_NET_F_CTRL_RX_EXTRA 20   /* Extra RX mode control support
> > */
> > 
> > +#define VIRTIO_NET_F_CTRL_MAC_ADDR   23 /* Set MAC address */
> > +
> 
> I wonder whether we need a DEFINE_PROP_BIT to disable and compat this feature. 
> Consider we may migrate from a new version to an old version.

I agree, migration needs to be handled.  The bit should never change
while the device is initialized and running.  We should also never start
rejecting or ignoring the command if it was available before.

Stefan
Michael S. Tsirkin Jan. 16, 2013, 9:07 a.m. UTC | #4 
On Wed, Jan 16, 2013 at 09:59:14AM +0100, Stefan Hajnoczi wrote:
> On Wed, Jan 16, 2013 at 02:37:34PM +0800, Jason Wang wrote:
> > On Wednesday, January 16, 2013 02:16:47 PM akong@redhat.com wrote:
> > > From: Amos Kong <akong@redhat.com>
> > > 
> > > In virtio-net guest driver, currently we write MAC address to
> > > pci config space byte by byte, this means that we have an
> > > intermediate step where mac is wrong. This patch introduced
> > > a new control command to set MAC address in one time.
> > > 
> > > VIRTIO_NET_F_CTRL_MAC_ADDR is a new feature bit for compatibility.
> > > 
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > ---
> > > V2: check guest's iov_len before memcpy
> > > ---
> > >  hw/virtio-net.c | 10 ++++++++++
> > >  hw/virtio-net.h |  9 ++++++++-
> > >  2 files changed, 18 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > > index dc7c6d6..d05f98f 100644
> > > --- a/hw/virtio-net.c
> > > +++ b/hw/virtio-net.c
> > > @@ -247,6 +247,7 @@ static uint32_t virtio_net_get_features(VirtIODevice
> > > *vdev, uint32_t features) VirtIONet *n = to_virtio_net(vdev);
> > > 
> > >      features |= (1 << VIRTIO_NET_F_MAC);
> > > +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
> > > 
> > >      if (!peer_has_vnet_hdr(n)) {
> > >          features &= ~(0x1 << VIRTIO_NET_F_CSUM);
> > > @@ -282,6 +283,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice
> > > *vdev) /* Linux kernel 2.6.25.  It understood MAC (as everyone must), * but
> > > also these: */
> > >      features |= (1 << VIRTIO_NET_F_MAC);
> > > +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
> > >      features |= (1 << VIRTIO_NET_F_CSUM);
> > >      features |= (1 << VIRTIO_NET_F_HOST_TSO4);
> > >      features |= (1 << VIRTIO_NET_F_HOST_TSO6);
> > > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t
> > > cmd, {
> > >      struct virtio_net_ctrl_mac mac_data;
> > > 
> > > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > > +        /* Set MAC address */
> > > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > > +        return VIRTIO_NET_OK;
> > > +    }
> > > +
> > >      if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET || elem->out_num != 3 ||
> > >          elem->out_sg[1].iov_len < sizeof(mac_data) ||
> > >          elem->out_sg[2].iov_len < sizeof(mac_data))
> > > diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> > > index d46fb98..9394cc0 100644
> > > --- a/hw/virtio-net.h
> > > +++ b/hw/virtio-net.h
> > > @@ -44,6 +44,8 @@
> > >  #define VIRTIO_NET_F_CTRL_VLAN  19      /* Control channel VLAN filtering
> > > */ #define VIRTIO_NET_F_CTRL_RX_EXTRA 20   /* Extra RX mode control support
> > > */
> > > 
> > > +#define VIRTIO_NET_F_CTRL_MAC_ADDR   23 /* Set MAC address */
> > > +
> > 
> > I wonder whether we need a DEFINE_PROP_BIT to disable and compat this feature. 
> > Consider we may migrate from a new version to an old version.
> 
> I agree, migration needs to be handled.  The bit should never change
> while the device is initialized and running.  We should also never start
> rejecting or ignoring the command if it was available before.
> 
> Stefan

It's the same for all feature bits. Soo all we need to do is
clear them when running with a compat machine type.
Rusty Russell Jan. 17, 2013, 1:19 a.m. UTC | #5 
akong@redhat.com writes:
> @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>  {
>      struct virtio_net_ctrl_mac mac_data;
>  
> +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> +        elem->out_sg[1].iov_len == ETH_ALEN) {
> +        /* Set MAC address */
> +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> +        return VIRTIO_NET_OK;
> +    }

Does the rest of the net device still rely on the layout of descriptors?
If so, OK, we'll fix them all together.  If not, this introduces a new
one.

Cheers,
Rusty.
Amos Kong Jan. 17, 2013, 5:45 a.m. UTC | #6 
On Thu, Jan 17, 2013 at 11:49:20AM +1030, Rusty Russell wrote:
> akong@redhat.com writes:
> > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> >  {
> >      struct virtio_net_ctrl_mac mac_data;
> >  
> > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > +        /* Set MAC address */
> > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > +        return VIRTIO_NET_OK;
> > +    }
> 
> Does the rest of the net device still rely on the layout of descriptors?

No, only info string of net client relies on n->mac

> If so, OK, we'll fix them all together.  If not, this introduces a new
> one.
> 
> Cheers,
> Rusty.
Amos Kong Jan. 17, 2013, 8:37 a.m. UTC | #7 
On Thu, Jan 17, 2013 at 01:45:11PM +0800, Amos Kong wrote:
> On Thu, Jan 17, 2013 at 11:49:20AM +1030, Rusty Russell wrote:
> > akong@redhat.com writes:
> > > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > >  {
> > >      struct virtio_net_ctrl_mac mac_data;
> > >  
> > > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > > +        /* Set MAC address */
> > > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > > +        return VIRTIO_NET_OK;
> > > +    }
> > 
> > Does the rest of the net device still rely on the layout of descriptors?
> 
> No, only info string of net client relies on n->mac

I misunderstood. There is no clear limitation of how much descriptor are
used for each vq command, but many commands rely on the layout of
descriptiors. eg:

virtio-net:
   VIRTIO_NET_CTRL_RX_PROMISC
   VIRTIO_NET_CTRL_RX_ALLMULTI
   VIRTIO_NET_CTRL_MAC_TABLE_SET
   etc
 
> > If so, OK, we'll fix them all together.  If not, this introduces a new
> > one.
> > 
> > Cheers,
> > Rusty.
Stefan Hajnoczi Jan. 17, 2013, 8:39 a.m. UTC | #8 
On Thu, Jan 17, 2013 at 01:45:11PM +0800, Amos Kong wrote:
> On Thu, Jan 17, 2013 at 11:49:20AM +1030, Rusty Russell wrote:
> > akong@redhat.com writes:
> > > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > >  {
> > >      struct virtio_net_ctrl_mac mac_data;
> > >  
> > > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > > +        /* Set MAC address */
> > > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > > +        return VIRTIO_NET_OK;
> > > +    }
> > 
> > Does the rest of the net device still rely on the layout of descriptors?
> 
> No, only info string of net client relies on n->mac

I think the question is whether the hw/virtio-net.c code makes
assumptions about virtqueue descriptor layout (e.g. sg[0] is the header,
sg[1] is the data buffer).

The answer is yes, the control virtqueue function directly accesses
iov[n].

Additional patches would be required to convert the existing
hw/virtio-net.c code to make no assumptions about virtqueue descriptor
layout.  It's outside the scope of this series.

Stefan
Michael S. Tsirkin Jan. 17, 2013, 9:34 a.m. UTC | #9 
On Thu, Jan 17, 2013 at 09:39:54AM +0100, Stefan Hajnoczi wrote:
> On Thu, Jan 17, 2013 at 01:45:11PM +0800, Amos Kong wrote:
> > On Thu, Jan 17, 2013 at 11:49:20AM +1030, Rusty Russell wrote:
> > > akong@redhat.com writes:
> > > > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > > >  {
> > > >      struct virtio_net_ctrl_mac mac_data;
> > > >  
> > > > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > > > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > > > +        /* Set MAC address */
> > > > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > > > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > > > +        return VIRTIO_NET_OK;
> > > > +    }
> > > 
> > > Does the rest of the net device still rely on the layout of descriptors?
> > 
> > No, only info string of net client relies on n->mac
> 
> I think the question is whether the hw/virtio-net.c code makes
> assumptions about virtqueue descriptor layout (e.g. sg[0] is the header,
> sg[1] is the data buffer).
> 
> The answer is yes, the control virtqueue function directly accesses
> iov[n].
> 
> Additional patches would be required to convert the existing
> hw/virtio-net.c code to make no assumptions about virtqueue descriptor
> layout.  It's outside the scope of this series.
> 
> Stefan

It's not hard at all though - the harder part is data path
processing, this has been done already. Will send a
patch shortly.
diff mbox

Patch

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index dc7c6d6..d05f98f 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -247,6 +247,7 @@  static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
     VirtIONet *n = to_virtio_net(vdev);
 
     features |= (1 << VIRTIO_NET_F_MAC);
+    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
 
     if (!peer_has_vnet_hdr(n)) {
         features &= ~(0x1 << VIRTIO_NET_F_CSUM);
@@ -282,6 +283,7 @@  static uint32_t virtio_net_bad_features(VirtIODevice *vdev)
     /* Linux kernel 2.6.25.  It understood MAC (as everyone must),
      * but also these: */
     features |= (1 << VIRTIO_NET_F_MAC);
+    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
     features |= (1 << VIRTIO_NET_F_CSUM);
     features |= (1 << VIRTIO_NET_F_HOST_TSO4);
     features |= (1 << VIRTIO_NET_F_HOST_TSO6);
@@ -349,6 +351,14 @@  static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
 {
     struct virtio_net_ctrl_mac mac_data;
 
+    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
+        elem->out_sg[1].iov_len == ETH_ALEN) {
+        /* Set MAC address */
+        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
+        qemu_format_nic_info_str(&n->nic->nc, n->mac);
+        return VIRTIO_NET_OK;
+    }
+
     if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET || elem->out_num != 3 ||
         elem->out_sg[1].iov_len < sizeof(mac_data) ||
         elem->out_sg[2].iov_len < sizeof(mac_data))
diff --git a/hw/virtio-net.h b/hw/virtio-net.h
index d46fb98..9394cc0 100644
--- a/hw/virtio-net.h
+++ b/hw/virtio-net.h
@@ -44,6 +44,8 @@ 
 #define VIRTIO_NET_F_CTRL_VLAN  19      /* Control channel VLAN filtering */
 #define VIRTIO_NET_F_CTRL_RX_EXTRA 20   /* Extra RX mode control support */
 
+#define VIRTIO_NET_F_CTRL_MAC_ADDR   23 /* Set MAC address */
+
 #define VIRTIO_NET_S_LINK_UP    1       /* Link is up */
 
 #define TX_TIMER_INTERVAL 150000 /* 150 us */
@@ -106,7 +108,7 @@  typedef uint8_t virtio_net_ctrl_ack;
  #define VIRTIO_NET_CTRL_RX_MODE_NOBCAST      5
 
 /*
- * Control the MAC filter table.
+ * Control the MAC
  *
  * The MAC filter table is managed by the hypervisor, the guest should
  * assume the size is infinite.  Filtering should be considered
@@ -119,6 +121,10 @@  typedef uint8_t virtio_net_ctrl_ack;
  * first sg list contains unicast addresses, the second is for multicast.
  * This functionality is present if the VIRTIO_NET_F_CTRL_RX feature
  * is available.
+ *
+ * The ADDR_SET command requests one out scatterlist, it contains a
+ * 6 bytes MAC address. This functionality is present if the
+ * VIRTIO_NET_F_CTRL_MAC_ADDR feature is available.
  */
 struct virtio_net_ctrl_mac {
     uint32_t entries;
@@ -126,6 +132,7 @@  struct virtio_net_ctrl_mac {
 };
 #define VIRTIO_NET_CTRL_MAC    1
  #define VIRTIO_NET_CTRL_MAC_TABLE_SET        0
+ #define VIRTIO_NET_CTRL_MAC_ADDR_SET         1
 
 /*
  * Control VLAN filtering