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

[Michael S. Tsirkin]

On Mon, Nov 18, 2013 at 02:33:16PM -0700, Alex Williamson wrote:
> On Mon, 2013-11-18 at 15:57 -0500, Vlad Yasevich wrote:
> > On 11/18/2013 03:33 PM, Alex Williamson wrote:
> > > 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.
> > 
> > Yes, there is a slight issue with validity of mac at the time of
> > processing packets.  I have an outstanding question on the Intel
> > list about this behavior with real HW.  But, with e1000, the validity
> > bit provides a much higher guarantee that a guest that will be
> > setting the mac address will write the high register second to
> > guarantee that when the valid bit is written, the mac is fully
> > valid.  As a result we don't really need the e1000 part of the
> > cd5be5829.
> 
> But doesn't that valid bit mean that a mac update will start and end
> with a write to the high order register?  So we're assuming:
> 
> a) write RA + 1 (invalidate)
> b) write RA (write low)
> c) write RA + 1 (write high + valid)
> 
> Without cd5be5829 the only change is that we don't store a new mac into
> the monitor at b).  The mac stored in the monitor is still wrong from a)
> until c).  So it's ever so slightly less broken without cd5be5829.

Yes.

> > > 
> > >> 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.
> > > 
> > 
> > This one is actually not as bad either.  RTL spec requires that
> > receive register writes happen as 32 bit word writes.  This is
> > what linux and bsd drivers do, so from driver perspective, the
> > issue is the same.  What our emulation layer does is turn these
> > 32 bit writes into 4 8-bit writes.  This is likely due to some
> > very broken and very old drivers, but I am not sure.
> > 
> > So, the information in the monitor will be broken if the guest
> > does: write_hi(); write_lo();  A part of me would really like
> > to see a guest that does this :)
> 
> So the argument for reverting here is that it seems unlikely that the
> dwords get written in the hi->lo order and we'd rather the monitor get
> stuck with the wrong mac forever than it show the wrong mac address for
> a tiny window for everybody else?  I think you say something about
> sub-optimal here...

The argument is that I don't think it makes sense to maintain two
differently broken implementations in 1.6 and 1.7.

It's better to have them broken in the same way and
then maybe have a non broken one in 1.8.
Or just leave well alone - no one complained so far,
this change is of the "I read the spec and it seems
to say we should do X" variety.

> 
> > The current code isn't perfect either.  It still has a potential
> > to show the wrong mac address in the monitor.  I doesn't make
> > a lot of sense to me to replace one sub-optimal solution
> > with another sub-optimal solution, especially since no-one
> > complained.
> 
> Exactly, the code isn't perfect either way and this revert is just
> replacing one sub-optimal solution for another.  So why do it?

Because we had the old buggy behaviour for years and no one
complained. Maybe the new buggy behaviour will also be fine.
But I'm not inclined to take the chance.

>  Thanks,
> 
> Alex
> 
> > >> 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;
> > >>>
> > >>>
> > >>>
> > >>
> > > 
> > > 
> > > 
> > 
> 
>