Re: [Qemu-devel] [RFC] sync NIC's MAC maintained in NICConf as soon as emualted NIC's MAC changed in guest

[Markus Armbruster <armbru@redhat.com>]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Sep 25, 2013 at 10:14:49AM +0000, Zhanghaoyu (A) wrote:
>> >> >> >> Hi, all
>> >> >> >> 
>> >> >> >> Do live migration if emulated NIC's MAC has been changed, RARP 
>> >> >> >> with wrong MAC address will broadcast via
>> >> >> >> qemu_announce_self in destination, so, long time network
>> >> >> >> disconnection probably happen.
>> >> >> >
>> >> >> >Good catch.
>> >> >> >
>> >> >> >> I want to do below works to resolve this problem, 1. change 
>> >> >> >> NICConf's MAC as soon as emulated NIC's MAC changed in guest
>> >> >> >
>> >> >> >This will make it impossible to revert it correctly on reset, won't it?
>> >> >> >
>> >> >> You are right.
>> >> >> virsh reboot <domain>, or virsh reset <domain>, or reboot VM
>> >> >> from guest, will revert emulated NIC's MAC to original one
>> >> >> maintained in NICConf.
>> >> >> During the reboot/reset flow in qemu, emulated NIC's reset handler 
>> >> >> will sync the MAC address in NICConf to the MAC address in
>> >> >> emulated NIC structure, e.g., virtio_net_reset sync the MAC
>> >> >> address in NICConf to VirtIONet'mac.
>> >> >> 
>> >> >> BTW, in native scenario, reboot will revert the changed MAC to
>> >> >> original one, too.
>> >> >> 
>> >> >> >> 2. sync NIC's (more precisely, queue) MAC to corresponding 
>> >> >> >> NICConf in NIC's migration load handler
>> >> >> >> 
>> >> >> >> Any better ideas?
>> >> >> >> 
>> >> >> >> Thanks,
>> >> >> >> Zhang Haoyu
>> >> >> >
>> >> >> >I think announce needs to poke at the current MAC instead of
>> >> >> > the default one in NICConf.
>> >> >> >We can make it respect link down state while we are at it.
>> >> >> >
>> >> >> NICConf structures are incorporated in different emulated NIC's 
>> >> >> structure, e.g., VirtIONet, E1000State_st, RTL8139State, etc.,
>> >> >> since so many kinds of emulated NICs, they are described by
>> >> >> different structures, how to find all NICs' current MAC?
>> >> >> 
>> >> >> Maybe we can introduce a pointer member 'current_mac' to NICConf 
>> >> >> structure, which points to the current MAC, then we can find
>> >> >> all current MACs from NICConf.current_mac.
>> >> >
>> >> >I wouldn't make it a pointer, just a buffer with the mac, copy it there.
>> >> >Maybe call it "softmac" that's what it is really.
>> >> >
>> >> >> Can we broadcast the RARP with current MAC in NIC's migration
>> >> >> load handler respectively?
>> >> >> 
>> >> >> Thanks,
>> >> >> Zhang Haoyu
>> >> >
>> >> >It's not so simple, you need to retry several times.
>> >> >
>> >> Could you make a statement for 'retry several times' ?
>> >> Is it the process of retrying several times to sending RARP in
>> >> qemu_announce_self_once?
>> >
>> >yes
>> >
>> >> 'broadcast the RARP with current MAC in NIC's migration load handler 
>> >> respectively' is distributing the job of what qemu_announce_self
>> >> does to every NIC's migration load handler, e.g., in virtio NIC's
>> >> migration load handler virtio_net_load, we can create a timer to
>> >> retry several times to send ARAP with current MAC for this NIC,
>> >> just as same as qemu_announce_self does.
>> >
>> >I don't see a lot of value in this yet.
>> >
>> In my opinion, it's not so good to introduce a 'softmac' member to
>> NICConf, which is not essential function of NICConf.
>
> Maybe not essential but 100% of hardware we emulate supports softmacs.

Yes, but NICConf is about NIC *configuration*, not random common NIC
state.

We can capture common NIC state in a separate, properly named data type.

If we want to bunch it together with common configuration in NICConf
instead, then better rename NICConf to something that actually reflects
its changed purpose.  I doubt this would be a good idea.

>> And, distributing the job of what qemu_announce_self does to every
>> NIC's migration load handler has no disadvantages over
>> qemu_announce_self,
>
> I see some disadvantages, yes.
> You are going to add code to all devices instead of
> doing it in one place, there better be a good reason for this.

Keeping code common to many (most?) NICs factored out makes sense.
We've started doing that for block devices, in hw/block/block.c.  So
far, the only code there is about configuration, thus we work with
BlockConf.

[...]