Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written

[Vlad Yasevich <vyasevic@redhat.com>]

On 11/07/2013 09:33 AM, Alex Williamson wrote:
> On Thu, 2013-11-07 at 12:26 +0200, Michael S. Tsirkin wrote:
>> On Thu, Nov 07, 2013 at 03:32:29PM +0800, Amos Kong wrote:
>>> On Thu, Nov 07, 2013 at 08:59:22AM +0200, Michael S. Tsirkin wrote:
>>>> On Tue, Nov 05, 2013 at 07:17:18PM +0800, Amos Kong wrote:
>>>>> We currently just update the HMP NIC info when the last bit of macaddr
>>>>> is written. This assumes that guest driver will write all the macaddr
>>>>> from bit 0 to bit 5 when it changes the macaddr, this is the current
>>>>> behavior of linux driver (e1000/rtl8139cp), but we can't do this
>>>>> assumption.
>>>>>
>>>>> The macaddr that is used for rx-filter will be updated when every bit
>>>>> is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
>>>>> info when every bit is changed. It will be same as virtio-net.
>>>>>
>>>>> Signed-off-by: Amos Kong <akong@redhat.com>
>>>>
>>>> I'm not sure I buy this.
>>>>
>>>> If we actually implement e.g. mac change notifications,
>>>> sending them on writes of random bytes will confuse
>>>> the host.
>>>
>>> This patch just effects the monitor display of macaddr.
>>> During each writing, the macaddr is used for rx-filter is really
>>> changed.
>>>
>>> In the real hardware, it supports to just write part of bits,
>>> the rx-filtering is effected by every bit writing.
>>
>> Yes but again, the window can just be too small to matter
>> on real hardware.
>>
>> Our emulation is not perfect, fixing this to be just like real
>> hardware just might expose other bugs we can't fix
>> that easily.
>
> If we were to implement mac change notification, then every partial
> update would send a notify and the host.  Is that a problem?  It seems
> no different than if the device had an atomic mac update procedure and
> the guest admin changed the mac several times.

Yes, but the issue is exercebated in the non-atomic case.  RTL8139
is the worst since it has byte oriented writes so that for ever mac
change, we have the worst case potential to generate 6 notificates
(assuming libvirt is so fast as to pick up ever change).

Additionally, once libvirt is updated, this would cause rather serious
churn on the host as for ever update, libvirt is going to push the
address down to the physical host nic and the fewer of these updates
we do the better.

>
> The problem with assuming that a given byte is always written last is
> that unless the hardware spec identifies an order, we're just basing our
> code on examples where we know what the guest driver does, either by
> code inspection or access tracing.  If there are examples of guests that
> write the last byte first, then the host will never know the correct mac
> address.  Maybe there are no guests that use the wrong order, but that's
> a pretty exhaustive search.
>
> The patch doesn't change anything about how the NIC operates, only when
> mac changes are updated.  During an update the mac is in a transitory
> state and we can't go back in time to make it atomic on this hardware
> design to avoid a window where the wrong mac may be seen.  I think the
> patch is the right thing to do.  Thanks,
>

Reporting half-complete state is not the right thing, IMO.  Right now,
it's doesn't have much impact, but if we start writing these addresses
to the host nic, then this will have a much bigger impact as I said above.

-vlad

> Alex
>
>>>> I would say let's leave e1000/rtl8139 well alone unless
>>>> we see guests that actually write mac without touching
>>>> the last byte.
>>>
>>> At least, linux rtl8139cp/e1000 writes macaddr from bit 0 to bit 5.
>>> It works to just watch the last bit.
>>>
>>> Thanks, Amos
>>>
>>>> Then think of ways to detect when mac change is done
>>>> for these.
>>>>
>>>>> ---
>>>>>   hw/net/e1000.c   | 2 +-
>>>>>   hw/net/rtl8139.c | 5 +----
>>>>>   2 files changed, 2 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>>>>> index ec8ecd7..2d60639 100644
>>>>> --- a/hw/net/e1000.c
>>>>> +++ b/hw/net/e1000.c
>>>>> @@ -1110,7 +1110,7 @@ mac_writereg(E1000State *s, int index, uint32_t val)
>>>>>
>>>>>       s->mac_reg[index] = val;
>>>>>
>>>>> -    if (index == RA + 1) {
>>>>> +    if (index == RA || index == RA + 1) {
>>>>>           macaddr[0] = cpu_to_le32(s->mac_reg[RA]);
>>>>>           macaddr[1] = cpu_to_le32(s->mac_reg[RA + 1]);
>>>>>           qemu_format_nic_info_str(qemu_get_queue(s->nic), (uint8_t *)macaddr);
>>>>> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
>>>>> index 5329f44..7f2b4db 100644
>>>>> --- a/hw/net/rtl8139.c
>>>>> +++ b/hw/net/rtl8139.c
>>>>> @@ -2741,10 +2741,7 @@ static void rtl8139_io_writeb(void *opaque, uint8_t addr, uint32_t val)
>>>>>
>>>>>       switch (addr)
>>>>>       {
>>>>> -        case MAC0 ... MAC0+4:
>>>>> -            s->phys[addr - MAC0] = val;
>>>>> -            break;
>>>>> -        case MAC0+5:
>>>>> +        case MAC0 ... MAC0+5:
>>>>>               s->phys[addr - MAC0] = val;
>>>>>               qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
>>>>>               break;
>>>>> --
>>>>> 1.8.3.1
>>
>
>
>
>