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 <ak...@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 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
> >>
