Re: [Qemu-devel] [RFC PATCH 1/2] e1000: Use Address_Available bit as HW latch

[Vlad Yasevich <vyasevic@redhat.com>]

On 11/22/2013 04:47 AM, Jason Wang wrote:
> On 11/22/2013 04:04 AM, Vlad Yasevich wrote:
>> e1000 provides a E1000_RAH_AV bit on every complete write
>> to the Receive Address Register.  We can use this bit
>> 2 ways:
>>  1) To trigger HMP notifications.  When the bit is set the
>>     mac address is fully set and we can update the HMP.
>>
>>  2) We can turn off he bit on the write to low order bits of
>>     the Receive Address Register, so that we would not try
>>     to match received traffic to this address when it is
>>     not completely set.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>>  hw/net/e1000.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>> index ae63591..82978ea 100644
>> --- a/hw/net/e1000.c
>> +++ b/hw/net/e1000.c
>> @@ -1106,10 +1106,19 @@ mac_writereg(E1000State *s, int index, uint32_t val)
>>  
>>      s->mac_reg[index] = val;
>>  
>> -    if (index == RA || index == RA + 1) {
>> +    switch (index) {
>> +    case RA:
>> +        /* Mask off AV bit on the write of the low dword.  The write of
>> +         * the high dword will set the bit.  This way a half-written
>> +         * mac address will not be used to filter on rx.
>> +         */
>> +        s->mac_reg[RA+1] &= ~E1000_RAH_AV;
> 
> If a stupid driver write high dword first, it won't receive any packets.

I need to ping Intel guys again.  I asked them what happens when only
the low register is set, but haven't heard back.

>> +        break;
>> +    case (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);
> 
> Guest may invalid the mac address by clearing the AV bit through writing
> to high dword. So this may notify a wrong mac address.

In this case, testing for the AV bit would solve this issue.
> 
> Generally, we could teset the AV bit before notification, and try to do
> the this on both high and low dword. This obeys specs and
> receive_filter() above.

This will not really help since the AV bit would already be set from the
prior mac address.  So, if a stupid driver writes just the low word,
the AV bit would already be set.

> 
> If we don't want half-written status, driver should clear AV bit before
> each writing of new mac address. But looks like linux and freebsd does
> not do this.  But the window is really small and harmless.

We can emulate this.  Thanks for the idea.

-vlad

>> +        break;
>>      }
>>  }
>>  
>