Re: [Qemu-devel] [QEMU PATCH v2] virtio-net: introduce a new macaddr control

[Michael S. Tsirkin]

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 address@hidden wrote:
> > > From: Amos Kong <address@hidden>
> > > 
> > > 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 <address@hidden>
> > > ---
> > > 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.