[Qemu-devel] [RFC virt-spec PATCH] only writing out the last byte of MAC makes it have effect

[Qemu-devel] [RFC virt-spec PATCH] only writing out the last byte of MAC makes it have effect

[Amos Kong]

The lengcy guests don't have mac programming command, we don't know when
it's safe to use MAC. We can change QEMU to make MAC change effect when
the last byte of MAC is written to config space.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 virtio-spec.lyx | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index dbc4ef0..bb289fb 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -5430,7 +5430,7 @@ T_CTRL_MAC_TABLE_SET.
 
 \begin_layout Standard
 
-\change_inserted -1930653948 1358506710
+\change_inserted -1930653948 1363832689
 The config space 
 \begin_inset Quotes eld
 \end_inset
@@ -5464,6 +5464,15 @@ mac
  Therefore, VIRTIO_NET_CTRL_MAC_ADDR_SET is preferred, especially while
  the NIC is up.
  The command-specific-data is a 6-byte MAC address.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted -1930653948 1363833477
+The legacy guests don't support the new command, they still change MAC address
+ in original way, that's not atomic.
+ For more robust, QEMU only makes the MAC change effect when the last byte
+ of MAC address is written to config space.
 \change_unchanged
 
 \end_layout
-- 
1.8.1.4

[Qemu-devel] [RFC qemu PATCH] only writing out the last byte of MAC makes it have effect

[Amos Kong]

The lengcy guests don't have mac programming command, we don't know when
it's safe to use MAC. This patch changed qemu to makes MAC change effect
when the last byte of MAC is written to config space.

MAC address takes first 6 bytes of config space of virtio-net, the addr
is 5 when the last byte is written in virtio_config_writeb().

MAC change will effect when n->mac is updated in virtio_net_set_config().

Signed-off-by: Amos Kong <akong@redhat.com>
---
 hw/virtio.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/virtio.c b/hw/virtio.c
index 26fbc79..2e08302 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -605,8 +605,9 @@ void virtio_config_writeb(VirtIODevice *vdev, uint32_t addr, uint32_t data)
 
     stb_p(vdev->config + addr, val);
 
-    if (vdev->set_config)
+    if (vdev->set_config && (addr == 5 || strcmp(vdev->name, "virtio-net"))) {
         vdev->set_config(vdev, vdev->config);
+    }
 }
 
 void virtio_config_writew(VirtIODevice *vdev, uint32_t addr, uint32_t data)
-- 
1.7.1

Re: [Qemu-devel] [RFC qemu PATCH] only writing out the last byte of MAC makes it have effect

[Michael S. Tsirkin]

On Thu, Mar 21, 2013 at 02:44:50PM +0800, Amos Kong wrote:
> The lengcy guests don't have mac programming command, we don't know when
> it's safe to use MAC. This patch changed qemu to makes MAC change effect
> when the last byte of MAC is written to config space.
> 
> MAC address takes first 6 bytes of config space of virtio-net, the addr
> is 5 when the last byte is written in virtio_config_writeb().
> 
> MAC change will effect when n->mac is updated in virtio_net_set_config().
> 
> Signed-off-by: Amos Kong <akong@redhat.com>

Let's see what Rusty says about the spec change.

> ---
>  hw/virtio.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 26fbc79..2e08302 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -605,8 +605,9 @@ void virtio_config_writeb(VirtIODevice *vdev, uint32_t addr, uint32_t data)
>  
>      stb_p(vdev->config + addr, val);
>  
> -    if (vdev->set_config)
> +    if (vdev->set_config && (addr == 5 || strcmp(vdev->name, "virtio-net"))) {
>          vdev->set_config(vdev, vdev->config);
> +    }
>  }
>  
>  void virtio_config_writew(VirtIODevice *vdev, uint32_t addr, uint32_t data)
> -- 
> 1.7.1

Re: [Qemu-devel] [RFC qemu PATCH] only writing out the last byte of MAC makes it have effect

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Thu, Mar 21, 2013 at 02:44:50PM +0800, Amos Kong wrote:
>> The lengcy guests don't have mac programming command, we don't know when
>> it's safe to use MAC. This patch changed qemu to makes MAC change effect
>> when the last byte of MAC is written to config space.
>> 
>> MAC address takes first 6 bytes of config space of virtio-net, the addr
>> is 5 when the last byte is written in virtio_config_writeb().
>> 
>> MAC change will effect when n->mac is updated in virtio_net_set_config().
>> 
>> Signed-off-by: Amos Kong <akong@redhat.com>
>
> Let's see what Rusty says about the spec change.

Implementation notes like this belong as a footnote, eg:

        For older systems, it is recommended and typical that the device
        write byte 5 of the mac address last, so devices can use that as
        a trigger to commit the mac address change.

Now, is this a real, or theoretical issue?  Have we seen this problem in
practice, or should we continue to ignore it?

Cheers,
Rusty.

Re: [Qemu-devel] [RFC qemu PATCH] only writing out the last byte of MAC makes it have effect

[Amos Kong]

On Fri, Mar 22, 2013 at 10:45:09AM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Thu, Mar 21, 2013 at 02:44:50PM +0800, Amos Kong wrote:
> >> The lengcy guests don't have mac programming command, we don't know when
> >> it's safe to use MAC. This patch changed qemu to makes MAC change effect
> >> when the last byte of MAC is written to config space.
> >> 
> >> MAC address takes first 6 bytes of config space of virtio-net, the addr
> >> is 5 when the last byte is written in virtio_config_writeb().
> >> 
> >> MAC change will effect when n->mac is updated in virtio_net_set_config().
> >> 
> >> Signed-off-by: Amos Kong <akong@redhat.com>
> >
> > Let's see what Rusty says about the spec change.
> 
> Implementation notes like this belong as a footnote, eg:
> 
>         For older systems, it is recommended and typical that the device
>         write byte 5 of the mac address last, so devices can use that as
>         a trigger to commit the mac address change.
> 
> Now, is this a real, or theoretical issue?  Have we seen this problem in
> practice, or should we continue to ignore it?

Hi Rusty, Michael

I didn't touch any problem. MST, and you?

In Linux guest, we should disable the interface before changing mac address.
 ifconfig eth0 down
 ifconfig eth0 hw ether 10:12:13:14:15:16
 ifconfig eth0 up
 
In Windows 7 guest, after changing mac address in register table,
re-enabling interface to make it effect.
 reg add HKLM SYSTEM\CurrentControlSet\Control\Class\{4D36E972-E325-11CE-BFC1-08002BE10318}\0001] /v NetworkAddress /d 0123456789AB
 netsh interface set interface "Local Area Connection" DISABLED
 netsh interface set interface "Local Area Connection" ENABLED


So when we change the mac address, guest os always disable interface
to receive all packages. It seems the theoretical issue doesn't exist?

> Cheers,
> Rusty.
> 

-- 
			Amos.

Re: [Qemu-devel] [RFC qemu PATCH] only writing out the last byte of MAC makes it have effect

[Michael S. Tsirkin]

On Mon, Mar 25, 2013 at 10:23:57AM +0800, Amos Kong wrote:
> On Fri, Mar 22, 2013 at 10:45:09AM +1030, Rusty Russell wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > > On Thu, Mar 21, 2013 at 02:44:50PM +0800, Amos Kong wrote:
> > >> The lengcy guests don't have mac programming command, we don't know when
> > >> it's safe to use MAC. This patch changed qemu to makes MAC change effect
> > >> when the last byte of MAC is written to config space.
> > >> 
> > >> MAC address takes first 6 bytes of config space of virtio-net, the addr
> > >> is 5 when the last byte is written in virtio_config_writeb().
> > >> 
> > >> MAC change will effect when n->mac is updated in virtio_net_set_config().
> > >> 
> > >> Signed-off-by: Amos Kong <akong@redhat.com>
> > >
> > > Let's see what Rusty says about the spec change.
> > 
> > Implementation notes like this belong as a footnote, eg:
> > 
> >         For older systems, it is recommended and typical that the device
> >         write byte 5 of the mac address last, so devices can use that as
> >         a trigger to commit the mac address change.
> > 
> > Now, is this a real, or theoretical issue?  Have we seen this problem in
> > practice, or should we continue to ignore it?
> 
> Hi Rusty, Michael
> 
> I didn't touch any problem. MST, and you?
> 
> In Linux guest, we should disable the interface before changing mac address.
>  ifconfig eth0 down
>  ifconfig eth0 hw ether 10:12:13:14:15:16
>  ifconfig eth0 up
>  
> In Windows 7 guest, after changing mac address in register table,
> re-enabling interface to make it effect.
>  reg add HKLM SYSTEM\CurrentControlSet\Control\Class\{4D36E972-E325-11CE-BFC1-08002BE10318}\0001] /v NetworkAddress /d 0123456789AB
>  netsh interface set interface "Local Area Connection" DISABLED
>  netsh interface set interface "Local Area Connection" ENABLED
> 
> 
> So when we change the mac address, guest os always disable interface
> to receive all packages. It seems the theoretical issue doesn't exist?
> 
> > Cheers,
> > Rusty.
> > 
> 
> -- 
> 			Amos.

Nope. Looks like no spec change is necessary. We already say it's
not atomic and it looks like guests expect exactly that
and disable link to prevent strange issues.

-- 
MST

Re: [Qemu-devel] [RFC qemu PATCH] only writing out the last byte of MAC makes it have effect

[Amos Kong]

On Mon, Mar 25, 2013 at 08:58:49AM +0200, Michael S. Tsirkin wrote:
> On Mon, Mar 25, 2013 at 10:23:57AM +0800, Amos Kong wrote:
> > On Fri, Mar 22, 2013 at 10:45:09AM +1030, Rusty Russell wrote:
> > > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > > > On Thu, Mar 21, 2013 at 02:44:50PM +0800, Amos Kong wrote:
> > > >> The lengcy guests don't have mac programming command, we don't know when
> > > >> it's safe to use MAC. This patch changed qemu to makes MAC change effect
> > > >> when the last byte of MAC is written to config space.
> > > >> 
> > > >> MAC address takes first 6 bytes of config space of virtio-net, the addr
> > > >> is 5 when the last byte is written in virtio_config_writeb().
> > > >> 
> > > >> MAC change will effect when n->mac is updated in virtio_net_set_config().
> > > >> 
> > > >> Signed-off-by: Amos Kong <akong@redhat.com>
> > > >
> > > > Let's see what Rusty says about the spec change.
> > > 
> > > Implementation notes like this belong as a footnote, eg:
> > > 
> > >         For older systems, it is recommended and typical that the device
> > >         write byte 5 of the mac address last, so devices can use that as
> > >         a trigger to commit the mac address change.
> > > 
> > > Now, is this a real, or theoretical issue?  Have we seen this problem in
> > > practice, or should we continue to ignore it?
> > 
> > Hi Rusty, Michael
> > 
> > I didn't touch any problem. MST, and you?
> > 
> > In Linux guest, we should disable the interface before changing mac address.
> >  ifconfig eth0 down
> >  ifconfig eth0 hw ether 10:12:13:14:15:16
> >  ifconfig eth0 up
> >  
> > In Windows 7 guest, after changing mac address in register table,
> > re-enabling interface to make it effect.
> >  reg add HKLM SYSTEM\CurrentControlSet\Control\Class\{4D36E972-E325-11CE-BFC1-08002BE10318}\0001] /v NetworkAddress /d 0123456789AB
> >  netsh interface set interface "Local Area Connection" DISABLED
> >  netsh interface set interface "Local Area Connection" ENABLED
> > 
> > 
> > So when we change the mac address, guest os always disable interface
> > to receive all packages. It seems the theoretical issue doesn't exist?
> > 
> > > Cheers,
> > > Rusty.
> > > 
> > 
> > -- 
> > 			Amos.
> 
> Nope. Looks like no spec change is necessary. We already say it's
> not atomic and it looks like guests expect exactly that
> and disable link to prevent strange issues.

I just found Jiri has a commit to allow changing mac when iface
is running. It seems only the virt nics are allowned, right?

But the leagcy guests don't allow to change mac when interface is
running, so no spec change is necessary.

[upstream kernel]
commit d4fc6918f4b3cc844185f59fc518351525950449
Author: Jiri Pirko <jpirko@redhat.com>
Date:   Wed Jun 27 05:27:46 2012 +0000

    virtio_net: allow to change mac when iface is running
    
    Signed-off-by: Jiri Pirko <jpirko@redhat.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

RH-BZ: https://bugzilla.redhat.com/show_bug.cgi?id=882868

-- 
			Amos.

Re: [Qemu-devel] [RFC virt-spec PATCH] only writing out the last byte of MAC makes it have effect

[Michael S. Tsirkin]

On Thu, Mar 21, 2013 at 11:02:36AM +0800, Amos Kong wrote:
> The lengcy guests don't have mac programming command, we don't know when
> it's safe to use MAC. We can change QEMU to make MAC change effect when
> the last byte of MAC is written to config space.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  virtio-spec.lyx | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> index dbc4ef0..bb289fb 100644
> --- a/virtio-spec.lyx
> +++ b/virtio-spec.lyx
> @@ -5430,7 +5430,7 @@ T_CTRL_MAC_TABLE_SET.
>  
>  \begin_layout Standard
>  
> -\change_inserted -1930653948 1358506710
> +\change_inserted -1930653948 1363832689
>  The config space 
>  \begin_inset Quotes eld
>  \end_inset
> @@ -5464,6 +5464,15 @@ mac
>   Therefore, VIRTIO_NET_CTRL_MAC_ADDR_SET is preferred, especially while
>   the NIC is up.
>   The command-specific-data is a 6-byte MAC address.
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted -1930653948 1363833477
> +The legacy guests don't support the new command, they still change MAC address
> + in original way, that's not atomic.
> + For more robust, QEMU only makes the MAC change effect when the last byte
> + of MAC address is written to config space.
>  \change_unchanged
>  
>  \end_layout

The wording can be improved, but before that - Rusty, what
do you think about such a hack? Worth supporting or
let's just ask everyone to update drivers?

> -- 
> 1.8.1.4