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

[Vlad Yasevich <vyasevic@redhat.com>]

On 11/18/2013 02:42 PM, Michael S. Tsirkin wrote:
> On Mon, Nov 18, 2013 at 12:33:20PM -0500, Vlad Yasevich wrote:
>> On 11/18/2013 10:02 AM, 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>
>>>
>>> Vlad here told me he did some research and this
>>> does not actually match hardware behaviour
>>> for either e1000 or rtl8139.
>>>
>>> Vlad, would you like to elaborate on-list?
>>
>> Sure.
>>
>> I decided to dig into the hardware data-sheets and the OS drivers
>> that use the HW to see what's really going on and how the
>> hw expects this data to be programmed.
>>
>> Here is what I've found so far:
>> E1000:
>>    e1000 stores each mac address in 2 registers:
>>        RAL - receive register low
>>        RAH - receive register high
>>    The highest order bit of RAH (bit 31) is called the address available
>>    bit.  When this bit is set the HW will attempt to use the address for
>>    packet matching.  So, when the mac address is initially programmed
>>    into HW, that AV bit is not set until RAH is written.  Thus drivers
>>    really should do writes in RAL+RAH order, otherwise AV bit would be
>>    set on a partially set address.
>>    There is a slight issue when the receive address registers already
>>    have a value.  Since the address is written in 2 separate writes,
>>    there is a very small window of time when the RAL is set to the new
>>    value and RAH is set to the old value with AV still set.  I am
>>    talking to Intel guys about this now.
>>
>>    So from the POV of notifying libvirt/management that address is
>>    changed, it should be done when RAH is set.
>>
>> RTL8139:
>>    Realtek devices have a 9346CR Command Register that gates write
>>    access to certain configuration regions on the HW.  It is placed
>>    into "Configuration register write enabled" mode before driver can
>>    write to one of a set of configuration spaces.  Even though
>>    the data sheet doesn't mention this, it appears that this must
>>    also must be used to guard write access to receive address register
>>    of the card.  All variants of BSD and linux drivers that I've found
>>    use this along with comment that say that this is an undocumented
>>    requirement.
> 
> What does a windows driver do BTW?

I'll boot windows and find out.

-vlad

> 
>>    I am not sure what the HW does to incoming frames when
>>    the command register is to this mode.
>>    I see 2 things that we might be able to do here:
>>      1) A low-impact change might be to only notify the management when
>>         we've detected an address change and currently exiting
>> 	"config write" mode.
>>      2) A more invasive change might be to disable rx_handling while in
>>         "config wirte" mode.  This would prevent attempting to match
>> 	packets to a partially written mac address.
>>
>>    I have a patch that does (1) above.
>>
>>
>> Thoughts?
>> -vlad
> 
> Let's start by reverting cd5be5829c1ce87aa6b3a7806524fac07ac9a757.
> 
>>>
>>> I think we should revert this for 1.8 and
>>> look at emulating actual hardware behaviour.
>>>
>>>> ---
>>>>  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
>>>>