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

[Amos Kong]

On Thu, Nov 21, 2013 at 05:14:36PM +0200, Michael S. Tsirkin wrote:
> This reverts commit cd5be5829c1ce87aa6b3a7806524fac07ac9a757.
> Digging into hardware specs shows this does not
> actually make QEMU behave more like hardware:
> 
> There are valid arguments backed by the spec to indicate why the version
> of e1000 prior to cd5be582 was more correct: the high byte actually
> includes a valid bit, this is why all guests write it last.
> 
> For rtl8139 there's actually a separate undocumented valid bit, but we
> don't implement it yet.
> 
> To summarize all the drivers we know about behave in one way
> that allows us to make an assumption about write order and avoid
> spurious, incorrect mac address updates to the monitor.
> 
> Let's stick to the tried heuristic for 1.7 and
> possibly revisit for 1.8.
> 
> Reported-by: Vlad Yasevich <address@hidden>
> Reviewed-by: Vlad Yasevich <address@hidden>
> Cc: Amos Kong <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>

Reviewed-by: Amos Kong <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;
> -- 
> MST
> 

-- 
                        Amos.