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

[Alex Williamson]

On Mon, 2013-11-18 at 15:09 -0500, Vlad Yasevich wrote:
> On 11/18/2013 02:58 PM, Alex Williamson wrote:
> > On Mon, 2013-11-18 at 21:47 +0200, Michael S. Tsirkin wrote:
> >> This reverts commit cd5be5829c1ce87aa6b3a7806524fac07ac9a757.
> >> Digging into hardware specs shows this does not
> >> actually make QEMU behave more like hardware.
> >> Let's stick to the tried heuristic for 1.7 and
> >> possibly revisit for 1.8.
> > 
> > If this is broken, then so are these:
> > 
> > 23c37c37f0280761072c23bf67d3a4f3c0ff25aa
> > 7c36507c2b8776266f50c5e2739bd18279953b93
> 
> These aren't really broken.  They just assume that the high order
> writes will happen after the low order writes.
> 
> In the case of e1000, this is a little more then an assumption
> (particularly in the case of nic initilization).

But AIUI there's also a valid bit in that high order byte on e1000, so
reverting cd5be582 means we stuff a new mac into qemu less often, but
it's still only accurate some of the time.

> In the case of RTL nic, it is just an  assumption, but it hasn't
> been shown faulty yet.  We do plan to address this a bit more
> thoroughly.

So how is RTL less broken without cd5be582?  AIUI the valid bit is off
in a separate register on RTL, so we have no guarantee about order of
updating the mac.  Without cd5be582 the info in the monitor may be
permanently broken if the guest uses a write order other than what we
assume.

> The patch that was applied was controversial and more then 1 person
> expressed reservations.

Understood, but it also raised and addressed a shortcoming in the
previous patches.  If cd5be582 was controversial because the monitor was
getting updated with incorrect mac addresses then this simple revert
doesn't solve that problem.  Thanks,

Alex

> > 
> > None of these change the behavior of hardware, they only change when the
> > monitor gets told about mac address changes.  I'd suggest either add the
> > emulation described in each spec or revert all of them.  A partial
> > revert is just noise.  Thanks,
> > 
> > Alex
> > 
> >>
> >> Reported-by: Vlad Yasevich <address@hidden>
> >> Cc: Amos Kong <address@hidden>
> >> Cc: Alex Williamson <address@hidden>
> >> ---
> >>  hw/net/e1000.c   | 2 +-
> >>  hw/net/rtl8139.c | 5 ++++-
> >>  2 files changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> >> index ae63591..8387443 100644
> >> --- a/hw/net/e1000.c
> >> +++ b/hw/net/e1000.c
> >> @@ -1106,7 +1106,7 @@ mac_writereg(E1000State *s, int index, uint32_t val)
> >>  
> >>      s->mac_reg[index] = val;
> >>  
> >> -    if (index == RA || index == RA + 1) {
> >> +    if (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 7f2b4db..5329f44 100644
> >> --- a/hw/net/rtl8139.c
> >> +++ b/hw/net/rtl8139.c
> >> @@ -2741,7 +2741,10 @@ static void rtl8139_io_writeb(void *opaque, uint8_t 
> >> addr, uint32_t val)
> >>  
> >>      switch (addr)
> >>      {
> >> -        case MAC0 ... MAC0+5:
> >> +        case MAC0 ... MAC0+4:
> >> +            s->phys[addr - MAC0] = val;
> >> +            break;
> >> +        case MAC0+5:
> >>              s->phys[addr - MAC0] = val;
> >>              qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
> >>              break;
> > 
> > 
> > 
>