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

[Zhanghaoyu (A)]

>> >> >> >> >> 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.
>
>I agree, it should go into NetClientState, not NICConf.
>My main point is it's a common thing, let's not duplicate code.
>
Yes, put it into NetClientState is better.
But, need to add updating code for NetClientState.softmac to all devices, right?

>> >> 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.
Comparing with qemu_announce_self, there is indeed no advantages, on the 
contrary, has disadvantages, just as what you said.
but comparing with introducing 'softmac' or something into NIC-related 
structures, 
it does not need to add any data to NIC-related structures, and introducing 
'softmac' also need to add updating code for 'softmac' to all devices, right?
And, I don't think it's a good idea to store the identical data in two buffers, 
its consistency should be guaranteed.

Thanks,
Zhang Haoyu

>> 
>> 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.
>> 
>> [...]